Bug 129048 - Setting playback rate on video with media controller is not ignored
Summary: Setting playback rate on video with media controller is not ignored
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-02-19 09:55 PST by Piotr Grad
Modified: 2014-05-17 14:36 PDT (History)
11 users (show)

See Also:


Attachments
Patch & layout test (9.83 KB, patch)
2014-02-19 10:06 PST, Piotr Grad
piotr.grad: commit-queue-
Details | Formatted Diff | Diff
Patch & layout test (9.12 KB, patch)
2014-02-19 10:36 PST, Piotr Grad
jer.noble: review-
Details | Formatted Diff | Diff
Patch & layout test (9.68 KB, patch)
2014-02-19 11:39 PST, Piotr Grad
no flags Details | Formatted Diff | Diff
Patch & layout test (10.53 KB, patch)
2014-05-13 14:40 PDT, Piotr Grad
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion (520.29 KB, application/zip)
2014-05-13 16:17 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (475.84 KB, application/zip)
2014-05-13 16:45 PDT, Build Bot
no flags Details
Patch & layout test (10.18 KB, patch)
2014-05-14 02:34 PDT, Piotr Grad
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (504.82 KB, application/zip)
2014-05-14 03:27 PDT, Build Bot
no flags Details
Patch & layout test (10.18 KB, patch)
2014-05-16 14:23 PDT, Piotr Grad
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Piotr Grad 2014-02-19 09:55:28 PST
According to spec. playback rate on video with media controller should be ignored:
http://www.w3.org/TR/html5/embedded-content-0.html#dom-media-playbackrate
Comment 1 Piotr Grad 2014-02-19 10:06:52 PST
Created attachment 224647 [details]
Patch & layout test
Comment 2 Piotr Grad 2014-02-19 10:32:10 PST
Comment on attachment 224647 [details]
Patch & layout test

will remove not needed effectivePlaybackRate method
Comment 3 Piotr Grad 2014-02-19 10:36:05 PST
Created attachment 224650 [details]
Patch & layout test
Comment 4 Jer Noble 2014-02-19 10:45:57 PST
Comment on attachment 224650 [details]
Patch & layout test

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

> Source/WebCore/html/HTMLMediaElement.cpp:2571
>  double HTMLMediaElement::playbackRate() const
>  {
> -    return m_playbackRate;
> +    return m_mediaController ? m_mediaController->playbackRate() : m_playbackRate;
>  }

