Closed Bug 1065397 Opened 10 years ago Closed 10 years ago

YouTube HTML5 playback speed can't be changed back to 'Normal'

Categories

(Core :: Audio/Video, defect)

34 Branch
x86_64
Windows 7
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla35
Tracking Status
firefox32 --- unaffected
firefox33 --- unaffected
firefox34 + verified
firefox35 + verified

People

(Reporter: bram.speeckaert, Assigned: jya)

References

Details

(Keywords: regression, Whiteboard: [testday-20141003])

Attachments

(1 file, 1 obsolete file)

STR:
1) Open https://www.youtube.com/watch?v=WgQ7qOCDFYQ and make sure the HTML5 player is used.
2) Change the playback speed to anything but 'Normal'.
3) Try to change the playback speed back to 'Normal'.

Even though 'Normal' is selected, the video continues playing at the previously selected speed. Changing the video to other speeds than 'Normal' still works, but it cannot be changed back to 'Normal'.

This happens in the latest Nightly. Firefox 32 has the correct behavior.
Not only youtube but also all HTML5 video. (e.g., http://ie.microsoft.com/testdrive/Graphics/VideoFormatSupport/Default.html , https://people.mozilla.org/~rgiles/2013/demo.webm etc.)

Regression window(m-i)
Good:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a8b815c493e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140901014616
Bad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/98c97c77ee78
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140901100741
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9a8b815c493e&tochange=98c97c77ee78

Regressed by:
bdb0a8c2902d	Jean-Yves Avenard — Bug 1055843 - Do not pause when playbackRate = 0. Instead don't play anything. r=padenot
Blocks: 1055843
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Version: 35 Branch → 34 Branch
Do not exit early when we revert back to initial speed
Attachment #8489833 - Flags: review?(cpearce)
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
Comment on attachment 8489833 [details] [diff] [review]
Allow playback rate to be reset to normal

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

mInitialPlaybackRate is tricky. We might be better off just using something like |HTMLMediaElement::mPlaybackRate| which is updated whenever HTMLMediaElement::SetPlaybackRate() is called and passed to mDecoder when needed.
You need to also store the initial rate as when you pause then play, it has to return to the initial rate as per the spec.

I do agree however that we should store the current playback rate, simply because SetPlaybackRate as it is is called several times in a row whenever a video starts, which cause several Play event to be emitted for nothing.

I went for the simplest fix, which is also reverting back to the original behaviour (while still allowing not to play when the playback rate is 0)
(In reply to Jean-Yves Avenard [:jya] from comment #4)
> You need to also store the initial rate as when you pause then play, it has
> to return to the initial rate as per the spec.

Is it 4.8.10.13?
The "play" function in the user agent's interface must set the playbackRate attribute to the value of the defaultPlaybackRate attribute before invoking the play() method.

I think that should be handled by HTMLMediaElement::mDefaultPlaybackRate.
"controller . defaultPlaybackRate [ = value ]

    Returns the default rate of playback.

    Can be set, to change the default rate of playback.

    This default rate has no direct effect on playback, but if the user switches to a fast-forward mode, when they return to the normal playback mode, it is expected that rate of playback (playbackRate) will be returned to this default rate.
"
I think that is the same DefaultPlaybackRate we are talking about.
Do not exit early when we revert back to initial speed. Prevent SetPlaybackRate to be called twice at setup.
Attachment #8489882 - Flags: review?(cpearce)
Attachment #8489833 - Attachment is obsolete: true
Attachment #8489833 - Flags: review?(cpearce)
Comment on attachment 8489882 [details] [diff] [review]
Allow playback rate to be reset to normal

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

I would like jwwang to start reviewing code in MediaDecoder*. He can review this.
Attachment #8489882 - Flags: review?(cpearce) → review?(jwwang)
Comment on attachment 8489882 [details] [diff] [review]
Allow playback rate to be reset to normal

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

I prefer to remove |mInitialPlaybackRate| and have |mCurrentPlaybackRate| only which will be updated every time MediaDecoder::SetPlaybackRate() is called to remember latest playbackRate set by the user. Then in MediaDecoder::SetStateMachineParameters(), we just need to send the latest value of playbackRate to the state machine to reflect the change in playbackRate before state machine is created.

::: content/media/MediaDecoder.cpp
@@ +573,5 @@
>    ReentrantMonitorAutoEnter mon(GetReentrantMonitor());
>    mDecoderStateMachine->SetDuration(mDuration);
>    mDecoderStateMachine->SetVolume(mInitialVolume);
>    mDecoderStateMachine->SetAudioCaptured(mInitialAudioCaptured);
> +  if (mInitialPlaybackRate != mCurrentPlaybackRate) {

If SetPlaybackRate(0.5) happened before |mDecoderStateMachine| is created, we would have |mCurrentPlaybackRate == mInitialPlaybackRate == 0.5| here and SetPlaybackRate(mInitialPlaybackRate) won't be called. This seems wrong.
Attachment #8489882 - Flags: review?(jwwang)
(In reply to JW Wang [:jwwang] from comment #10)

> I prefer to remove |mInitialPlaybackRate| and have |mCurrentPlaybackRate|
> only which will be updated every time MediaDecoder::SetPlaybackRate() is
> called to remember latest playbackRate set by the user. Then in
> MediaDecoder::SetStateMachineParameters(), we just need to send the latest
> value of playbackRate to the state machine to reflect the change in
> playbackRate before state machine is created.

It would seem wiser to me to revert back to the original behaviour for the purpose of fixing this bug ASAP so there aren't any unforeseen consequences (which is what the patch I first attached does)

And have another ticket that changes how the handling of playback rate is done.
Because *that* would have likely unforeseen consequences and unlikely to be accepted back to aurora

> If SetPlaybackRate(0.5) happened before |mDecoderStateMachine| is created,
> we would have |mCurrentPlaybackRate == mInitialPlaybackRate == 0.5| here and
> SetPlaybackRate(mInitialPlaybackRate) won't be called. This seems wrong.

no because SetPlaybackRate with the same argument is always called right before... actually the call to SetPlaybackRate in the setup is unecessary alltogether.
(In reply to Jean-Yves Avenard [:jya] from comment #11)
> It would seem wiser to me to revert back to the original behaviour for the
> purpose of fixing this bug ASAP so there aren't any unforeseen consequences
> (which is what the patch I first attached does)
> 
> And have another ticket that changes how the handling of playback rate is
> done.
> Because *that* would have likely unforeseen consequences and unlikely to be
> accepted back to aurora

OK.

> no because SetPlaybackRate with the same argument is always called right
> before... actually the call to SetPlaybackRate in the setup is unecessary
> alltogether.

See [1]. It will go to the ELSE block when |mDecoderStateMachine| is not created and |mDecoderStateMachine->SetPlaybackRate| won't be called.

[1] http://hg.mozilla.org/mozilla-central/file/3b7921328fc1/content/media/MediaDecoder.cpp#l1460
Attachment #8489882 - Attachment is obsolete: true
Btw, I prefer to click the "review" button to add review comments instead of clicking "reply" to avoid nested comments going deep.
Attachment #8489833 - Attachment is obsolete: false
Attachment #8489833 - Flags: review?(jwwang)
Comment on attachment 8489833 [details] [diff] [review]
Allow playback rate to be reset to normal

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

mInitialPlaybackRate is tricky. We might be better off just using something like |HTMLMediaElement::mPlaybackRate| which is updated whenever HTMLMediaElement::SetPlaybackRate() is called and passed to mDecoder when needed.
Attachment #8489833 - Flags: review?(jwwang) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/228559ab6d1a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
34 is also marked as affected. Can you please request Aurora uplift approval?
Flags: needinfo?(jyavenard)
Comment on attachment 8489833 [details] [diff] [review]
Allow playback rate to be reset to normal

Approval Request Comment
[Feature/regressing bug #]: 1055843
[User impact if declined]: User can't change playback rate on sites such as youtube
[Describe test coverage new/current, TBPL]: Tested and problem confirmed as fixed. Passes all mochitests.
[Risks and why]: Risk is low. It is really a revert back to the previous behaviour as used in 33 and earlier
[String/UUID change made/needed]: None
Attachment #8489833 - Flags: approval-mozilla-aurora?
Flags: needinfo?(jyavenard)
Comment on attachment 8489833 [details] [diff] [review]
Allow playback rate to be reset to normal

Aurora+
Attachment #8489833 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
QA Whiteboard: [good first verify]
I can verify it's fixed using Windows 7 and the latest Aurora build (Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0)
I can verify it's fixed using Windows 7 and the latest Aurora build (Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0)
Whiteboard: [testday-20141003]
Marking as Verified for Firefox 34 based on comment 22 above... Thank you Gabriela!
Status: RESOLVED → VERIFIED
Target Milestone: mozilla35 → mozilla34
You need to log in before you can comment on or make changes to this bug.