Closed Bug 934425 Opened 11 years ago Closed 6 years ago

No option to select audio output device (implement HTMLMediaElement.setSinkId)

Categories

(Core :: Audio/Video: Playback, defect, P2)

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla64
Tracking Flags:
Tracking Status
firefox64 --- fixed
Blocking Flags:
backlog webrtc/webaudio+

People

(Reporter: mgoodwin, Assigned: achronop)

References

(Depends on 1 open bug,
URL
)

Details

(Keywords: dev-doc-complete, feature, parity-chrome)

Attachments

(14 files, 3 obsolete files)

Bug 934425 - Add device info in MediaDevice.
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 934425 - Create a method in MediaManager to look up a given sink id.
59 bytes, text/x-review-board-request
Details
Bug 934425 - Set device info in MediaSink and switch sink device.
59 bytes, text/x-review-board-request
Details
Bug 934425 - Add SinkId in webidl for HTMLMediaElement.
59 bytes, text/x-review-board-request
smaug
: review+
bryce
: review+
Details
Bug 934425 - Add mochitest for setSinkId.
59 bytes, text/x-review-board-request
jib
: review-
Details
Bug 934425 - Add device info in MediaDevice. r?jib
46 bytes, text/x-phabricator-request
jib
: review+
Details | Review
Bug 934425 - Create a method in MediaManager to look up a given sink id. r?jib
46 bytes, text/x-phabricator-request
jib
: review+
Details | Review
Bug 934425 - Set device info in MediaSink and switch sink device. r?jya
46 bytes, text/x-phabricator-request
jya
: review+
Details | Review
Bug 934425 - Implement asynchronous method to switch sink in MediaDecoder. r?pehrsons
46 bytes, text/x-phabricator-request
Details | Review
Bug 934425 - Add SinkId in webidl for HTMLMediaElement. r?bryce,smaug
46 bytes, text/x-phabricator-request
smaug
: review+
Details | Review
Bug 934425 - Implement setSinkId in HTMLMediaElement. r?pehrsons
46 bytes, text/x-phabricator-request
Details | Review
Bug 934425 - Add mochitest for setSinkId. r?jib
46 bytes, text/x-phabricator-request
Details | Review
Bug 934425 - Web platform http test for setSinkId. r?jib
46 bytes, text/x-phabricator-request
Details | Review
Bug 934425 - Enable the pref for wpt https test for setSinkId and update it to expect NotAllowedError. r?jib
46 bytes, text/x-phabricator-request
jib
: review+
Details | Review 
It's common to use a headset for RTC applications (especially on Desktop). It'd be nice if, when we're presenting the getUserMedia selection choices for video and / or microphone, we could also allow the user to choose output device (e.g. headset instead of default audio output device).
With Hello now in release, we're getting more folks asking for this functionality.  Making this a P2, though this is not a small amount of work.
Rank: 25
Priority: -- → P2
Blocks: 1129694
This is definitely a serious dogfood issue for me with Hello. Not being able to use my headphone output makes it less useful than Vidyo.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #2)
> This is definitely a serious dogfood issue for me with Hello. Not being able
> to use my headphone output makes it less useful than Vidyo.

You should be able to alter the default device in the system settings and have Firefox respect that. Granted though that's a work around rather than a fix.
backlog: --- → webRTC+
It would be very useful for almost any phone-type communication app to be able to set a specific "ring device" (ex: speaker) that is different from the main device (ex: headphones).

Almost any desktop softphone has this kind of parameter.
Rank: 25 → 35
Component: WebRTC: Audio/Video → Audio/Video: Playback
OS: Linux → All
Priority: P2 → P3
Hardware: x86_64 → All
Summary: No option to select audio output device → No option to select audio output device (implement HTMLMediaElement.setSinkId)
Whiteboard: [parity-Chrome]
Depends on: multimsg
Also to note navigator.mediaDevices.enumerateDevices() doesn't return audiooutput devices either where as Chrome does.

This appears to be a limitation in MediaDevices::EnumDevResolver: http://searchfox.org/mozilla-central/source/dom/media/MediaDevices.cpp#115
That's bug 1152401 which this bug depends on.
Would just like to toss my support behind this work, as well as navigator.mediaDevices.enumerateDevices().  We have a bunch of customer using Firefox long-term support versions that really need this feature for WebRTC, but are lacking it.  That leads to a lot of issues with output device configuration and failed calls.
Rank: 35 → 15
Priority: P3 → P1
We're offering a bounty of US$500 each for this and issue 1152401 (output devices from enumerateDevices()).

To claim please contact thomas (at) inqualitymedia.com.
Mass change P1->P2 to align with new Mozilla triage process
Priority: P1 → P2
Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)
Keywords: parity-chrome
Whiteboard: [parity-Chrome]
Assignee: nobody → achronop
Rank: 15 → 19
I have started implementing this feature and I would like to mention an important issue that I am seeing.

Concerning HTMLMediaElement there are two possible code paths to output audio, (roughly): 
1) through the decoder to the MediaSink, to the AudioSink etc. This is used for playback of audio files.
2) through a MediaStream, to MediaStreamGraph, to GraphDriver etc. This is used when the source is not a file, like when audio is created through webaudio or it is a Stream from a GetUserMedia request etc.

The issue appears in the second case and when more than one HTMLMediaElements are operating in the same doc. If one of the elements requests a different sink device, MediaStreamGraph(MSG) cannot support it. It does not support outputting audio from different streams to different devices. It is not also an easy enhancement to support it.

In the hypothetical case that sink id was known in advance, before the MSG is created, we could create one MSG per requested sink. This is something we support already. However, sink id can change on the fly, when the element is already playing audio, it's not supported to open a different output device for one specific stream.

I am not sure how we want to move from here. We can implement the feature for the case 1 and wait till the necessary enhancements are ready in order to support case 2.
I think the most important use case is #2, to switch between internal OS speaker and a bluetooth headset or external speaker during a WebRTC call. #1 is of no interest without that.

Try https://webrtc.github.io/samples/src/content/devices/input-output/ in Chrome. I don't have an external speaker to test, so on OSX I use the "Audio MIDI Setup" tool, where I hit "+" to create a "Multi-Output Device", to have a choice.

When I try this in Chrome, I appear to be able to switch between the two speaker choices while my microphone feedback is playing. I hear a tiny click when I do this, which I think means it's working (since I don't have an actual external speaker to confirm).

That's what we want.

If we can get that to work, I think it's worth discussing compromises in general functionality to work around limitations in our stack. setSinkId is allowed to reject with AbortError if it cannot succeed. 

That said, MSG is an implementation detail. I think it's important to distinguish the JS control objects from the underlying implementation objects. For example, if we're able to tear down and recreate some underlying implementation objects without disturbing the JS objects, maybe we could consider that.

Let's discuss the limitations in detail on Monday, to see if we can work around some of them.
In addition to comment 18, it would be nice if the following worked:
 1. All track-sourced elements on the page playing to the same sinkId output device.
 2. Allow live-switching all track-sourced elements on the page to a new single sinkId, provided they're done in lock-step in JS.
 3. Allow live-switching non-track-sourced elements to different sinkIds.
Thanks for the response. IMO opinion case #1 is also important due to extensive use of playback. WebRTC is great but I think it is used by less users comparing to playback.

Just to clarify, our limitation is when more that one element are operating in the same tab. The WebRTC sample contains just one element which we could make it work. The problem is that our code is not generic enough in order to support more that one media element when they request different outputs.

Feel free to ping me in order to discuss the details.
Comment on attachment 8994296 [details]
Bug 934425 - Implement setSinkId in HTMLMediaElement.

