1259916 - youtube video stops at approx 10:00

Status: RESOLVED FIXED

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

Description

[Alice0775 White]

[Tracking Requested - why for this release]: Playback problem of youtube video

Build Identifier:

Reproducible: always

Steps To Reproduce:
1. Open youtube long video  e.g., https://www.youtube.com/watch?v=eyU3bRy2x44


Actual Results:
Video stops at 09:54.
A throbber is spinning.

Expected Results:
Video should playback properly. Should not stop.

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=8d7ed429bf8537950c26158d9044b6cf1871b9ee&tochange=9ffa65c64e14209ebd0ce7a44b7254a6c5fc6b9e

Regressed by: Bug 1216460

Comment 1

[Alice0775 White]

Forgot 
Build Identifier:
https://hg.mozilla.org/mozilla-central/rev/b2dbee5ca727e87bdaeab9ab60fb83df2a9846a2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0 ID:20160325085113


Graphics
--------

Adapter Description: AMD Radeon HD 6450
Adapter Drivers: aticfx64 aticfx64 aticfx64 aticfx32 aticfx32 aticfx32 atiumd64 atidxx64 atidxx64 atiumdag atidxx32 atidxx32 atiumdva atiumd6a atitmm64
Adapter RAM: 1024
Asynchronous Pan/Zoom: wheel input enabled; touch input enabled
ClearType Parameters: Gamma: 2200 Pixel Structure: R ClearType Level: 100 Enhanced Contrast: 300
Device ID: 0x6779
Direct2D Enabled: true
DirectWrite Enabled: true (6.2.9200.17568)
Driver Date: 7-28-2015
Driver Version: 15.200.1062.1003
GPU #2 Active: false
GPU Accelerated Windows: 1/1 Direct3D 11 (OMTC)
Subsys ID: 23111787
Supports Hardware H264 Decoding: Yes; Using D3D9 API
Vendor ID: 0x1002
WebGL Renderer: Google Inc. -- ANGLE (AMD Radeon HD 6450 Direct3D11 vs_5_0 ps_5_0)
windowLayerManagerRemote: true
AzureCanvasAccelerated: 0
AzureCanvasBackend: direct2d 1.1
AzureContentBackend: direct2d 1.1
AzureFallbackCanvasBackend: cairo



about:media

HTMLMediaElement debug data

https://www.youtube.com/watch?v=eyU3bRy2x44
	mediasource:https://www.youtube.com/2a63ea08-3e5c-43ef-a7c4-25ed9600c2a9
	currentTime: 594.7345
	Quality: 100% (total:17839 dropped:0 corrupted:0)
	Buffered ranges: [(172.301, 594.741)(615.481, 638.941)(660.001, 678.341)(700.701, 723.241)(745.681, 767.521)(790.781, 814.321)(835.801, 859.941)(880.001, 897.581)(920.921, 940.941)]
		SourceBuffer 0
			start=172.301 end=594.741
			start=615.481 end=638.941
			start=660.001 end=678.341
			start=700.701 end=723.241
			start=745.681 end=767.521
			start=790.781 end=814.321
			start=835.801 end=859.941
			start=880.001 end=897.581
			start=920.921 end=940.941
		SourceBuffer 1
			start=0 end=940.941
	Internal Data:
	audio decoder: opus audio decoder
	audio frames decoded: 29737
	video decoder: ffvpx video decoder
	hardware video decoding: disabled
	video frames decoded: 17838 (skipped:0)
	Dumping data for demuxer 193d59f0:
		Dumping Audio Track Buffer(audio/webm; codecs=opus): - mLastAudioTime: 594.741000
			NumSamples:29695 Size:15554983 NextGetSampleIndex:21122 NextInsertionIndex:29695
			Buffered: ranges=[(172.301000, 594.741000), (615.481000, 638.941000), (660.001000, 678.341000), (700.701000, 723.241000), (745.681000, 767.521000), (790.781000, 814.321000), (835.801000, 859.941000), (880.001000, 897.581000), (920.921000, 940.941000)]
		Dumping Video Track Buffer(video/webm; codecs=vp9) - mLastVideoTime: 595.228000
			NumSamples:28200 Size:52295010 NextGetSampleIndex:17839 NextInsertionIndex:28200
			Buffered: ranges=[(0.000000, 940.941000)]

Comment 3

[Ludovic Hirlimann [:Usul]]

I have the same issue on Linux, If I "scroll" then playback starts again.

Comment 4

[Jean-Yves Avenard [:jya]]

In bug 1259713, audio buffer size threshold was changed from 100MB to 12MB.

It appears that YouTube doesn't check for eviction when appending audio data.

(940s buffered is rather over the top too)

Richard, we can increase the size allowed in a source buffer that contains only audio; which size would you recommend?
But having said that, YouTube player obviously doesn't handle this case properly and apparently makes invalid assumptions.

Comment 6

[Jean-Yves Avenard [:jya]]

Temporary workaround,

Create a integer preference media.mediasource.eviction_threshold.audio with the value 104857600

Comment 7

[Alice0775 White]

