Closed Bug 1259916 Opened 4 years ago Closed 4 years ago
youtube video stops at approx 10:00
58 bytes, text/x-review-board-request
MozReview Request: Bug 1259916: [MSE] P2. Bump audio source buffer eviction threshold to 30MB. r?gerald
58 bytes, text/x-review-board-request
58 bytes, text/x-review-board-request
[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
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)]
4 years ago
Duplicate of this bug: 1259713
I have the same issue on Linux, If I "scroll" then playback starts again.
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.
Temporary workaround, Create a integer preference media.mediasource.eviction_threshold.audio with the value 104857600
(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.
Now that would be surprising. Setting this pref to 100MB is identical to reverting bug 1259713
I also continue to see the problem with the pref set...
(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.
regression is caused by https://hg.mozilla.org/integration/mozilla-inbound/rev/6495286a5790#l4.62 i should have noticed in my review :(
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/
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/
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 on attachment 8735258 [details] MozReview Request: Bug 1259916: [MSE] P1. Fix eviction. r?gerald https://reviewboard.mozilla.org/r/42671/#review39139
Attachment #8735258 - Flags: review?(gsquelart) → review+
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
Attachment #8735259 - Flags: review?(gsquelart) → review+
Comment on attachment 8735260 [details] MozReview Request: Bug 1259916: [MSE] P3. Simplify eviction calculation logic. r?gerald https://reviewboard.mozilla.org/r/42675/#review39143
Attachment #8735260 - Flags: review?(gsquelart) → review+
Could you please verify that this is now working for you? Builds available at https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=bcd0f253bb32
(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. The 3hr video was properly playback.  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
(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/
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..
(In reply to Jean-Yves Avenard [:jya] from comment #19) Both 32 & 64-bit 'inbound' builds are playing fine (unlike regular updates).
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  because of the arithmetic conversion in  (i would be initialized with |(int32_t) ((size_t) -1))|).  https://dxr.mozilla.org/mozilla-central/source/dom/media/mediasource/TrackBuffersManager.cpp#502  https://dxr.mozilla.org/mozilla-central/source/dom/media/mediasource/TrackBuffersManager.cpp#501
(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  because of the arithmetic conversion in  (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.
QA Whiteboard: [good first verify]
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]
You need to log in before you can comment on or make changes to this bug.