https://reviewboard.mozilla.org/r/258886/#review265828


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/html/HTMLMediaElement.cpp:8181
(Diff revision 1)
> +                        [promise, win] (nsresult res) {
> +                          promise->MaybeReject(new MediaStreamError(win,
> +                            MediaStreamError::Name::AbortError));
> +                        });
> +               return;
> +             } else if (self->GetSrcMediaStream()) {

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]

             } else if (self->GetSrcMediaStream()) {
               ^~~~~~~~
Comment on attachment 8994295 [details]
Bug 934425 - Add SinkId in webidl for HTMLMediaElement.

https://reviewboard.mozilla.org/r/258884/#review265856
Attachment #8994295 - Flags: review?(bugs) → review+
This needs tests.
Flags: needinfo?(achronop)
Comment on attachment 8994293 [details]
Bug 934425 - Set device info in MediaSink and switch sink device.

https://reviewboard.mozilla.org/r/258880/#review266028

Why are we not trashing the stream and opening a new one? I thought it was the purpose of the AudioSink abstraction.

::: dom/media/AudioStream.h:218
(Diff revision 1)
>  
> +  // Set the sink device info. When null the default device is used.
> +  void SetSink(AudioDeviceInfo* aInfo);
> +
> +  // Re-initialize the current stream. If any of the parameters has changed
> +  // since the last Init new stream will be updated.

s/new stream/a new stream/

::: dom/media/AudioStream.cpp:583
(Diff revision 1)
>  
>    uint64_t position = 0;
>    if (InvokeCubeb(cubeb_stream_get_position, &position) != CUBEB_OK) {
>      return -1;
>    }
> -  return std::min<uint64_t>(position, INT64_MAX);
> +  mTotalFrames += position;

You need to check that this works well when switching between devices with high and low latency. In particular, A/V sync must be preservered, we've been bitten by this in the past.

::: dom/media/mediasink/DecodedStream.h:67
(Diff revision 1)
>    void SetPreservesPitch(bool aPreservesPitch) override;
>    void SetPlaying(bool aPlaying) override;
>  
>    void Start(const media::TimeUnit& aStartTime, const MediaInfo& aInfo) override;
>    void Stop() override;
> +  nsresult SwitchSink() override;

Why is there two methods here?
Attachment #8994293 - Flags: review?(padenot)
Comment on attachment 8994295 [details]
Bug 934425 - Add SinkId in webidl for HTMLMediaElement.

https://reviewboard.mozilla.org/r/258884/#review266136

::: dom/webidl/HTMLMediaElement.webidl:227
(Diff revision 1)
> +
> +/* Audio Output Devices API */
> +partial interface HTMLMediaElement {
> +  [Pref="media.setsinkid.enabled"]
> +  readonly attribute DOMString sinkId;
> +  [Throws, Pref="media.setsinkid.enabled"]

Can we do away with the [Throws] here and just use the promise?
Throwing inside webidl method implementation which returns a promise does automatically convert the exception to promise rejection value.
(In reply to Olli Pettay [:smaug] from comment #35)
> Throwing inside webidl method implementation which returns a promise does
> automatically convert the exception to promise rejection value.

Forgive my lack of familiarity with webidl: does this mean the [Throws] can indeed be dropped? I.e. any internal throwing will automatically be converted. Or is the [Throws] annotation required in order to enable this handling?
[Throws] gives you ErrorResult out param.

https://searchfox.org/mozilla-central/source/__GENERATED__/dom/bindings/CacheStorageBinding.cpp#141-144,162 as an example
and without [Throws] promisefying happens only if there are some other errors, like
creating JS wrapper failing
https://searchfox.org/mozilla-central/source/__GENERATED__/dom/bindings/HTMLMediaElementBinding.cpp#1812-1815,1823,1827
In my case I do not throw anything intentionally so, I guess, I could remove it. I put it there at the beginning of the implementation but end up not using it at all.
Hmmm probably I am wrong, I need it to return some internal state error like like when a dom promise is created which is an ErrorResult out param like smaug mentioned Olli.
Comment on attachment 8994295 [details]
Bug 934425 - Add SinkId in webidl for HTMLMediaElement.

https://reviewboard.mozilla.org/r/258884/#review266136

> Can we do away with the [Throws] here and just use the promise?

I had the same impression but after the discussion in the bug I've fingered out that we need it to return some internal state errors like when we do Promise::Create(). This is part of the next patch so check it altogether and let me know what you think.
Comment on attachment 8994293 [details]
Bug 934425 - Set device info in MediaSink and switch sink device.

https://reviewboard.mozilla.org/r/258880/#review266028

This is also an option. I found better this way some of the reasons bellow:

- The `AudioSink::InitializeAudioStream` method starts the stream internally which we do not want to do in every case. For example if the playback was paused we want to keep it paused. In addition there is no way to check in AudioSink (mPlaying is not very accurate) level if the stream is started. All these are already available in AudioStream
- AudioStream handles the reset of the position internally and AudioSink does not understand the difference.
- AudioStream preserves the Playback parameters, AudioSink does not store the playback parameters at all.

Of-course all the above can be modified if this is what we want.

> You need to check that this works well when switching between devices with high and low latency. In particular, A/V sync must be preservered, we've been bitten by this in the past.

I'm gonna test using a BT device which usually has larger latency. In general, AudioStream configures a latency of 100 ms. This is usually long enough for many devices. In addition to that cubeb does not have an api to obtain the latency of a given device id, `cubeb_get_min_latency()` provides the latency of the default device.

> Why is there two methods here?

Every MediaSink child has 2 methods concerning sink id. One method is used to set the new device info when the stream is not playing, not started yet or paused. This is a simple update on internal class members and does not need special async handling. The second method will change the sink on the fly and report back asynchronously success or fail.
Comment on attachment 8994291 [details]
Bug 934425 - Add device info in MediaDevice.

https://reviewboard.mozilla.org/r/258876/#review267208

::: dom/media/MediaManager.cpp:822
(Diff revision 1)
>  MediaDevice::MediaDevice(MediaEngineSource* aSource,
>                           const nsString& aName,
>                           const nsString& aID,
>                           const nsString& aRawID)
>    : mSource(aSource)
> +  , mAudioInfo(nullptr)

Am I right in observing that AudioDeviceInfo in general can describe either input and outout?

If so, might it be clearer to s/mAudioInfo/mSinkInfo/ or mSinkAudioInfo here?

I think that would make it clearer that mSource and mSinkInfo are mutually exclusive (i.e. a device is either input or output, never both).

Once we're deeper into code solely about sinks, we can drop the prefix, but not up here, I think.

::: dom/media/MediaManager.cpp:864
(Diff revision 1)
> +  , mKind(mAudioInfo->Type() == AudioDeviceInfo::TYPE_INPUT ? dom::MediaDeviceKind::Audioinput
> +                                                            : dom::MediaDeviceKind::Audiooutput)

This allows creating a MediaDevice with no source from an AudioDeviceInfo of TYPE_INPUT. When is this useful? If it is not useful, should we MOZ_ASSERT it to be TYPE_OUTPUT instead? Fewer variants to worry about down below.
Attachment #8994291 - Flags: review?(jib) → review+
Comment on attachment 8994292 [details]
Bug 934425 - Create a method in MediaManager to look up a given sink id.

https://reviewboard.mozilla.org/r/258878/#review267210

r- for altering how EnumerateRawDevices() works on navigation. Not your fault. We'll need to figure out if MozPromise can be used in this case without altering behavior on navigation.

Aside from that this looks good.

::: commit-message-0285d:1
(Diff revision 1)
> +Bug 934425 - Create a method in MediaManager to lookup for a given sink id. r?jib

s/lookup for/look up/

::: commit-message-0285d:3
(Diff revision 1)
> +Bug 934425 - Create a method in MediaManager to lookup for a given sink id. r?jib
> +
> +MediaManager contains the logic to enumerate asynchronously for audio devices. This logic has been reused to look up synchronously for a given sink id.

I'm not following what you mean here with asynchronous vs synchronous. The new method returns a promise, so no part here seems synchronous to me.

Can you rephrase?

::: dom/media/MediaManager.h:145
(Diff revision 1)
>    const nsString mID;
>    const nsString mRawID;
>  };
>  
>  typedef nsRefPtrHashtable<nsUint64HashKey, GetUserMediaWindowListener> WindowTable;
> +typedef MozPromise<RefPtr<AudioDeviceInfo>, nsresult, true> DevicePromise;

Nit: maybe s/DevicePromise/SinkInfoPromise/ ?

Too many things called device in MediaManager...

::: dom/media/MediaManager.h:232
(Diff revision 1)
> +  RefPtr<DevicePromise> ExistInSinks(nsPIDOMWindowInner* aWindow,
> +                                     const nsString& aDeviceId);

Maybe s/ExistInSinks/FindSinkInfoById/ ?

::: dom/media/MediaManager.cpp:3211
(Diff revision 1)
>            }
>          }
>        }
>  
>        RefPtr<PledgeMediaDeviceSet> p = mgr->mOutstandingPledges.Remove(id);
> -      if (!p || !mgr->IsWindowStillActive(aWindowId)) {
> +      if (!p) {

EnumerateRawDevices() waits for an async IPC call, so by the time it returns, the window may no longer be active, in which case we want to leave the pledge hanging (never resolve nor reject it).

It looks like you've attempted to move the check further down in the same synchronous code (Pledge->Resolve() calls .Then() synchronously), but without understanding why, I would suggest this test be done as early as possible, i.e. here.

I suspect this has something to do with wanting to use MozPromise, which I'll comment on below.

::: dom/media/MediaManager.cpp:3299
(Diff revision 1)
>      onSuccess->OnSuccess(array);
> +    MediaManager* mgr = MediaManager::GetIfExists();
> +    if (mgr && !mgr->IsWindowStillActive(windowId)) {
> +      onSuccess->OnSuccess(array);

You have

    onSuccess->OnSuccess(array);

....twice here. Seems redundant.

Also, the check for IsWindowStillActive() is too late here. We don't want to resolve the pledge in that case. More on this below.

::: dom/media/MediaManager.cpp:3312
(Diff revision 1)
> +RefPtr<DevicePromise>
> +MediaManager::ExistInSinks(nsPIDOMWindowInner* aWindow,
> +                           const nsString& aDeviceId)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(aWindow);
> +
> +  RefPtr<DevicePromise::Private> promise = new DevicePromise::Private(__func__);
> +  RefPtr<PledgeMediaDeviceSet> p =
> +    EnumerateDevicesImpl(aWindow->WindowID(),

So... the reason EnumerateDevicesImpl() still returns a Pledge, and hasn't yet been converted to MozPromise, is that EnumerateDevicesImpl() does quite a lot of work in the master process and waits for an IPC response. Pledge was written to allow clean tear-down of the content process even with outstanding IPC responses, and neither resolve nor reject in that case, by design. 

*Last I checked*, which admittedly was a month or more ago, it wasn't clear that MozPromise could tolerate this, and would assert or worse if that happened. I've since learned that MozPromise now has IPC support, so maybe things have changed? Unfortunately, I haven't had time to investigate. We'd need to check how it behaves, to see how it behaves on navigation.

In lieu of such support, we'd need to use Pledge here, and not MozPromise, to maintain behavior. While there is some debate over this, my impression is we currently favor not rejecting promises on navigation (and the setSinkId spec, like most specs, doesn't say one way or the other).
Attachment #8994292 - Flags: review?(jib) → review-
Comment on attachment 8994292 [details]
Bug 934425 - Create a method in MediaManager to look up a given sink id.

https://reviewboard.mozilla.org/r/258878/#review267314

::: dom/media/MediaManager.cpp:3312
(Diff revision 1)
> +RefPtr<DevicePromise>
> +MediaManager::ExistInSinks(nsPIDOMWindowInner* aWindow,
> +                           const nsString& aDeviceId)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(aWindow);
> +
> +  RefPtr<DevicePromise::Private> promise = new DevicePromise::Private(__func__);
> +  RefPtr<PledgeMediaDeviceSet> p =
> +    EnumerateDevicesImpl(aWindow->WindowID(),

> I've since learned that MozPromise now has IPC support, so maybe things have changed? 
> 

To follow up on this, I'm referring to bug 1313200.

Said differently, if you can get MozPromise here to work without the changes you made to EnumerateRawDevices, then I would approve. But I suspect you made those changes for a reason... Did something fail?

I'm unsure if we have a test for enumerateDevices (and now setSinkId) to ensure their promises don't reject on navigation. We should add that if we don't.
Comment on attachment 8994291 [details]
Bug 934425 - Add device info in MediaDevice.

https://reviewboard.mozilla.org/r/258876/#review267208

> Am I right in observing that AudioDeviceInfo in general can describe either input and outout?
> 
> If so, might it be clearer to s/mAudioInfo/mSinkInfo/ or mSinkAudioInfo here?
> 
> I think that would make it clearer that mSource and mSinkInfo are mutually exclusive (i.e. a device is either input or output, never both).
> 
> Once we're deeper into code solely about sinks, we can drop the prefix, but not up here, I think.

In general I agree. The only negative effect is that we will create extra renaming when that change.

> This allows creating a MediaDevice with no source from an AudioDeviceInfo of TYPE_INPUT. When is this useful? If it is not useful, should we MOZ_ASSERT it to be TYPE_OUTPUT instead? Fewer variants to worry about down below.

Actually this constructor works in place of the one just above. The other one should have been removed since it is not of any use. I have removed the unnecessary ctor, and I've added the asserts here.
Comment on attachment 8994294 [details]
Bug 934425 - Implement asynchronous method to switch sink in MediaDecoder.

https://reviewboard.mozilla.org/r/258882/#review266780

::: dom/media/MediaDecoder.h:159
(Diff revision 1)
>    void SetVolume(double aVolume);
>  
>    void SetPlaybackRate(double aPlaybackRate);
>    void SetPreservesPitch(bool aPreservesPitch);
>    void SetLooping(bool aLooping);
> +  void SetSinkDevice(AudioDeviceInfo* aInfo);

Could we document the two different methods added? Am I correct in understanding one is for the sync case where nothing is currently playing, and the other is an async case where we switch mid playback? It would be useful to have comments to that effect.

::: dom/media/MediaDecoder.h:184
(Diff revision 1)
> +    bool operator!=(const SwitchSinkData& rhs) const
> +    {
> +      return !operator==(rhs);
> +    }
> +    AudioDeviceInfo* mSinkInfo;
> +    GenericPromise::Private* mPromise;

Have you considered using a MozPromiseHolder here?

::: dom/media/MediaDecoder.h:647
(Diff revision 1)
>  
>    Canonical<bool> mPreservesPitch;
>  
>    Canonical<bool> mLooping;
>  
> +  Canonical<AudioDeviceInfo*> mSinkDeviceInfo;

I'm unsure on if we should be using Canonicals around pointers. The mirroring of the value should be safe, but once that pointer is dereferenced we're circumventing the mirror and in possible threading danger.

::: dom/media/MediaDecoder.cpp:173
(Diff revision 1)
>    mVolume = aVolume;
>  }
>  
>  void
> +MediaDecoder::SetSinkDevice(AudioDeviceInfo* aInfo)
> +{

Should this method also assert that aInfo is not null?

::: dom/media/MediaDecoder.cpp:180
(Diff revision 1)
> +  AbstractThread::AutoEnter context(AbstractMainThread());
> +  mSinkDeviceInfo = aInfo;
> +}
> +
> +already_AddRefed<GenericPromise>
> +MediaDecoder::SwitchSink(AudioDeviceInfo* info)

info -> aInfo
Comment on attachment 8994295 [details]
Bug 934425 - Add SinkId in webidl for HTMLMediaElement.

https://reviewboard.mozilla.org/r/258884/#review267318
Attachment #8994295 - Flags: review?(bvandyk) → review+
Comment on attachment 8994292 [details]
Bug 934425 - Create a method in MediaManager to look up a given sink id.

https://reviewboard.mozilla.org/r/258878/#review267210

> I'm not following what you mean here with asynchronous vs synchronous. The new method returns a promise, so no part here seems synchronous to me.
> 
> Can you rephrase?

Updated, feel free to reopen the issue if it is not clear enough.

> EnumerateRawDevices() waits for an async IPC call, so by the time it returns, the window may no longer be active, in which case we want to leave the pledge hanging (never resolve nor reject it).
> 
> It looks like you've attempted to move the check further down in the same synchronous code (Pledge->Resolve() calls .Then() synchronously), but without understanding why, I would suggest this test be done as early as possible, i.e. here.
> 
> I suspect this has something to do with wanting to use MozPromise, which I'll comment on below.

The problem here is that `mgr->IsWindowStillActive(aWindowId)` returns always `false` when we call it `ExistInSinks()` method. 

The `IsWindowStillActive()` is looking for a windows previously stored `mActiveWindows` member. In our case the current window does not exist there. A window id is stored in `GetUserMedia()` and `EnumerateDevices()` methods [1].

The `ExistInSinks()` method make use of the `EnumerateDevicesImpl()` directly so it does not add the window id in `mActiveWindows` . In addition to that we would need a `GetUserMediaWindowListener` pointer if we wanted to add a window in `ExistInSinks()`. So I need to create one but the window listener has no other use in our case. I see that `EnumerateDevices` creates a listener even if it does not need it. If I create a window listener in `ExistInSinks()` that change (and the one above) is not needed and works as is.

Please let me know what would be the best approach here.

[1] https://searchfox.org/mozilla-central/search?q=symbol:_ZN7mozilla12MediaManager11AddWindowIDEmPNS_26GetUserMediaWindowListenerE&redirect=false
Comment on attachment 8994292 [details]
Bug 934425 - Create a method in MediaManager to look up a given sink id.

https://reviewboard.mozilla.org/r/258878/#review267314

> > I've since learned that MozPromise now has IPC support, so maybe things have changed? 
> > 
> 
> To follow up on this, I'm referring to bug 1313200.
> 
> Said differently, if you can get MozPromise here to work without the changes you made to EnumerateRawDevices, then I would approve. But I suspect you made those changes for a reason... Did something fail?
> 
> I'm unsure if we have a test for enumerateDevices (and now setSinkId) to ensure their promises don't reject on navigation. We should add that if we don't.

I mentioned above the reason of that change. I could solve it but it does not cover the case of outstanding IPC response. In that case the MozPromise will assert if neither resolve or reject when it is released.

My understanding is that we need to change the `PMedia.ipdl` declaration in order to make use of support for async returns with MozPromise. That would need some more details and consideration. If I can ping you to discuss this option would be great. Thanks.
Comment on attachment 8994294 [details]
Bug 934425 - Implement asynchronous method to switch sink in MediaDecoder.

https://reviewboard.mozilla.org/r/258882/#review266780

> Have you considered using a MozPromiseHolder here?

I have been trying to implement it by using a MozPromiseHolder but it does not work well. MozPromiseHolder does not have a copy constructor, which make the mirroring awkward and tricky. Also it is not a ref-counted object so if I use a pointer I have to handle the life time of it. For the purpose of that struct, which is to simple convey the data from one side to the other, I believe using an MozPromise::Private pointer is good enough.

Please feel free to let me know if you have different opinion.

> I'm unsure on if we should be using Canonicals around pointers. The mirroring of the value should be safe, but once that pointer is dereferenced we're circumventing the mirror and in possible threading danger.

I changed the code to use a RefPtr instead and works well. I'll keep it that way.

> Should this method also assert that aInfo is not null?

That method can accept null aInfo which is a valid option. This is because null device info specify the default device. It is also used from HTMLMediaElement, on load, to set the sink device. The device can be null at that point.
Comment on attachment 8994291 [details]
Bug 934425 - Add device info in MediaDevice.

https://reviewboard.mozilla.org/r/258876/#review267990

::: commit-message-4f12d:3
(Diff revision 2)
> +Bug 934425 - Add device info in MediaDevice. r?jib
> +
> +MediaDevice is the core object for audio device enumeration. By adding AudioDeviceInfo every information of audio devices will be available. In this case device id will be available from the first enumeration and there is no need to enumerate multiple times to get that info.

Paul is introducing a CubebDeviceEnumerator in bug 1404977 that handles caching of devices.

Can we build on top of that and add output device enumeration to it too?

It's a more solid path because it both sits commonly in MediaEngineWebRTC (higher than the MediaDevice-level caching you have here), and it's prepared for unit tests.
Comment on attachment 8994292 [details]
Bug 934425 - Create a method in MediaManager to look up a given sink id.

https://reviewboard.mozilla.org/r/258878/#review267998

I'd also like to be a reviewer of this patch as you iterate on it. Thanks.

::: dom/media/MediaManager.cpp:3294
(Diff revision 2)
> +RefPtr<SinkInfoPromise>
> +MediaManager::FindSinkInfoById(nsPIDOMWindowInner* aWindow,
> +                               const nsString& aDeviceId)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(aWindow);
> +
> +  // We have to add the window id here because enumerate methods
> +  // check for that and abort silently if it does not exist.
> +  uint64_t windowId = aWindow->WindowID();
> +  nsIPrincipal* principal = aWindow->GetExtantDoc()->NodePrincipal();
> +  RefPtr<GetUserMediaWindowListener> windowListener = GetWindowListener(windowId);
> +  if (windowListener) {
> +    PrincipalHandle existingPrincipalHandle =
> +      windowListener->GetPrincipalHandle();
> +    MOZ_ASSERT(PrincipalHandleMatches(existingPrincipalHandle, principal));
> +  } else {
> +    windowListener = new GetUserMediaWindowListener(mMediaThread, windowId,
> +                                                    MakePrincipalHandle(principal));
> +    AddWindowID(windowId, windowListener);
> +  }
> +  // Create an inactive SourceListener to act as a placeholder, so the
> +  // window listener doesn't clean itself up until we're done.
> +  RefPtr<SourceListener> sourceListener = new SourceListener();
> +  windowListener->Register(sourceListener);
> +
> +  RefPtr<SinkInfoPromise::Private> promise = new SinkInfoPromise::Private(__func__);
> +  RefPtr<PledgeMediaDeviceSet> p =
> +    EnumerateDevicesImpl(aWindow->WindowID(),
> +                         MediaSourceEnum::Other,
> +                         MediaSourceEnum::Other,
> +                         MediaSinkEnum::Speaker,
> +                         DeviceEnumerationType::Normal,
> +                         DeviceEnumerationType::Normal);
> +  p->Then([aDeviceId, promise](MediaDeviceSet*& aDevices) mutable {
> +    UniquePtr<MediaDeviceSet> devices(aDevices);
> +    for (RefPtr<MediaDevice>& device : *devices) {
> +      if (device->mID.Equals(aDeviceId)) {
> +        promise->Resolve(device->mSinkInfo, __func__);
> +        return;
> +      }
> +    }
> +    promise->Resolve(nullptr, __func__);
> +  }, [promise](MediaStreamError*& reason) mutable {
> +    promise->Reject(NS_ERROR_DOM_NOT_FOUND_ERR, __func__);
> +  });
> +
> +  return promise.forget();
> +}

Instead of all the boilerplate needed to support this hack (it'll always do a full enumeration, no?) of a lookup, can we just pop over to the media thread [1] and ask mBackend whether it knows about aDeviceId.

Looking a bit holistically at this, bug 1404977 adds MediaEngineWebRTC::mEnumerator, which is where output enumeration should happen (and be cached) anyway.

Remember that our focus is quality. If we're then going to get away with feature work we need to be improving code quality and class hierarchy in the process, IMHO.


[1] https://searchfox.org/mozilla-central/rev/d0a0ae30d534dc55e712bd3eedc1b4553ba56323/dom/media/MediaManager.cpp#2391
Comment on attachment 8994292 [details]
Bug 934425 - Create a method in MediaManager to look up a given sink id.

https://reviewboard.mozilla.org/r/258878/#review267210

> So... the reason EnumerateDevicesImpl() still returns a Pledge, and hasn't yet been converted to MozPromise, is that EnumerateDevicesImpl() does quite a lot of work in the master process and waits for an IPC response. Pledge was written to allow clean tear-down of the content process even with outstanding IPC responses, and neither resolve nor reject in that case, by design. 
> 
> *Last I checked*, which admittedly was a month or more ago, it wasn't clear that MozPromise could tolerate this, and would assert or worse if that happened. I've since learned that MozPromise now has IPC support, so maybe things have changed? Unfortunately, I haven't had time to investigate. We'd need to check how it behaves, to see how it behaves on navigation.
> 
> In lieu of such support, we'd need to use Pledge here, and not MozPromise, to maintain behavior. While there is some debate over this, my impression is we currently favor not rejecting promises on navigation (and the setSinkId spec, like most specs, doesn't say one way or the other).

MozPromiseRequestHolder can be used to disconnect a MozPromise and leave it hanging unresolved.
https://searchfox.org/mozilla-central/rev/d0a0ae30d534dc55e712bd3eedc1b4553ba56323/xpcom/threads/MozPromise.h#1319-1321
Comment on attachment 8994294 [details]
Bug 934425 - Implement asynchronous method to switch sink in MediaDecoder.

https://reviewboard.mozilla.org/r/258882/#review268002

::: dom/media/MediaDecoder.h:193
(Diff revision 2)
> +    GenericPromise::Private* mPromise;
> +  };
> +
> +  // Set new sink and switch the sink device. This method is used when
> +  // the playback is playing. Report back the result asynchronously.
> +  already_AddRefed<GenericPromise> SwitchSink(AudioDeviceInfo* aInfo);

This should return `RefPtr<GenericPromise>` for the sake of chaining.

::: dom/media/MediaDecoder.cpp:179
(Diff revision 2)
> +already_AddRefed<GenericPromise>
> +MediaDecoder::SwitchSink(AudioDeviceInfo* aInfo)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(aInfo);
> +  AbstractThread::AutoEnter context(AbstractMainThread());
> +  RefPtr<GenericPromise::Private> p = new GenericPromise::Private(__func__);
> +  mSwitchSinkData = SwitchSinkData(aInfo, p.get());
> +  return p.forget();
> +}

