1323847 - webm/vp9 audio breaks off in some youtube musics

Status: VERIFIED FIXED

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

Description

[Alice0775 White]

Attachment: 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

Comment 1

[Alice0775 White]

Attachment: about:support

Comment 2

[Jean-Yves Avenard [:jya]]

surprising it's not affecting 52.. the changes mentioned as regressing this is found in 52

Comment 3

[Alice0775 White]

[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

Comment 4

[Marcia Knous [:marcia]]

Tracking 53+ as this affects youtube.

Comment 5

[Karolien]

Clearing needinfo since jya is handling this.

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1323847: [MSE] P1. Add extra logging.

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

Comment 7

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1323847: [MSE] P2. Don't evict sample containing currentTime.

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

Comment 8

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1323847: P3. Don't allocate data for empty buffer.

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

Comment 9

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1323847: [MSE] P4. Bump audio buffer size.

Under some cases YouTube attempts to append more than 10MB of data ahead of currentTime. This cause the appendBuffer as per spec.

Bug 1320829 slightly increased the size of the MediaRawData object (by 36 bytes) which on average caused an increase of 470kB of the source buffer size causing the eviction threshold to be crossed quicker.
YouTube clears the entire source buffer once a buffer full is reported and reload it all, causing an audible silence.

Bumping the threshold slightly is the only way to get around the problem.

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

Comment 10

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

Comment on attachment 8819179 [details]
Bug 1323847: [MSE] P1. Add extra logging.

https://reviewboard.mozilla.org/r/99028/#review99280

Comment 11

[Jean-Yves Avenard [:jya]]

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 12

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

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.

Comment 13

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

Comment on attachment 8819181 [details]
Bug 1323847: P3. Don't allocate data for empty buffer.

https://reviewboard.mozilla.org/r/99032/#review99290

Comment 14

[Jean-Yves Avenard [:jya]]

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 15

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

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"

Comment 16

[Jean-Yves Avenard [:jya]]

Comment on attachment 8819180 [details]
Bug 1323847: [MSE] P2. Don't evict sample containing currentTime.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99030/diff/1-2/

Comment 17

[Jean-Yves Avenard [:jya]]

Comment on attachment 8819181 [details]
Bug 1323847: P3. Don't allocate data for empty buffer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99032/diff/1-2/

Comment 18

[Jean-Yves Avenard [:jya]]

Comment on attachment 8819182 [details]
Bug 1323847: [MSE] P4. Bump audio buffer size.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99034/diff/1-2/

Comment 19

[Pulsebot]

Pushed by [e-mail]:
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

Comment 20

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

https://hg.mozilla.org/mozilla-central/rev/dd4e6ff73927
https://hg.mozilla.org/mozilla-central/rev/6fb5f5e18b90
https://hg.mozilla.org/mozilla-central/rev/a617d810f29c
https://hg.mozilla.org/mozilla-central/rev/b368a6563650

Comment 21

[Jean-Yves Avenard [:jya]]

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

Comment 22

[Jean-Yves Avenard [:jya]]

In above comment: uplift request is for all patches.

Comment 23

[Gerry Chang [:gchang]]

Hi Alice,
Could you help verify if this issue is fixed as expected on the latest Nightly build? Thanks!

Comment 24

[Alice0775 White]

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

Comment 25

[Gerry Chang [:gchang]]

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.

Comment 26

[Iris Hsiao [:ihsiao]]

Check-in: 
https://hg.mozilla.org/releases/mozilla-aurora/rev/5e31406f2b19
https://hg.mozilla.org/releases/mozilla-aurora/rev/432f1e56a9d7
https://hg.mozilla.org/releases/mozilla-aurora/rev/2e98fec3a9f9
https://hg.mozilla.org/releases/mozilla-aurora/rev/5cfa37b18bd7

Comment 27

[Iris Hsiao [:ihsiao]]

Check-in:
https://hg.mozilla.org/releases/mozilla-beta/rev/9af916419d6a
https://hg.mozilla.org/releases/mozilla-beta/rev/91c9572a8ef5
https://hg.mozilla.org/releases/mozilla-beta/rev/6ea09379d5f7
https://hg.mozilla.org/releases/mozilla-beta/rev/49d337c802e1

Comment 28

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

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?

Comment 29

[Jean-Yves Avenard [:jya]]

Yes, it is definitely possible that YouTube has changed their handling of low memory conditions.

The fix is purely a workaround...

Comment 30

[Alice0775 White]

(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).

Comment 31

[Paul Silaghi, QA [:pauly]]

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).