1055843 - Video doesn't pause when playbackRate=0

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Videos are supposed to pause when the playbackRate is set to 0. However they just play muted.

Comment 1

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Fix to existing mochitest to make it fail properly

Comment 2

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Simple test case

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8475549 [details] [diff] [review]
Fix to existing mochitest to make it fail properly

Review of attachment 8475549 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/test/test_playback_rate.html
@@ +56,5 @@
>      ok(checkPlaybackRate(delta_wallclock, delta, SLOW_RATE, 0.25), "We are effectively slowing down playback. (" + delta_wallclock + ", " + delta + ") for " + t.token);
>      t.removeEventListener("timeupdate", ontimeupdate);
>      t.addEventListener("pause", onpaused);
>      t.playbackRate = NULL_RATE;
> +    t.play();

What is the purpose to call play() again?

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #0)
> Videos are supposed to pause when the playbackRate is set to 0. However they
> just play muted.

It is muted because the playback rate is beyond the valid range.
http://hg.mozilla.org/mozilla-central/file/149d3ce6e020/content/html/content/src/HTMLMediaElement.cpp#l3841

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

It looks like HTMLMediaElement::CheckAutoplayDataReady() doesn't respect playback rate and starts playing despite the playback rate is 0.

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: Do not pause when playbackRate = 0. Instead don't play anything

Bug 1055843: Do not pause when playbackRate = 0. Instead don't play anything

Comment 7

[Jean-Yves Avenard [:jya]]