This seems like you are mixing multiple paradigms as you are using both a Canonical/Mirror _and_ a MozPromise.

I don't think there's precedence for that.

How about instead making a call straight to the state machine and returning (or chaining if you need another type) the promise it returns?

See [1] for a simple example of a one-off method. Or look for mSeekRequest for something more advanced.


[1] https://searchfox.org/mozilla-central/rev/d0a0ae30d534dc55e712bd3eedc1b4553ba56323/dom/media/MediaDecoder.cpp#1399
Comment on attachment 8994296 [details]
Bug 934425 - Implement setSinkId in HTMLMediaElement.

https://reviewboard.mozilla.org/r/258886/#review268004

::: dom/html/HTMLMediaElement.h:1781
(Diff revision 3)
> +
> +  already_AddRefed<Promise> SetSinkId(const nsAString& aSinkId, ErrorResult& aRv);
> +  void GetSinkId(nsString& retval) {retval = mSinkId;}

Move this to the WebIDL-impl section, https://searchfox.org/mozilla-central/rev/d0a0ae30d534dc55e712bd3eedc1b4553ba56323/dom/html/HTMLMediaElement.h#437

::: dom/html/HTMLMediaElement.h:1859
(Diff revision 3)
> +  // Contains the unique id of the sink device. It can be invalid if the device
> +  // has been unplugged. It can be empty.

