Bug 46142 – Built-in HTML5 <audio> (and sometimes <video>) UI doesn't update playhead location or time displays

• Component: Media
• Status: RESOLVED FIXED

[Ridley Combs]

Well, the description pretty much says it all, but here's some specifics:

It seems to happen in every case with <audio> (including pointing a tab directly to a .mp3) [at least, I haven't found a case where it doesn't]
It seems to come and go with <video>, more common when you browse to the video file directly. I can't find a pattern.

[Jer Noble]

<rdar://problem/8452827>

[Jer Noble]

Created attachment 78725 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 78725 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=78725&action=review

> Source/WebCore/rendering/RenderMedia.cpp:162
> +        LayoutStateMaintainer statePusher(view(), this, IntSize(x(), y()), style()->isFlippedBlocksWritingMode());

I think you may need to check hasTransform() || hasReflection() also.

It should be possible to make a repaint test that tests this.

> Source/WebCore/rendering/RenderMedia.cpp:175
> +        setChildNeedsLayout(false);        

You have extra whitespace at the end of this line.

[Jer Noble]

Created attachment 78740 [details]
Patch

[Jer Noble]

Created attachment 78753 [details]
Patch

[Jer Noble]

My local tree is out of date, enough so that these patches can't be applied clearly.  I'll rebase and reupload.

[WebKit Review Bot]

Attachment 78753 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7565001

[Jer Noble]

Created attachment 78838 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 78838 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=78838&action=review

r- to get a correct repaint test.

