Bug 87304 – Early returns in MediaPlayer setters

• Status: RESOLVED
• Resolution: INVALID
• Component: Media

[Philippe Normand]

Some calls to the MediaPlayerPrivate implementation can be avoided if the value to set and the current value are equal.

[Philippe Normand]

Created attachment 143632 [details]
Patch

[Philippe Normand]

Thanks Martin for the quick review. I asked about this small issue with Eric on IRC. I'll just land the patch later today or tomorrow unless he raises any red flag :)

I checked the patch with the media suite on the GTK+ port.

[WebKit Review Bot]

Comment on attachment 143632 [details]
Patch

Attachment 143632 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12797022

New failing tests:
media/audio-garbage-collect.html
media/track/track-cues-pause-on-exit.html
fast/loader/loadInProgress.html
platform/chromium/compositing/video-frame-size-change.html
fast/loader/unload-form-post-about-blank.html
media/media-element-play-after-eos.html
media/media-fragments/TC0001-TC0009.html
http/tests/xmlhttprequest/zero-length-response.html
http/tests/security/script-crossorigin-loads-correctly.html
media/track/track-cues-cuechange.html
media/track/track-cues-missed.html
media/media-fragments/TC0010-TC0019.html
media/media-document-audio-repaint.html
media/media-fragments/TC0070-TC0079.html
media/media-fragments/TC0050-TC0059.html
media/media-controller-playback.html
media/track/track-cues-enter-exit.html
media/audio-concurrent-supported.html
http/tests/media/media-source/webm/video-media-source-play.html
media/media-fragments/TC0020-TC0029.html
media/media-continues-playing-after-replace-source.html
http/tests/media/media-source/webm/video-media-source-seek.html

[WebKit Review Bot]

Created attachment 143697 [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: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Philippe Normand]

Hum, looks like this patch makes Chromium sad. :(
I'll try to see what's going on.

[Philippe Normand]

Created attachment 144575 [details]
Patch

[Philippe Normand]

I got back to this patch today, wanted to check the tests locally again before commiting and found one issue, MediaPlayerPrivate::setRate() not called anymore. I'll upload a new patch adding an explicit call in MediaPlayer::loadWithNextMediaEngine()

[Philippe Normand]

Created attachment 159402 [details]
Patch

[WebKit Review Bot]

Comment on attachment 159402 [details]
Patch

Attachment 159402 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13537708

New failing tests:
media/audio-garbage-collect.html
media/media-fragments/TC0011.html
media/track/track-cues-pause-on-exit.html
fast/loader/loadInProgress.html
platform/chromium/compositing/video-frame-size-change.html
inspector/debugger/source-frame.html
fast/loader/unload-form-post-about-blank.html
http/tests/media/media-source/video-media-source-play.html
fast/canvas/webgl/shader-precision-format.html
media/media-fragments/TC0006.html
media/media-fragments/TC0004.html
media/media-element-play-after-eos.html
http/tests/xmlhttprequest/zero-length-response.html
media/track/track-cues-cuechange.html
media/track/track-cues-missed.html
media/media-controller-time-clamp.html
media/media-document-audio-repaint.html
fast/frames/cached-frame-counter.html
http/tests/media/video-buffered-range-contains-currentTime.html
media/track/track-cues-sorted-before-dispatch.html
media/media-fragments/TC0015.html
media/media-fragments/TC0017.html
media/track/track-cues-enter-exit.html
media/audio-concurrent-supported.html
media/media-fragments/TC0005.html
media/media-controller-playback.html
http/tests/media/media-source/video-media-source-seek.html

[WebKit Review Bot]

Created attachment 159407 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Philippe Normand]

Oh well, the ::setRate() method doesn't need early return because the HTMLMediaElement already performs pre-emptive checks.

[Philippe Normand]

Created attachment 159413 [details]
Patch

[Eric Carlson]

Comment on attachment 159413 [details]
Patch

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

> Source/WebCore/platform/graphics/MediaPlayer.cpp:410
>          m_private->setPreservesPitch(preservesPitch());
> +        m_private->setRate(m_rate);
>          if (m_shouldPrepareToRender)

This may be a fine change, but why is it required? If so, it should be described in the ChangeLog.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:472
>  void MediaPlayerPrivateGStreamer::seek(float time)
>      if (m_errorOccured)
>          return;
>  
> +    if (!m_player->rate())
> +        return;
> +

Why should seeking while rate is 0 be ignored?

[Philippe Normand]

(In reply to comment #13)
> (From update of attachment 159413 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159413&action=review
> 
> > Source/WebCore/platform/graphics/MediaPlayer.cpp:410
> >          m_private->setPreservesPitch(preservesPitch());
> > +        m_private->setRate(m_rate);
> >          if (m_shouldPrepareToRender)
> 
> This may be a fine change, but why is it required? If so, it should be described in the ChangeLog.
> 

I'll add a note in the ChangeLog.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:472
> >  void MediaPlayerPrivateGStreamer::seek(float time)
> >      if (m_errorOccured)
> >          return;
> >  
> > +    if (!m_player->rate())
> > +        return;
> > +
> 
> Why should seeking while rate is 0 be ignored?

Oh, this is an unintended change, I'll revert it.
Thanks for the review!

[Philippe Normand]

Created attachment 159625 [details]
Patch

[Philippe Normand]

Comment on attachment 159625 [details]
Patch

Clearing flags on attachment: 159625

Committed r126157: <http://trac.webkit.org/changeset/126157>

[Philippe Normand]

All reviewed patches have been landed.  Closing bug.

[Jer Noble]

This change breaks a lot of assumptions.  Many of the MediaPlayerPrivate implementations will ignore calls to, for example, setVolume() before a load() occurs.  Since MediaPlayer is now bailing out early, further calls to setVolume() with the same value will be ignored, and post-load, the volume value will never be changed from the default.

In other words, after this patch:

video.volume = 0;
video.src = "foo.ogv";
video.load();
// wait for loadedmetadata
alert(video.volume); // prints "1".

[Jer Noble]

As far as I can tell, this breaks the following properties when set pre-load:

setVolume() - mac, avfoundation, win-quicktime, blackberry
setMuted() - mac, avfoundation, win-quicktime, blackberry, gstreamer
setPreservesPitch() - mac, win-quicktime

The following properties already check to see if the properties being set are the same as previous and bail out early:

setSize() - blackberry, qt
setVisible() - mac, avfoundation, 

Many of the remaining have empty implementations, so this optimization is pointless.

I see no good reason why this optimization was done, and it significantly breaks things.  I think this patch should be rolled out.

[WebKit Review Bot]

Re-opened since this is blocked by bug 101954

[Philippe Normand]

Sorry for the trouble introduced by this patch. Let's close this bug then.