Closed Bug 942657 Opened 6 years ago Closed 6 years ago

Devirtualize AudioStream (and remove MOZ_CUBEB configure option)

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: kinetik, Assigned: kinetik)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qa-])

Attachments

(1 file, 3 obsolete files)

We only have a single AudioStream implementation now, but that implementation is spread between the AudioStream abstract class and the BufferedAudioStream concrete class.  I've got a patch that flattens the hierarchy down to a single AudioStream class.
Attachment #8337487 - Attachment is obsolete: true
Attached patch bug942657_v1.patch (obsolete) — Splinter Review
As pushed to try.  Main difference in this version is the removal of MOZ_CUBEB, since it's messy to make AudioStream optional when it's non-virtual without littering the rest of the code with MOZ_CUBEB ifdefs and I don't think it makes sense to allow it to be disabled anyway (individual libcubeb backends can be disabled).
Blocks: 943161
Apparently that broken some WebRTC stuff... https://tbpl.mozilla.org/?tree=Try&rev=f0d527368fbc
Minor fixes vs try version.

r? gps for build changes (removing MOZ_CUBEB)
r? doublec for everything else.
Attachment #8339715 - Attachment is obsolete: true
Attachment #8339752 - Flags: review?(gps)
Attachment #8339752 - Flags: review?(chris.double)
Comment on attachment 8339752 [details] [diff] [review]
bug942657_v3.patch

Review of attachment 8339752 [details] [diff] [review]:
-----------------------------------------------------------------

r+ covers just the build bits: I can't speak for the sanity of the rest of the patch.

::: configure.in
@@ +5546,5 @@
>  dnl = Check alsa availability on Linux if using libcubeb
>  dnl ====================================================
>  
>  dnl If using libcubeb with Linux, ensure that the alsa library is available
> +if test "$OS_TARGET" = "Linux"; then

The comment above needs updating.

@@ +5570,5 @@
>  dnl = Disable PulseAudio
>  dnl ========================================================
>  
>  dnl If using libcubeb with Linux, ensure that the PA library is available
> +if test "$OS_TARGET" = "Linux" -a -z "$MOZ_B2G"; then

Comment.
Attachment #8339752 - Flags: review?(gps) → review+
Attachment #8339752 - Flags: review?(chris.double) → review+
Thanks! Landed with configure.in comments fixed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc9ebdf27e98
Oops, reintroduced a deadlock.  Simple fix, pushed again:

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c15f3df605a
https://hg.mozilla.org/mozilla-central/rev/6c15f3df605a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Summary: Devirtualize AudioStream → Devirtualize AudioStream (and remove MOZ_CUBEB configure option)
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.