> LayoutTests/media/media-document-audio-rendering.html:17
> +    function frameLoaded()
> +    {
> +        setTimeout(function() {             
> +            document.body.offsetLeft;
> +            if (window.layoutTestController)
> +                layoutTestController.notifyDone();
> +        }, 1500);

We have infrastructure for repaint tests, see fast/resources/repaint.js

Is there a way to avoid making this test take 1.5 seconds?

> Source/WebCore/rendering/RenderMedia.cpp:163
> +        LayoutStateMaintainer statePusher(view(), this, IntSize(x(), y()), hasTransform() || hasReflection() || style()->isFlippedBlocksWritingMode());
> +

So this also may change behavior of an in-page audio element, particularly when transformed or reflected. You may also want to add a repaint test for that case too.

[Jer Noble]

(In reply to comment #9)
> (From update of attachment 78838 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78838&action=review
> 
> r- to get a correct repaint test.
> 
> > LayoutTests/media/media-document-audio-rendering.html:17
> > +    function frameLoaded()
> > +    {
> > +        setTimeout(function() {             
> > +            document.body.offsetLeft;
> > +            if (window.layoutTestController)
> > +                layoutTestController.notifyDone();
> > +        }, 1500);
> 
> We have infrastructure for repaint tests, see fast/resources/repaint.js

Sure.

> Is there a way to avoid making this test take 1.5 seconds?

The error state won't manifest for at least 1 second (the amount of time it would take the time field transition from 0:00s to 0:01s).  Waiting 1.5 seconds guarantees a paint should have occurred after the transition.  

> > Source/WebCore/rendering/RenderMedia.cpp:163
> > +        LayoutStateMaintainer statePusher(view(), this, IntSize(x(), y()), hasTransform() || hasReflection() || style()->isFlippedBlocksWritingMode());
> > +
> 
> So this also may change behavior of an in-page audio element, particularly when transformed or reflected. You may also want to add a repaint test for that case too.

Actually, no, this change won't affect in-page audio; in-page media element trigger accelerated rendering, which in turn doesn't use the LayoutStateManitainer's paintOffset() function.  But I can add another test regardless.  It will also have a 1.5s delay.

[Jer Noble]

Created attachment 78973 [details]
Patch

repaint.js was not useful for these tests because the layoutTestController needs to wait until notifyDone() is called; other tests in LayoutTests/fast/repaint/ which call waitUntilDone() do not use repaint.js.  But I reproduced the logic contained both in repaint.js and in other repaint tests (such as layout-state-scrolloffset.html) within these new repaint tests.  Also, to address Simon's comment regarding in-page media elements, a new repaint test for in-page audio elements was added.

[Darin Adler]

Comment on attachment 78973 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=78973&action=review

I’m not going to say review+ because I don’t think I’m enough of an expert on how to write layout functions for renderers. I’m not going to say review- because I don’t think my comments are review-blockers.

> Source/WebCore/ChangeLog:9
> +        Push a LayoutStateMaintainer in RenderMedia::layout() before calling layout() on the
> +        container elements; otherwise, the results of computeRectForRepaint() will be incorrect.

This is a mysterious comment. I see no call to computeRectForRepaint in this function? What exactly creates the need for LayoutStateMaintainer?

> Source/WebCore/rendering/RenderMedia.cpp:160
>      if (newSize != oldSize || controlsRenderer->needsLayout()) {

Can we use early return here instead of nesting the code?

> Source/WebCore/rendering/RenderMedia.cpp:163
> +        LayoutStateMaintainer statePusher(view(), this, IntSize(x(), y()), hasTransform() || hasReflection() || style()->isFlippedBlocksWritingMode());
> +

Can this go after the seVisible calls? I’d like to see this wrapped more tightly around the exact code that needs it rather than before the large block.

Can we add a why comment here? I assume this is needed because we are doing our own layout that is different from the usual layout system; that requires a comment.

[Jer Noble]

(In reply to comment #12)
> (From update of attachment 78973 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78973&action=review
> 
> I’m not going to say review+ because I don’t think I’m enough of an expert on how to write layout functions for renderers. I’m not going to say review- because I don’t think my comments are review-blockers.
> 
> > Source/WebCore/ChangeLog:9
> > +        Push a LayoutStateMaintainer in RenderMedia::layout() before calling layout() on the
> > +        container elements; otherwise, the results of computeRectForRepaint() will be incorrect.
> 
> This is a mysterious comment. I see no call to computeRectForRepaint in this function? What exactly creates the need for LayoutStateMaintainer?

You're right, that's totally unclear.  I'll update the comment, but basically: during controlsRenderer->layout(), RenderBox:computeRectForRepaint() is called.  This function tries to shift the repaint rect by the current LayoutState m_paintOffset, which is currently set to (0,0).  So it tells the window it needs to repaint from (0,0) to (controlsRender->height(), controlsRenderer->width()), which is at the top left corner of the window.  We need to push a new LayoutState so that there is a correct value stored in view()->layoutState()->m_paintOffset.

> > Source/WebCore/rendering/RenderMedia.cpp:160
> >      if (newSize != oldSize || controlsRenderer->needsLayout()) {
> 
> Can we use early return here instead of nesting the code?

Sure could!

> > Source/WebCore/rendering/RenderMedia.cpp:163
> > +        LayoutStateMaintainer statePusher(view(), this, IntSize(x(), y()), hasTransform() || hasReflection() || style()->isFlippedBlocksWritingMode());
> > +
> 
> Can this go after the seVisible calls? I’d like to see this wrapped more tightly around the exact code that needs it rather than before the large block.

There's a possibility that updateTImeDisplayVisibility() will cause a call to repaint() (by virtue of changing the style of one of the RenderTexts that serve as the time remaining and elapsed values).  And we definitely want the correct LayoutState pushed on the stack at that point.

> Can we add a why comment here? I assume this is needed because we are doing our own layout that is different from the usual layout system; that requires a comment.

Sure thing.  Even more basically, we're asking the RenderMedia to behave like a RenderBox or a RenderBlock, both of which push a LayoutState onto the stack before laying out their children.  I'll add a comment to that effect.

[Jer Noble]

Created attachment 80701 [details]
Patch

[WebKit Review Bot]

Attachment 80701 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7685411

[Darin Adler]

Comment on attachment 80701 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80701&action=review

> Source/WebCore/rendering/RenderMedia.cpp:88
> +    // Make sure to push a new LayoutState onto the LayoutState stack, so that subsequent queries of 
> +    // view()->layoutState() return valid values for m_paintOffset and m_layoutOffset.  Otherwise
> +    // repainting operations will happen at incorrect coordinates.

I know I asked for this, but I am still confused by the comment. It talks about the mechanism, view()->layoutState(), but none of the code below explicitly accesses view()->layoutState().

When exactly does one need to use a LayoutStateMaintainer? Why did we need it here?

[Collabora GTK+ EWS bot]

Attachment 80701 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7684450

[WebKit Review Bot]

Attachment 80701 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7684494

[Jer Noble]

(In reply to comment #18)
> Attachment 80701 [details] did not build on chromium:
> Build output: http://queues.webkit.org/results/7684494

Whoops, looks like a merge took out a necessary header.  Canceling r? till I get a new patch up.

[Jer Noble]

(In reply to comment #16)
> (From update of attachment 80701 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80701&action=review
> 
> > Source/WebCore/rendering/RenderMedia.cpp:88
> > +    // Make sure to push a new LayoutState onto the LayoutState stack, so that subsequent queries of 
> > +    // view()->layoutState() return valid values for m_paintOffset and m_layoutOffset.  Otherwise
> > +    // repainting operations will happen at incorrect coordinates.
> 
> I know I asked for this, but I am still confused by the comment. It talks about the mechanism, view()->layoutState(), but none of the code below explicitly accesses view()->layoutState().

Well, no, none of it does directly.  But it's called from a variety of places, which are in turn called by RenderBox::layout().  For example, any time RenderObject::repaintRectangle() is called from inside controlsRenderer->layout() (and it will get called for both time displays, the timeline scrubber, the play button, etc.), the RenderObject will call its parent's computeRectForRepaint(), which calls it's parent's, etc.  And computeRectForRepaint() will ask its view for the current layoutState(), and will ask it in turn for the current painting offset.

And if the RenderMedia hasn't pushed its own LayoutState onto the stack, it's offsets will get "skipped" in the calculations.

> When exactly does one need to use a LayoutStateMaintainer? 

Doesn't that seem like documentation which belongs alongside LayoutStateMaintainer?  Or LayoutState? Believe me, I looked.  There is no such thing.  If there's a failure in documentation, I would argue that it's in the LayoutState or LayoutStateMaintainer level, and not in this function.

From code inspection, it appears one needs to use a LayoutStateMaintainer whenever one calls layout() on a child RenderObject().

> Why did we need it here?

Because child renderers need to know where within the view they will be drawing.  By not pushing the current LayoutState on the stack, we were telling them to draw in the upper left corner of the view.

[Jer Noble]

Created attachment 80717 [details]
Patch

[WebKit Review Bot]

Attachment 80701 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7681643

[Darin Adler]

Comment on attachment 80717 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80717&action=review

> Source/WebCore/rendering/RenderMedia.cpp:92
> +    // Make sure to push a new LayoutState onto the LayoutState stack, so that subsequent queries of 
> +    // view()->layoutState() return valid values for m_paintOffset and m_layoutOffset.  These values
> +    // are queried by our children, from within RenderBox::computeRectForRepaint(), which is called 
> +    // in turn by RenderObject::repaintRectangle(), which is called in turn by controlsRenderer->layout().
> +    // If we do not push our LayoutState onto the stack, repainting operations be attempted at incorrect 
> +    // coordinates.

Aggh! I have pushed you into writing a bad comment! I suggest this comment:

    // We are calling the layout function directly on controlsRenderer, so need to set up state with LayoutStateMaintainer.

Or something along those lines. I still don’t understand exactly under what circumstances use of LayoutStateMaintainer is needed, so I’m not entirely satisfied, but you must not let my feedback cause you to check in a giant comment!

> Source/WebCore/rendering/RenderMedia.cpp:103
> +    statePusher.pop();

I suggest a blank line before this to match the declaration above.

[Jer Noble]

(In reply to comment #23)
> (From update of attachment 80717 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80717&action=review
> 
> > Source/WebCore/rendering/RenderMedia.cpp:92
> > +    // Make sure to push a new LayoutState onto the LayoutState stack, so that subsequent queries of 
> > +    // view()->layoutState() return valid values for m_paintOffset and m_layoutOffset.  These values
> > +    // are queried by our children, from within RenderBox::computeRectForRepaint(), which is called 
> > +    // in turn by RenderObject::repaintRectangle(), which is called in turn by controlsRenderer->layout().
> > +    // If we do not push our LayoutState onto the stack, repainting operations be attempted at incorrect 
> > +    // coordinates.
> 
> Aggh! I have pushed you into writing a bad comment! I suggest this comment:
> 
>     // We are calling the layout function directly on controlsRenderer, so need to set up state with LayoutStateMaintainer.
> 
> Or something along those lines. I still don’t understand exactly under what circumstances use of LayoutStateMaintainer is needed, so I’m not entirely satisfied, but you must not let my feedback cause you to check in a giant comment!

LLOL. (Literal laugh out loud.)  Perhaps the LayoutState & LayoutStateMaintainer classes deserve a mention inside <http://trac.webkit.org/wiki/LayoutAndRendering>, and this comment can simply refer to that page?

> > Source/WebCore/rendering/RenderMedia.cpp:103
> > +    statePusher.pop();
> 
> I suggest a blank line before this to match the declaration above.

Sure thing!

[Jer Noble]

After talking to Hyatt, the answer to Darin's question ("When exactly does one need to use a LayoutStateMaintainer?") is the following:

When calling layout() on child nodes, the parent needs to either push a LayoutState via a LayoutStateMaintainer, or call view->disableLayoutState().  Pushing a LayoutState is slightly more efficient.

[Dimitri Glazkov (Google)]

Comment on attachment 80717 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80717&action=review

BTW, I am pretty much completely rebuilding this now.

> LayoutTests/platform/mac/media/audio-repaint-expected.checksum:1
> +91d3f86ac809c3d94411a9f35cf2d0f5

The pixel baseline here looks wrong.

[Dimitri Glazkov (Google)]

(In reply to comment #26)
> (From update of attachment 80717 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80717&action=review
> 
> BTW, I am pretty much completely rebuilding this now.

The LayoutStateMaintainer code is still good though! :)

> 
> > LayoutTests/platform/mac/media/audio-repaint-expected.checksum:1
> > +91d3f86ac809c3d94411a9f35cf2d0f5
> 
> The pixel baseline here looks wrong.

[Jer Noble]

(In reply to comment #26)
> (From update of attachment 80717 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80717&action=review
> 
> BTW, I am pretty much completely rebuilding this now.

Would you like me to hold off checking this in?

[Dimitri Glazkov (Google)]

(In reply to comment #28)
> (In reply to comment #26)
> > (From update of attachment 80717 [details] [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=80717&action=review
> > 
> > BTW, I am pretty much completely rebuilding this now.
> 
> Would you like me to hold off checking this in?

No, not because of my refactoring -- don't want to block you on that.

But are you worried that the baseline looks wrong?

[Dimitri Glazkov (Google)]

BTW, I posted the refactoring-in-progress I mentioned here: https://bugs.webkit.org/show_bug.cgi?id=53020

[Jer Noble]

Committed r79318: <http://trac.webkit.org/changeset/79318>

[Jer Noble]

Looks like the new tests are flakey. (The slider position, and thus the DRT output, is slightly different from test to test.)

[WebKit Review Bot]

http://trac.webkit.org/changeset/79318 might have broken SnowLeopard Intel Release (Tests)
The following tests are not passing:
media/audio-repaint.html

[Jer Noble]

Filed https://bugs.webkit.org/show_bug.cgi?id=54970 to track media test flakiness.

[Eric Seidel (no email)]

This test seems broken on most (all?) bots.

[Eric Carlson]

(In reply to comment #35)
> This test seems broken on most (all?) bots.

https://bugs.webkit.org/show_bug.cgi?id=54970