Playback of short audio data truncated on Vista upwards

RESOLVED FIXED in Firefox 36

Status

()

Core
Audio/Video
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Angelo Borsotti, Assigned: padenot)

Tracking

({pp, regression})

35 Branch
mozilla38
x86
Windows 7
pp, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox35 wontfix, firefox36+ fixed, firefox37+ fixed, firefox38+ fixed, firefox-esr31 unaffected)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
Created attachment 8551864 [details]
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.

Updated

3 years ago
Component: Untriaged → Video/Audio
Product: Firefox → Core

Comment 1

3 years ago
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
Blocks: 1109802
Status: UNCONFIRMED → NEW
status-firefox35: --- → affected
status-firefox36: --- → affected
status-firefox37: --- → affected
status-firefox38: --- → affected
status-firefox-esr31: --- → unaffected
Ever confirmed: true
Keywords: pp, regression

Comment 2

3 years ago
[Tracking Requested - why for this release]:
tracking-firefox36: --- → ?
tracking-firefox37: --- → ?
tracking-firefox38: --- → ?
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?
tracking-firefox36: ? → +
tracking-firefox37: ? → +
tracking-firefox38: ? → +
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.
Summary: ogg audio formats not played properly in firefox 35 → Playback of short audio data truncated
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.
Flags: needinfo?(padenot)
Summary: Playback of short audio data truncated → Playback of short audio data truncated on Vista upwards
status-firefox35: affected → wontfix
Created attachment 8559240 [details] [diff] [review]
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.
Flags: needinfo?(padenot)
Attachment #8559240 - Flags: review?(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.
Attachment #8559240 - Flags: review?(kinetik) → review-
(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
Assignee: nobody → 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.
Created attachment 8559765 [details] [diff] [review]
backout patch
Attachment #8559240 - Attachment is obsolete: true
Attachment #8559765 - Flags: review?(kinetik)
Created attachment 8559781 [details] [diff] [review]
backout patch

hrm, forgot to qref
Attachment #8559765 - Attachment is obsolete: true
Attachment #8559765 - Flags: review?(kinetik)
Attachment #8559781 - Flags: review?(kinetik)
Attachment #8559781 - Flags: review?(kinetik) → review+
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
Attachment #8559781 - Flags: approval-mozilla-beta?
Attachment #8559781 - Flags: approval-mozilla-aurora?
Comment on attachment 8559781 [details] [diff] [review]
backout patch

Taking ir to make sure we have it in beta 8
Attachment #8559781 - Flags: approval-mozilla-beta?
Attachment #8559781 - Flags: approval-mozilla-beta+
Attachment #8559781 - Flags: approval-mozilla-aurora?
Attachment #8559781 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/47d0c95998a7
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox38: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Blocks: 866675
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.