Closed Bug 1248822 Opened 4 years ago Closed 4 years ago

Compositor-side LayerTreeInvalidation can use incorrect visible region

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox46 --- fixed
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: botond, Assigned: botond)

References

Details

(Keywords: correctness, Whiteboard: [gfx-noted])

Attachments

(1 file)

The compositor uses LayerTreeInvalidation in LayerManagerComposite::UpdateAndRender() [1] to determine the invalid region to composite. This computation uses the layers' visible regions as one of its inputs.

The visible region of a layer can be updated in the compositor to account for async transformations; this change is reflected in the shadow visible region.

In bug 1161978 and bug 1247452, LayerTreeInvalidation was updated to use the shadow visible region when invoked on the compositor side, so that such async transformations are correctly reflected in the computed invalid region.

However, I believe those fixes are incomplete, because LayerManagerComposite::PostProcessLayers(), which is called *after* the invalid region computation [2] can modify the shadow visible region [3], in ways other than minimizing it (e.g. it can apply an async translation to it).

[1] https://dxr.mozilla.org/mozilla-central/rev/6ea654cad929c9bedd8a4161a182b6189fbeae6a/gfx/layers/composite/LayerManagerComposite.cpp#401
[2] https://dxr.mozilla.org/mozilla-central/rev/6ea654cad929c9bedd8a4161a182b6189fbeae6a/gfx/layers/composite/LayerManagerComposite.cpp#447
[3] https://dxr.mozilla.org/mozilla-central/rev/6ea654cad929c9bedd8a4161a182b6189fbeae6a/gfx/layers/composite/LayerManagerComposite.cpp#311
I believe this can be fixed by calling PostProcessLayers() before the invalid region computation in LayerManagerComposite::UpdateAndRender().
The reason is that PostProcessLayers() can change the effective visible region,
and LayerTreeInvalidation needs the final effective visible region.

Review commit: https://reviewboard.mozilla.org/r/35195/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35195/
Unfortunately, an issue like this cannot be caught in a reftest, because when drawing to an external draw target (as reftests do), the invalid region is expanded to include the entire bounds of the draw target [1].

After some discussion with Matt, I filed bug 1248828 for implementing a capability to test compositor-side invalidation in the reftest harness.

[1] https://dxr.mozilla.org/mozilla-central/rev/6ea654cad929c9bedd8a4161a182b6189fbeae6a/gfx/layers/composite/LayerManagerComposite.cpp#415
Matt, would you prefer that this land without a test, or that we wait until bug 1248828 is fixed and write a test using that capability?
Flags: needinfo?(matt.woodrow)
See Also: → 1161978, 1247452
It's a real bug, so I think we should land the fix now.
Flags: needinfo?(matt.woodrow)
Attachment #8720066 - Flags: review?(matt.woodrow)
Comment on attachment 8720066 [details]
MozReview Request: Bug 1248822 - In LayerManagerComposite::UpdateAndRender(), call PostProcessLayers() before LayerTreeInvalidation. r=mattwoodrow

