Closed Bug 1254134 Opened 8 years ago Closed 8 years ago

Bad repainting while scrolling on cnn.com (HTML5 videos glitch)

Categories

(Core :: Panning and Zooming, defect, P3)

48 Branch
defect

Tracking

()

RESOLVED WORKSFORME
mozilla48
Tracking Status
firefox44 --- unaffected
firefox45 --- unaffected
firefox46 --- wontfix
firefox47 --- wontfix
firefox48 --- wontfix
firefox49 --- wontfix
firefox50 --- fixed

People

(Reporter: vtamas, Assigned: kats)

References

()

Details

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

Attachments

(9 files)

Attached image GIF.gif
[Affected versions]:
Firefox 47.0a1 (2016-03-06)
Firefox 46.0a2 (2016-03-06)
Firefox 45.0b10 with e10s and APZ enabled

[Affected platforms]:
Windows 10 64-bit
Ubuntu 14.04 32-bit

[Steps to reproduce]:
1.Launch Firefox with clean profile.
2.Navigate to http://edition.cnn.com/2016/01/22/entertainment/adele-hello-1-billion-views-youtube/index.html
3.Play all the videos.
4.Scroll up and down using mouse wheel.

[Expected Results]:
The scrolling is smooth, without any jerkiness or rendering issues.

[Actual Results]:
Repainting issues are visible while scrolling on cnn.com

