810302 - Invalidation problems during scrolling after playing video

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

STEPS TO REPRODUCE
1. create a fresh profile, start Firefox and then disable
   "Use smooth scrolling" in Preferences->Advanced
(also, click away any dialogs and other panels that may appear)
2. load https://hacks.mozilla.org/2012/11/firefox-os-video-presentations-and-slides-on-the-os-webapis-hacking-and-writing-apps/
3. scroll down a bit, then start one of the videos
4. click near the end in the video time slider and wait until the
   video stops
5. grab the scrollbar thumb and drag it the end of the page,
   then drag it to the top, then slowly one page down about where
   the video elements are
6. repeat 5 until you see content disappearing.  If it doesn't
   occur, try starting another of the videos and repeat 4,5

ACTUAL RESULTS
Content is not repainted properly, see attached video.
This is 100% reproducible even after restarting Fx.

PLATFORMS AND BUILDS TESTED
Nightly 19.0a1 (2012-11-08), Linux64

Comment 1

[Mats Palmgren (inactive)]

Attachment: screen capture

Comment 2

[Mats Palmgren (inactive)]

Actually, the same STR is also reproducible with smooth scrolling, so it's
not necessary to change that pref.

Comment 3

[Bob Clary [:bc] (inactive)]

Attachment: screen shot of scroll painting issue

I am seeing something similar scrolling divs in the autolog replacement for tbpl. No video involved. This appears to have started with this mornings update on OSX 10.7. I've seen it elsewhere this morning. I'll try to find a public instance.

Comment 4

[Mats Palmgren (inactive)]

Also reproducible after watching the "The HydroICE Solar Project" video at
http://inhabitat.com/hydroice-solar-powered-combustion-engine-could-cut-solar-costs-by-75/
and then scrolling the page to the end, using 20.0a1 (2012-11-27) Linux64.

Comment 5

[Matt Woodrow (:mattwoodrow)]

Alright, I can reproduce this.

Note that it requires hardware acceleration to be disabled, and for you to have opted into the html5 trial on youtube.

Comment 6

[Matt Woodrow (:mattwoodrow)]

Attachment: Avoid singular transforms

Well, this was fun.

Once the video finishes playing, the size of the frame changes to 1x1 (pixels), and we adjust that for the aspect ratio to 1x0. We then use that to build a singular transform, which triggers cairo errors during painting and is the reason for us not painting half the page.

This patch fixes the issue in two places, and adds an assertion so that it's easier to catch.

Comment 7

[Matt Woodrow (:mattwoodrow)]

There's a few other issues on this page too.

1) HTML5 video is causing component alpha flattening on BasicLayers - I assume something in the controls has text without opaque content behind it. This is pretty bad for video performance, but is easily fixed (use LAYER_ACTIVE_FORCE for the video layer) if losing subpixel AA for the controls text is acceptable.

2) Scrolling this page after it's started flattening component alpha repaints the whole page. This is probably the cause of other poor scrolling performance on BasicLayers bugs, as soon as anything triggers this, then scrolling will be slow.

We should only need to repaint the entire page if the component alpha flattening is being triggered by a fixed position background.

When we have a flattened container, we set the active scrolled root of all display items within it to be the containers reference frame, and all scrolling is detected (correctly) as a shift relative to the scrolled root, which invalidates.

There seems to be two options here, set the active scrolled root to be a frame that is above the actual scroll point (what we currently do), or set the active scrolled root to be the primary scroll frame for the page.

The former means that on pages with a fixed position background, we only invalidate the shifted content when we scroll, which is presumably considerably less than the entire page. However, on pages without a fixed background then we're invalidating the entire page.

The latter would result in no unnecessary invalidation when scrolling a page without a fixed background, but would invalidate more when we do have one.

I'd like to find a way to decide how much of the visible area is covered in fixed content, and use that to decide which of the two approaches will perform better for the current page.

Comment 8

[Matt Woodrow (:mattwoodrow)]

I *think* that when we first decide to flatten layers, we can inspect the layer tree built, make the decision, and store that information for further paints.

I think the important question to answer is 'which scrolled root would be the best choice for all child layers to minimize the amount of painting during scrolling?'.

Need to figure out what heuristics to use to answer that.

Comment 9

[Matt Woodrow (:mattwoodrow)]

Attachment: Use the background to decide the active scrolled root

This is a fairly simple approach for fixing problem 2.

It just takes the back-most display item for the flattened layer, and uses that to decide what active scrolled root to use.

Little bit worried about regressions with a change like this.

Comment 10

[Matt Woodrow (:mattwoodrow)]

Attachment: Force videos to always have active layers

Youtubes video controls are definitely transparent (rgba 0,0,0,204) which is forcing the video to be inactive and ruining performance.

I think we should favour video playback performance over having subpixel AA in text on top of the video.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 686419 [details] [diff] [review]
Use the background to decide the active scrolled root

Review of attachment 686419 [details] [diff] [review]:
-----------------------------------------------------------------

can this lead to the active scrolled root for an item not being an ancestor of the item's frame?

Comment 12

[Matt Woodrow (:mattwoodrow)]

That seems unlikely, but possible, yes.

Comment 13

[Matt Woodrow (:mattwoodrow)]