Should we mention that this follows the `sinkId` attribute in the spec?

::: dom/html/HTMLMediaElement.h:1863
(Diff revision 3)
> +
> +  // Contains the unique id of the sink device. It can be invalid if the device
> +  // has been unplugged. It can be empty.
> +  nsString mSinkId;
> +
> +  // Information about the sink when the mSinkId is set.

s/the mSinkId/mSinkId/

::: dom/html/HTMLMediaElement.h:1870
(Diff revision 3)
> +
> +  // Check asynchronously if the current sink id exists in the active sinks.
> +  // It is resolved in MediaManager.
> +  RefPtr<MozPromise<RefPtr<AudioDeviceInfo>, nsresult, true>> mSinkIdPromise;
> +
> +  // Switch sink device asynchronously if the sink id set

s/id/id is/

::: dom/html/HTMLMediaElement.h:1871
(Diff revision 3)
> +  // Check asynchronously if the current sink id exists in the active sinks.
> +  // It is resolved in MediaManager.
> +  RefPtr<MozPromise<RefPtr<AudioDeviceInfo>, nsresult, true>> mSinkIdPromise;
> +
> +  // Switch sink device asynchronously if the sink id set
> +  // in the middle of the playback. It is resolved in MDSM.

s/the playback/playback/
s/MDSM/MediaDecoderStateMachine/