(In reply to JW Wang [:jwwang] from comment #5)
> It looks like HTMLMediaElement::CheckAutoplayDataReady() doesn't respect
> playback rate and starts playing despite the playback rate is 0.

It is my understand that when playback rate is 0, it should play. E.g. it is not in pause state but in play state.

It just happens to play nothing as the rate is 0.

As such, the control bar appears to be playing, showing the pause button.
This is how chrome handles it as well

Comment 8

[Paul Adenot (:padenot)]

This is how it is specified, yes [0]. I'm sure it was working before, though.

[0]: http://www.whatwg.org/specs/web-apps/current-work/multipage/embedded-content.html#effective-playback-rate

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8476580 [details] [diff] [review]
Do not pause when playbackRate = 0. Instead don't play anything

Review of attachment 8476580 [details] [diff] [review]:
-----------------------------------------------------------------

Paul knows more about playback rate than I.

Comment 10

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Jean-Yves Avenard [:jya] from comment #7)
> It is my understand that when playback rate is 0, it should play. E.g. it is
> not in pause state but in play state.
> 
> It just happens to play nothing as the rate is 0.
> 
> As such, the control bar appears to be playing, showing the pause button.
> This is how chrome handles it as well

Right. And our test does respect the spec.
http://hg.mozilla.org/mozilla-central/file/c14e5feadc61/content/media/test/test_playback_rate.html#l83

Comment 11

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8476580 [details] [diff] [review]
Do not pause when playbackRate = 0. Instead don't play anything

Review of attachment 8476580 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/MediaDecoder.cpp
@@ +1429,5 @@
>  
>  void MediaDecoder::SetPlaybackRate(double aPlaybackRate)
>  {
>    if (aPlaybackRate == 0) {
>      mPausedForPlaybackRateNull = true;

Can we remove |mPausedForPlaybackRateNull| by keeping |mPlaybackRate| only? I think checking |mPlaybackRate == 0| should be equivalent to |mPausedForPlaybackRateNull|. Then we will have fewer state variables to maintain.

@@ -1428,5 @@
>  {
>    if (aPlaybackRate == 0) {
>      mPausedForPlaybackRateNull = true;
>      Pause();
> -    return;

Why removing the early return here? Passing playbackRate=0 to MediaDecoderStateMachine will cause assertion.

@@ +1435,4 @@
>    } else if (mPausedForPlaybackRateNull) {
>      // If the playbackRate is no longer null, restart the playback, iff the
>      // media was playing.
> +    mPausedForPlaybackRateNull = false;

Why move this line up? I guess it doesn't make a difference.

Comment 12

[Jean-Yves Avenard [:jya]]

(In reply to JW Wang [:jwwang] from comment #11)
> Comment on attachment 8476580 [details] [diff] [review]
> Do not pause when playbackRate = 0. Instead don't play anything
> 
> Review of attachment 8476580 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/MediaDecoder.cpp
> @@ +1429,5 @@
> >  
> >  void MediaDecoder::SetPlaybackRate(double aPlaybackRate)
> >  {
> >    if (aPlaybackRate == 0) {
> >      mPausedForPlaybackRateNull = true;
> 
> Can we remove |mPausedForPlaybackRateNull| by keeping |mPlaybackRate| only?
> I think checking |mPlaybackRate == 0| should be equivalent to
> |mPausedForPlaybackRateNull|. Then we will have fewer state variables to
> maintain.

This is used to know if the decoder was previously paused should the rate be 0. I guess the logic could be changed and re-use mPlaybackRate instead. But not being the author of that code, I always tend to keep the change to a minimal.

having mPausedForPlaybackRateNull was also required because mInitialPlaybackRate isn't modified when MediaDecoder::SetPlaybackRate is called with a value of 0.

> 
> @@ -1428,5 @@
> >  {
> >    if (aPlaybackRate == 0) {
> >      mPausedForPlaybackRateNull = true;
> >      Pause();
> > -    return;
> 
> Why removing the early return here? Passing playbackRate=0 to
> MediaDecoderStateMachine will cause assertion.

so it falls back to setting mInitialPlaybackRate to the proper value.
mInitialPlaybackRate is set to 1.0 in the constructor.
As such, the following call to MediaDecoder::SetStateMachineParameters() will now call SetPlaybackRate with a value of 1.0. Meaning the following call to play, will start playback with a playrate of 1.0.

The whole concept that playback rate has been set is now lost.

You're right, it would cause an assertion (didn't notice it as the state machine isn't defined yet in that particular example).

I do believe that a playbackrate shouldn't be handled any differently than a non-zero playback rate.

> 
> @@ +1435,4 @@
> >    } else if (mPausedForPlaybackRateNull) {
> >      // If the playbackRate is no longer null, restart the playback, iff the
> >      // media was playing.
> > +    mPausedForPlaybackRateNull = false;
> 
> Why move this line up? I guess it doesn't make a difference.

it does, as Play() checks the value of mPausedForPlaybackRateNull. If you didn't set to false before the call to play, then playback wouldn't restart

Comment 13

[Jean-Yves Avenard [:jya]]

Attachment: Do not pause when playbackRate = 0. Instead don't play anything

Bug 1055843: Do not pause when playbackRate = 0. Instead don't play anything

Comment 14

[Jean-Yves Avenard [:jya]]

I've changed the logic so HTMLMediaElement has ownership should the decoder play or not. Seems more logical to me, instead of having MediaDecoder::Play() do nothing

Comment 15

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Jean-Yves Avenard [:jya] from comment #14)
> I've changed the logic so HTMLMediaElement has ownership should the decoder
> play or not. Seems more logical to me, instead of having
> MediaDecoder::Play() do nothing

This doesn't seem to conform to the spec. Does it pass test_playback_rate.html? The playbackRate of the media element should not affect the play/pause state. This is also the reason MediaDecoder have its own play state which could be different from the one of the media element. Therefore, it is possible for an media element to be in play state with its media decoder in pause state.

Comment 16

[Jean-Yves Avenard [:jya]]

(In reply to JW Wang [:jwwang] from comment #15)
> (In reply to Jean-Yves Avenard [:jya] from comment #14)
> > I've changed the logic so HTMLMediaElement has ownership should the decoder
> > play or not. Seems more logical to me, instead of having
> > MediaDecoder::Play() do nothing
> 
> This doesn't seem to conform to the spec. Does it pass
> test_playback_rate.html? The playbackRate of the media element should not
> affect the play/pause state. This is also the reason MediaDecoder have its
> own play state which could be different from the one of the media element.
> Therefore, it is possible for an media element to be in play state with its
> media decoder in pause state.

It's exactly what is happening..

The media element will be in play state (and have emitted the "play" event). But the media decoder will still be in pause state (it enters pause state when we set the playback rate to 0).

And yes it passes the test.
241 INFO TEST-OK | /tests/content/media/test/test_playback_rate.html | took 72206ms
242 INFO TEST-START | Shutdown
243 INFO Passed:  89
244 INFO Failed:  0
245 INFO Todo:    0
246 INFO Slowest: 72206ms - /tests/content/media/test/test_playback_rate.html
247 INFO SimpleTest FINISHED
248 INFO TEST-INFO | Ran 1 Loops
249 INFO SimpleTest FINISHED

Comment 17

[Jean-Yves Avenard [:jya]]

To clarify if it wasn't clear enough.
by:
"so HTMLMediaElement has ownership should the decoder play or not"

I mean, HTMLMediaElement will not call MediaDecoder::Play() , but it does everything else as if it was playing

Comment 18

[JW Wang [:jwwang] [:jw_wang]]

Ok, I see. But the previous patch seems simpler where MediaDecoder::Play() just returns when |mPausedForPlaybackRateNull| is true.

Comment 19

[Jean-Yves Avenard [:jya]]

(In reply to JW Wang [:jwwang] from comment #18)
> Ok, I see. But the previous patch seems simpler where MediaDecoder::Play()
> just returns when |mPausedForPlaybackRateNull| is true.

The patch is indeed simpler.. The logic is conceptually different however (but in effect identical). I'm happy either way

Comment 20

[Jean-Yves Avenard [:jya]]

Attachment: Do not pause when playbackRate = 0. Instead don't play anything

Include comments from jwwang. Simplify code. Also remove all ambiguities in testing a double as 0

Comment 21

[Jean-Yves Avenard [:jya]]

Attachment: Do not pause when playbackRate = 0. Instead don't play anything

Revert earlier changes. The initial playrate value would be lost should the user pause then play, breaking the specs

Comment 22

[Jean-Yves Avenard [:jya]]

https://tbpl.mozilla.org/?tree=Try&rev=396f5b6de97e

Comment 23

[JW Wang [:jwwang] [:jw_wang]]

test_playback_rate.html timed out on both Android and B2G. Can  you check if it is so even without this patch?

Comment 24

[Jean-Yves Avenard [:jya]]

If I had used the proper try link, it would probably have been better :)
https://tbpl.mozilla.org/?tree=Try&rev=b8a10e959a1d

Comment 25

[JW Wang [:jwwang] [:jw_wang]]

Can you remove the patch https://hg.mozilla.org/try/rev/7ceea5a3497f and Try again? Calling play() again is suspicious to me and might defeat the test of null-playback-rate shouldn't pause.

Comment 26

[Jean-Yves Avenard [:jya]]

> Can you remove the patch https://hg.mozilla.org/try/rev/7ceea5a3497f and Try
> again? Calling play() again is suspicious to me and might defeat the test of
> null-playback-rate shouldn't pause.

https://tbpl.mozilla.org/?tree=Try&rev=8842469f0837(In reply to JW Wang [:jwwang] from comment #25)

Comment 27

[Paul Adenot (:padenot)]

Comment on attachment 8475549 [details] [diff] [review]
Fix to existing mochitest to make it fail properly

Review of attachment 8475549 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/test/test_playback_rate.html
@@ +56,5 @@
>      ok(checkPlaybackRate(delta_wallclock, delta, SLOW_RATE, 0.25), "We are effectively slowing down playback. (" + delta_wallclock + ", " + delta + ") for " + t.token);
>      t.removeEventListener("timeupdate", ontimeupdate);
>      t.addEventListener("pause", onpaused);
>      t.playbackRate = NULL_RATE;
> +    t.play();

This actually reset playbackRate to defaultPlaybackRate, per spec, so it's wrong.

Comment 28

[Paul Adenot (:padenot)]

Comment on attachment 8477354 [details] [diff] [review]
Do not pause when playbackRate = 0. Instead don't play anything

Review of attachment 8477354 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, sorry for the slow review.

::: content/html/content/src/HTMLMediaElement.cpp
@@ +3836,5 @@
>    mPlaybackRate = ClampPlaybackRate(aPlaybackRate);
>  
> +  if (mPlaybackRate != 0.0 &&
> +      (mPlaybackRate < 0 || mPlaybackRate > THRESHOLD_HIGH_PLAYBACKRATE_AUDIO ||
> +       mPlaybackRate < THRESHOLD_LOW_PLAYBACKRATE_AUDIO)) {

This should not be necessary, because if we are pausing (internally, while showing that we are in playing state) the media, no sound will be output anyways.

::: content/media/MediaDecoder.cpp
@@ +1429,5 @@
>  
>  void MediaDecoder::SetPlaybackRate(double aPlaybackRate)
>  {
> +  if (aPlaybackRate == mInitialPlaybackRate)
> +    return;

nit: always use braces, even for single line statements.

Comment 29

[Jean-Yves Avenard [:jya]]

Attachment: Do not pause when playbackRate = 0. Instead don't play anything

Follow r+

Comment 30

[Jean-Yves Avenard [:jya]]

Attachment: Do not pause when playbackRate = 0. Instead don't play anything

Follow r+

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bdb0a8c2902d

Comment 32

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/bdb0a8c2902d