Closed Bug 1278612 Opened 3 years ago Closed 3 years ago

OS X sound output device doesn't update when OS output device is changed

Categories

(Core :: Audio/Video: cubeb, defect, P1, major)

50 Branch
Unspecified
macOS
defect

Tracking

()

VERIFIED FIXED
mozilla51
Tracking Status
firefox47 --- unaffected
firefox48 --- wontfix
firefox49 --- verified
firefox50 --- verified
firefox51 --- verified

People

(Reporter: haik, Assigned: kinetik)

References

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

While watching a streaming video from Amazon.com using Nightly 50 on OS X 10.11 latest, if the system sound output device is switched, for example, from Line Out to Headphones, the sound doesn't switch and continues to play on Line Out.

This works correctly if Silverlight is used instead. (Testing this by disabling DRM playback in the preferences and allowing Silverlight to be used.) The same video on Chrome with Widevine works.

A quick way to switch the system sound output device is to option-click on the speaker icon. See screenshot.

A workaround seems to be jumping forward or backwards to a different place in the video. In my tests, that caused the video to switch to the correct sound output device after it has been changed.
Priority: -- → P1
Component: Audio/Video: Playback → Audio/Video: cubeb
Paul, Matthew -- How much work would be required to fix this?
Rank: 17
Flags: needinfo?(padenot)
Flags: needinfo?(kinetik)
This work has already happened on windows. I believe the osx version can be modeled from the Windows code. I'd say two weeks.
Flags: needinfo?(padenot)
Flags: needinfo?(kinetik)
this is not isolated to widevine. plain youtube shows the same problem. the audio device is only changing once the current video completes.
Summary: [Widevine] OS X sound output device doesn't update when OS output device is changed → OS X sound output device doesn't update when OS output device is changed
this used to work fine btw...
Flags: needinfo?(achronop)
Matthew -- Both Padenot and Achronop are out on PTO for the next 2 weeks. Is this something you can look at, or do I need to find someone else?
Flags: needinfo?(achronop) → needinfo?(kinetik)
Sure thing.
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
Flags: needinfo?(kinetik)
Rank: 17 → 10
Before bug 1251502, we used the DefaultOutputUnit (kAudioUnitSubType_DefaultOutput) which uses whichever output is selected in the Sound->Output preferences and changes automatically as this preference changes.

After bug 1251502, we use the AudioDeviceOutput (kAudioUnitSubType_HALOutput) which requires specifying a device.  Note that using AudioDeviceOutput is essential for capturing input; it's not possible to capture input from DefaultOutputUnit, which is why this change was made.

If cubeb_stream_init wasn't passed a specific device (which is the case for regular media playback), we'd query for the default and start with the same value DefaultOutputUnit might've used but the output would no longer follow preference changes.  Fixing this directly would require the work Paul alludes to in comment 2.

A quicker fix is to use the DefaultOutputUnit in the situation where we are creating an output stream and cubeb_stream_init was not passed a specific device to use.  This restores the pre-bug 1251502 behaviour for regular media playback streams without affecting capture and full-duplex streams.
Attached file patch v0 (obsolete) —
r? jesup since the usual suspects are unavailable.
Attachment #8779966 - Flags: review?(rjesup)
Comment on attachment 8779966 [details] [review]
patch v0

I added a note in github: the "if device == NULL" is now inside "if device != NULL || is_input"; so if it's NULL, and no input, everything gets skipped - no set property at all.  I suspect this isn't intended.  Perhaps moving the "if device == NULL" part out of the new if is the right thing to do?
Attachment #8779966 - Flags: review?(rjesup) → review-
(In reply to Randell Jesup [:jesup] from comment #10)
> Comment on attachment 8779966 [details] [review]
> patch v0
> 
> I added a note in github: the "if device == NULL" is now inside "if device
> != NULL || is_input"; so if it's NULL, and no input, everything gets skipped
> - no set property at all.  I suspect this isn't intended.  Perhaps moving
> the "if device == NULL" part out of the new if is the right thing to do?

It is intended, it's invalid to set those properties on the DefaultOutputUnit.
Comment on attachment 8779966 [details] [review]
patch v0

In that case, r+!
Attachment #8779966 - Flags: review- → review+
What should we do to land this?  Normally it would be import the lib, but I don't know what might else come along, especially for aurora/beta.  My suggestion is to land point-patches in firefox for aurora and beta (or at least for beta), and do an import for Nightly (maybe aurora).  Thanks!
Flags: needinfo?(kinetik)
(In reply to Matthew Gregan [:kinetik] from comment #9)
> Created attachment 8779966 [details] [review]
> patch v0
> 
> r? jesup since the usual suspects are unavailable.

The patch works for me.
(In reply to Randell Jesup [:jesup] from comment #14)
> What should we do to land this?  Normally it would be import the lib, but I
> don't know what might else come along, especially for aurora/beta.  My
> suggestion is to land point-patches in firefox for aurora and beta (or at
> least for beta), and do an import for Nightly (maybe aurora).  Thanks!

We're actually in a tricky position, because importing the library is blocked on bug 1286041 after merging a fix for that bug and then having ASAN reveal a bug.

So I'll prepare a standalone patch for this fix for all branches.
Flags: needinfo?(kinetik)
Carrying forward r+
Attachment #8779966 - Attachment is obsolete: true
Attachment #8780953 - Flags: review+
Attached patch patch for betaSplinter Review
Carrying forward r+
Attachment #8780954 - Flags: review+
Pushed by mgregan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e6551eb6c8e
Use DefaultOutputUnit as the default output device.  r=jesup
Comment on attachment 8780953 [details] [diff] [review]
patch for central and aurora

Approval Request Comment
[Feature/regressing bug #]: bug 1251502
[User impact if declined]: Gecko will not automatically follow the default audio output device on OS X if it is switched during playback
[Describe test coverage new/current, TreeHerder]: tested locally, not covered by automated tests
[Risks and why]: low risk, this reverts the behaviour of the default output device to the pre-bug 1251502 state; if this change causes any regressions they'd likely be in the full-duplex/capture case, which is not yet enabled outside of Nightly for OS X (afaik)
[String/UUID change made/needed]: none
Attachment #8780953 - Flags: approval-mozilla-aurora?
Comment on attachment 8780954 [details] [diff] [review]
patch for beta

Approval Request Comment
[Feature/regressing bug #]: bug 1251502
[User impact if declined]: Gecko will not automatically follow the default audio output device on OS X if it is switched during playback
[Describe test coverage new/current, TreeHerder]: tested locally, not covered by automated tests
[Risks and why]: low risk, this reverts the behaviour of the default output device to the pre-bug 1251502 state; if this change causes any regressions they'd likely be in the full-duplex/capture case, which is not yet enabled outside of Nightly for OS X (afaik)
[String/UUID change made/needed]: none
Attachment #8780954 - Flags: approval-mozilla-beta?
Comment on attachment 8780953 [details] [diff] [review]
patch for central and aurora

Recent regression since 48, we should uplift to Aurora50.
Attachment #8780953 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/2e6551eb6c8e
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Comment on attachment 8780954 [details] [diff] [review]
patch for beta

I just ran into this problem twice today on 48. And it sounds like the risk of causing more regressions is low. Let's uplift to beta!
Attachment #8780954 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Duplicate of this bug: 1294222
This is VERIFIED FIXED on Mac OS X 10.11 for the following builds:

- Firefox RC 49.0 build 2 (id: 20160907153016)
- Firefox DevEdition 50.0a2 (id: 20160909004004)
- Firefox Nightly 51.0a1 (id: 20160909030428)
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.