::: dom/html/HTMLMediaElement.cpp:3896
(Diff revision 3)
>    , mShutdownObserver(new ShutdownObserver)
>    , mPlayed(new TimeRanges(ToSupports(OwnerDoc())))
>    , mPaused(true, "HTMLMediaElement::mPaused")
>    , mErrorSink(new ErrorSink(this))
>    , mAudioChannelWrapper(new AudioChannelAgentCallback(this))
> +  , mSinkId()

Unnecessary

::: dom/html/HTMLMediaElement.cpp:5063
(Diff revision 3)
> +  // If MediaSink is a DecodedStream the
> +  // sink id will be ignored.
> +  mDecoder->SetSinkDevice(mSinkDeviceInfo);

What if the device is no longer available?

If this is supported per "2.1.1 Sink no longer available" in the spec please mention in this comment.


Where is the normative "2.1.2 New sink available" from the spec implemented?

::: dom/html/HTMLMediaElement.cpp:8157
(Diff revision 3)
> +    return promise.forget();
> +  }
> +
> +  nsString sinkId(aSinkId);
> +  RefPtr<HTMLMediaElement> self = this;
> +  mSinkIdPromise = MediaManager::Get()->FindSinkInfoById(win, sinkId);

Why store this in a member? You're only ever using it in this method.

