This video stutters after bug 1163223

VERIFIED FIXED in Firefox 46

Status

()

defect
P1
normal
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: Fanolian+bugzilla, Assigned: jwwang)

Tracking

({regression})

41 Branch
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed, firefox47 fixed, firefox48 fixed)

Details

()

Attachments

(2 attachments)

Reporter

Description

3 years ago
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.
Reporter

Updated

3 years ago
Blocks: 1163223
Has STR: --- → yes
Keywords: regression
Version: 47 Branch → 41 Branch
Status: UNCONFIRMED → NEW
Component: Audio/Video → Audio/Video: Playback
Ever confirmed: true
Assignee

Comment 1

3 years ago
Checking.
Assignee

Updated

3 years ago
Assignee: nobody → jwwang
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.
Assignee

Updated

3 years ago
Duplicate of this bug: 1253903
You also should remove the call to RemoveGraphFromRunningObjectTable in DirectShowReader's destructor.
Priority: -- → P1
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
Assignee

Comment 8

3 years ago
(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 10

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1c4cf5562aa3
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Is this something we can uplift?
Flags: needinfo?(jwwang)
Assignee

Comment 12

3 years ago
Yes, I am preparing patches for Aurora and Beta.
Flags: needinfo?(jwwang)
Assignee

Comment 13

3 years ago
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
Attachment #8728217 - Flags: review+
Attachment #8728217 - Flags: approval-mozilla-beta?
Attachment #8728217 - Flags: approval-mozilla-aurora?
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.
Reporter

Comment 18

3 years ago
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.
Attachment #8728217 - Flags: approval-mozilla-beta?
Attachment #8728217 - Flags: approval-mozilla-beta+
Attachment #8728217 - Flags: approval-mozilla-aurora?
Attachment #8728217 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.