Closed Bug 511163 Opened 11 years ago Closed 8 years ago

We repaint the whole video while controls are visible and video is playing

Categories

(Toolkit :: Video/Audio Controls, defect)

x86
Windows Vista
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: pavlov, Unassigned)

References

Details

Attachments

(1 file)

The whole page, like, scroll down and take the video off screen and we repaint you.  This is pretty easy to see in Fennec with paint flashing on, but should also be easy to see in Firefox with platform-specific paint flashing on.  This isn't helping performance.
Component: Video/Audio → Video/Audio Controls
Product: Core → Toolkit
QA Contact: video.audio → video.audio
Stuart mentioned that this might have been a case of the video inside a table.

It seems like the container for the videocontrols should probably be a reflow root.

What are the possible children of a video frame?  It seems like we'd want either the video frame or its child that holds the controls to be a reflow root.
I think in theory we should just make video frames be reflow roots.  The question is whether that might cause problems like bug 366791 or bug 430887; I probably need to go through existing reflow root logic a bit.
I've tested this out and I can only reproduce the invalidation within the <video> bounding rectangle while the controls are visible.

To reproduce, I enabled the nglayout.debug.paint_flashing preference and viewed the video at http://clips.vorwaerts-gmbh.de/big_buck_bunny.webm.
This attachment narrows down the cause of our invalidations. When the <label class="timeLabel"/> has a value attribute set, the video bounding rectangle will invalidate nonstop while the controls are visible. If the |.timeLabel| is set to |visibility:collapse;| or |display:none;| then these unnecessary invalidations disappear.

To see this, you can apply the patch and rebuild, and enable the nglayout.debug.paint_flashing and play a webm video with the controls enabled. This should only show the control bar invalidating. Then set a value attribute or a textContent on the <label class="timeLabel"/> and the invalidations will come back.

David: Could this be an issue with the layout here?
Attachment #590092 - Flags: feedback?(dbaron)
Summary: We repaint the whole page while controls are visible and video is playing → We repaint the whole video while controls are visible and video is playing
I think making video frames or the controls be reflow roots has already been tested to fix the invalidation -- I think somebody else may have filed a second bug on this as well.

The question is whether it's safe to do that -- and I think it isn't quite, but might be if we had a variant of reflow root that did overflow propagation (which we could do now).
Attachment #590092 - Flags: feedback?(dbaron) → feedback-
Blocks: 766811
Appears to be fixed by DLBI.
Depends on: dlbi
Confirmed fixed :)
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
No longer blocks: 722261

(In reply to David Baron :dbaron: 🇨🇦 ⌚UTC-4 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it) from comment #5)

I think making video frames or the controls be reflow roots has already been
tested to fix the invalidation -- I think somebody else may have filed a
second bug on this as well.

We've at least got bug 1472607, if there isn't another older bug on this.

The question is whether it's safe to do that -- and I think it isn't quite,
but might be if we had a variant of reflow root that did overflow
propagation (which we could do now).

I suspect this concern has been addressed, now that we've taken your patch [1] that allows reflow roots to have overflow -- right?

[1] https://hg.mozilla.org/integration/autoland/rev/29d8ae277caa (Bug 1159042 p2)

Flags: needinfo?(dbaron)
See Also: → 1472607

Maybe... what causes scrollbars to update in that codepath if the change in overflow reaches a scrollable frame?

Flags: needinfo?(dbaron)

So, suppose we reflow inside some target frame which is a reflow root and is not the actual root of the tree; and suppose its scrollable overflow area changes during that reflow. (e.g. due to some child getting larger and overflowing)

Here's what happens, to ensure that scrollbars on ancestors get updated appropriately:
(0) ProcessReflowCommands calls DoReflow for this target frame (the reflow root in question), and passes in a temporary OverflowChangedTracker for it to use:
https://searchfox.org/mozilla-central/rev/6dab6dad9cc852011a14275a8b2c2c03ed7600a7/layout/base/PresShell.cpp#9298

