1139271 - Video stalls due to a gap in audio

Status: RESOLVED FIXED

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

Description

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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

Comment 1

[Jean-Yves Avenard [:jya]]

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.

Comment 2

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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.

Comment 3

[Jean-Yves Avenard [:jya]]

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.

Comment 4

[Sheila Mooney]

[Tracking Requested - why for this release]:

Comment 5

[Jean-Yves Avenard [:jya]]

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

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: Part1. Add logging when encountering invalid atmos

Add logs when mp4's atoms are invalid

Comment 7

[Jean-Yves Avenard [:jya]]

Attachment: imported patch _p2.patch

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.

Comment 8

[Jean-Yves Avenard [:jya]]

Attachment: Part2. Ignore partial moof

Can return a sample if we found one moof, so do so only abort if we found no valid moof.

Comment 9

[Jean-Yves Avenard [:jya]]

Attachment: Part3. Only consider a Box to be available if entire content is available

Only consider a box as available if entire content is present. Avoid having non properly handled partial read.

Comment 10

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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.

Comment 11

[Jean-Yves Avenard [:jya]]

Following rebase due to bug 1141785

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8d9bee3deba7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4d81c5f8293d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/87d3cef0f4aa

Comment 12

[Lawrence Mandel [:lmandel] (use needinfo)]

Tracking 37+ for MSE related playback bug.

Comment 13

[Jean-Yves Avenard [:jya]]

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

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8d9bee3deba7
https://hg.mozilla.org/mozilla-central/rev/4d81c5f8293d
https://hg.mozilla.org/mozilla-central/rev/87d3cef0f4aa

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8577612 [details] [diff] [review]
Part1. Add logging when encountering invalid atmos

Approving uplift for Beta given low risk.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b742cde7beed
https://hg.mozilla.org/releases/mozilla-aurora/rev/22a4e4d8d14a
https://hg.mozilla.org/releases/mozilla-aurora/rev/ea08b1f7c7be

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/202177831c59
https://hg.mozilla.org/releases/mozilla-beta/rev/21384861c447
https://hg.mozilla.org/releases/mozilla-beta/rev/f4c0cec35772

Comment 20

[Karl Tomlinson (:karlt)]

Part 3 was essentially reverted in https://hg.mozilla.org/integration/mozilla-inbound/rev/cd65aead5419#l1.30