Bug 63595 - MediaControlSeekButtonElement should support seeking by changing the playback rate.
Summary: MediaControlSeekButtonElement should support seeking by changing the playback...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-28 18:44 PDT by Jer Noble
Modified: 2011-10-10 15:41 PDT (History)
4 users (show)

See Also:


Attachments
Patch (17.21 KB, patch)
2011-06-29 13:27 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.43 MB, application/zip)
2011-06-29 13:47 PDT, WebKit Review Bot
no flags Details
Patch (14.71 KB, patch)
2011-09-20 16:27 PDT, Jer Noble
eric.carlson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2011-06-28 18:44:21 PDT
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.
Comment 1 Jer Noble 2011-06-29 13:27:42 PDT
Created attachment 99137 [details]
Patch
Comment 2 Alexis Menard (darktears) 2011-06-29 13:42:27 PDT
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?
Comment 3 WebKit Review Bot 2011-06-29 13:46:58 PDT
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
Comment 4 WebKit Review Bot 2011-06-29 13:47:03 PDT
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
Comment 5 Jer Noble 2011-06-29 13:52:39 PDT
(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.
Comment 6 Jer Noble 2011-06-29 13:53:53 PDT
(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.
Comment 7 Alexis Menard (darktears) 2011-06-29 13:56:32 PDT
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.
Comment 8 Andy Estes 2011-06-29 21:56:14 PDT
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.
Comment 9 Jer Noble 2011-06-29 22:11:19 PDT
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.
Comment 10 Andy Estes 2011-06-29 22:15:22 PDT
(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?
Comment 11 Jer Noble 2011-06-29 22:31:24 PDT
(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.
Comment 12 Jer Noble 2011-09-20 16:27:53 PDT
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.
Comment 13 Eric Carlson 2011-09-21 07:27:03 PDT
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?
Comment 14 Jer Noble 2011-09-21 09:18:20 PDT
(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!
Comment 15 Jer Noble 2011-10-10 15:41:31 PDT
Committed r97100: <http://trac.webkit.org/changeset/97100>