Bug 63595 – MediaControlSeekButtonElement should support seeking by changing the playback rate.

• Component: Media
• Status: RESOLVED FIXED

[Jer Noble]

MediaControlSeekButtonElement currently pauses the movie, and repeatedly jumps forward in time to simulate fast-forward and rewind operations.  This is non-optimal for media engines which support playing at rates greater than 1x.  For those engines, the seek element should scan instead of skipping.

[Jer Noble]

Created attachment 99137 [details]
Patch

[Alexis Menard (darktears)]

Comment on attachment 99137 [details]
Patch

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

Is scanning not misleading? what about supportChangingPlaybackRate?

> Source/WebCore/html/shadow/MediaControlElements.cpp:70
> +static const float cScanMaximumRate = 8;

This will not break now that everyone is relaying on those constants? Or I'd say will change the behavior.

> Source/WebCore/html/shadow/MediaControlElements.cpp:553
> +void MediaControlSeekButtonElement::setActive(bool flag, bool pause)

Stupid question : How setActive is called? Who calls it?

[WebKit Review Bot]

Comment on attachment 99137 [details]
Patch

Attachment 99137 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8966132

New failing tests:
media/video-controls-scanning.html

[WebKit Review Bot]

Created attachment 99143 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Jer Noble]

(In reply to comment #4)
> 
> The attached test failures were seen while running run-webkit-tests on the chromium-ews.

From that attached test failure:

"CONSOLE MESSAGE: line 14: Uncaught Failed to find button seek-forward-button"

I'll add this new test to the Chromium expected results list.

[Jer Noble]

(In reply to comment #2)
> (From update of attachment 99137 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=99137&action=review
> 
> Is scanning not misleading? what about supportChangingPlaybackRate?

I considered naming it "supportsPlaybackRate(float rate)", but I was convinced that there is no good way to answer that question for a specific playback rate.  All media engines support changing the rate between 0 and 1 to implement pausing at least, so "supportsChangingPlaybackRate()" is too broad.

> > Source/WebCore/html/shadow/MediaControlElements.cpp:70
> > +static const float cScanMaximumRate = 8;
> 
> This will not break now that everyone is relaying on those constants? Or I'd say will change the behavior.

I'm not sure what you mean.  The new constants will only apply if the media engine returns 'true' for supportsScanning().  Currently, that's only QTKit and AVFoundation.  The rest will fall back to the old behavior of pausing, then skipping.

> > Source/WebCore/html/shadow/MediaControlElements.cpp:553
> > +void MediaControlSeekButtonElement::setActive(bool flag, bool pause)
> 
> Stupid question : How setActive is called? Who calls it?

It's called from RenderLayer::updateHoverActiveState(), as a result of a hitTest request from the EventHandler.

[Alexis Menard (darktears)]

Comment on attachment 99137 [details]
Patch

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

>>> Source/WebCore/html/shadow/MediaControlElements.cpp:70
>>> +static const float cScanMaximumRate = 8;
>> 
>> This will not break now that everyone is relaying on those constants? Or I'd say will change the behavior.
> 
> I'm not sure what you mean.  The new constants will only apply if the media engine returns 'true' for supportsScanning().  Currently, that's only QTKit and AVFoundation.  The rest will fall back to the old behavior of pausing, then skipping.

It was just used in the previous code that's what I meant, shouldn't be big deal.

[Andy Estes]

Comment on attachment 99137 [details]
Patch

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

> Source/WebCore/html/shadow/MediaControlElements.cpp:578
> +        mediaElement()->setPlaybackRate(mediaElement()->defaultPlaybackRate());

This call doesn't seem necessary. Is it possible for the media to be at a non-default playback rate at the time we start seeking?

> Source/WebCore/html/shadow/MediaControlElements.cpp:608
> +        float rate = std::min(cScanMaximumRate, fabsf(mediaElement()->playbackRate() * 2));
> +        if (!isForwardButton())
> +            rate *= -1;
> +        mediaElement()->setPlaybackRate(rate);

It'd be nice to mention in the ChangeLog that the new seek behavior is to gradually increase the playback rate from 1x to 8x as the seek button remains pressed, whereas the old skip behavior is to not accelerate the seek.

[Jer Noble]

Comment on attachment 99137 [details]
Patch

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

>> Source/WebCore/html/shadow/MediaControlElements.cpp:578
>> +        mediaElement()->setPlaybackRate(mediaElement()->defaultPlaybackRate());
> 
> This call doesn't seem necessary. Is it possible for the media to be at a non-default playback rate at the time we start seeking?

With scripting, anything is possible. (Zombo.com) :)

>> Source/WebCore/html/shadow/MediaControlElements.cpp:608
>> +        mediaElement()->setPlaybackRate(rate);
> 
> It'd be nice to mention in the ChangeLog that the new seek behavior is to gradually increase the playback rate from 1x to 8x as the seek button remains pressed, whereas the old skip behavior is to not accelerate the seek.

Sure.

[Andy Estes]

(In reply to comment #9)
> (From update of attachment 99137 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=99137&action=review
> 
> >> Source/WebCore/html/shadow/MediaControlElements.cpp:578
> >> +        mediaElement()->setPlaybackRate(mediaElement()->defaultPlaybackRate());
> > 
> > This call doesn't seem necessary. Is it possible for the media to be at a non-default playback rate at the time we start seeking?
> 
> With scripting, anything is possible. (Zombo.com) :)
> 

In that case, wouldn't it be odd for the playback rate to decrease back to 1x when seeking starts? Can't the playback rate increase from the current rate to 8x instead of resetting to 1x?

[Jer Noble]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 99137 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=99137&action=review
> > 
> > >> Source/WebCore/html/shadow/MediaControlElements.cpp:578
> > >> +        mediaElement()->setPlaybackRate(mediaElement()->defaultPlaybackRate());
> > > 
> > > This call doesn't seem necessary. Is it possible for the media to be at a non-default playback rate at the time we start seeking?
> > 
> > With scripting, anything is possible. (Zombo.com) :)
> > 
> 
> In that case, wouldn't it be odd for the playback rate to decrease back to 1x when seeking starts? Can't the playback rate increase from the current rate to 8x instead of resetting to 1x?

Except then, the rate might increase from 1.33x (or some other fraction) to 2.66x to 5.32x to 8x, or it might slow anyway from 32x to 8x for that matter.  The way the patch is currently formulated makes behavior very consistent: pressing the FF button will always start out at 2x the default playback rate, which I'd argue is less odd.

Alternatively, the playback could be clamped to the next largest power of 2x, with a max of 8x, which takes care of the fractional multiplier problem, but it adds a lot of complexity in return.

[Jer Noble]

Created attachment 108076 [details]
Patch

The new behavior now matches QuickTime Player: the fast forward button now increases the speed to the next larger power of 2x.

[Eric Carlson]

Comment on attachment 108076 [details]
Patch

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

> Source/WebCore/html/shadow/MediaControlElements.cpp:588
> +    if (m_seekType == Scan)
> +        mediaElement()->setPlaybackRate(mediaElement()->defaultPlaybackRate());
> +

Would it make more sense to restore the original playback rate instead of resuming at the default rate?

> Source/WebCore/html/shadow/MediaControlElements.cpp:614
> +    } else {
> +        mediaElement()->setPlaybackRate(nextRate());
>      }

Nit: braces aren't needed here. I wonder why the style-pedant didn't catch this?

[Jer Noble]

(In reply to comment #13)
> (From update of attachment 108076 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108076&action=review
> 
> > Source/WebCore/html/shadow/MediaControlElements.cpp:588
> > +    if (m_seekType == Scan)
> > +        mediaElement()->setPlaybackRate(mediaElement()->defaultPlaybackRate());
> > +
> 
> Would it make more sense to restore the original playback rate instead of resuming at the default rate?

I considered this, and decided against it.  The patch's behavior matches that of QuickTime Player, which resets the playback rate to 1x when canceling scrubbing.  The difference being, QTP cancels scrubbing when the user clicks the play button, while WebKit has a press-and-hold behavior for the FF button.

A further improvement to the FF button would be to add a rate status indicator to the default controls, and change the button behavior from "click-and-hold to increase rate" to "click again to increase rate".

> > Source/WebCore/html/shadow/MediaControlElements.cpp:614
> > +    } else {
> > +        mediaElement()->setPlaybackRate(nextRate());
> >      }
> 
> Nit: braces aren't needed here. I wonder why the style-pedant didn't catch this?

I'll remove them.  Thanks!

[Jer Noble]

Committed r97100: <http://trac.webkit.org/changeset/97100>