(In reply to Jean-Yves Avenard [:jya] from comment #6)
> Temporary workaround,
> 
> Create a integer preference media.mediasource.eviction_threshold.audio with
> the value 104857600

Unfortunately, Setting this preference does not fix the problem. The video stops at 32:25.

Comment 8

[Jean-Yves Avenard [:jya]]

Now that would be surprising. Setting this pref to 100MB is identical to reverting bug 1259713

Comment 9

[Radek 'sysKin' Czyz]

I also continue to see the problem with the pref set...

Comment 10

[Radek 'sysKin' Czyz]

(In reply to Jean-Yves Avenard [:jya] from comment #8)
> Setting this pref to 100MB is identical to reverting bug 1259713

Is it this changeset: https://hg.mozilla.org/mozilla-central/rev/9ffa65c64e14 ?

Because if so, there is one further change. Old code was saying:

-    (mEvictionThreshold > aLength) ? mEvictionThreshold - aLength : aLength;

New code replaced it with:

+    std::max(EvictionThreshold() - aLength, aLength);

which is

     (mEvictionThreshold - aLength > aLength) ? mEvictionThreshold - aLength : aLength;

Apologies if I'm looking at the wrong thing or otherwise wasting your time.

Comment 11

[Jean-Yves Avenard [:jya]]

regression is caused by https://hg.mozilla.org/integration/mozilla-inbound/rev/6495286a5790#l4.62

i should have noticed in my review :(

Comment 12

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1259916: [MSE] P1. Fix eviction. r?gerald

Bug 1216460 introduced a regression which would cause to always evict from both ends of the current track buffer.

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

Comment 13

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1259916: [MSE] P2. Bump audio source buffer eviction threshold to 30MB. r?gerald

20MB appears to work, but just to be safe until we get confirmation from YouTube on what is a safe value to use.

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

Comment 14

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1259916: [MSE] P3. Simplify eviction calculation logic. r?gerald

Also, remove no longer used code and update comments to properly reflect the current algorithms used.

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

Comment 15

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8735258 [details]
MozReview Request: Bug 1259916: [MSE] P1. Fix eviction. r?gerald

https://reviewboard.mozilla.org/r/42671/#review39139

Comment 16

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8735259 [details]
MozReview Request: Bug 1259916: [MSE] P2. Bump audio source buffer eviction threshold to 30MB. r?gerald

https://reviewboard.mozilla.org/r/42673/#review39141

Comment 17

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8735260 [details]
MozReview Request: Bug 1259916: [MSE] P3. Simplify eviction calculation logic. r?gerald

https://reviewboard.mozilla.org/r/42675/#review39143

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d864ffed536f
https://hg.mozilla.org/integration/mozilla-inbound/rev/732f589f694e
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcd0f253bb32

Comment 19

[Jean-Yves Avenard [:jya]]

Could you please verify that this is now working for you?

Builds available at https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=bcd0f253bb32

Comment 20

[Alice0775 White]

(In reply to Jean-Yves Avenard [:jya] from comment #19)
> Could you please verify that this is now working for you?
> 
> Builds available at
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=bcd0f253bb32

I can confirm the the problem is fixed on the m-i build[1].
The 3hr video was properly playback.


[1]
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcd0f253bb32a6d1db5235dd03ab5996b6e0efbf
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0 ID:20160327165228

Comment 21

[Radek 'sysKin' Czyz]

(In reply to Jean-Yves Avenard [:jya] from comment #19)
> Could you please verify that this is now working for you?

It is indeed working perfectly \o/

Comment 22

[Jean-Yves Avenard [:jya]]

Would be interesting to find out what the lowest value we can get away with media.mediasource.eviction_threshold.audio and still have YouTube playing fine.

20MB (so set it to 20*1024*1024) worked for me.

I had been under the assumption that chrome used 15MB for audio, but obviously was mistaken..

Comment 23

[psn]

(In reply to Jean-Yves Avenard [:jya] from comment #19)

Both 32 & 64-bit 'inbound' builds are playing fine (unlike regular updates).

Comment 25

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

https://hg.mozilla.org/mozilla-central/rev/d864ffed536f
https://hg.mozilla.org/mozilla-central/rev/732f589f694e
https://hg.mozilla.org/mozilla-central/rev/bcd0f253bb32

Comment 26

[Eugen Sawin [:esawin]]

Thanks for the quick fix, jya. The buffer-size situation is so fragile.

Side note:
We expect |buffer.Length() > 0| in TrackBuffersManager::DoEvictData, but there is nothing explicitly that would guard against it in that function. The early return for the case |buffer.Length() == 0| was there to prevent out-of-bounds access in [1] because of the arithmetic conversion in [2] (i would be initialized with |(int32_t) ((size_t) -1))|).

[1] https://dxr.mozilla.org/mozilla-central/source/dom/media/mediasource/TrackBuffersManager.cpp#502
[2] https://dxr.mozilla.org/mozilla-central/source/dom/media/mediasource/TrackBuffersManager.cpp#501

Comment 27

[Jean-Yves Avenard [:jya]]

(In reply to Eugen Sawin [:esawin] from comment #26)
> Thanks for the quick fix, jya. The buffer-size situation is so fragile.
> 
> Side note:
> We expect |buffer.Length() > 0| in TrackBuffersManager::DoEvictData, but
> there is nothing explicitly that would guard against it in that function.
> The early return for the case |buffer.Length() == 0| was there to prevent
> out-of-bounds access in [1] because of the arithmetic conversion in [2] (i
> would be initialized with |(int32_t) ((size_t) -1))|).

No you don't, because it wouldn't go beyond https://dxr.mozilla.org/mozilla-central/source/dom/media/mediasource/TrackBuffersManager.cpp#489

As mSizeSourceBuffer would be zero.

You only entered into the case you described where you read the array because you had changed the order in which finalSize was calculated; and more particularly mSizeSourceBuffer is updated during CodedFrameRemoval. As such, it is imperative to read mSizeSourceBuffer *before* calling CodedFrameRemoval

This was the real cause of this regression.

I shouldn't have noticed during my review, sorry for that.

Comment 29

[Fahmida Noor]

I have reproduced this bug with Firefox Nightly 48.0a1 (Build Id : 20160326030430) on Windows 7(64-bit) with the instructions from comment 0.

Verified as fixed with Latest Firefox Beta 48.0b7 (Build ID : 20160711002726)

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0

[bugday-20160713]