https://reviewboard.mozilla.org/r/35195/#review32995
Attachment #8720066 - Flags: review?(matt.woodrow) → review+
(In reply to Botond Ballo [:botond] from comment #6)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=4112e7e78d0b

Asan doesn't seem to be happy; not sure if that's related to this patch or not.

Here's a Try push based on a more recent m-c that includes this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=273840f35687
Assignee: nobody → botond
(In reply to Botond Ballo [:botond] from comment #8)
> Here's a Try push based on a more recent m-c that includes this patch:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=273840f35687

This Try push has errors due to another patch included in it.

Here's a green Try push that includes this patch:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dda40dc2e26a
https://hg.mozilla.org/mozilla-central/rev/7a02a813a179
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
This appears to have been responsible for a ~15% improvement in tscrollx opt e10s on windows XP [1]. Do you think that's expected? If so please flag for uplift to 47 and/or 46 if appropriate, it would be nice get this win on aurora/beta as well.

[1] https://treeherder.mozilla.org/perf.html#/graphs?series=[mozilla-inbound,e5c782b789ec02e8add7dae8c97ca3f42ed46442,1]&series=[mozilla-inbound,c3eb69f0719aacca596bca0626205e4b30953034,1]&zoom=1457226875047.2878,1457518384400.5564,2.595652041227921,3.9869563890540083
Flags: needinfo?(botond)
Hmm, interesting. I suppose it's conceivable, as the patch could improve compositor performance if it results in a smaller invalid region being composited, and as an ASAP mode test, tscrollx could reflect that improvement. However, I'd like to confirm this with some experimentation before requesting any uplifts. Leaving needinfo for now.
I did "before" and "after" Try runs with logging that displays the invalid region that LayerManagerComposite::UpdateAndRender() passes to Render() [1] [2]. The pushes reproduce the 15% speedup for Windows XP.

Analysis of the results shows that before this patch, the average size of the invalid region for a frame is about 380k pixels; after the patch, it's about 180k pixels. That's a reduction of over 50%! Assuming the size of the invalid region being composited has a proportional effect on a significant chunk of the composition time, I can see this causing a 15% overall speedup in ASAP mode.

The Try pushes also have data for Windows 7 (unintentionally, but fortuitously), where we see a similar reduction in the average size of the invalid region per frame, and about a 10% speedup.

Matt, I have a couple of questions for you:

  (1) Does this reduction in the size of the invalid region seem plausible to you
      as a reason for the speedup?

      Does the fact that it caused more of a speedup (15%) on Windows XP than on
      Windows 7 (10%) make sense (for example, because Windows XP doesn't use 
      hardware acceleration, so the size of the area being composited has a larger
      effect on the composition time)?

  (2) If the answer to the above is "yes", how risky do you think this patch is
      for uplift? I'm inclined to be cautious about touching core compositor code
      like this, but if it's causing a significant win it makes sense to consider
      uplifting as Kats suggested.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd861e4ae135
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c75beedc2d9
Flags: needinfo?(botond) → needinfo?(matt.woodrow)
(In reply to Botond Ballo [:botond] from comment #14) 
>   (1) Does this reduction in the size of the invalid region seem plausible
> to you
>       as a reason for the speedup?
> 
>       Does the fact that it caused more of a speedup (15%) on Windows XP
> than on
>       Windows 7 (10%) make sense (for example, because Windows XP doesn't
> use 
>       hardware acceleration, so the size of the area being composited has a
> larger
>       effect on the composition time)?

Yes and maybe.

It's definitely believable that compositing less pixels improves throughput in ASAP mode.

Windows XP *does* use hardware acceleration (for reftests, and I assume talos too), but it doesn't take the invalid region into account (we always composite the whole frame).

So it's a little weird that WinXP wins by more than Win7.

I assume the total combined visible region of all layers we composite is significantly smaller too, which reduces overdraw, regardless of the invalid region. Given the high bandwidth requirements of compositing the whole screen each time, I can believe that reducing overdraw would help a fair bit.


> 
>   (2) If the answer to the above is "yes", how risky do you think this patch
> is
>       for uplift? I'm inclined to be cautious about touching core compositor
> code
>       like this, but if it's causing a significant win it makes sense to
> consider
>       uplifting as Kats suggested.

It seems low risk to me, I don't have any problems with taking it.
Flags: needinfo?(matt.woodrow)
Comment on attachment 8720066 [details]
MozReview Request: Bug 1248822 - In LayerManagerComposite::UpdateAndRender(), call PostProcessLayers() before LayerTreeInvalidation. r=mattwoodrow

Thanks, Matt! Requesting uplift per your comments.

Approval Request Comment
[Feature/regressing bug #]:
  Not a regression, but may help gain back some performance
  regressed by APZ.

[User impact if declined]:
  Users miss out on a possible speedup in compositor performance, 
  which may become noticeable in situations where compositing is
  the bottleneck (probably not very often).

[Describe test coverage new/current, TreeHerder]:
  On m-c since 2015-02-08. The code in question is exercised on
  every composite, i.e. every time we draw something on the screen.

[Risks and why]:
  Low risk as per comment 15. This is a small and straightforward 
  change. Given that we're early in the beta cycle, I'm requesting
  beta uplift as well.

[String/UUID change made/needed]:
  None.
Attachment #8720066 - Flags: approval-mozilla-beta?
Attachment #8720066 - Flags: approval-mozilla-aurora?
Comment on attachment 8720066 [details]
MozReview Request: Bug 1248822 - In LayerManagerComposite::UpdateAndRender(), call PostProcessLayers() before LayerTreeInvalidation. r=mattwoodrow

Fix has been in Nightly for 6 weeks, size of patch is small, taking it in Beta46 and Aurora47.
Attachment #8720066 - Flags: approval-mozilla-beta?
Attachment #8720066 - Flags: approval-mozilla-beta+
Attachment #8720066 - Flags: approval-mozilla-aurora?
Attachment #8720066 - Flags: approval-mozilla-aurora+
(In reply to Ritu Kothari (:ritu) from comment #17)
> Fix has been in Nightly for 6 weeks

Sorry, I made a typo!

>   On m-c since 2015-02-08.

That should have been "2015-03-08", i.e. 1 week ago.

Hopefully that doesn't affect the uplift decision, but just in case, I wanted to point it out.
You need to log in before you can comment on or make changes to this bug.