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.

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

[Tracking Requested - why for this release]:

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.

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)

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?

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.

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.

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

(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

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

hrm, forgot to qref

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 on attachment 8559781 [details] [diff] [review] backout patch Taking ir to make sure we have it in beta 8

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