Summary: | MediaControlSeekButtonElement should support seeking by changing the playback rate. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jer Noble <jer.noble> | ||||||||
Component: | Media | Assignee: | Jer Noble <jer.noble> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | aestes, dglazkov, eric.carlson, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Jer Noble
2011-06-28 18:44:21 PDT
Created attachment 99137 [details]
Patch
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 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 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
(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. (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 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 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 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. (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? (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. 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 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? (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! Committed r97100: <http://trac.webkit.org/changeset/97100> |