Check return values for Sk[I]Rect::intersect #54577
Conversation
flow/diff_context.cc
Outdated
| [[maybe_unused]] | ||
| bool ret2 = res.frame_damage.intersect(frame_clip); | ||
| if (!ret1 || !ret2) { | ||
| FML_LOG(ERROR) << "intersect was not handled: " << ret1 << ", " << ret2; |
There was a problem hiding this comment.
@knopp this is the interesting case here where I wanted to either turn these into FML_DCHECK(ret) calls or write a test case that verifies that we do the right thing here.
I was unable to figure out how to write a test case that specifically tested this case (see my ifdef'd out test case in diff_context_unittests.cc), but when I put in the FML_DCHECK calls they got triggered in several flow layer tests.
Please advise how I can write a better test case in the unittests and which gets the wrong answer if we don't handle the return values here and I'll change these to deal with the return value. Typically I use if (!r1.intersect(r2)) { r1.setEmpty(); } if that will work in this case...
There was a problem hiding this comment.
Obviously the other tests in the *_layer_unittests.cc files do run into this case, but they aren't specifically trying to test it, so they may either be hitting this as a side effect of the way they're written to test something else, or they are bad tests for what they are really trying to do. In either case, I don't want to rely on them to test this particular change...
There was a problem hiding this comment.
Looking further into the cases where the other unit tests were triggering this code, in all cases it was because the damage rects were already empty so ignoring the return value in that case didn't matter as the result rects were already empty and did not "need" to be set to empty again.
So, I'd like to be able to rule this out or find a test case that can trigger it without the damage rects being "already empty".
There was a problem hiding this comment.
BTW, I fixed the test code and found a way to trigger this problem so I just need a review now.
flow/diff_context_unittests.cc
Outdated
| EXPECT_EQ(damage.buffer_damage, SkIRect::MakeLTRB(16, 16, 64, 64)); | ||
| } | ||
|
|
||
| #ifdef NOT_WORKING_YET |
There was a problem hiding this comment.
@knopp here was my attempt to write a test case that triggered the above code, but it didn't seem to work (in fact it crashed a few times as I was working on it).
There was a problem hiding this comment.
I'll take a look at this first thing tomorrow.
There was a problem hiding this comment.
I actually fixed it. The reason I was having trouble reproducing it is that the test helper function was inserting a clip layer which effectively deleted the out-of-range layer. By "rolling my own" in the test case I was able to get the code that checks the return value to actually encounter a case where the frame/buffer damage was not empty, but needed to be set to empty and the test case triggers that condition now.
I wonder if there were any bugs in partial repaint caused by this? (i.e. something animating off screen and we kept repainting even though we didn't need to?)
There was a problem hiding this comment.
That's what I was thinking. I don't know how likely it is though for a layer tree like that to come from a framework in practice since there are are usually clips present.
flar
changed the title
flar
changed the title
knopp
left a comment
knopp
left a comment
There was a problem hiding this comment.
LGTM! Thanks for catching this.
…153753) flutter/engine@48d7b04...0ac9e97 2024-08-20 [email protected] Check return values for Sk[I]Rect::intersect (flutter/engine#54577) 2024-08-20 [email protected] Roll Skia from ada9a367c5f2 to cc9c81d7fc4d (1 revision) (flutter/engine#54641) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…lutter#153753) flutter/engine@48d7b04...0ac9e97 2024-08-20 [email protected] Check return values for Sk[I]Rect::intersect (flutter/engine#54577) 2024-08-20 [email protected] Roll Skia from ada9a367c5f2 to cc9c81d7fc4d (1 revision) (flutter/engine#54641) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
2 participants
The
SkRect::intersectandSkIRect::intersectmethods return values that must be handled or the calling code might end up with a non-empty answer for non-intersecting rectangles. There were 3 or 4 places in our code that we weren't doing this so now we check them all, in some cases even if we believe that the answer will not be empty.This is prep work so that Skia can add
[[nodiscard]]directives to these 2 methods so that nobody else makes that mistake.