Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@ColdPaleLight
Copy link
Member

@ColdPaleLight ColdPaleLight commented Jul 30, 2022

Based on flutter/flutter#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:

    if (!testP.is_draw_display_list()) {
    EXPECT_EQ(static_cast<int>(display_list->op_count()),
    sk_picture->approximateOpCount())
    << info;
    }

    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):

    TEST(DisplayList, DisplayListsWithVaryingOpComparisons) {

    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.

@ColdPaleLight ColdPaleLight marked this pull request as draft July 30, 2022 03:00
@flutter-dashboard
Copy link

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
Copy link
Contributor

flar commented Jul 31, 2022

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...

// Draw another primitive to disable peephole optimizations

@flar
Copy link
Contributor

flar commented Jul 31, 2022

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
Copy link
Contributor

Hixie commented Oct 25, 2022

@ColdPaleLight is this still on your radar?

@ColdPaleLight
Copy link
Member Author

@ColdPaleLight is this still on your radar?

Yes, I will restart this work when the PR #34365 lands.

@ColdPaleLight
Copy link
Member Author

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants