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
Created attachment 224647 [details] Patch & layout test
Comment on attachment 224647 [details] Patch & layout test will remove not needed effectivePlaybackRate method
Created attachment 224650 [details] Patch & layout test
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)."
(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 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.
Created attachment 224659 [details] Patch & layout test
(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 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.
Created attachment 231408 [details] Patch & layout test
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
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 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
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
Created attachment 231439 [details] Patch & layout test
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
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 on attachment 231439 [details] Patch & layout test Looks good, though I'm concerned about the crashing test. Marking r+ but cq-.
Created attachment 231589 [details] Patch & layout test resend for mac-wk2 test
Comment on attachment 231439 [details] Patch & layout test Resend patch doesn't cause mac-wk2 failure.
Comment on attachment 231439 [details] Patch & layout test Clearing flags on attachment: 231439 Committed r168996: <http://trac.webkit.org/changeset/168996>
All reviewed patches have been landed. Closing bug.