1123768 - Playback of short audio data truncated on Vista upwards

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Angelo Borsotti]

Attachment: CLICK10A.ogg

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20150108202552

Steps to reproduce:

Open the attached ogg file, and play it.


Actual results:

The file is played, but the result is quite different from the one played by Firefox 34.


Expected results:

Play it with Firefox 34 or Chrome, and hear the difference.

Comment 1

[Alice0775 White]

Regressionn window(m-i)
Good: (like shutter sound of a camera)
https://hg.mozilla.org/integration/mozilla-inbound/rev/76b604c4556c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:37.0) Gecko/20100101 Firefox/37.0 ID:20141211174354
Bad: (ban!)
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca68c68e4835
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:37.0) Gecko/20100101 Firefox/37.0 ID:20141211182054
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=76b604c4556c&tochange=ca68c68e4835

Regressed by: Bug 1109802

Comment 2

[Alice0775 White]

[Tracking Requested - why for this release]:

Comment 3

[Matthew Gregan [:kinetik]]

The root cause of this seems to be the stream drain functionality not behaving correctly.  That's the signal that all audio has become audible and we should be waiting for that before destroying the stream.

What was the behaviour in Firefox 33?  Bug 1027713 landed in Firefox 34, which included a leak of IAudioStreamVolume that would keep the stream alive after we tried to destroy it.  That would allow queued audio to continue playing.

That leak was fixed in Firefox 35 with bug 1109802, and should return us to the behaviour we had in 33.  If there's a difference between 33 and 35, there may be another factor contributing to this bug.

Comment 4

[Matthew Gregan [:kinetik]]

Confirming that this was broken before 34 as well -- in 33 the audio is truncated and sounds the same as nightly.

33 - truncated
34 - OK
nightly - truncated

(also note: this shouldn't be Ogg specific if it's a bug in the audio code, any short sound will be affected)

Comment 5

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

Tracking for 36+. This is the first report that I've seen about audio issues in 35. Do you have an idea of the duration of sound that will be affected by this issue?

Comment 6

[Matthew Gregan [:kinetik]]

Confirmed it's not Ogg specific by testing with a Wave; updating summary to reflect.

Also tested with the old (now only used on Windows XP) WinMM audio backend, where it works fine, so the original regression may go all the way back to bug 866675 in Firefox 27.


(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #5)
> Do you have an idea of the duration of sound that will be affected by
> this issue?

Seems to be any audio (tested a 2.5s long file of the original testcase concatenated 16 times), not length specific.  We're probably losing the last 50-100ms of audio.  It's more obvious with a short file because longer form content (e.g. video, music) tends to be quiet in the last fractions of a second.

Comment 7

[Matthew Gregan [:kinetik]]

Here's the code in question:

http://hg.mozilla.org/mozilla-central/annotate/c2df1daf74c3/media/libcubeb/src/cubeb_wasapi.cpp#l468

We used to wait until padding was 0 before calling state_callback to indicate the stream had drained.  That code was removed in bug 1108455, which landed in Firefox 34 and was backported to 31.

It doesn't look like the change in bug 1108455 was critical for the underlying fix, but was included on a hunch.  I'll ni? padenot to see where that went.  If we don't have good data that it solved a problem, I think we should revert that change.

Comment 8

[Paul Adenot (:padenot)]

Attachment: patch

So, we could just backout the patch, but I went and look at all the code I could find that use WASAPI, and they all Sleep() (when they care about draining, some code does not bother) in the callback, instead of checking the padding value, so I decided to do the same. This fixes the issue as well.

Comment 9

[Matthew Gregan [:kinetik]]

Comment on attachment 8559240 [details] [diff] [review]
patch

What's the argument for not using the old method?

I'm not a huge fan of using a Sleep().  If we have to go down this route, I'd rather use a timeout on the WaitForMultipleObjects or something.

Comment 10

[Paul Adenot (:padenot)]

(In reply to Matthew Gregan [:kinetik] from comment #9)
> Comment on attachment 8559240 [details] [diff] [review]
> patch
> 
> What's the argument for not using the old method?
> 
> I'm not a huge fan of using a Sleep().  If we have to go down this route,
> I'd rather use a timeout on the WaitForMultipleObjects or something.

I could be wrong, but I've seen cases where Get

Comment 11

[Paul Adenot (:padenot)]

... where GetCurrentPadding seemed to not work (under some configuration, and only looking at crash dumps), and nobody else uses it for this purpose.

We can certainly just backout the initial patch, though, and see what happens, I'll prepare that now.

Comment 12

[Paul Adenot (:padenot)]

Attachment: backout patch

Comment 13

[Paul Adenot (:padenot)]

Attachment: backout patch

hrm, forgot to qref

Comment 14

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/47d0c95998a7

Comment 15

[Paul Adenot (:padenot)]

Comment on attachment 8559781 [details] [diff] [review]
backout patch

Approval Request Comment
[Feature/regressing bug #]: 1108455
[User impact if declined]: the end of short sounds played with <audio> is cut
[Describe test coverage new/current, TreeHerder]: unchanged
[Risks and why]: this is a backout, it worked fine before
[String/UUID change made/needed]: none

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8559781 [details] [diff] [review]
backout patch

Taking ir to make sure we have it in beta 8

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/47d0c95998a7

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d5d2d9c8e4f3
https://hg.mozilla.org/releases/mozilla-beta/rev/9a36ec122aa5