I would prefer a new method called "effectivePlaybackRate()".  Changing playbackRate() to return the media controller rate violates the spec: "The [playbackRate] attribute is mutable: on getting it must return the last value it was set to, or 1.0 if it hasn't yet been set; on setting the attribute must be set to the new value, and the playback will change speed (if the element is potentially playing and there is no current media controller)."
Comment 5 Piotr Grad 2014-02-19 10:53:40 PST
(In reply to comment #4)
> (From update of attachment 224650 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=224650&action=review
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:2571
> >  double HTMLMediaElement::playbackRate() const
> >  {
> > -    return m_playbackRate;
> > +    return m_mediaController ? m_mediaController->playbackRate() : m_playbackRate;
> >  }
> 
> I would prefer a new method called "effectivePlaybackRate()".  Changing playbackRate() to return the media controller rate violates the spec: "The [playbackRate] attribute is mutable: on getting it must return the last value it was set to, or 1.0 if it hasn't yet been set; on setting the attribute must be set to the new value, and the playback will change speed (if the element is potentially playing and there is no current media controller)."

Yes, but earlier you have:
The playbackRate attribute gives the effective playback rate (assuming there is no current media controller overriding it), which is the speed at which the media resource plays,

and then:
The effective playback rate is not necessarily the element's playbackRate. When a media element has a current media controller, its effective playback rate is the MediaController's media controller playback rate.
Comment 6 Eric Carlson 2014-02-19 10:55:39 PST
Comment on attachment 224650 [details]
Patch & layout test

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

> LayoutTests/media/video-controller-child-rate.html:32
> +                setTimeout(halfSecLater,500);

Nit: space after the comma.

> LayoutTests/media/video-controller-child-rate.html:38
> +            function halfSecLater()
> +            {
> +                if (video.currentTime > currentTime + 1)
> +                        failTest("Playback rate was aplied directly to video with media controller.");

This is asking for occasional flakiness, and .5 seconds is a long time to wait for any one test. Instead, why don't you have a 'time update' event handler that triggers a small number of times (5-10?) which compares the .currentTime delta with the wall clock delta.

> LayoutTests/media/video-controller-child-rate.html:39
> +                    endTest();

Nit: odd indentation here.

> Source/WebCore/html/HTMLMediaElement.cpp:2590
> -    double effectiveRate = m_mediaController ? m_mediaController->playbackRate() : m_playbackRate;
> -    if (m_player && potentiallyPlaying() && m_player->rate() != effectiveRate)
> -        m_player->setRate(effectiveRate);
> +    if (m_player && potentiallyPlaying() && m_player->rate() != playbackRate())
> +        m_player->setRate(playbackRate());

This doesn't actually change anything, and it adds two method calls. Why change it?

> Source/WebCore/html/HTMLMediaElement.cpp:3884
> -        if (loop() && !m_mediaController && m_playbackRate > 0) {
> +        if (loop() && !m_mediaController && playbackRate() > 0) {
>              m_sentEndEvent = false;
>              // then seek to the earliest possible position of the media resource and abort these steps when the direction of
>              // playback is forwards,
>              if (now >= dur)
>                  seek(0);
> -        } else if ((now <= 0 && m_playbackRate < 0) || (now >= dur && m_playbackRate > 0)) {
> +        } else if ((now <= 0 && playbackRate() < 0) || (now >= dur && playbackRate() > 0)) {

It is probably worth caching playbackRate() in a local instead of calling it (potentially) three times.

> Source/WebCore/html/HTMLMediaElement.cpp:4181
> -    if (m_playbackRate > 0)
> +    if (playbackRate() > 0)
>          return dur > 0 && now >= dur && (!loop() || m_mediaController);
>  
>      // or the current playback position is the earliest possible position and the direction 
>      // of playback is backwards
> -    if (m_playbackRate < 0)
> +    if (playbackRate() < 0)

Ditto.
Comment 7 Piotr Grad 2014-02-19 11:39:13 PST
Created attachment 224659 [details]
Patch & layout test
Comment 8 Jer Noble 2014-02-20 12:19:45 PST
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 224650 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=224650&action=review
> > 
> > > Source/WebCore/html/HTMLMediaElement.cpp:2571
> > >  double HTMLMediaElement::playbackRate() const
> > >  {
> > > -    return m_playbackRate;
> > > +    return m_mediaController ? m_mediaController->playbackRate() : m_playbackRate;
> > >  }
> > 
> > I would prefer a new method called "effectivePlaybackRate()".  Changing playbackRate() to return the media controller rate violates the spec: "The [playbackRate] attribute is mutable: on getting it must return the last value it was set to, or 1.0 if it hasn't yet been set; on setting the attribute must be set to the new value, and the playback will change speed (if the element is potentially playing and there is no current media controller)."
> 
> Yes, but earlier you have:
> The playbackRate attribute gives the effective playback rate (assuming there is no current media controller overriding it), which is the speed at which the media resource plays,
> 
> and then:
> The effective playback rate is not necessarily the element's playbackRate. When a media element has a current media controller, its effective playback rate is the MediaController's media controller playback rate.

Yes exactly.  The playbackRate _is_ the effective playback rate, _unless_ there is a current media controller.

I think you might be confusing the causality of "effective playback rate".  playbackRate is setting what the "effective playback rate" should be, not returning what the media engine reports the "effective playback rate" to be.

As such, your most recent patch is still incorrect.  We should not be returning the media controller's playbackRate from HTMLMediaElement::playbackRate().
Comment 9 Eric Carlson 2014-02-20 12:43:01 PST
Comment on attachment 224659 [details]
Patch & layout test

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

> LayoutTests/media/video-controller-child-rate.html:49
> +            function timeupdate()
> +            {
> +                if (videoLastTime == -1) {
> +                    videoLastTime = video.currentTime;
> +                    lastRealTime = new Date();
> +                    return;
> +                }
> +
> +                var videoTimeDelta = video.currentTime - videoLastTime;
> +                var realTimeDelta = new Date() - lastRealTime;
> +                if (videoTimeDelta > realTimeDelta / 500)
> +                    failTest("Playback rate was aplied directly to video with media controller.");
> +
> +                endTest();
> +            }

This is not exactly what I suggested in my earlier review. First, I think you should look at several 'timeupdate' events because the latency before the first is extremely variable. Second, I think you should directly compare the wall clock and media clock deltas and fail if they are more than a small percentage different, e.g. something like: 

    testExpected(Math.abs(videoTimeDelta - realTimeDelta) / 100, 2, ">")

I typed this in the browser so it may not be quite right, but you should get the idea.
Comment 10 Piotr Grad 2014-05-13 14:40:44 PDT
Created attachment 231408 [details]
Patch & layout test
Comment 11 Build Bot 2014-05-13 16:16:59 PDT
Comment on attachment 231408 [details]
Patch & layout test

Attachment 231408 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4859142875381760

New failing tests:
media/video-reverse-play-duration.html
media/video-controller-child-rate.html
media/video-timeupdate-reverse-play.html
Comment 12 Build Bot 2014-05-13 16:17:03 PDT
Created attachment 231415 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 13 Build Bot 2014-05-13 16:44:54 PDT
Comment on attachment 231408 [details]
Patch & layout test

Attachment 231408 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5105433480003584

New failing tests:
media/video-reverse-play-duration.html
media/video-controller-child-rate.html
media/video-timeupdate-reverse-play.html
Comment 14 Build Bot 2014-05-13 16:45:00 PDT
Created attachment 231420 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 15 Piotr Grad 2014-05-14 02:34:41 PDT
Created attachment 231439 [details]
Patch & layout test
Comment 16 Build Bot 2014-05-14 03:27:17 PDT
Comment on attachment 231439 [details]
Patch & layout test

Attachment 231439 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6343423174901760

New failing tests:
media/W3C/video/networkState/networkState_during_loadstart.html
Comment 17 Build Bot 2014-05-14 03:27:22 PDT
Created attachment 231441 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 18 Jer Noble 2014-05-16 14:20:49 PDT
Comment on attachment 231439 [details]
Patch & layout test

Looks good, though I'm concerned about the crashing test. Marking r+ but cq-.
Comment 19 Piotr Grad 2014-05-16 14:23:08 PDT
Created attachment 231589 [details]
Patch & layout test

resend for mac-wk2 test
Comment 20 Piotr Grad 2014-05-16 22:45:16 PDT
Comment on attachment 231439 [details]
Patch & layout test

Resend patch doesn't cause mac-wk2 failure.
Comment 21 WebKit Commit Bot 2014-05-17 14:36:45 PDT
Comment on attachment 231439 [details]
Patch & layout test

Clearing flags on attachment: 231439

Committed r168996: <http://trac.webkit.org/changeset/168996>
Comment 22 WebKit Commit Bot 2014-05-17 14:36:51 PDT
All reviewed patches have been landed.  Closing bug.