Closed Bug 1323847 Opened 3 years ago Closed 3 years ago

webm/vp9 audio breaks off in some youtube musics

Categories

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

53 Branch
x86
Windows 10
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla53
Tracking Status
firefox50 --- unaffected
firefox51 --- verified
firefox52 --- verified
firefox53 + verified

People

(Reporter: alice0775, Assigned: jya)

References

Details

(Keywords: regression)

Attachments

(6 files)

Attached image screenshot
+++ This bug was initially created as a clone of Bug #1305284 +++

The problem is back.

Reproducible: always

Steps To Reproduce:
1. Open https://www.youtube.com/watch?v=vmotO-7Bzt4
2. Right-click the HTML5 Player, click "Stats for nerds" and ensure 'Mime Type: video/webm; codecs="vp9"'
3. Listen the music


Actual Results:
At around 4:46, sound breaks off.
I'm feeling it was stopping about hundreds mili-second. It's a large gap.
The progress bar of the HTML5 Player looks like buffer was dropped, or adjusted.(?)

Expected Results:
Finished normally
Attached file about:support
Priority: P1 → --
Version: Trunk → 53 Branch
Assignee: nobody → jyavenard
surprising it's not affecting 52.. the changes mentioned as regressing this is found in 52
[Tracking Requested - why for this release]: Regression, youtube webm playback is broken

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=4802b32ee10badfa52dce93fbb15691108283d14&tochange=c599df1589cc18d70b70b93d1bebb81dbe008ca3

Regressed by: c599df1589cc	karo — Bug 1320829 - updated WebM demuxer to surface alpha information. r=jya
Blocks: 1320829
No longer blocks: 1280023
Flags: needinfo?(kkoorts)
Tracking 53+ as this affects youtube.
Clearing needinfo since jya is handling this.
Flags: needinfo?(kkoorts)
Comment on attachment 8819179 [details]
Bug 1323847: [MSE] P1. Add extra logging.

https://reviewboard.mozilla.org/r/99028/#review99280
Attachment #8819179 - Flags: review?(gsquelart) → review+
52 will be affected, albeit less severely on this particular video, but I don't see why it wouldn't affected on other either.

in fact even 51 would benefit from the changes there
Comment on attachment 8819180 [details]
Bug 1323847: [MSE] P2. Don't evict sample containing currentTime.

https://reviewboard.mozilla.org/r/99030/#review99288

r+ with test fixed (or explained please)

::: dom/media/mediasource/TrackBuffersManager.cpp:447
(Diff revision 1)
> -    if (frame->mTime >= lowerLimit.ToMicroseconds()) {
> +    if (frame->mTime >= lowerLimit.ToMicroseconds() ||
> +        frame->GetEndTime() >= lowerLimit.ToMicroseconds()) {

Don't you mean `frame->GetEndTime() < lowerLimit.ToMicroseconds()`? (or maybe '<='?)

And if not: If frame end time >= lowerLimit, doesn't that imply that frame start time >= lowerLimit as well? I.e., the first test could be dropped.
Attachment #8819180 - Flags: review?(gsquelart) → review+
Comment on attachment 8819181 [details]
Bug 1323847: P3. Don't allocate data for empty buffer.

https://reviewboard.mozilla.org/r/99032/#review99290
Attachment #8819181 - Flags: review?(gsquelart) → review+
Comment on attachment 8819180 [details]
Bug 1323847: [MSE] P2. Don't evict sample containing currentTime.

https://reviewboard.mozilla.org/r/99030/#review99288

> Don't you mean `frame->GetEndTime() < lowerLimit.ToMicroseconds()`? (or maybe '<='?)
> 
> And if not: If frame end time >= lowerLimit, doesn't that imply that frame start time >= lowerLimit as well? I.e., the first test could be dropped.

yes, the first test could be dropped indeed.

however the test is valid, we stop as soon as we find a sample past the currentTime.
Comment on attachment 8819182 [details]
Bug 1323847: [MSE] P4. Bump audio buffer size.

https://reviewboard.mozilla.org/r/99034/#review99296

r+ with commit description fixed:

::: dom/media/mediasource/TrackBuffersManager.cpp:1
(Diff revision 1)
>  /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

In commit description:
- "This cause the appendBuffer as per spec." -- Causes what? :-)
- "reload" -> "reloads"
Attachment #8819182 - Flags: review?(gsquelart) → review+
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dd4e6ff73927
[MSE] P1. Add extra logging. r=gerald
https://hg.mozilla.org/integration/autoland/rev/6fb5f5e18b90
[MSE] P2. Don't evict sample containing currentTime. r=gerald
https://hg.mozilla.org/integration/autoland/rev/a617d810f29c
P3. Don't allocate data for empty buffer. r=gerald
https://hg.mozilla.org/integration/autoland/rev/b368a6563650
[MSE] P4. Bump audio buffer size. r=gerald
Blocks: 1324306
Comment on attachment 8819179 [details]
Bug 1323847: [MSE] P1. Add extra logging.

Approval Request Comment
[Feature/Bug causing the regression]: 1280023
[User impact if declined]: Audio interruption while playing youtube
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: Steps are provided in the excellent description
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: additional logs, and bumping memory threshold to a value closer to what was prior 128023
[String changes made/needed]: none
Attachment #8819179 - Flags: approval-mozilla-beta?
Attachment #8819179 - Flags: approval-mozilla-aurora?
In above comment: uplift request is for all patches.
Hi Alice,
Could you help verify if this issue is fixed as expected on the latest Nightly build? Thanks!
Flags: needinfo?(alice0775)
I verified that the problem(w/ STR comment#0) is no longer reproduced on nightly53.0a1.

https://hg.mozilla.org/mozilla-central/rev/567894f026558e6dada617a3998f29aed06ac7d8
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0 ID:20161220030215
Flags: needinfo?(alice0775)
Comment on attachment 8819179 [details]
Bug 1323847: [MSE] P1. Add extra logging.

Fix a regression related to webm/vp9 audio. Beta51+ & Aurora52+. Should be in 51 beta 10.
Attachment #8819179 - Flags: approval-mozilla-beta?
Attachment #8819179 - Flags: approval-mozilla-beta+
Attachment #8819179 - Flags: approval-mozilla-aurora?
Attachment #8819179 - Flags: approval-mozilla-aurora+
Status: RESOLVED → VERIFIED
Flags: qe-verify+
I could not reproduce this issue following the steps from the description. I also used the nightly from 2016-12-18 and Fx51.0b1 - Fx 51.0b4 on Windows 10 x32 and Windows 10 x64. Is there a possibility that the issue is from the site or am I missing something?
Flags: needinfo?(alice0775)
Yes, it is definitely possible that YouTube has changed their handling of low memory conditions.

The fix is purely a workaround...
(In reply to Cristian Comorasu from comment #28)
> I could not reproduce this issue following the steps from the description. I
> also used the nightly from 2016-12-18 and Fx51.0b1 - Fx 51.0b4 on Windows 10
> x32 and Windows 10 x64. Is there a possibility that the issue is from the
> site or am I missing something?

I do not know.

But I can reproduce on Nightly53.0a1(2016-12-16).
Flags: needinfo?(alice0775)
Reproduced on 53.0a1(2016-12-16), Win 7, media.mediasource.webm.enabled=TRUE.
Verified fixed FX 51b11, 52.0a2 (2017-01-05) (32-bit).
You need to log in before you can comment on or make changes to this bug.