Closed Bug 1123768 Opened 5 years ago Closed 5 years ago

Playback of short audio data truncated on Vista upwards

Categories

(Core :: Audio/Video, defect)

35 Branch
x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox35 --- wontfix
firefox36 + fixed
firefox37 + fixed
firefox38 + fixed
firefox-esr31 --- unaffected

People

(Reporter: angelo.borsotti, Assigned: padenot)

References

Details

(Keywords: platform-parity, regression)

Attachments

(2 files, 2 obsolete files)

Attached video 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.
Component: Untriaged → Video/Audio
Product: Firefox → Core
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
Ever confirmed: true
Keywords: pp, regression
[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.
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
Attached patch patch (obsolete) — Splinter Review
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.
Attached patch backout patch (obsolete) — Splinter Review
Attachment #8559240 - Attachment is obsolete: true
Attachment #8559765 - Flags: review?(kinetik)
Attached patch backout patchSplinter Review
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
Closed: 5 years ago
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.