Bug 86769 – mediaControls()->playbackStarted() may be called more than once

• Component: Media
• Status: UNCONFIRMED

[Max Feil]

This patch fixes a problem where the fullscreen HTML5 video controls do not fade out after the desired 3 second timeout. Calls to HTMLMediaElement::mediaPlayerTimeChanged() happen at least once per second while playing, and each of these calls ends up in HTMLMediaElement::updatePlayState() where MediaControlRootElement::playbackStarted() gets called every time.  This keeps repeatedly restarting the 3 second timer and it never has a chance to expire.

The playbackStarted() call should be moved so it happens only on the play transition.

[Max Feil]

Created attachment 142535 [details]
Patch

[Jer Noble]

Comment on attachment 142535 [details]
Patch

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

> Source/WebCore/ChangeLog:19
> +        No new tests because the visibility of the controls is not
> +        available from Javascript.

The visibility of the controls /is/ available from JavaScript.  Look at the test in LayoutTests/fullscreen/video-controls-override.html.  And a test is needed here.

> Source/WebCore/html/HTMLMediaElement.cpp:-3596
> +            if (hasMediaControls())
> +                mediaControls()->playbackStarted();
>          }
>  
> -        if (hasMediaControls())
> -            mediaControls()->playbackStarted();

Why does this change only affect full-screen mode?

[Jer Noble]

(In reply to comment #2)
> (From update of attachment 142535 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142535&action=review
> 
> Why does this change only affect full-screen mode?

Ahh, to answer my own question, because the hideFullscreenControlsTimer is being reset every time the time updates.

[Max Feil]

(In reply to comment #2)
> The visibility of the controls /is/ available from JavaScript.  Look at the test in LayoutTests/fullscreen/video-controls-override.html.  And a test is needed here.

Thanks for that information. I'll work on a test...

[Jer Noble]

Apart from the lack of a test, this looks like a good change.  But has it been broken since r81283 <http://trac.webkit.org/changeset/81283>?

[Jer Noble]

(In reply to comment #5)
> Apart from the lack of a test, this looks like a good change.  But has it been broken since r81283 <http://trac.webkit.org/changeset/81283>?

Ah, to answer my own question again: this is probably MediaPlayerPrivate-specific behavior.  Some platform media engines (namely, QTKit and AVFoundation) only emit a mediaPlayerTimeChanged callback during discontinuities (I.e. scrubbing).  So on those platforms, this behavior will not occur.  On what platform are you seeing this behavior?

[Max Feil]

(In reply to comment #6)
> Ah, to answer my own question again: this is probably MediaPlayerPrivate-specific behavior.  Some platform media engines (namely, QTKit and AVFoundation) only emit a mediaPlayerTimeChanged callback during discontinuities (I.e. scrubbing).  So on those platforms, this behavior will not occur.  On what platform are you seeing this behavior?

This is on BlackBerry. We need to send frequent time updates to make sure that the currentTimeDisplay text and timeRemainingDisplay text stay up to date.

[Jer Noble]

(In reply to comment #7)
> (In reply to comment #6)
> > Ah, to answer my own question again: this is probably MediaPlayerPrivate-specific behavior.  Some platform media engines (namely, QTKit and AVFoundation) only emit a mediaPlayerTimeChanged callback during discontinuities (I.e. scrubbing).  So on those platforms, this behavior will not occur.  On what platform are you seeing this behavior?
> 
> This is on BlackBerry. We need to send frequent time updates to make sure that the currentTimeDisplay text and timeRemainingDisplay text stay up to date.

That should be handled independent of the MediaPlayerPrivate by HTMLMediaElement::playbackProgressTimerFired(Timer<HTMLMediaElement>*) and thus by mediaControls()->playbackProgressed().

This is a good fix, but I think you may be fixing the symptom of a larger issue.  And perhaps we should make explicit the expectation that mediaPlayerTimeChanged() is only called during scrubbing / seeking.

[Max Feil]

(In reply to comment #8)
> This is a good fix, but I think you may be fixing the symptom of a larger issue.  And perhaps we should make explicit the expectation that mediaPlayerTimeChanged() is only called during scrubbing / seeking.

Ah, yes. I see what you mean. I was not aware until now that mediaPlayerTimeChanged() needs to be called to indicate the end of seeking. I made sure this happens now in the BlackBerry platform media player code, and the playbackProgressTimer works reliably.

I've stopped calling mediaPlayerTimeChanged() unnecessarily, and the patch attached to this PR is no longer needed. Or at least not needed to fix any perceptible problem. I'll have a hard time coming up with an automated test now ;)

[Jer Noble]

(In reply to comment #9)
> I've stopped calling mediaPlayerTimeChanged() unnecessarily, and the patch attached to this PR is no longer needed. Or at least not needed to fix any perceptible problem. I'll have a hard time coming up with an automated test now ;)

Oh, I can imagine that a script that did:

video.play()
setTimeout(function() { video.currentTime = video.currentTime - 1.0; }, 1000);

Would make the full screen controls reappear after seeking.  But you're right, it's less of an important bug now. :)

[Rob Buis]

Comment on attachment 142535 [details]
Patch

r- based on comment #9.

[Max Feil]

(In reply to comment #11)
> (From update of attachment 142535 [details])
> r- based on comment #9.

Rob, can you give more details on why you gave my patch an r-? It's my understanding that the only outstanding item is a test. If you have a problem with the code diff itself, please explain and I will withdraw this bug.

[Rob Buis]

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 142535 [details] [details])
> > r- based on comment #9.
> 
> Rob, can you give more details on why you gave my patch an r-? It's my understanding that the only outstanding item is a test. If you have a problem with the code diff itself, please explain and I will withdraw this bug.

"I've stopped calling mediaPlayerTimeChanged() unnecessarily, and the patch attached to this PR is no longer needed."

[Max Feil]

(In reply to comment #13):

The fix is not needed to address the original synopsis of this bug: "HTML5 video controls are not automatically fading out when fullscreen". That is why I changed the synopsis to "mediaControls()->playbackStarted() may be called more than once".

There is still an error in logic that was revealed by the original bug symptoms. Please let me know if you think this error in logic, which is fixed by my patch and was called "a good fix" by Jer Noble in comment #8, still needs to be fixed.

[Rob Buis]

Hi Max,

(In reply to comment #14)
> (In reply to comment #13):
> 
> The fix is not needed to address the original synopsis of this bug: "HTML5 video controls are not automatically fading out when fullscreen". That is why I changed the synopsis to "mediaControls()->playbackStarted() may be called more than once".
> 
> There is still an error in logic that was revealed by the original bug symptoms. Please let me know if you think this error in logic, which is fixed by my patch and was called "a good fix" by Jer Noble in comment #8, still needs to be fixed.

Do you feel the commit message is still accurate? Does a test make sense like Jer suggested?
Cheers,

Rob.

[Max Feil]

(In reply to comment #15)
> Do you feel the commit message is still accurate? Does a test make sense like Jer suggested?

I fully plan to update the patch with more accurate commit message and a test. But I do not plan to change the code itself.

[Rob Buis]

Hi Max,

(In reply to comment #16)
> (In reply to comment #15)
> > Do you feel the commit message is still accurate? Does a test make sense like Jer suggested?
> 
> I fully plan to update the patch with more accurate commit message and a test. But I do not plan to change the code itself.

Great! Please do that, I'll see it once it shows up in the queue and I can do a review then.
Cheers,

Rob.