[Additional notes]:
- The issue doesn't reproduce with APZ pref disabled.
- I am marking Firefox 44 as unaffected since e10s cannot be enabled for this version.
- See the attached video.
Which repainting issues are you referring to? Is it the videos that sometimes have part of the video repeated at the top/bottom?
Shouldn't we call this a regression since it depends on having apz enabled?
Make sense.
Keywords: regression
Whiteboard: [gfx-noted]
Attached image GIF2.gif
(In reply to Markus Stange [:mstange] from comment #1)
> Which repainting issues are you referring to? Is it the videos that
> sometimes have part of the video repeated at the top/bottom?

Yes and I’m also referring to those white areas which appear during scrolling. I tried to capture these blank areas more clearly in another video.
Ok, but the white areas are completely expected. They're the price we pay for more responsive scrolling.
Marking unaffected on 45 since APZ is off by default there. I tried reproducing this on Windows nightly and was unable to. I tried scrolling up and down the page with the mousewheel with all the videos going. It was wild! There's an ad in the top-right that seems to do some scroll-linked effect and resizes over the main content area. I tried different scroll methods and eventually the tab crashed.
Ok I tried it a bunch more and was able to see the repeated painting issue intermittently. Interestingly the videos are playing in the HTML5 video player and not the flash player like I originally thought. Can you confirm that when you see the issue, it happens on a video that is being played in a HTML5 video player? (You can check by right-clicking on the video - if the menu has "About the HTML5 player" then it's an HTML5 player, but if you get "About Adobe Flash Player" it's Flash).
Flags: needinfo?(vasilica.mihasca)
The html5 videos (all from the page except the first one) have repainting issues while scrolling. This is also seen in Vasilica's second gif where the first video wasn't playing.

If scrolling while the first video plays (the flash one), I can reproduce the behavior described by you in comment 6.
Flags: needinfo?(vasilica.mihasca)
Summary: Bad repainting while scrolling on cnn.com → Bad repainting while scrolling on cnn.com (HTML5 videos glitch)
From the two stills I posted (taken from Vasilica's GIF.gif) I think the problem here is that when we run into a checkerboarding area, the video frame still draws completely, and doesn't get cleared. Or something along those lines.

Note how in the first still the glitchy behaviour happens right around where the checkerboarding starts (where the black border on the video and the right sidebar are also cut off). The second still again looks like checkerboarding (the black border on the video is cut off). The "vevo" text in the bottom-right might be an overlay that's not part of the video and therefore is also getting cut off.
I can force this to reproduce by hard-coding a displayport that results in some checkerboarding. It looks like the part of the video that's within the displayport area gets updated as the video is playing, but anything outside the displayport gets left showing whatever it was showing when it was last inside the displayport. Not really sure what the right fix here is.
This is turning out to be more complicated than I thought. The layer tree on this page is quite complex, and even if I force the drawing of the background color on all checkerboarded layers (ignoring the opacity check) it doesn't fix the problem. Somehow the video is drawing stale stuff outside of the displayport after the background is filled in by the checkerboarding code. And only on Linux and Windows, not on OS X.
Updated STR for easy reproduction:
- Apply this patch (Linux or Windows desktop build)
- Set the apz.y_skate_size_multiplier and apz.y_stationary_size_multiplier prefs to 0
- Go to the URL in comment 0 and wait for it to load
- Scroll down to one of the HTML5 videos (I was using "Uptown Funk") so that the top of the video is near the top of the screen
- Start the video, and scroll around a little

What happens is that the displayport starts partway down the screen (you can turn on the APZ minimap to see this) and the part of the video that extends outside the displayport shows rendering artifacts. Turning on apz.highlight_checkerboard_areas may also be useful as it will show you exactly which area is outside the displayport.
Could it be caused by partial composition? You can cause Mac to go down the partial composition path if you disable hardware acceleration and remove the code in nsAppRunner.cpp starting at around line 4733 that disables e10s if hardware acceleration is off.
Yes, I can repro on OS X with those changes. So this appears to be related to partial composition. I'll dig a bit more.
Assignee: nobody → bugmail.mozilla
So I have a patch to fix it, but I have no idea if it's the correct fix or if it will have bad side-effects. Also not really sure if this bug is actually something we want to fix at all. It's sort of an edge case when you have a video bumping up against the edge of the displayport, and as you scroll the partial composition area slides down the image layer. So the sequence of composites each composite a different slice of the video and you get the observed repainting issue. I think part of the problem is that the visible region of the image layer is truncated by the displayport, but the image layer actually contains the full video frame. So if the partial composition area includes the part of the image layer outside the current visible region (from the OldTransformedBounds()) it still draws the image even though we might not be expecting it to.

Anyway, I'll flag Matt for review to decide, I don't have a good mental model of this code at all and am not really sure what to do here.
When image layers are being used to play video, and the user is scrolling,
the image layers may stick outside of the displayport area. In such cases,
we still want to composite the entire image layer/video frame because otherwise
we end up with stale user-visible screen areas from previous compositions that
are showing parts of old video frames.

Review commit: https://reviewboard.mozilla.org/r/41761/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41761/
Attachment #8733398 - Flags: review?(matt.woodrow)
My fix (the second patch above) basically requests that the entire clip region of the image layer get composited, instead of just the visible region, because the visible region is cropped by the displayport and the clip region is not. That ensures the entire video frame is composited every time and we don't get the repainting problem.
Attached file Reduced test case
Here's a reduced test case. It's just a youtube video on a scrollable page, with an extra scrollable div so that the scrollbar is offscreen and doesn't interfere with the partial composition area. you still need attachment 8733017 [details] [diff] [review] and the STR from comment 14 to repro, this page is just a much simpler one than the CNN one.
Also now that I understand the problem I don't think a fix here is worth uplifting to 46, so I'm going to mark it as wontfix.
Attachment #8733397 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8733397 [details]
MozReview Request: Bug 1254134 - Add debugging logs to LayerTreeInvalidation.cpp. r?mattwoodrow

https://reviewboard.mozilla.org/r/41759/#review38337
So is the problem that in the initial state, the video sits only partially within the displayport area, and so layout sets the visible region for the ImageLayer to be the part that sits within the displayport.

Then when we APZ scroll far enough to checkerboard, the compositor is drawing the entire ImageLayer each time, but invalidation is only recording changes for areas within the displayport (non-checkerboard) area?
If that's true, then can we instead overload {New|Old}TransformedBounds for ImageLayerProperties and make the return the bounds that the compositor/BasicLayerManager actually use.

I think that way makes sense, and we can probably fold the mIsMask path in to use the same code, since they both are solving the same problem (the visible region isn't what we actually draw for ImageLayers).
(In reply to Matt Woodrow (:mattwoodrow) from comment #24)
> So is the problem that in the initial state, the video sits only partially
> within the displayport area, and so layout sets the visible region for the
> ImageLayer to be the part that sits within the displayport.
> 
> Then when we APZ scroll far enough to checkerboard, the compositor is
> drawing the entire ImageLayer each time, but invalidation is only recording
> changes for areas within the displayport (non-checkerboard) area?

Yeah, I believe that is what is happening.

(In reply to Matt Woodrow (:mattwoodrow) from comment #25)
> If that's true, then can we instead overload {New|Old}TransformedBounds for
> ImageLayerProperties and make the return the bounds that the
> compositor/BasicLayerManager actually use.

That makes sense. It looks like BasicLayerManager uses the image size [1] and the compositor uses something equivalent [2].

> I think that way makes sense, and we can probably fold the mIsMask path in
> to use the same code, since they both are solving the same problem (the
> visible region isn't what we actually draw for ImageLayers).

That sounds reasonable, I'll implement that and make sure it works.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/basic/BasicImageLayer.cpp?rev=294b89a6fdae#87
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/ImageHost.cpp?rev=29bf03ba2b47#391
Comment on attachment 8733397 [details]
MozReview Request: Bug 1254134 - Add debugging logs to LayerTreeInvalidation.cpp. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41759/diff/1-2/
Comment on attachment 8733398 [details]
MozReview Request: Bug 1254134 - Use the full image size as the bounds for image layers during layer tree invalidation. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41761/diff/1-2/
Attachment #8733398 - Attachment description: MozReview Request: Bug 1254134 - When doing a partial composition, include the entire clip area of image layers. r?mattwoodrow → MozReview Request: Bug 1254134 - Use the full image size as the bounds for image layers during layer tree invalidation. r?mattwoodrow
Comment on attachment 8733398 [details]
MozReview Request: Bug 1254134 - Use the full image size as the bounds for image layers during layer tree invalidation. r?mattwoodrow

https://reviewboard.mozilla.org/r/41761/#review38545
Attachment #8733398 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8733900 [details]
MozReview Request: Bug 1254134 - Remove unused mask flag. r?mattwoodrow

https://reviewboard.mozilla.org/r/41995/#review38547
Attachment #8733900 - Flags: review?(matt.woodrow) → review+
I don't understand why this Werror is firing now and wasn't before - the patch just changes the signature of the function in both the base and derived classes.
Presumably it's complaining that some of the functions in the class are override and others are not. I see "Winconsistent-missing-override" in the error message. I'll build it locally to verify and reland.
Flags: needinfo?(bugmail.mozilla)
Oh I get it - for any given derived class, it wants either all the functions that are overrides to be marked thusly, or none of them (accomodating either coding convention). The patches, in addition to changing a signature, _add_ a new function which is marked as an override, introducing an inconsistency.
(If you're building with clang locally, the reason your local build didn't trigger this Werror may be that it was introduced in a recent clang version. Clang 3.5 doesn't give it, but 3.7 does.)
I built the patch on Linux and Windows, so I didn't see the error. I have a new enough clang on my OS X machine that I saw the error there too once I built it. I added another patch to tack on the override and landed it. I felt that was better than removing the override from the new functions, since moar override is moar better.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #39)
> I built the patch on Linux and Windows, so I didn't see the error.

Off-topic, but clang is easy to setup on Linux (download prebuilt binaries from [1], extract, and add to PATH).

> I added another patch to tack on the override and landed it. I
> felt that was better than removing the override from the new functions,
> since moar override is moar better.

Agreed :)


[1] http://llvm.org/releases/download.html
Depends on: 1259645
Backed out for causing bug 1259645.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3994143ea12
Status: RESOLVED → REOPENED
Flags: in-testsuite?
Resolution: FIXED → ---
Target Milestone: mozilla48 → ---
Depends on: 1259714
Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/a3994143ea12
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
This was backed out, no? If so, why did it get marked fixed again? Reopening to make sure this gets sorted out.

FWIW, it's attachment 8733398 [details] that caused the Youtube invalidation issue.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Because Bugherder does strange things if you're not paying attention sometimes :P. Thanks for reopening.
Because this first "ships" in 48, calling it a 48 regression.
Version: Trunk → 48 Branch
Markus said this might be fixed by bug 1263192. Can you please check to see if it still happens on the latest Nightly?
Flags: needinfo?(vasilica.mihasca)
Scrolling is definitely improved in latest Nightly 50.0a1 2016-06-28 under Win 10 64-bit.
There are still some issues for this page: 
 - The adds on the right side are overlapping with "Top Stories" / "Stories for you section": http://i.imgur.com/IciDO84.png
 - scrolling down while the first video is played puts it on the top right side - this transition is not smooth
 - checkerboarding after scrolling up and down for a few minutes

With the same build, but under Ubuntu 14.04 64-bit, the scrolling seems better, but in this case, the checkerboarding is much worse than on Windows, larger areas are white so it's difficult to verify the effect of the wheel scrolling over videos
Flags: needinfo?(vasilica.mihasca)
Thanks! I think all the issues you pointed out are covered by other bugs or are known issues.

(In reply to Petruta Rasa [QA] [:petruta] from comment #49)
>  - The adds on the right side are overlapping with "Top Stories" / "Stories
> for you section": http://i.imgur.com/IciDO84.png
>  - scrolling down while the first video is played puts it on the top right
> side - this transition is not smooth

These are because of interaction with JS-based positioning and async scrolling. Same category as the scroll-linked effects bugs.

>  - checkerboarding after scrolling up and down for a few minutes
> With the same build, but under Ubuntu 14.04 64-bit, the scrolling seems
> better, but in this case, the checkerboarding is much worse than on Windows,
> larger areas are white so it's difficult to verify the effect of the wheel
> scrolling over videos

The page is pretty expensive to paint so yeah, checkerboarding is kinda expected. We have the meta bug 1256677 to track checkerboarding.

As long as the video overlapping issue doesn't occur still I think we can call this bug fixed.
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.