Bug 129048 – Setting playback rate on video with media controller is not ignored

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Piotr Grad]

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

[Piotr Grad]

Created attachment 224647 [details]
Patch & layout test

[Piotr Grad]

Comment on attachment 224647 [details]
Patch & layout test

will remove not needed effectivePlaybackRate method

[Piotr Grad]

Created attachment 224650 [details]
Patch & layout test

[Jer Noble]

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)."

[Piotr Grad]

(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.

[Eric Carlson]

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.

[Piotr Grad]

Created attachment 224659 [details]
Patch & layout test

[Jer Noble]

(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().

[Eric Carlson]

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.

[Piotr Grad]

Created attachment 231408 [details]
Patch & layout test

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Piotr Grad]

Created attachment 231439 [details]
Patch & layout test

[Build Bot]

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

[Build Bot]

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

[Jer Noble]

Comment on attachment 231439 [details]
Patch & layout test

Looks good, though I'm concerned about the crashing test. Marking r+ but cq-.

[Piotr Grad]

Created attachment 231589 [details]
Patch & layout test

resend for mac-wk2 test

[Piotr Grad]

Comment on attachment 231439 [details]
Patch & layout test

Resend patch doesn't cause mac-wk2 failure.

[WebKit Commit Bot]

Comment on attachment 231439 [details]
Patch & layout test

Clearing flags on attachment: 231439

Committed r168996: <http://trac.webkit.org/changeset/168996>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.