Closed Bug 1139271 Opened 5 years ago Closed 5 years ago

Video stalls due to a gap in audio

Categories

(Core :: Audio/Video, defect, P2)

37 Branch
x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox36 --- unaffected
firefox37 + fixed
firefox38 + fixed
firefox39 + fixed

People

(Reporter: ajones, Assigned: jya)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

This video seems to have somehow hit a gap in the audio stream:

HTMLMediaElement debug data

https://www.youtube.com/watch?v=d7zLIm3ipkM&index=26&list=LL2pmfLm7iq6Ov1UwYrWYkZA
	mediasource:https://www.youtube.com/69090b09-4e6e-4ab0-8f33-08548ccbedb3
	currentTime: 68.840022
		SourceBuffer 0
			start=0 end=70.007505
			start=80.015963 end=278.383537
		SourceBuffer 1
			start=0 end=278.28
	Internal Data:
	Dumping data for reader c2aeac0:
		Dumping Audio Track Decoders: - mLastAudioTime: 70.008162
			Reader 0: 100bf000 ranges=[(0.000000, 70.007505), (80.015963, 278.383537)] active=false size=4469781
		Dumping Video Track Decoders - mLastVideoTime: 69.280000
			Reader 3: ee81400 ranges=[(210.600000, 278.280000)] active=false size=4105181
			Reader 2: 15a4bc00 ranges=[(48.600000, 210.600000)] active=true size=4886615
			Reader 1: 15315800 ranges=[(27.000000, 48.600000)] active=false size=659028
			Reader 0: 10131400 ranges=[(0.000000, 32.400000)] active=false size=446501
I've seen this happening from time to time. No eviction occurred, yet the gap was there.. Seems to I dicate to me a bug with youtube.
We've got 1 audio decoder with two time ranges, but we only append to an existing decoder if the end of one time range matches the start of the next. Something is not right. I'll try to get a log.
one possibility would be that the data passed to appendBuffer doesn't start with a moof or styp, but instead the previous segment contained a partial moof, or contained a moof+partial mdat.

We don't handle partial media segment at this stage.
Priority: -- → P2
Assignee: nobody → jyavenard
[Tracking Requested - why for this release]:
I have a way to reproduce the issue consistently now thanks to info provided by Google. 
I'm hoping I'll have it done this week-end. 

I hope this is the last case that can cause stalls in YouTube
Add logs when mp4's atoms are invalid
Attachment #8577612 - Flags: review?(ajones)
Attached patch imported patch _p2.patch (obsolete) — Splinter Review
If a moof is incomplete, MoofParser::mOffset would be updated causing this moof to never be parsed again, leading to gaps in the buffered range. Ideally, Box.IsAvailable() should return false if we don't have the entire data available in the stream. To come.
Attachment #8577613 - Flags: review?(ajones)
Can return a sample if we found one moof, so do so only abort if we found no valid moof.
Attachment #8577614 - Flags: review?(ajones)
Attachment #8577613 - Attachment is obsolete: true
Attachment #8577613 - Flags: review?(ajones)
Only consider a box as available if entire content is present. Avoid having non properly handled partial read.
Attachment #8577617 - Flags: review?(ajones)
Comment on attachment 8577612 [details] [diff] [review]
Part1. Add logging when encountering invalid atmos

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

::: media/libstagefright/binding/MoofParser.cpp
@@ +737,5 @@
>    mValid = true;
>  }
> +
> +#undef LOG
> +#undef LOGV

Looks like we're undefing the wrong things here. Should be LOG, TOSTIRNG, STRINGIFY, __func__.

Suggestion: It probably belongs in a separate header file.
Attachment #8577612 - Flags: review?(ajones) → review+
Attachment #8577614 - Flags: review?(ajones) → review+
Attachment #8577617 - Flags: review?(ajones) → review+
Comment on attachment 8577612 [details] [diff] [review]
Part1. Add logging when encountering invalid atmos

Approval is for all patches.

Approval Request Comment
[Feature/regressing bug #]:1139271
[User impact if declined]: YouTube playback will sometimes stall
[Describe test coverage new/current, TreeHerder]:Not hit central yet. Sample test case to reproduce issue provided by Google.
[Risks and why]: Low. This is MSE so no regression can be introduced
[String/UUID change made/needed]:None
Attachment #8577612 - Flags: approval-mozilla-beta?
Attachment #8577612 - Flags: approval-mozilla-aurora?
Comment on attachment 8577612 [details] [diff] [review]
Part1. Add logging when encountering invalid atmos

Approving uplift for Beta given low risk.
Attachment #8577612 - Flags: approval-mozilla-beta?
Attachment #8577612 - Flags: approval-mozilla-beta+
Attachment #8577612 - Flags: approval-mozilla-aurora?
Attachment #8577612 - Flags: approval-mozilla-aurora+
Duplicate of this bug: 1142992
Duplicate of this bug: 1144172
You need to log in before you can comment on or make changes to this bug.