I'm not sure how we'd prevent that, maybe an extra iteration over the display items?

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes, maybe if we have to flatten then we should traverse the display items in a separate pass to determine what the correct activeScrolledRoot should be.

Comment 15

[Matt Woodrow (:mattwoodrow)]

Pushed the two reviewed patches:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a785d81d314
https://hg.mozilla.org/integration/mozilla-inbound/rev/08d3516faa3a

Comment 16

[Matt Woodrow (:mattwoodrow)]

Attachment: Use the background to decide the active scrolled root v2

https://tbpl.mozilla.org/?tree=Try&rev=7a76c534a030

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1a785d81d314
https://hg.mozilla.org/mozilla-central/rev/08d3516faa3a

Comment 18

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/54f28b6c51b5

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/54f28b6c51b5

Comment 20

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 686349 [details] [diff] [review]
Avoid singular transforms

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Unsure exactly, bug has existed for a long time, but was made worse/exposed by bug 741682.
User impact if declined: Possible missing rendering on pages with videos and no hardware acceleration.
Testing completed (on m-c, etc.): Been on m-c for a week.
Risk to taking this patch (and alternatives if risky): Very low risk, just stops us trying to draw 0 sized videos.
String or UUID changes made by this patch: None.

Comment 21

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 686420 [details] [diff] [review]
Force videos to always have active layers

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 741682
User impact if declined: Regressed youtube html5 video playback performance without hardware acceleration.
Testing completed (on m-c, etc.): Been on m-c for a week.
Risk to taking this patch (and alternatives if risky): Low risk, this takes us back closer to the original behaviour of having the video in it's own layer.
String or UUID changes made by this patch: None

Comment 22

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 686938 [details] [diff] [review]
Use the background to decide the active scrolled root v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 741682
User impact if declined: Reduced scrolling performance on pages that have text on top of a transparent layer. HTML5 youtube videos trigger this, and this is likely the cause of bugs 781414, and 781414.
Testing completed (on m-c, etc.): Been on m-c for a week without issues.
Risk to taking this patch (and alternatives if risky): Higher risk than the other two, but shouldn't be too bad. I would expect to have seen regressions from m-c by now if there were issues. Definitely high impact, I think it's worth taking this.
String or UUID changes made by this patch: None

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 686349 [details] [diff] [review]
Avoid singular transforms

Willing to take this high-impact fix since it's still early enough in 18 betas that we can make sure to get testing pre-release.  Is there anything QA can do to verify this is fixed and doesn't cause regressions?

Comment 24

[Ioana (away)]

I don't see this issue on the 12/07 build on Ubuntu 12.04 64bit, but I can't reproduce it on the 11/09 build either.

Mats, or anyone else that can reproduce the initial issue, please try to verify the fix on the latest Nightly.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5a273aa9e1bf
https://hg.mozilla.org/releases/mozilla-aurora/rev/735426e182ac
https://hg.mozilla.org/releases/mozilla-aurora/rev/b64693f63118

https://hg.mozilla.org/releases/mozilla-beta/rev/0634db16d368
https://hg.mozilla.org/releases/mozilla-beta/rev/d750d4c9c4b0
https://hg.mozilla.org/releases/mozilla-beta/rev/3f6d8b336338

Comment 26

[Mihai Morar, (:MihaiMorar)]

Could not reproduce this issue following STR from Comment 0 and doing some exploratory in plus.

18b03
Mozilla/5.0 (X11; Linux i686; rv:18.0) Gecko/20100101 Firefox/18.0 (20121205060959)

Latest Nightly
Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20121209 Firefox/20.0 (20121209030817)

Based on this and on Comment 25 I consider this issue verified for ff18. If someone can still reproduce this issue please move back status for ff18 as fixed.

Comment 27

[Ioana (away)]

Considering comment 24, are there any reliable steps for reproducing this bug?

Comment 28

[Ioana (away)]

(In reply to MarioMi from comment #26)
> Could not reproduce this issue following STR from Comment 0 and doing some
> exploratory in plus.
> 
> 18b03
> Mozilla/5.0 (X11; Linux i686; rv:18.0) Gecko/20100101 Firefox/18.0
> (20121205060959)
> 
> Latest Nightly
> Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20121209 Firefox/20.0
> (20121209030817)
> 
> Based on this and on Comment 25 I consider this issue verified for ff18. If
> someone can still reproduce this issue please move back status for ff18 as
> fixed.

MarioMi, can you reproduce this issue on the build it was filed on? If so, please verify it on all the versions it's tracked on.

Comment 29

[Mihai Morar, (:MihaiMorar)]

Ioana I was not able to reproduce this issue on Mozilla/5.0 (X11; Linux i686; rv:19.0) Gecko/19.0 Firefox/19.0 (20121109030635). I tryed following STR from Comment 0 for all videos on that page but with no result.

Comment 30

[Ioana (away)]

Thanks for the reply, MarioMi. Please only mark the bug back as verified after performing the following steps:
1. reproduce the issue on a build without the fix;
2. verify the fix in the same environment as at step 1, but with a build with the fix.

Comment 31

[Mats Palmgren (inactive)]

Note also the additional requirements to reproduce the problem in comment 5.