::: dom/html/HTMLMediaElement.cpp:8161
(Diff revision 3)
> +             if (!info) {
> +               promise->MaybeReject(new MediaStreamError(win,
> +                 MediaStreamError::Name::NotFoundError));
> +               return;

When can this happen?

::: dom/html/HTMLMediaElement.cpp:8166
(Diff revision 3)
> +             if (!info) {
> +               promise->MaybeReject(new MediaStreamError(win,
> +                 MediaStreamError::Name::NotFoundError));
> +               return;
> +             }
> +             // Sink found switch output device.

Here we need to link to a followup for the security history and explain in the comment what needs to be done for the security rejection.

::: dom/html/HTMLMediaElement.cpp:8168
(Diff revision 3)
> +               self->mSwitchSinkPromise = self->mDecoder->SwitchSink(info);
> +               self->mSwitchSinkPromise

mSwitchSinkPromise is only used here -- it doesn't need to be a member.

::: dom/html/HTMLMediaElement.cpp:8191
(Diff revision 3)
> +               self->mSinkId = sinkId;
> +               self->mSinkDeviceInfo = info;
> +               promise->MaybeResolveWithUndefined();
> +             }
> +           },
> +           // Sink is not valid.

Sink was not found, I'd say.

::: dom/media/AudioStream.h:218
(Diff revision 3)
>  
>    // Set the sink device info. When null the default device is used.
>    void SetSink(AudioDeviceInfo* aInfo);
>  
>    // Re-initialize the current stream. If any of the parameters has changed
> -  // since the last Init new stream will be updated.
> +  // since the last Init the new stream will be updated.

wrong patch?
Comment on attachment 8994292 [details]
Bug 934425 - Create a method in MediaManager to look up a given sink id.

https://reviewboard.mozilla.org/r/258878/#review267998

