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)
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)
1.04 KB,
patch
|
jwwang
:
review+
lmandel
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Comment 1•10 years ago
|
||
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
status-firefox33:
--- → unaffected
status-firefox34:
--- → affected
status-firefox35:
--- → affected
tracking-firefox34:
--- → ?
tracking-firefox35:
--- → ?
Ever confirmed: true
Keywords: regression
Version: 35 Branch → 34 Branch
Updated•10 years ago
|
Assignee | ||
Comment 2•10 years ago
|
||
Do not exit early when we revert back to initial speed
Attachment #8489833 -
Flags: review?(cpearce)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
Comment 3•10 years ago
|
||
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.
Assignee | ||
Comment 4•10 years ago
|
||
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)
Comment 5•10 years ago
|
||
(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.
Assignee | ||
Comment 6•10 years ago
|
||
"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. "
Comment 7•10 years ago
|
||
I think that is the same DefaultPlaybackRate we are talking about.
Assignee | ||
Comment 8•10 years ago
|
||
Do not exit early when we revert back to initial speed. Prevent SetPlaybackRate to be called twice at setup.
Attachment #8489882 -
Flags: review?(cpearce)
Assignee | ||
Updated•10 years ago
|
Attachment #8489833 -
Attachment is obsolete: true
Attachment #8489833 -
Flags: review?(cpearce)
Comment 9•10 years ago
|
||
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 10•10 years ago
|
||
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)
Assignee | ||
Comment 11•10 years ago
|
||
(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.
Comment 12•10 years ago
|
||
(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
Assignee | ||
Updated•10 years ago
|
Attachment #8489882 -
Attachment is obsolete: true
Comment 13•10 years ago
|
||
Btw, I prefer to click the "review" button to add review comments instead of clicking "reply" to avoid nested comments going deep.
Assignee | ||
Updated•10 years ago
|
Attachment #8489833 -
Attachment is obsolete: false
Attachment #8489833 -
Flags: review?(jwwang)
Comment 14•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 15•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/228559ab6d1a
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/228559ab6d1a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment 17•10 years ago
|
||
34 is also marked as affected. Can you please request Aurora uplift approval?
Flags: needinfo?(jyavenard)
Assignee | ||
Comment 18•10 years ago
|
||
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 19•10 years ago
|
||
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+
Updated•10 years ago
|
QA Whiteboard: [good first verify]
Comment 21•10 years ago
|
||
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)
Comment 22•10 years ago
|
||
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)
Updated•10 years ago
|
Whiteboard: [testday-20141003]
Comment 23•10 years ago
|
||
Marking as Verified for Firefox 34 based on comment 22 above... Thank you Gabriela!
Updated•10 years ago
|
Status: RESOLVED → VERIFIED
Target Milestone: mozilla35 → mozilla34
Updated•10 years ago
|
Target Milestone: mozilla34 → mozilla35
You need to log in
before you can comment on or make changes to this bug.
Description
•