MozReview Request: Bug 1253928 - adjust the time passed to RequestVideoData() by the start time to avoid incorrectly skipping key frames. r=jya.
58 bytes, text/x-review-board-request
1.49 KB, patch
|Details | Diff | Splinter Review|
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:47.0) Gecko/20100101 Firefox/47.0 Build ID: 20160306030215 Steps to reproduce: Visit https://streamable.com/fbvj, or direct video link: https://cdn.streamable.com/video/mp4/fbvj.mp4 (WARNING: loud noise) Actual results: Video stutters but audio plays smoothly. From Mozregression: Last good Nightly: 2015-06-17 First bad Nightly: 2015-06-18 Pushlog_url: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d7c148c84594&tochange=a3f280b6f8d5 Further bisection: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f049fcbdfe1e150a50914c68f9caa63061c8dbff&tochange=c1b33c43f0c5debb3968d236c32b80a86d79c13a Just FYI, https://cdn.streamable.com/video/mp4-mobile/fbvj.mp4, which can be found at https://streamable.com/e/fbvj, does not have any stutterings.
5 years ago
Status: UNCONFIRMED → NEW
Component: Audio/Video → Audio/Video: Playback
Ever confirmed: true
This change had been confirmed as regressing another case a while back due to the miscalculation of the start time. It had been fixed though. Maybe it's another.
You also should remove the call to RemoveGraphFromRunningObjectTable in DirectShowReader's destructor.
Review commit: https://reviewboard.mozilla.org/r/38569/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/38569/
Attachment #8727645 - Flags: review?(jyavenard)
Attachment #8727645 - Flags: review?(jyavenard) → review+
Comment on attachment 8727645 [details] MozReview Request: Bug 1253928 - adjust the time passed to RequestVideoData() by the start time to avoid incorrectly skipping key frames. r=jya. https://reviewboard.mozilla.org/r/38569/#review35221 ::: dom/media/MediaDecoderStateMachine.cpp:1778 (Diff revision 1) > + DECODER_STATE_SEEKING || !mStartTimeRendezvous->HaveStartTime() Please use the helper MDSM::StartTime() to be consistent with the rest of the code. except for the first frame, HaveStartTime() should always be true, may be more explicit/readable to test mSentFirstFrameLoadedEvent don't you think? or add a comment explaining that here. Otherwise, what about having a helper function that returns the time in the MDSM time reference? seeing that this calculation is done in more than one place. like int64_t currentTime = mState ==DECODER_STATE_SEEKING ? 0 : GetReaderTime(GetMediaTime)) Where GetReaderTime does aTime + StartTime() Ideally should switch GetMediaTime() to TimeUnit while at it
(In reply to Jean-Yves Avenard [:jya] from comment #7) > except for the first frame, HaveStartTime() should always be true, may be > more explicit/readable to test mSentFirstFrameLoadedEvent don't you think? Yeah, I will test |mSentFirstFrameLoadedEvent| instead. > Otherwise, what about having a helper function that returns the time in the > MDSM time reference? seeing that this calculation is done in more than one > place. Once bug 1250054 is fixed, MDSM won't have to deal with start time anymore. > Ideally should switch GetMediaTime() to TimeUnit while at it That will deserve another bug to switch all use of int64_t to TimeUnit for MDSM.
Is this something we can uplift?
Yes, I am preparing patches for Aurora and Beta.
Approval Request Comment [Feature/regressing bug #]:1163223 [User impact if declined]: intermittent stuck will be experienced when watching some videos like: https://cdn.streamable.com/video/mp4/fbvj.mp4 https://streamable.com/ndak https://streamable.com/tu0l [Describe test coverage new/current, TreeHerder]:TreeHerder + manual test [Risks and why]: low, the change is very simple [String/UUID change made/needed]:none
Just tested on win7 x64 m-c win32 builds both with e10s 'on' & 'off' and for me the video still stalls out.
(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #14) > Just tested on win7 x64 m-c win32 builds both with e10s 'on' & 'off' and for > me the video still stalls out. which m-c build did you use? something you built yourself or one of the nightly built ? the current nightly is version 68d3781deda0d4d58ec9877862830db89669b3a5 and doesn't contain the change yet. Tomorrow's nightly will. You can test this one: http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-central-win32/1457481749/firefox-48.0a1.en-US.win32.installer.exe warning, this version will not auto-update, you'll need to reinstall the original nightly if you install the above in the default location.
I tested with the cset that this bug landed in: cset: 886b5480b5781f204b89dc9e10bc991a0fa3d3c9 from m-c (tinderbox) builds. I mostly test all the time with the latest m-c builds.
I just tested the latest nightly (with this fix) and it fixes the problem for me.
Fixed for me too.
Status: RESOLVED → VERIFIED
Comment on attachment 8728217 [details] [diff] [review] 1253928_aurora_beta_fix.patch This fix has been verified, taking it in Aurora47 and Beta46.
You need to log in before you can comment on or make changes to this bug.