> Instead of all the boilerplate needed to support this hack (it'll always do a full enumeration, no?) of a lookup, can we just pop over to the media thread [1] and ask mBackend whether it knows about aDeviceId.
> 
> Looking a bit holistically at this, bug 1404977 adds MediaEngineWebRTC::mEnumerator, which is where output enumeration should happen (and be cached) anyway.
> 
> Remember that our focus is quality. If we're then going to get away with feature work we need to be improving code quality and class hierarchy in the process, IMHO.
> 
> 
> [1] https://searchfox.org/mozilla-central/rev/d0a0ae30d534dc55e712bd3eedc1b4553ba56323/dom/media/MediaManager.cpp#2391

Why do you call it a hack? It enumerates the output device every time you call it, not the input. I am not sure what do you mean by full enumeration. We need to enumerate every time in case something has changed in between. Even if you see something wrong I haven't implemented a hack.

How the MediaEngine would possibly knows about aDeviceId without a new enumeration? In addition, `EnumerateDevicesImpl()` makes use of the mBackend and post the task in Media thread.

Since bug 1404977 updates the way we enumerate devices this patch will be benefited as well. But it is not landed yet, when I started this one I had to make do without the enumerator.

In general I aggree about code quality, I am not sure how to link it with that change.
Comment on attachment 8994293 [details]
Bug 934425 - Set device info in MediaSink and switch sink device.

https://reviewboard.mozilla.org/r/258880/#review266028

I agree with Paul in that we don't want to add a new path in MediaSink and below (because complexity). It looks like MDSM::SetAudioCaptured [1] already does something similar to what you need to do so let's continue that pattern shall we?


[1] https://searchfox.org/mozilla-central/rev/d0a0ae30d534dc55e712bd3eedc1b4553ba56323/dom/media/MediaDecoderStateMachine.cpp#3770-3780
Comment on attachment 8994293 [details]
Bug 934425 - Set device info in MediaSink and switch sink device.

https://reviewboard.mozilla.org/r/258880/#review266028

This works well, I am testing a patch and everything is working fine. I need to take care of the error handling through the existing promise mechanishm.
Comment on attachment 8994292 [details]
Bug 934425 - Create a method in MediaManager to look up a given sink id.

https://reviewboard.mozilla.org/r/258878/#review267210

> MozPromiseRequestHolder can be used to disconnect a MozPromise and leave it hanging unresolved.
> https://searchfox.org/mozilla-central/rev/d0a0ae30d534dc55e712bd3eedc1b4553ba56323/xpcom/threads/MozPromise.h#1319-1321

I filed bug 1479841 to cleanup this to use MozPromise throughout.
Comment on attachment 8994292 [details]
Bug 934425 - Create a method in MediaManager to look up a given sink id.

https://reviewboard.mozilla.org/r/258878/#review267998

> Why do you call it a hack? It enumerates the output device every time you call it, not the input. I am not sure what do you mean by full enumeration. We need to enumerate every time in case something has changed in between. Even if you see something wrong I haven't implemented a hack.
> 
> How the MediaEngine would possibly knows about aDeviceId without a new enumeration? In addition, `EnumerateDevicesImpl()` makes use of the mBackend and post the task in Media thread.
> 
> Since bug 1404977 updates the way we enumerate devices this patch will be benefited as well. But it is not landed yet, when I started this one I had to make do without the enumerator.
> 
> In general I aggree about code quality, I am not sure how to link it with that change.

Thanks Andreas for being a reviewer on this. I think that's a great idea.

I think I agree with Alex on needing full enumeration here. The sinkId here may come from a cookie from a previous session, or may have been made up. MediaManager also may not even exist at the time of this call. So I don't see how we can skip a full enumeration. Unless there are invariants provided by cubeb here I'm unaware of?

As to storing principals, the code comments here are a bit light. Since FindSinkById() will be tasked primarily with implementing much of the setSinkId() algorithm, part of that work will need to involve user permission eventually, which means we'll need to examine principals eventually, so I'm not worried so much about that.

Some of the other boilerplate is the same MediaManager::EnumerateDevices() inherited from MediaManager::GetUserMedia() I've filed bug 1481450 so let's discuss cleaning that up there.
Comment on attachment 8994294 [details]
Bug 934425 - Implement asynchronous method to switch sink in MediaDecoder.

https://reviewboard.mozilla.org/r/258882/#review268750

::: dom/media/MediaDecoder.h:163
(Diff revision 2)
>    void SetLooping(bool aLooping);
>  
> +  // Set sink device info.
> +  // It is used when nothing currently is playing.
> +  // That device will be used for next playback.
> +  void SetSinkDevice(AudioDeviceInfo* aInfo);

Why the use of raw pointers?

If it's an object that isn't supposed to be modified, then appropriate constification should be done.

As a rule, unless you plan to addref the object, passing raw pointer is a bad idea

::: dom/media/MediaDecoder.h:167
(Diff revision 2)
> +  // That device will be used for next playback.
> +  void SetSinkDevice(AudioDeviceInfo* aInfo);
> +
> +  // Helper struct to carry the information needed
> +  // to switch sink device asynchronously
> +  struct SwitchSinkData {

{ on new line

::: dom/media/MediaDecoder.h:174
(Diff revision 2)
> +    : mSinkInfo(nullptr)
> +    , mPromise(nullptr)
> +    {}
> +
> +    SwitchSinkData(AudioDeviceInfo* aSinkInfo,
> +                   GenericPromise::Private* aPromise)

Why use a Promise::Private?

Why use raw pointers ?

if ownership is is to be transferred, then either use rvalues or already_AddRefed

::: dom/media/MediaDecoder.h:176
(Diff revision 2)
> +    {}
> +
> +    SwitchSinkData(AudioDeviceInfo* aSinkInfo,
> +                   GenericPromise::Private* aPromise)
> +    : mSinkInfo(aSinkInfo)
> +    , mPromise(aPromise)

bad indentation, please check the coding style
Comment on attachment 8994292 [details]
Bug 934425 - Create a method in MediaManager to look up a given sink id.

https://reviewboard.mozilla.org/r/258878/#review267998

> Thanks Andreas for being a reviewer on this. I think that's a great idea.
> 
> I think I agree with Alex on needing full enumeration here. The sinkId here may come from a cookie from a previous session, or may have been made up. MediaManager also may not even exist at the time of this call. So I don't see how we can skip a full enumeration. Unless there are invariants provided by cubeb here I'm unaware of?
> 
> As to storing principals, the code comments here are a bit light. Since FindSinkById() will be tasked primarily with implementing much of the setSinkId() algorithm, part of that work will need to involve user permission eventually, which means we'll need to examine principals eventually, so I'm not worried so much about that.
> 
> Some of the other boilerplate is the same MediaManager::EnumerateDevices() inherited from MediaManager::GetUserMedia() I've filed bug 1481450 so let's discuss cleaning that up there.

Perhaps calling it a hack was a bit premature. Sorry about that.

I understand now that his code needs to go through uuid anonymization and so we cannot call the enumerator directly. That probably does leave it simplest to use EnumerateDevicesImpl for this. The alternative is to add a new path throughout the chain along the lines of `Maybe<SinkInfo> FindOutputDevice(aDeviceId)`, but the simplicity of re-using the anonymization plumbing might outweigh the cleanliness we get from a separate path as mentioned.

The calling convention model can probably be simplified once we've moved from Pledge to MozPromise, if we also modularize the current methods so they don't all mask thread jumps but we handle that at a higher layer instead. That would allow us to compose a lambda that does more than one thing on the media manager thread for instance. I think that would result in a net simplification.

You are right in that this path will eventually use the enumerator if we add output enumeration to it.

Please do remove the need for the window and source listener dance however, in bug 1481450.
Comment on attachment 8994294 [details]
Bug 934425 - Implement asynchronous method to switch sink in MediaDecoder.

https://reviewboard.mozilla.org/r/258882/#review268856

This just can't go as-is...
Attachment #8994294 - Flags: review-
Comment on attachment 8994296 [details]
Bug 934425 - Implement setSinkId in HTMLMediaElement.

https://reviewboard.mozilla.org/r/258886/#review268004

> What if the device is no longer available?
> 
> If this is supported per "2.1.1 Sink no longer available" in the spec please mention in this comment.
> 
> 
> Where is the normative "2.1.2 New sink available" from the spec implemented?

If the device get unglugged we follow the same behavior similar to any other case. This behavior is implemented deep down in cubeb and it is the same as every case of audio output. It is not similar to 2.1.1, 2.1.2 of the spec. The only thing I can tell is that chrome also does not follow that part of the spec and switch devices in a way similar to us.

> Why store this in a member? You're only ever using it in this method.

Because I need to keep it alive. The current method may return before that promise get resolved or rejected. If it was a local variable it would be removed by the end of the method.

> When can this happen?

This happens when the given sink id does not corresponds to any device in the system.

> Here we need to link to a followup for the security history and explain in the comment what needs to be done for the security rejection.

I can provide an abstract description of what we need. We haven't decided the final security model for this.

> mSwitchSinkPromise is only used here -- it doesn't need to be a member.

It's a member in order to stay alive after the lamda method has returned.
Comment on attachment 8994291 [details]
Bug 934425 - Add device info in MediaDevice.

https://reviewboard.mozilla.org/r/258876/#review267990

> Paul is introducing a CubebDeviceEnumerator in bug 1404977 that handles caching of devices.
> 
> Can we build on top of that and add output device enumeration to it too?
> 
> It's a more solid path because it both sits commonly in MediaEngineWebRTC (higher than the MediaDevice-level caching you have here), and it's prepared for unit tests.

Bug 1482150 created to add output device enumerator in CubebDeviceEnumerator.
Comment on attachment 8994296 [details]
Bug 934425 - Implement setSinkId in HTMLMediaElement.

https://reviewboard.mozilla.org/r/258886/#review268004

> If the device get unglugged we follow the same behavior similar to any other case. This behavior is implemented deep down in cubeb and it is the same as every case of audio output. It is not similar to 2.1.1, 2.1.2 of the spec. The only thing I can tell is that chrome also does not follow that part of the spec and switch devices in a way similar to us.

Good question. Ultimately, I think we want to implement the spec here, not Chrome's behavior. 

But in the interest of incrementing, I'm also open to defer this part until we fix bug 1272588.
Comment on attachment 8996966 [details]
Bug 934425 - Add mochitest for setSinkId.

https://reviewboard.mozilla.org/r/260940/#review269402

Forgot to hit submit on this review I had sitting. No rush.

::: dom/media/tests/mochitest/test_setSinkId.html:16
(Diff revision 2)
> +<script>
> +  createHTML({
> +    title: "SetSinkId in HTMLMediaElement",
> +    bug: "934425",
> +  });
> +  SpecialPowers.setBoolPref("media.setsinkid.enabled", true);

Please use 

    await SpecialPowers.pushPrefEnv

in new code (bug 1056851).

::: dom/media/tests/mochitest/test_setSinkId.html:21
(Diff revision 2)
> +  SpecialPowers.setBoolPref("media.setsinkid.enabled", true);
> +
> +  /**
> +   * Run a test to verify set sink id in audio element.
> +   */
> +  scriptsReady.then(() => runTestWhenReady(async () => {

Any reason to avoid runTest() here? [1]

Also, we generally want to avoid mixing then-chains and async-await, if we can avoid it.

[1] https://searchfox.org/mozilla-central/rev/2466b82b729765fb0a3ab62f812c1a96a7362478/dom/media/tests/mochitest/mediaStreamPlayback.js#247-250

::: dom/media/tests/mochitest/test_setSinkId.html:22
(Diff revision 2)
> +
> +  /**
> +   * Run a test to verify set sink id in audio element.
> +   */
> +  scriptsReady.then(() => runTestWhenReady(async () => {
> +    let audioDevice = SpecialPowers.getCharPref("media.audio_loopback_dev", "");

We want to use s/let/const/ wherever possible in new code.

::: dom/media/tests/mochitest/test_setSinkId.html:24
(Diff revision 2)
> +   * Run a test to verify set sink id in audio element.
> +   */
> +  scriptsReady.then(() => runTestWhenReady(async () => {
> +    let audioDevice = SpecialPowers.getCharPref("media.audio_loopback_dev", "");
> +    if (!audioDevice) {
> +      todo(false, "No loopback device set by framework. Try --use-test-media-devices");

Do we plan on adding additional tests to take advantage of loopback devices and verify that setSinkId actually works?

::: dom/media/tests/mochitest/test_setSinkId.html:25
(Diff revision 2)
> +   */
> +  scriptsReady.then(() => runTestWhenReady(async () => {
> +    let audioDevice = SpecialPowers.getCharPref("media.audio_loopback_dev", "");
> +    if (!audioDevice) {
> +      todo(false, "No loopback device set by framework. Try --use-test-media-devices");
> +      return Promise.resolve();

Redundant. Just `return` will do.

::: dom/media/tests/mochitest/test_setSinkId.html:28
(Diff revision 2)
> +    if (!audioDevice) {
> +      todo(false, "No loopback device set by framework. Try --use-test-media-devices");
> +      return Promise.resolve();
> +    }
> +
> +    let allDevices = await navigator.mediaDevices.enumerateDevices()

missing ;

Applies throughout.

::: dom/media/tests/mochitest/test_setSinkId.html:34
(Diff revision 2)
> +      await a1.setSinkId(audioDevices[0].deviceId)
> +      ok(true, "Sink device is set, id: " + a1.sinkId);

We should test a few more things around this API.

1. Test that initial value of a1.sinkId is ""
2. Test that a1.sinkId value is unchanged upon function return
3. Test that promise resolves with undefined
4. Upon resolution of promise, test that is(a1.sinkId == audioDevices[0].deviceId)

Also, in the interest of not adding more powerful features in http, should we perhaps limit this function to https, and throw NotFoundError for http?

I guess there's no way to test AbortError.

In that case, we could test that as well here, though we might need to split this test into a separate http and https file for that.

::: dom/media/tests/mochitest/test_setSinkId.html:36
(Diff revision 2)
> +    ok(audioDevices.length > 0, "More than one output device found");
> +
> +    try {
> +      await a1.setSinkId(audioDevices[0].deviceId)
> +      ok(true, "Sink device is set, id: " + a1.sinkId);
> +    } catch(error) {

Nit: space after catch [1]

[1] While `catch` is not mentioned in
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures

...it's a JS keyword like the others, and we seem to be inconsistent from a quick grep.

Applies throughout.
Attachment #8996966 - Flags: review?(jib) → review-
Comment on attachment 8994296 [details]
Bug 934425 - Implement setSinkId in HTMLMediaElement.

https://reviewboard.mozilla.org/r/258886/#review268004

> Good question. Ultimately, I think we want to implement the spec here, not Chrome's behavior. 
> 
> But in the interest of incrementing, I'm also open to defer this part until we fix bug 1272588.

As long as we fix it before shipping setSinkId. If you land this without compliance to 2.1.2, please file the appropriate followups and make them block the bug where we will enable setSinkId by default.

> Because I need to keep it alive. The current method may return before that promise get resolved or rejected. If it was a local variable it would be removed by the end of the method.

I haven't dug up proof but I think there's a refcount holding it alive until its resolve or reject runnables have been dispatched.
Please verify that it indeed gets destroyed prematurely like you say (which should fail some dtor asserts too) before using a member.

> This happens when the given sink id does not corresponds to any device in the system.

Shouldn't the promise be rejected in this case?

> It's a member in order to stay alive after the lamda method has returned.

Again I think it will stay alive beyond this on its own.
Comment on attachment 8996966 [details]
Bug 934425 - Add mochitest for setSinkId.

https://reviewboard.mozilla.org/r/260940/#review269402

> Please use 
> 
>     await SpecialPowers.pushPrefEnv
> 
> in new code (bug 1056851).

Let's not forget our head.js shorthand, https://searchfox.org/mozilla-central/rev/2466b82b729765fb0a3ab62f812c1a96a7362478/dom/media/tests/mochitest/head.js#398-400
> should we perhaps limit this function to https, and throw NotFoundError for http?

Cut'n'paste error. I meant SecurityError here.
Also, as we discussed off-line, the test for SecurityError (at least) could and probably should be a web-platform-test (the remaining tests will probably have to remain as mochitests until we support fake devices for wpt.
Comment on attachment 8994292 [details]
Bug 934425 - Create a method in MediaManager to look up a given sink id.

MozReview is no more.
Attachment #8994292 - Flags: review?(jib)
MediaDevice is the core object for audio device enumeration. By adding AudioDeviceInfo every information of audio devices will be available. In this case device id will be available from the first enumeration and there is no need to enumerate multiple times to get that info.

Bug 934425 - Create a method in MediaManager to look up a given sink id. r?jib

Implement a new method in MediaManager that enumerates audio output devices and looks up for a given sink id asynchronously.

Bug 934425 - Set device info in MediaSink and switch sink device. r?padenot

Bug 934425 - Implement asynchronous method to switch sink in MediaDecoder. r?pehrsons

Bug 934425 - Add SinkId in webidl for HTMLMediaElement. r?bryce,smaug

Bug 934425 - Implement setSinkId in HTMLMediaElement. r?pehrsons

Bug 934425 - Add mochitest for setSinkId. r?jib

Bug 934425 - Web platform http test for setSinkId. r?jib

Bug 934425 - Enable the pref for wpt https test for setSinkId and update it to expect NotAllowedError. r?jib
Comment on attachment 8994294 [details]
Bug 934425 - Implement asynchronous method to switch sink in MediaDecoder.

Dropping myself off old reviews as reviewboard is retired. Please add me to the phabricator review as needed.

I'm also going to obsolete the patches where I'm sole reviewer to unclutter the attachments (feels presumptuous to do it to ones with other persons involved).
Attachment #8994294 - Attachment is obsolete: true
Attachment #8994294 - Flags: review?(bvandyk)
Attachment #8994296 - Attachment is obsolete: true
Attachment #8994296 - Flags: review?(bvandyk)
MediaDevice is the core object for audio device enumeration. By adding AudioDeviceInfo every information of audio devices will be available. In this case device id will be available from the first enumeration and there is no need to enumerate multiple times to get that info.
Implement a new method in MediaManager that enumerates audio output devices and looks up for a given sink id asynchronously.
Comment on attachment 9009141 [details]
Bug 934425 - Add SinkId in webidl for HTMLMediaElement. r?bryce,smaug

Olli Pettay [:smaug] has approved the revision.
Attachment #9009141 - Flags: review+
Comment on attachment 9009145 [details]
Bug 934425 - Enable the pref for wpt https test for setSinkId and update it to expect NotAllowedError. r?jib

Jan-Ivar Bruaroey [:jib] (needinfo? me) has approved the revision.
Attachment #9009145 - Flags: review+
Comment on attachment 9009139 [details]
Bug 934425 - Set device info in MediaSink and switch sink device. r?jya

Jean-Yves Avenard [:jya] has approved the revision.
Attachment #9009139 - Flags: review+
Comment on attachment 9009137 [details]
Bug 934425 - Add device info in MediaDevice. r?jib

Jan-Ivar Bruaroey [:jib] (needinfo? me) has approved the revision.
Attachment #9009137 - Flags: review+
See Also: → 928644
Attachment #9007823 - Attachment is obsolete: true
Depends on: 1493982
Depends on: 1493990
Comment on attachment 9009138 [details]
Bug 934425 - Create a method in MediaManager to look up a given sink id. r?jib

Jan-Ivar Bruaroey [:jib] (needinfo? me) has approved the revision.
Attachment #9009138 - Flags: review+
Blocks: 1498512
Pushed by achronopoulos@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7640652718bd
Add device info in MediaDevice. r=jib
https://hg.mozilla.org/integration/autoland/rev/ae5bc4f62937
Create a method in MediaManager to look up a given sink id. r=jib
https://hg.mozilla.org/integration/autoland/rev/923d8e25777e
Set device info in MediaSink and switch sink device. r=jya
https://hg.mozilla.org/integration/autoland/rev/87921c31f0b7
Implement asynchronous method to switch sink in MediaDecoder. r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/8aef0f43b2d8
Add SinkId in webidl for HTMLMediaElement. r=smaug
https://hg.mozilla.org/integration/autoland/rev/55c832b1f2fe
Implement setSinkId in HTMLMediaElement. r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/94caf76cf9a3
Add mochitest for setSinkId. r=jib
https://hg.mozilla.org/integration/autoland/rev/341a3c4a36fd
Web platform http test for setSinkId. r=jib
https://hg.mozilla.org/integration/autoland/rev/8fae8b6f7572
Enable the pref for wpt https test for setSinkId and update it to expect NotAllowedError. r=jib
See Also: → 1498679
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/13493 for changes under testing/web-platform/tests
(In reply to Paul Adenot (:padenot) from comment #32)
> This needs tests.

Tests added clearing the NI.
Flags: needinfo?(achronop)
Depends on: 1499615
Note to documentation team:

I've added an entry about this to the Experimental features page:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Experimental_features#DOM

Once this is enabled by default (see https://bugzilla.mozilla.org/show_bug.cgi?id=1498512), this will need to be removed, you'll need to add it to the relevant rel notes, and and you'll also have to update the compat data. It looks to be documented already. but give the docs a review to make sure they are up to scratch.
Attachment #8994293 - Flags: review?(padenot)
Documentation done, for now:

I've edited/improved the setSinkId() compat data:
https://developer.mozilla.org/en-US/docs/Web/API/HTMLMediaElement/setSinkId

I've also submitted a PR to update the relevant compat data:
https://github.com/mdn/browser-compat-data/pull/3119/files
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: