Closed Bug 1055843 Opened 10 years ago Closed 10 years ago

Video doesn't pause when playbackRate=0

Categories

(Core :: Audio/Video, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: ajones, Assigned: jya)

References

Details

Attachments

(2 files, 6 obsolete files)

Videos are supposed to pause when the playbackRate is set to 0. However they just play muted.
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?
(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
It looks like HTMLMediaElement::CheckAutoplayDataReady() doesn't respect playback rate and starts playing despite the playback rate is 0.
Assignee: nobody → jyavenard
Bug 1055843: Do not pause when playbackRate = 0. Instead don't play anything
Attachment #8476580 - Flags: review?(cpearce)
(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
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 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.
Attachment #8476580 - Flags: review?(cpearce) → review?(paul)
(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 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.
(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
Bug 1055843: Do not pause when playbackRate = 0. Instead don't play anything
Attachment #8477240 - Flags: review?(paul)
Attachment #8476580 - Attachment is obsolete: true
Attachment #8476580 - Flags: review?(paul)
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
(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.
(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
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
Ok, I see. But the previous patch seems simpler where MediaDecoder::Play() just returns when |mPausedForPlaybackRateNull| is true.
(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
Include comments from jwwang. Simplify code. Also remove all ambiguities in testing a double as 0
Attachment #8477337 - Flags: review?(paul)
Attachment #8477240 - Attachment is obsolete: true
Attachment #8477240 - Flags: review?(paul)
Revert earlier changes. The initial playrate value would be lost should the user pause then play, breaking the specs
Attachment #8477354 - Flags: review?(paul)
Attachment #8477337 - Attachment is obsolete: true
Attachment #8477337 - Flags: review?(paul)
test_playback_rate.html timed out on both Android and B2G. Can  you check if it is so even without this patch?
If I had used the proper try link, it would probably have been better :)
https://tbpl.mozilla.org/?tree=Try&rev=b8a10e959a1d
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.
> 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 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 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.
Attachment #8477354 - Flags: review?(paul) → review+
Attachment #8475549 - Attachment is obsolete: true
Attachment #8477354 - Attachment is obsolete: true
Attachment #8482088 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bdb0a8c2902d
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Depends on: 1065397
You need to log in before you can comment on or make changes to this bug.