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

Status: VERIFIED FIXED

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

Description

[Haik Aftandilian [:haik]]

Attachment: Screenshot showing how I switch the sound output device, using option-click on the speaker icon

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.

Comment 1

[Maire Reavy [:mreavy]]

Paul, Matthew -- How much work would be required to fix this?

Comment 2

[Paul Adenot (:padenot)]

This work has already happened on windows. I believe the osx version can be modeled from the Windows code. I'd say two weeks.

Comment 3

[Jean-Yves Avenard [:jya]]

this is not isolated to widevine. plain youtube shows the same problem. the audio device is only changing once the current video completes.

Comment 4

[Jean-Yves Avenard [:jya]]

this used to work fine btw...

Comment 5

[Matthew Gregan [:kinetik]]

This was regressed by https://github.com/kinetiknz/cubeb/commit/6b2c610ec860d1859d206d66327c952e9d4b0316 which landed in bug 1251502.

Comment 6

[Maire Reavy [:mreavy]]

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?

Comment 7

[Matthew Gregan [:kinetik]]

Sure thing.

Comment 8

[Matthew Gregan [:kinetik]]

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.

Comment 9

[Matthew Gregan [:kinetik]]

Attachment: patch v0

r? jesup since the usual suspects are unavailable.

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

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?

Comment 11

[Matthew Gregan [:kinetik]]

(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 12

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8779966 [details] [review]
patch v0

In that case, r+!

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Too late for 48

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

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!

Comment 15

[Haik Aftandilian [:haik]]

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

Comment 16

[Matthew Gregan [:kinetik]]

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

Comment 17

[Matthew Gregan [:kinetik]]

Attachment: patch for central and aurora

Carrying forward r+

Comment 18

[Matthew Gregan [:kinetik]]

Attachment: patch for beta

Carrying forward r+

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e6551eb6c8e
Use DefaultOutputUnit as the default output device.  r=jesup

Comment 20

[Matthew Gregan [:kinetik]]

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

Comment 21

[Matthew Gregan [:kinetik]]

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

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8780953 [details] [diff] [review]
patch for central and aurora

Recent regression since 48, we should uplift to Aurora50.

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2e6551eb6c8e

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8b47af8a1ad2

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

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!

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6b44534d8e74

Comment 28

[Alexandru Simonca [:asimonca], Release Desktop QA]

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)