1253928 - This video stutters after bug 1163223

Status: VERIFIED FIXED

(Core :: Audio/Video: Playback, defect, P1)

Description

[Fanolian]

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.

Comment 1

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

Checking.

Comment 2

[Jean-Yves Avenard [:jya]]

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.

Comment 4

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

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ee97f72fff9

Comment 5

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

You also should remove the call to RemoveGraphFromRunningObjectTable in DirectShowReader's destructor.

Comment 6

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

Attachment: MozReview Request: Bug 1253928 - adjust the time passed to RequestVideoData() by the start time to avoid incorrectly skipping key frames. r=jya.

Review commit: https://reviewboard.mozilla.org/r/38569/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38569/

Comment 7

[Jean-Yves Avenard [:jya]]

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

Comment 8

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

(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.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c4cf5562aa3

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1c4cf5562aa3

Comment 11

[Timothy Nikkel (:tnikkel)]

Is this something we can uplift?

Comment 12

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

Yes, I am preparing patches for Aurora and Beta.

Comment 13

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

Attachment: 1253928_aurora_beta_fix.patch

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

Comment 14

[Jim Jeffery not reading bug-mail 1/2/11]

Just tested on win7 x64 m-c win32 builds both with e10s 'on' & 'off' and for me the video still stalls out.

Comment 15

[Jean-Yves Avenard [:jya]]

(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.

Comment 16

[Jim Jeffery not reading bug-mail 1/2/11]

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.

Comment 17

[Timothy Nikkel (:tnikkel)]

I just tested the latest nightly (with this fix) and it fixes the problem for me.

Comment 18

[Fanolian]

Fixed for me too.

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8728217 [details] [diff] [review]
1253928_aurora_beta_fix.patch

This fix has been verified, taking it in Aurora47 and Beta46.

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1ca1b92f9c10

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/7ebbf2a73158