Omit NOP rendering operation which is outside of the clip bounds

[ColdPaleLight]

Based on https://github.com/flutter/flutter/issues/100172, we can treat ops out of the clip bounds as NOPs and omit them

For details, see how the following submission handles clipRect.

void DisplayListBuilder::drawRect(const SkRect& rect) {
  if (AccumulateOpBounds(rect, kDrawRectFlags)) {
    Push<DrawRectOp>(0, 1, rect);
  }
  CheckLayerOpacityCompatibility();
}

As you can see, some tests fail with this optimization.

1. display_list_rendertests: https://github.com/flutter/engine/blob/56c6c1845163e945e81dcb3143585cc68283a3d3/display_list/display_list_canvas_unittests.cc#L1860-L1864 failed log https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8807250199871484161/+/u/test:_Host_Tests_for_host_profile/stdout?format=raw

2. display_list_unittests (if ignore failed tests in display_list_rendertests): https://github.com/flutter/engine/blob/56c6c1845163e945e81dcb3143585cc68283a3d3/display_list/display_list_unittests.cc#L207 failed log https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8807249085090327521/+/u/test:_Host_Tests_for_host_profile/stdout?format=raw

Pre-launch Checklist

• [ ] I read the Contributor Guide and followed the process outlined there for submitting PRs.
• [ ] I read the Tree Hygiene wiki page, which explains my responsibilities.
• [ ] I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
• [ ] I listed at least one issue that this PR fixes in the description above.
• [ ] I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
• [ ] I updated/added relevant documentation (doc comments with ///).
• [ ] I signed the CLA.
• [ ] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[flutter-dashboard[bot]]

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

[flar]

The reason the saveLayer tests trigger the optimization is that they have a rendering op specifically inserted to prevent NOP optimization, and that op successfully prevents Skia from removing the op, but it doesn't prevent this new DL code from removing it.

Unfortunately, we need our saveLayer test cases to be robust against the removal of ops, but we are making that harder and harder to accomplish. The saveLayer code uses a "safe_restore" that adds an op that isn't visible, but it chose to do so in a way that isn't optimized out by Skia by using an op that is OOB. The new code for DL will notice that bounds mismatch and eliminate it.

The intent here is to actually test the rendering of saveLayer via DL and Skia and show that they match without either system optimizing out the content in any way. It's almost like we want to say "just record this and don't optimize it", but Skia doesn't provide a way to do that so I tried to accomplish it by using an op that was cleverly, but "undetectably" not going to perturb the results.

This testing conundrum should not inform our optimization strategy, though. We'll just need a way to make the tests not have optimization opportunities in them.

Here is the line of code that is causing the problem. Deleting it will just trigger optimizations in SkPicture that I don't want to happen, but perhaps we can replace it with something that will survive both mechanism's optimization algorithms...

https://github.com/flutter/engine/blob/88abca04183d832d92f60e8c5d84cba075edd3ed/display_list/display_list_canvas_unittests.cc#L718

[flar]

The issue with the dl_unittests is likely that we have a whole suite of save-type calls that are all dumped into the DisplayLists being constructed without any regard for whether they will trigger an optimization. I'm betting there are interactions between the various calls there that will be violated by sharing a DL with some clip or save or attribute that turns the op into a NOP of some sort. We may have to rethink the way we test arbitrary DL construction in those tests...

[Hixie]

@ColdPaleLight is this still on your radar?

[ColdPaleLight]

@ColdPaleLight is this still on your radar?

Yes, I will restart this work when the PR https://github.com/flutter/engine/pull/34365 lands.

[ColdPaleLight]

Closing it since I don't have time to complete this PR in the near future.