(1) DoReflow saves a copy of the target's overflow areas before it actually does the reflow. And then after it's finished reflowing, it checks whether the target's overflow areas changed. If they did change, it tells the caller's OverflowChangedTracker about it:
https://searchfox.org/mozilla-central/rev/6dab6dad9cc852011a14275a8b2c2c03ed7600a7/layout/base/PresShell.cpp#9169-9172

(2) We pop back up to ProcessReflowCommands, which flushes the temporary OverflowChangedTracker:
https://searchfox.org/mozilla-central/rev/6dab6dad9cc852011a14275a8b2c2c03ed7600a7/layout/base/PresShell.cpp#9307
...which causes us to propagate the overflow change upwards:
https://searchfox.org/mozilla-central/rev/6dab6dad9cc852011a14275a8b2c2c03ed7600a7/layout/base/OverflowChangedTracker.h#137-140,163-164

(3) During that upward propagation: when we reach the scrollable frame, the scrollable frame calls nsIPresShell::FrameNeedsReflow for itself, via this backtrace:

#0  0x00007fb29710dba8 in nsIPresShell::FrameNeedsReflow(...)
    at $SRC/layout/base/PresShell.cpp:2566
#1  0x00007fb29733d8ff in mozilla::ScrollFrameHelper::ComputeCustomOverflow(...)
    at $SRC/layout/generic/nsGfxScrollFrame.cpp:5899
#2  0x00007fb29739d826 in nsHTMLScrollFrame::ComputeCustomOverflow(...)
     at ../../../mozilla/layout/generic/nsGfxScrollFrame.h:852
#3  0x00007fb2972f69bc in nsIFrame::UpdateOverflow()
    at $SRC/layout/generic/nsFrame.cpp:7366
#4  0x00007fb297152631 in mozilla::OverflowChangedTracker::Flush()
    at ../../../mozilla/layout/base/OverflowChangedTracker.h:110
#5  0x00007fb297119efa in nsIPresShell::ProcessReflowCommands(bool)
    at $SRC/layout/base/PresShell.cpp:9331

This results in mDirtyRoots getting repopulated with a reflow root that is an ancestor of the scrollframe (e.g. the actual viewport, or some other reflow root that is further towards the root of the frame tree than target is).

(4) Once OverflowChangedTracker::Flush completes, we pop back up to where we'd left off near the end of ProcessReflowCommands. We discover that our recent FrameNeedsReflow call has given us a dirty reflow root, so we schedule a reflow to handle that:
https://searchfox.org/mozilla-central/rev/6dab6dad9cc852011a14275a8b2c2c03ed7600a7/layout/base/PresShell.cpp#9325-9333

(5) On the next layout flush (the next refresh driver tick, in my case), we handle that queued reflow, which reflows the scrollable frame (the thing we called FrameNeedsReflow on), which causes its scrollbars to be updated.

Also: if a web page does something like...

    // Cause overflow on a descendant of a reflow root:
    inner.style.height = "200px";
    // Inspect the scrollHeight of some ancestor of that reflow root
    // (which is our first opportunity to react to the layout change):
    dump("scrollHeight: " + scrollable.scrollHeight + "\n");

...we still report the right result (e.g. 200 in this case), even though we aren't guaranteed to have reflowed the scrollable frame yet when we get to the end of this JS!

This surprised me -- I was expecting that .scrollHeight would just flush the first reflow described in comment 10 [this expectation is correct], and I thought we might report the wrong value because we woudln't have reached the later reflow for the scrollframe yet (the reflow labeled as (5) in comment 10).

But we do in fact report the right result because the .scrollHeight implementation works by querying the overflow areas (it uses nsIScrollableFrame::GetScrollRange which ends up calling nsIFrame::GetOverflowRect, a few stack levels down) -- and we do update the scrollframe's overflow areas synchronously as part of the first reflow (in step (3) from comment 10 -- not in the later reflow in step (5)).

Thanks for checking that -- step (3) -- and in particular the fact that part of UpdateOverflow is now refactored into ComputeCustomOverflow -- was the piece that I was missing. So it sounds like things should be good now.

You need to log in before you can comment on or make changes to this bug.