1248822 - Compositor-side LayerTreeInvalidation can use incorrect visible region

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Botond Ballo [:botond]]

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

Comment 1

[Botond Ballo [:botond]]

I believe this can be fixed by calling PostProcessLayers() before the invalid region computation in LayerManagerComposite::UpdateAndRender().

Comment 2

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1248822 - In LayerManagerComposite::UpdateAndRender(), call PostProcessLayers() before LayerTreeInvalidation. r=mattwoodrow

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/

Comment 3

[Botond Ballo [:botond]]

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

Comment 4

[Botond Ballo [:botond]]

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?

Comment 5

[Matt Woodrow (:mattwoodrow)]

It's a real bug, so I think we should land the fix now.

Comment 6

[Botond Ballo [:botond]]

Posted patch for review.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4112e7e78d0b

Comment 7

[Matt Woodrow (:mattwoodrow)]

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

Comment 8

[Botond Ballo [:botond]]

(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

Comment 9

[Botond Ballo [: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

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a02a813a179

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7a02a813a179

Comment 12

[Kartikaya Gupta (email:[e-mail])]

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

Comment 13

[Botond Ballo [: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.

Comment 14

[Botond Ballo [:botond]]

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

Comment 15

[Matt Woodrow (:mattwoodrow)]

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

Comment 16

[Botond Ballo [:botond]]

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.

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 18

[Botond Ballo [:botond]]

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

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c1af4f3c1570

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/980a7149520b