1156472 - Allow of capture of audio in various forms of screen sharing (tab/window/etc)

Status: RESOLVED FIXED

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

Description

[Maire Reavy [:mreavy]]

Various use cases are enhanced by being able to capture audio along with video for screen/tab/etc sharing.  In particular, it's most useful when combined with tab sharing and full screen sharing.  

Note: it may not be possible with full screen sharing on all platforms, but we can at least capture the audio from the browser in that case.

We also need to propose standardization for the constraints necessary to request this.

------

Here's the requirements write up:

There is a need for audio sharing to be combined with other forms of sharing.  This would allow sharing content such as a video over a PeerConnection.

A major reason we haven't done audio indicators in tabs in the past was that we couldn't tell if plugins (i.e. Flash) were generating audio.  This same issue would apply to audio capture for gUM; it's hard to capture audio generated by a plugin.

Assumption: mreavy to confirm with externals that not capturing plugin-generated audio is acceptable, at least for the initial use cases.

The other major issue is which forms of capture can this be combined with.  Right now we support tab, window, app and screen sharing for video.

Window and app sharing are feasible if the window/app are the same browser, but are effectively impossible if the window/app being shared is anything else.  This would be confusing to users and hard to explain, so this is out of scope for this phase.

Tab audio (if not a plugin) is feasible in all cases, and is the primary usecase.

Screen sharing with audio is feasible on at least some platforms (e.g. Windows, Linux/pulse).  This is less confusing, since we can simply remove or ghost out the option on other platforms.  This should be part of the target, at least as a stretch goal.

Comment 1

[Paul Adenot (:padenot)]

So, I'm planning to do this like so:

- Use the AudioChannelAgent and baku's new methods [0] to be able to address all the HTMLMediaElement and AudioContexts of a tab. When AudioContexts and HTMLMediaElement are created/destroyed, everything is already in place to refresh the list.
- Add a DOMWindowUtil::SetAudioCaptured(bool aCaptured), that returns a MediaStream that contains a mix of all the audio content of a tab.
- Use the too-be-written MediaStream constructor API and the MediaStream from a tab capture to have a MediaStream that has the audio for the tab and the video for the tab

Internally:

- When DOMWindowUtil::SetAudioCaptured is called, iterate over all agents (media elements + audiocontexts), and connect the stream from mozCaptureStream/the AudioDestinationNode to a new DOMMediaStream backed by a ProcessedMediaStream (or something) that mixes its inputs. We only keep the audio track there, video is dealt with in another way.
- We need to have something that bypasses CORS restrictions so we can capture an HTMLMediaElement output regardless of its origin.
- We need to be able to stop mozCaptureStream an HTMLMediaElement, so that we can stop capturing a tab. I don't think it's possible right now.

This will theoretically allow us to do the following:

- From chrome javascript or C++, capture all audio and get a MediaStream back
- Connect it to whatever we want (be it a PeerConnection, a MediaRecorder, etc.)
- Build another MediaStream with the audio track of the capture MediaStream. It will all come from the same MSG, so it should be synchronized

Maire, does that sound like something that would make the final use-cases solvable ? It's not entirely clear to me what those use-cases are, so I went for a generic "capture all audio for the tab into a MediaStream" approach.

[0]: https://dxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#3718

Comment 2

[Paul Adenot (:padenot)]

jesup, do you know if calling mozCaptureStream on an HTMLMediaElement that has a MediaStream as a source works ? If not that adds another step to the plan above, so we can capture tabs that are doing WebRTC calls or the like.

Comment 3

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

(In reply to Paul Adenot (:padenot) from comment #2)
> jesup, do you know if calling mozCaptureStream on an HTMLMediaElement that
> has a MediaStream as a source works ? If not that adds another step to the
> plan above, so we can capture tabs that are doing WebRTC calls or the like.

That *should* work; check with roc and/or pehrsons.  Also pretty easy to test (just modify gum_test to capture from the <video> and use that to feed to another <video>)

Comment 4

[Maire Reavy [:mreavy]]

Paul -- yes, this sounds reasonable.  The primary usecase here is capturing audio along with video from a tab via getUserMedia.

Comment 5

[Andreas Pehrson [:pehrsons]]

(In reply to Randell Jesup [:jesup] from comment #3)
> (In reply to Paul Adenot (:padenot) from comment #2)
> > jesup, do you know if calling mozCaptureStream on an HTMLMediaElement that
> > has a MediaStream as a source works ? If not that adds another step to the
> > plan above, so we can capture tabs that are doing WebRTC calls or the like.
> 
> That *should* work; check with roc and/or pehrsons.  Also pretty easy to
> test (just modify gum_test to capture from the <video> and use that to feed
> to another <video>)

It's unfortunately not supported at the moment.

See MediaDecoder's captureStream case at https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLMediaElement.cpp#1887

And there's this comment in HTMLMediaElement::SetupSrcMediaStreamPlayback:
>   // XXX if we ever support capturing the output of a media element which is
>   // playing a stream, we'll need to add a CombineWithPrincipal call here.

Comment 6

[Paul Adenot (:padenot)]

Attachment: Part 1 - Allow to capture all HTMLMediaElements and AudioContexts for a document. r=

This is built on top of the AudioChannel infrastructure. This patch does not
actually implement the capture, it just does the plumbing to be able to notify
all HTMLMediaElement/AudioContext for a document.

Comment 7

[Paul Adenot (:padenot)]

Attachment: Part 2 - Rename MediaEngineWebRTCAudioSource to MediaEngineWebRTCMicrophoneSource. r=

There are now two different possible audio source, so this was getting confusing.

Comment 8

[Paul Adenot (:padenot)]

Attachment: Part 3 - Implement AudioCaptureStream. r=

It is a ProcessMediaStream that simply mixes its inputs into a stereo stream,
up/down mixing appropriately.

Comment 9

[Paul Adenot (:padenot)]

Attachment: Part 4 - Add a new MediaStreamGraph API to connect a MediaStream to a capture stream. r=

Comment 10

[Paul Adenot (:padenot)]

Attachment: Part 5 - Add MediaEngineWebRTCAudioCaptureSource as a new audio source, and "audioCapture" as a new MediaSource. r=

This contains a lot of printfs, I'll make sure to remove them before landing.

Comment 11

[Paul Adenot (:padenot)]

Attachment: Part 6 - Allow to un-capture an HTMLMediaElement. r=

Comment 12

[Paul Adenot (:padenot)]

Attachment: Part 7 - Connect HTMLMediaElement and AudioContext to the capture stream when capturing is needed. r=

Comment 13

[Paul Adenot (:padenot)]

Attachment: Part 8 - Use fatal asserts in AudioChannelUpmix, because it would have crashed anyways

Comment 14

[Paul Adenot (:padenot)]

Attachment: Part 9 - Enable the AudioChannel pref

This is probably gonna land via baku's patches.

Comment 15

[Paul Adenot (:padenot)]

Attachment: Part 10 - Implement the minimum frontend to play with the feature

Comment 16

[Paul Adenot (:padenot)]

Attachment: Part 12 - Unbitrot MediaManager.cpp over jib's changes

Comment 17

[Paul Adenot (:padenot)]

Attachment: Part 13 - Allow to pipe the AudioCaptureStream into an AudioContext. r=

Comment 18

[Paul Adenot (:padenot)]

Attachment: Part 14 - Unbitrot MediaDecoderStateMachine after jwwang's changes. r=

Comment 19

[Paul Adenot (:padenot)]

Attachment: Part 11 - Test AudioCaptureStream

Comment 20

[Paul Adenot (:padenot)]

Also, this is behind a pref: `media.getusermedia.audiocapture.enabled`.

Comment 21

[Paul Adenot (:padenot)]

Comment on attachment 8630036 [details] [diff] [review]
Part 13 - Allow to pipe the AudioCaptureStream into an AudioContext. r=

Martin, roc, this is a first attempt at allowing authors to use the connect the capture MediaStream to, say, Web Audio API, or MediaRecorder.

Is the approach adequate, or am I going in the wrong direction?

Comment 22

[Paul Adenot (:padenot)]

All, I'd like to land this preffed-off, and work on the security bits later. We need to rework the way HTMLMediaElement and the MSG interact to be able to hear the captured HTMLMediaElements, and some other things.

Comment 23

[Martin Thomson [:mt:]]

Comment on attachment 8630036 [details] [diff] [review]
Part 13 - Allow to pipe the AudioCaptureStream into an AudioContext. r=

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

:padenot, you really need to get reviewboard working here.  Context matters.  In this case, it matters a lot.

I think that I'm obligated to r- this.  On the one hand, you have elevated the principal of the captured stream to the extreme, so very few things will have access to the contents.  On the other hand, you have carved out a special exception for MediaRecorder; the problem with that exception is that it violates the safeguards we put in place.  What if someone attempts to capture a media stream that happens to have a system principal for some other reason?

An anonymous CORS mode is probably safe, unless you are going to make HTTP requests and then it's going to work until you have a credential-protected resource.  I think that you don't make requests in this case, but I don't have the context.

Perhaps you can start by articulating what properties you want to have here.

Comment 24

[Martin Thomson [:mt:]]

Paul, to expand on what I said earlier, I don't think that the security piece is hard.  It just needs a little thought.  Land then fix in this case is not acceptable.

I think that what you need to do is set the principal of the captured stream to the current document principal.  Just like everyone else.  Then you don't need that super-dangerous code in MediaRecorder.

Comment 25

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8630035 [details] [diff] [review]
Part 12 - Unbitrot MediaManager.cpp over jib's changes

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

lgtm with nits

::: dom/media/MediaManager.cpp
@@ +388,5 @@
>  
>  AudioDevice::AudioDevice(MediaEngineAudioSource* aSource)
>    : MediaDevice(aSource, false)
>  {
> +  mMediaSource = aSources->GetMediaSource();

typo

@@ +1699,5 @@
> +                             audioType);
> +    // Only enable AudioCapture if the pref is enabled. If it's not, we can deny
> +    // right away.
> +    if (audioType == dom::MediaSourceEnum::AudioCapture &&
> +        !Preferences::GetBool("media.getusermedia.audiocapture.enabled")) {

Since mediaSource is a cross-type setting now, we should probably detect and fail on known-wrong values (MediaSourceEnum::Camera and the screensharing ones), much like the corresponding video case fails on MediaSourceEnum::Microphone (we should add MediaSourceEnum::AudioCapture) here:

http://mxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp?rev=0ac19d3bf7bf&mark=1643-1650#1600

Comment 26

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

Comment on attachment 8630026 [details] [diff] [review]
Part 2 - Rename MediaEngineWebRTCAudioSource to MediaEngineWebRTCMicrophoneSource. r=

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

::: dom/media/webrtc/MediaEngineWebRTC.h
@@ +137,2 @@
>                                       public webrtc::VoEMediaProcess,
>                                       private MediaConstraintsHelper

fix indents

@@ +142,1 @@
>                                 int aIndex, const char* name, const char* uuid)

indents, rewrap perhaps

Comment 27

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8630038 [details] [diff] [review]
Part 11 - Test AudioCaptureStream

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

Without going too deep on the details I think your approach is great but I'd like to see more usage of the testing frameworks. See dom/media/tests/mochitest/mediaStreamPlayback.js for a lot of free stuff on gUM testing, and dom/media/tests/mochitest/pc.js's AudioStreamAnalyser for some WebAudio tone testing features I put in a while back when writing the test_peerConnection_webAudio.html test. You could break it out to get some common ground for both pc and gUM testing, and adapt it to fit your needs.

::: dom/media/tests/mochitest/mochitest.ini
@@ +181,5 @@
>  skip-if = toolkit == 'gonk' || buildapp == 'mulet' # b2g (Bug 1059867)
>  [test_peerConnection_remoteReofferRollback.html]
>  skip-if = toolkit == 'gonk' || buildapp == 'mulet' # b2g (Bug 1059867)
> +[test_getUserMedia_audioCapture.html]
> +skip-if = toolkit == 'gonk' || buildapp == 'mulet' # b2g (Bug 1059867)

Keep the list sorted please.

Also, bug 1059867 is about peerconnections, shouldn't affect this test.

::: dom/media/tests/mochitest/test_getUserMedia_audioCapture.html
@@ +38,5 @@
> +  /**
> +   * Get two HTMLMediaElements:
> +   * - One playing a sine tone from a blob (of an opus file created on the fly)
> +   * - One being the output for an AudioContext's OscillatorNode, connected to
> +   *   a MediaSourceDestinationNode.

MediaStreamAudioDestinationNode

Comment 28

[Andreas Pehrson [:pehrsons]]

I'd also love to see reviewboard used for a big bug like this. Once set up it's trivial to get started - `hg push review` is all you need! :)

Comment 29

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8630030 [details] [diff] [review]
Part 6 - Allow to un-capture an HTMLMediaElement. r=

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +3174,5 @@
> +void MediaDecoderStateMachine::RemoveOutputStream()
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  DECODER_LOG("RemoveOutputStream!");
> +  ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());

This is against our goal to remove the decoder monitor from the state machine. We should only use the decoder monitor on the state machine thread.

Since DecodedStream has its own monitor, you can make DecodedStream::DestroyData() callable on any thread or create another method like DispatchDestroyData() which will delegate to DestroyData() which will run on the main thread.

Comment 30

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8630037 [details] [diff] [review]
Part 14 - Unbitrot MediaDecoderStateMachine after jwwang's changes. r=

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +3175,5 @@
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>    DECODER_LOG("RemoveOutputStream!");
>    ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
> +  if (!mDecodedStream) {

This patch depends on part 6. Please request review again after part 6 is addressed.

Comment 31

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

Comment on attachment 8630027 [details] [diff] [review]
Part 3 - Implement AudioCaptureStream. r=

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

r+ modulo the b2g question

::: dom/media/AudioCaptureStream.cpp
@@ +26,5 @@
> +namespace mozilla
> +{
> +
> +// We are mixing to stereo
> +static const uint32_t STEREO = 2;

What happens when feeding this to a (mono) PeerConnection?

@@ +119,5 @@
> +  }
> +  AudioChunk chunk;
> +  chunk.mBuffer = new mozilla::SharedChannelArrayBuffer<float>(&output);
> +  chunk.mDuration = aFrames;
> +  chunk.mBufferFormat = AUDIO_FORMAT_FLOAT32;

Is this correct for e.g. B2G?

::: dom/media/AudioSegment.cpp
@@ +149,5 @@
> +// This helps to to safely get a pointer to the position we want to start
> +// writing a planar audio buffer, depending on the channel and the offset in the
> +// buffer.
> +static AudioDataValue*
> +PointerForOffsetInChannel(AudioDataValue* aData, size_t aLengthSamples,

const?

Comment 32

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

Comment on attachment 8630028 [details] [diff] [review]
Part 4 - Add a new MediaStreamGraph API to connect a MediaStream to a capture stream. r=

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

::: dom/media/MediaStreamGraph.cpp
@@ +3192,5 @@
> +ProcessedMediaStream*
> +MediaStreamGraph::CreateAudioCaptureStream(DOMMediaStream* aWrapper)
> +{
> +  AudioCaptureStream* stream = new AudioCaptureStream(aWrapper);
> +  NS_ADDREF(stream);

Shouldn't this return already_AddRefed<>?
nsRefPtr<AudioCaptureStream> stream = ...
...
return stream.forget();

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8630036 [details] [diff] [review]
Part 13 - Allow to pipe the AudioCaptureStream into an AudioContext. r=

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

::: dom/media/MediaManager.cpp
@@ +794,5 @@
>      if (mAudioSource->GetMediaSource() == dom::MediaSourceEnum::AudioCapture) {
>        domStream = DOMLocalMediaStream::CreateAudioCaptureStream(window);
> +      // It should be possible to pipe that to anything. CORS is not a problem
> +      // here, we got explicit user content.
> +      domStream->SetPrincipal(nsContentUtils::GetSystemPrincipal());

What Martin said.

This comment doesn't make sense to me. This line ensures that only system principals can get the stream contents, which is good, but it doesn't mean "you can pipe that to anything". On the contrary, you can pipe anything into it, but only system principals can't get anything out.

Comment 34

[Paul Adenot (:padenot)]

(In reply to Randell Jesup [:jesup] from comment #31)
> Comment on attachment 8630027 [details] [diff] [review]
> Part 3 - Implement AudioCaptureStream. r=
> 
> Review of attachment 8630027 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ modulo the b2g question
> 
> ::: dom/media/AudioCaptureStream.cpp
> @@ +26,5 @@
> > +namespace mozilla
> > +{
> > +
> > +// We are mixing to stereo
> > +static const uint32_t STEREO = 2;
> 
> What happens when feeding this to a (mono) PeerConnection?

It most likely breaks. PeerConnection assumes mono input, there are numerous TODO in the code stating that. I'll post a followup patch.
 
> @@ +119,5 @@
> > +  }
> > +  AudioChunk chunk;
> > +  chunk.mBuffer = new mozilla::SharedChannelArrayBuffer<float>(&output);
> > +  chunk.mDuration = aFrames;
> > +  chunk.mBufferFormat = AUDIO_FORMAT_FLOAT32;
> 
> Is this correct for e.g. B2G?

Yes. The MSG only does floats, regardless of the platform.

> ::: dom/media/AudioSegment.cpp
> @@ +149,5 @@
> > +// This helps to to safely get a pointer to the position we want to start
> > +// writing a planar audio buffer, depending on the channel and the offset in the
> > +// buffer.
> > +static AudioDataValue*
> > +PointerForOffsetInChannel(AudioDataValue* aData, size_t aLengthSamples,
> 
> const?

Yeah.

Comment 35

[Paul Adenot (:padenot)]

(In reply to Randell Jesup [:jesup] from comment #32)
> Comment on attachment 8630028 [details] [diff] [review]
> Part 4 - Add a new MediaStreamGraph API to connect a MediaStream to a
> capture stream. r=
> 
> Review of attachment 8630028 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaStreamGraph.cpp
> @@ +3192,5 @@
> > +ProcessedMediaStream*
> > +MediaStreamGraph::CreateAudioCaptureStream(DOMMediaStream* aWrapper)
> > +{
> > +  AudioCaptureStream* stream = new AudioCaptureStream(aWrapper);
> > +  NS_ADDREF(stream);
> 
> Shouldn't this return already_AddRefed<>?
> nsRefPtr<AudioCaptureStream> stream = ...
> ...
> return stream.forget();

Well, maybe, but no other MediaStreamGraph::Create* do that.

Comment 36

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

Comment on attachment 8630029 [details] [diff] [review]
Part 5 - Add MediaEngineWebRTCAudioCaptureSource as a new audio source, and "audioCapture" as a new MediaSource. r=

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

::: dom/media/MediaManager.cpp
@@ +310,5 @@
>  }
>  
>  VideoDevice::VideoDevice(MediaEngineVideoSource* aSource)
>    : MediaDevice(aSource, true)
> +{ }

nit: {}

@@ +788,5 @@
> +
> +    nsRefPtr<DOMLocalMediaStream> domStream;
> +    // AudioCapture is a special case, here, in the sense that we're not really
> +    // using the audio source and the SourceMediaStream, which acts as
> +    // placeholders. We re-route a number of stream internaly in the MSG and mix

nit: internally

@@ -827,5 @@
> -      trackunion->SetPeerIdentity(mPeerIdentity.forget());
> -    } else {
> -      principal = window->GetExtantDoc()->NodePrincipal();
> -    }
> -    trackunion->CombineWithPrincipal(principal);

Probably covered by mt's review, but we totally lost this principal code for all captures here, not just audio capture

::: dom/media/webrtc/MediaEngineWebRTC.cpp
@@ +291,5 @@
>    // We spawn threads to handle gUM runnables, so we must protect the member vars
>    MutexAutoLock lock(mMutex);
>  
> +  if (aMediaSource == dom::MediaSourceEnum::AudioCapture) {
> +    printf("Found a media source audio capture\n");

debug cruft

::: dom/media/webrtc/MediaEngineWebRTC.h
@@ +141,5 @@
> +  MediaEngineWebRTCAudioCaptureSource(const char* aUuid)
> +    : MediaEngineAudioSource(kReleased)
> +  {
> +  }
> +  virtual void GetName(nsAString& aName) override;

My understanding is we now prefer now 'virtual' if it's 'override', but I don't care.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp
@@ +623,5 @@
>  
> +void
> +MediaEngineWebRTCAudioCaptureSource::GetName(nsAString &aName)
> +{
> +  aName.Assign(MOZ_UTF16("&getUserMedia.audioCapture.label;"));

A constant string with &variable?

@@ +628,5 @@
>  }
> +void
> +MediaEngineWebRTCAudioCaptureSource::GetUUID(nsACString &aUUID)
> +{
> +  printf("TRACE : %s\n", __PRETTY_FUNCTION__);

debug cruft....  And we should do *something* here...

@@ +635,5 @@
> +nsresult
> +MediaEngineWebRTCAudioCaptureSource::Deallocate()
> +{
> +  printf("TRACE : %s\n", __PRETTY_FUNCTION__);
> +  return NS_OK;

really, there's nothing to do on Allocatre/Deallocate?  1-liner comment if so.

Please put Allocate() here as well (easier to read).

@@ +657,5 @@
> +void
> +MediaEngineWebRTCAudioCaptureSource::SetDirectListeners(bool aDirect)
> +{
> +  printf("TRACE : %s\n", __PRETTY_FUNCTION__);
> +  MOZ_ASSERT(false, "to implement");

This is only used if the capture class needs to know if there are direct listeners (currently only Gonk video capture, for dynamic rotation - as a trick to avoid rotating if attached to something other than a PeerConnection).

Just have it be {} in the .h file like most others.  (lots of these should just be {} or { return false/true/NS_OK; } in the .h file)

@@ +707,5 @@
> +
> +void
> +MediaEngineWebRTCAudioCaptureSource::Shutdown()
> +{
> +  printf("TRACE : %s\n", __PRETTY_FUNCTION__);

Nothing to do here?

::: dom/webidl/Constraints.webidl
@@ +24,5 @@
>      "application",
>      "window",
>      "browser",
>      "microphone",
> +    "audioCapture",

officially you need a dom peer, and hg hooks will block you without it

Comment 37

[Florian Quèze [:florian]]

Comment on attachment 8630034 [details] [diff] [review]
Part 10 - Implement the minimum frontend to play with the feature

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

Looks reasonable.

The following comments are only from code inspection, I haven't tried the patchset yet.


More new strings will be needed in browser/locales/en-US/chrome/browser/webrtcIndicator.properties

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +549,2 @@
>  #                    getUserMedia.shareScreen.message, getUserMedia.shareCameraAndMicrophone.message,
>  #                    getUserMedia.shareScreenAndMicrophone.message):

The list of string ids in this localization note need to be updated.

@@ +553,5 @@
>  getUserMedia.shareMicrophone.message = Would you like to share your microphone with %S?
>  getUserMedia.shareScreen.message = Would you like to share your screen with %S?
>  getUserMedia.shareCameraAndMicrophone.message = Would you like to share your camera and microphone with %S?
>  getUserMedia.shareScreenAndMicrophone.message = Would you like to share your microphone and screen with %S?
> +getUserMedia.shareAudioCapture.message  = Would you like to share this tab's audio with %S?

nit: double space before '='.

The id for this string is: "getUserMedia.share" + requestTypes.join("And") + ".message".

From the discussion in #media a few minutes ago, it seems that having a camera + audio capture, or screen share + audio capture is possible.

@@ +621,5 @@
>  #                    getUserMedia.sharingMenuCameraBrowser,
>  #                    getUserMedia.sharingMenuMicrophoneApplication,
>  #                    getUserMedia.sharingMenuMicrophoneScreen,
>  #                    getUserMedia.sharingMenuMicrophoneWindow,
>  #                    getUserMedia.sharingMenuMicrophoneBrowser):

please update the l10n note.

@@ +625,5 @@
>  #                    getUserMedia.sharingMenuMicrophoneBrowser):
>  # %S is the website origin (e.g. www.mozilla.org)
>  getUserMedia.sharingMenuCamera = %S (camera)
>  getUserMedia.sharingMenuMicrophone = %S (microphone)
> +getUserMedia.sharingMenuAudioCapture = %S (tab capture)

did you mean "tab audio" rather than "tab capture" here?

::: browser/modules/webrtcUI.jsm
@@ +213,5 @@
>      accessKey: stringBundle.getString("getUserMedia.shareSelectedDevices.accesskey"),
>      // The real callback will be set during the "showing" event. The
>      // empty function here is so that PopupNotifications.show doesn't
>      // reject the action.
> +    callback: function() { }

nit: this change doesn't seem intentional.

@@ +248,2 @@
>      // Don't show the 'Always' action if the connection isn't secure, or for
>      // screen sharing (because we can't guess which window the user wants to

Looks like this comment needs updating.

@@ +269,5 @@
>          let popupId = "Devices";
>          if (requestTypes.length == 1 && requestTypes[0] == "Microphone")
>            popupId = "Microphone";
> +        if (requestTypes.length == 1 && requestTypes[0] == "AudioCapture")
> +          popupId = "AudioCapture";

The popupId you set here needs to match something from the CSS: http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/notification-icons.inc.css#82

If you don't intend to add a new icon, you should not create a new AudioCapture id.

@@ +391,5 @@
>  
>        let camMenupopup = chromeDoc.getElementById("webRTC-selectCamera-menupopup");
>        let windowMenupopup = chromeDoc.getElementById("webRTC-selectWindow-menupopup");
>        let micMenupopup = chromeDoc.getElementById("webRTC-selectMicrophone-menupopup");
> +      if (sharingScreen) {

I would prefer if we didn't change the coding style here, especially as most of this file seems to not use {} for if/else when there's only a single line.

@@ +403,3 @@
>        if (requestTypes.length == 2) {
>          let stringBundle = chromeDoc.defaultView.gNavigatorBundle;
> +        if (!sharingScreen && !sharingAudio)

please revert this...

@@ +406,2 @@
>            addDeviceToList(camMenupopup, stringBundle.getString("getUserMedia.noVideo.label"), "-1");
>          addDeviceToList(micMenupopup, stringBundle.getString("getUserMedia.noAudio.label"), "-1");

... and add |if (!sharingAudio)| before this line.

@@ +421,5 @@
>                        allowCamera ? perms.ALLOW_ACTION : perms.DENY_ACTION);
>            }
>          }
>          if (audioDevices.length) {
> +          let listId = "webRTC-select" + (sharingAudio ? "AudioCapture" : "Microphone") + "-menulist";

You are not showing a list of AudioCapture devices, so this doesn't make sense.

@@ +433,5 @@
> +                        allowMic ? perms.ALLOW_ACTION : perms.DENY_ACTION);
> +            }
> +          } else {
> +            // xxx wtf, what am i supposed to do here? I don't have a device or
> +            // anything.

This actually seems fine, just add a comment saying that in the audio sharing case there's always one and only one device, and that 0 is always the index of that device.

If you have plans to let users select which audio needs to be captured in the future, maybe include a bug number reference in the comment.

Comment 38

[Florian Quèze [:florian]]

Comment on attachment 8630034 [details] [diff] [review]
Part 10 - Implement the minimum frontend to play with the feature

Would it be difficult to write tests for this piece of UI?

Eg. checking that the prompt appears, that once the user granted permission, the indicators are showing up as expected, and that we can stop sharing and have all the indicators correctly removed.

Comment 39

[Paul Adenot (:padenot)]

Attachment: Part 1 - Allow to capture all HTMLMediaElements and AudioContexts for a document. r=

This is built on top of the AudioChannel infrastructure. This patch does not
actually implement the capture, it just does the plumbing to be able to notify
all HTMLMediaElement/AudioContext for a document.

Comment 40

[Paul Adenot (:padenot)]

Attachment: Part 2 - Rename MediaEngineWebRTCAudioSource to MediaEngineWebRTCMicrophoneSource. r=

There are now two different possible audio source, so this was getting confusing.

Comment 41

[Paul Adenot (:padenot)]

Attachment: Part 3 - Implement AudioCaptureStream. r=

It is a ProcessMediaStream that simply mixes its inputs into a stereo stream,
up/down mixing appropriately.

Comment 42

[Paul Adenot (:padenot)]

Attachment: Part 4 - Add a new MediaStreamGraph API to connect a MediaStream to a capture stream. r=

Comment 43

[Paul Adenot (:padenot)]

Attachment: Part 5 - Add MediaEngineWebRTCAudioCaptureSource as a new audio source, and "audioCapture" as a new MediaSource. r=

Addressed comments.

Comment 44

[Paul Adenot (:padenot)]

Attachment: Part 7 - Connect HTMLMediaElement and AudioContext to the capture stream when capturing is needed. r=

Comment 45

[Paul Adenot (:padenot)]

Attachment: Part 11 - Test AudioCaptureStream

That is indeed better. I'm not sure about the NO_FAKE_DEVICE thing, it's a bit
ugly.

Comment 46

[Paul Adenot (:padenot)]

Attachment: Part 12 - Unbitrot MediaManager.cpp over jib's changes

Comment 47

[Paul Adenot (:padenot)]

Attachment: Part 13 - Allow to pipe the AudioCaptureStream into an AudioContext. r=

Yeah I was being stupid, this works just fine.

Comment 48

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

Comment on attachment 8631693 [details] [diff] [review]
Part 5 - Add MediaEngineWebRTCAudioCaptureSource as a new audio source, and "audioCapture" as a new MediaSource. r=

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

::: dom/media/webrtc/MediaEngineWebRTC.h
@@ +150,5 @@
> +  {
> +    // Nothing to do here, everything is managed in MediaManager.cpp
> +    return NS_OK;
> +  }
> +  virtual nsresult Deallocate() override

let's be consistent about virtual ... override or not.  I'd say match the rest of the file, but otherwise make it consistent within the class definition (i.e. remove it here).

Comment 49

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8631699 [details] [diff] [review]
Part 11 - Test AudioCaptureStream

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

Looks good! r=me with the comments addressed.

::: dom/media/tests/mochitest/head.js
@@ +36,5 @@
> +  getByteFrequencyData: function() {
> +    this.analyser.getByteFrequencyData(this.data);
> +    return this.data;
> +  },
> +  /**

One blank line between functions.

@@ +38,5 @@
> +    return this.data;
> +  },
> +  /**
> +   * Append a canvas to the DOM where the frequency data are drawn.
> +   * Useful to debug tests.

Please add @param and @return as appropriate since they exist throughout the rest of the test framework.

@@ +67,5 @@
> +   * Return a Promise, that will be resolved when the function passed as
> +   * argument, when called, returns true (meaning the analysis was a
> +   * success).
> +   */
> +  performAnalysis: function(analysisFunction) {

Not really a strong opinion, but I found the name confusing compared to its semantics, how about "waitForX" for some X?

@@ +88,5 @@
> +   */
> +  binIndexForFrequency: function(frequency) {
> +    return 1 + Math.round(frequency *
> +                          this.analyser.fftSize /
> +                          this.audioContext.sampleRate);

Ah, I've been looking for something like this! :-)

@@ +205,5 @@
> +  var FAKE_ENABLED = true;
> +  try {
> +    var audioDevice = SpecialPowers.getCharPref('media.audio_loopback_dev');
> +    var videoDevice = SpecialPowers.getCharPref('media.video_loopback_dev');
> +    dump('TEST DEVICES: Using media devices:\n');

I think you should keep these where they were, then you can do

> scriptsReady()
> .then(() => FAKE_ENABLED = false)
> .then(() => runTest(...));

@@ +215,5 @@
> +  }
> +
> +  if (window.NO_FAKE_DEVICES) {
> +    FAKE_ENABLED = false;
> +  }

...and you won't need NO_FAKE_DEVICES anymore.

::: dom/media/tests/mochitest/test_getUserMedia_audioCapture.html
@@ +15,5 @@
> +});
> +
> +SpecialPowers.pushPrefEnv({ "set": [
> +  [ 'media.recorder.audio_node.enabled', true ],
> +  [ 'media.getusermedia.audiocapture.enabled', true ]

You can just add these to head.js: https://dxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/head.js#127

@@ +21,5 @@
> +  NO_FAKE_DEVICES = true;
> +  runTest(function() {
> +
> +SpecialPowers.pushPrefEnv({ "set": [
> +  ['media.navigator.streams.fake', false]

This shouldn't be necessary if you have FAKE_ENABLED setup properly.

@@ +92,5 @@
> +    checkMediaStreamTracks(constraints, stream);
> +    window.grip = stream;
> +    var analyser = new AudioStreamAnalyser(ac, stream);
> +    analyser.enableDebugCanvas();
> +    return analyser.performAnalysis(function(array) {

Should we also test that playing the stream in a media element works?

Like
> var playback = new LocalMediaStreamPlayback(testVideoAudio, stream);
> return playback.playMedia(false)
>   .then(() => analyser.performAnalysis(...

Should stream.stop() work here as well? If so we can easily test it with `playMediaWithStreamStop()`.

@@ +97,5 @@
> +      // We want to find three frequency components here, around 1000, 5000
> +      // and 10000Hz. Frequency are logarithmic.
> +      return (array[analyser.binIndexForFrequency(1000)]  > 200 &&
> +              array[analyser.binIndexForFrequency(5000)]  > 200 &&
> +              array[analyser.binIndexForFrequency(10000)] > 200);

This is brilliant! We should file a bug for rewriting the other webaudio tests on this form. We can probably toss out pc.js' checkReceivingToneFrom() altogether.

Should we check that some frequencies in between are, say `< 50` as well?

Comment 50

[Paul Adenot (:padenot)]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #49)
> @@ +92,5 @@
> > +    checkMediaStreamTracks(constraints, stream);
> > +    window.grip = stream;
> > +    var analyser = new AudioStreamAnalyser(ac, stream);
> > +    analyser.enableDebugCanvas();
> > +    return analyser.performAnalysis(function(array) {
> 
> Should we also test that playing the stream in a media element works?
> 
> Like
> > var playback = new LocalMediaStreamPlayback(testVideoAudio, stream);
> > return playback.playMedia(false)
> >   .then(() => analyser.performAnalysis(...
> 
> Should stream.stop() work here as well? If so we can easily test it with
> `playMediaWithStreamStop()`.

This is weird to test, because you're creating a cycle, if you're playing something audible in the same page, it's gonna feed into the capture stream itself. The MSG handles this, though.

> 
> @@ +97,5 @@
> > +      // We want to find three frequency components here, around 1000, 5000
> > +      // and 10000Hz. Frequency are logarithmic.
> > +      return (array[analyser.binIndexForFrequency(1000)]  > 200 &&
> > +              array[analyser.binIndexForFrequency(5000)]  > 200 &&
> > +              array[analyser.binIndexForFrequency(10000)] > 200);
> 
> This is brilliant! We should file a bug for rewriting the other webaudio
> tests on this form. We can probably toss out pc.js' checkReceivingToneFrom()
> altogether.

Bug 1182459
 
> Should we check that some frequencies in between are, say `< 50` as well?

Good idea.

Comment 51

[Paul Adenot (:padenot)]

Attachment: Part 10 - Implement the minimum frontend to play with the feature

Florian, do you think this is good enough? We'd like to land this preffed off,
and then we can do the polish.

Comment 52

[Paul Adenot (:padenot)]

Attachment: Part 6 - Allow to un-capture an HTMLMediaElement. r=

That should only take the monitor on the right threads.

Comment 53

[Paul Adenot (:padenot)]

Comment on attachment 8630037 [details] [diff] [review]
Part 14 - Unbitrot MediaDecoderStateMachine after jwwang's changes. r=

(This got folded into part 6)

Comment 54

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8632152 [details] [diff] [review]
Part 6 - Allow to un-capture an HTMLMediaElement. r=

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

Pretty close. Still some issues to address.

::: dom/media/MediaDecoderStateMachine.cpp
@@ +3153,5 @@
> +    MOZ_ASSERT(self->OnTaskQueue());
> +    ReentrantMonitorAutoEnter mon(self->mDecoder->GetReentrantMonitor());
> +    if (self->mAudioCaptured) {
> +      // Start again the audio sink
> +      self->mStreamStartTime = -1;

Reset it to 0 for start time is now always 0 thanks to bholly's refactoring.

@@ +3154,5 @@
> +    ReentrantMonitorAutoEnter mon(self->mDecoder->GetReentrantMonitor());
> +    if (self->mAudioCaptured) {
> +      // Start again the audio sink
> +      self->mStreamStartTime = -1;
> +      self->mAudioStartTime = self->GetMediaTime();

MDSM::mAudioStartTime is removed in bug 1181504.

@@ +3159,5 @@
> +      self->mAudioEndTime = -1;
> +      self->mAudioCaptured = false;
> +      // We need to start the audio thread after setting mAudioCaptured to
> +      // false, otherwise we are caught by an early return there.
> +      self->StartAudioThread();

Only StartAudioThread when IsPlaying() is true since this could happen when we are seeking or buffering. MaybeStartPlayback() will StartAudioThread if necessary.

@@ +3184,5 @@
> +void MediaDecoderStateMachine::RemoveOutputStream()
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  DECODER_LOG("RemoveOutputStream!");
> +  if (!mDecodedStream) {

mDecodedStream is always non-null.

Comment 55

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8632152 [details] [diff] [review]
Part 6 - Allow to un-capture an HTMLMediaElement. r=

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

::: dom/media/MediaDecoder.cpp
@@ +309,5 @@
>    MOZ_ASSERT(mDecoderStateMachine, "Must be called after Load().");
>    mDecoderStateMachine->AddOutputStream(aStream, aFinishWhenEnded);
>  }
>  
> +void MediaDecoder::RemoveOutputStream()

My main concern is the semantics of this method.

We have AddOutputStream() which takes a ProcessedMediaStream and adds it to the decoded stream. Seems to me that RemoveOutputStream() should then take a ProcessedMediaStream and remove it.

That said I'm not entirely sure how you use it so maybe it should just be StopAllOutputStreams() instead.

::: dom/media/MediaDecoder.h
@@ +396,5 @@
>    // Add an output stream. All decoder output will be sent to the stream.
>    // The stream is initially blocked. The decoder is responsible for unblocking
>    // it while it is playing back.
>    virtual void AddOutputStream(ProcessedMediaStream* aStream, bool aFinishWhenEnded);
> +  virtual void RemoveOutputStream();

Please document with a comment.

::: dom/media/MediaDecoderStateMachine.cpp
@@ +3163,5 @@
> +      self->StartAudioThread();
> +      self->ScheduleStateMachine();
> +      nsCOMPtr<nsIRunnable> cleanupRunnable =
> +        NS_NewRunnableFunction([self] () -> void {
> +          self->mDecodedStream->DestroyData();

When uncapturing we should also clear mOutputStreams (and perhaps destroy their ports?), otherwise what would happen if you do:
> AddOutputStream(stream1)
> RemoveOutputStream()
> AddOutputStream(stream2)

I think an assertion will fail (mOutputStreams not empty when creating DecodedStreamData the first time), and stream1 will be reconnected.

::: dom/media/MediaDecoderStateMachine.h
@@ +148,5 @@
>      DECODER_STATE_ERROR
>    };
>  
>    void AddOutputStream(ProcessedMediaStream* aStream, bool aFinishWhenEnded);
> +  void RemoveOutputStream();

Please document with a comment.

Comment 56

[Paul Adenot (:padenot)]

Attachment: Part 7 - Connect HTMLMediaElement and AudioContext to the capture stream when capturing is needed. r=

Comment 57

[Paul Adenot (:padenot)]

Attachment: Part 6 - Allow to un-capture an HTMLMediaElement. r=

This hopefully addresses the comments. Clearing the stream in the
HTMLMediaElement happens in an other patch.

Comment 58

[:Gijs (he/him)]

Comment on attachment 8632150 [details] [diff] [review]
Part 10 - Implement the minimum frontend to play with the feature

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

Looks good besides some nits below. Would have been r+ but for the webrtc-selectAudioCapture document.getElementById (which will be null, which will cause the statement to throw...). Not sure if the patch is just missing hunks or if that's just a change that needs to be reverted.

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +546,5 @@
>  identity.loggedIn.signOut.accessKey = O
>  
>  # LOCALIZATION NOTE (getUserMedia.shareCamera.message, getUserMedia.shareMicrophone.message,
>  #                    getUserMedia.shareScreen.message, getUserMedia.shareCameraAndMicrophone.message,
> +#                    getUserMedia.shareScreenAndMicrophone.message, getUserMedia.shareAudioCapture.message):

Nit: add the other two messages as well.

@@ +607,5 @@
>  getUserMedia.sharingMenu.label = Tabs sharing devices
>  getUserMedia.sharingMenu.accesskey = d
>  # LOCALIZATION NOTE (getUserMedia.sharingMenuCamera
>  #                    getUserMedia.sharingMenuMicrophone,
> +#                    getUserMedia.sharingMenuAudioCapture,

Nit: same here.

::: browser/modules/ContentWebRTC.jsm
@@ +94,5 @@
>    for (let device of aDevices) {
>      device = device.QueryInterface(Ci.nsIMediaDevice);
>      switch (device.type) {
>        case "audio":
> +        if (audio && (device.mediaSource == "microphone") != sharingAudio) {

My brain is short-circuiting trying to figure out what this if condition is meant to be checking, because sharingAudio also includes audio.mediaSource != "microphone", but audio != device, so audio.mediaSource and device.mediaSource are not (necessarily) the same (I think?). Then there's the "audio" vs. "microphone" overloading of terminology... Maybe the sharingAudio variable should be called "sharingPageAudio" or something to that effect?

Can you clarify (in a comment) or rewrite? I *think* what you're trying to do is check that either the device in question isn't a microphone and we're sharing (non-microphone) audio, OR the device is a microphone and we're sharing microphone audio.

I take it we can't be sharing both mic and page audio?

::: browser/modules/webrtcUI.jsm
@@ +113,5 @@
>  
>      // If we are also requesting audio in addition to screen sharing,
>      // always use a generic label.
> +    if (!document.getElementById("webRTC-selectMicrophone").hidden ||
> +        !document.getElementById("webrtc-selectAudioCapture").hidden)

Err... your patch is missing this? That is, where is this element?

@@ +248,2 @@
>      // Don't show the 'Always' action if the connection isn't secure, or for
> +    // screen sharing/audio (because we can't guess which window the user wants to

Nit: screen/audio sharing

Comment 59

[Andreas Pehrson [:pehrsons]]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #55)
> Comment on attachment 8632152 [details] [diff] [review]
> Part 6 - Allow to un-capture an HTMLMediaElement. r=
> 
> Review of attachment 8632152 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaDecoderStateMachine.cpp
> @@ +3163,5 @@
> > +      self->StartAudioThread();
> > +      self->ScheduleStateMachine();
> > +      nsCOMPtr<nsIRunnable> cleanupRunnable =
> > +        NS_NewRunnableFunction([self] () -> void {
> > +          self->mDecodedStream->DestroyData();
> 
> When uncapturing we should also clear mOutputStreams (and perhaps destroy
> their ports?), otherwise what would happen if you do:
> > AddOutputStream(stream1)
> > RemoveOutputStream()
> > AddOutputStream(stream2)
> 
> I think an assertion will fail (mOutputStreams not empty when creating
> DecodedStreamData the first time), and stream1 will be reconnected.

Can you clarify on this Paul?

And if it cannot happen, please put in place the proper comments/asserts.

Comment 60

[Paul Adenot (:padenot)]

(In reply to :Gijs Kruitbosch from comment #58)
> ::: browser/modules/ContentWebRTC.jsm
> @@ +94,5 @@
> >    for (let device of aDevices) {
> >      device = device.QueryInterface(Ci.nsIMediaDevice);
> >      switch (device.type) {
> >        case "audio":
> > +        if (audio && (device.mediaSource == "microphone") != sharingAudio) {
> 
> My brain is short-circuiting trying to figure out what this if condition is
> meant to be checking, because sharingAudio also includes audio.mediaSource
> != "microphone", but audio != device, so audio.mediaSource and
> device.mediaSource are not (necessarily) the same (I think?). Then there's
> the "audio" vs. "microphone" overloading of terminology... Maybe the
> sharingAudio variable should be called "sharingPageAudio" or something to
> that effect?
> 
> Can you clarify (in a comment) or rewrite? I *think* what you're trying to
> do is check that either the device in question isn't a microphone and we're
> sharing (non-microphone) audio, OR the device is a microphone and we're
> sharing microphone audio.
> 
> I take it we can't be sharing both mic and page audio?

Indeed, you cannot do that in a single getUserMedia call, it has to be two separate calls, on with audio capture, one with a microphone.

> ::: browser/modules/webrtcUI.jsm
> @@ +113,5 @@
> >  
> >      // If we are also requesting audio in addition to screen sharing,
> >      // always use a generic label.
> > +    if (!document.getElementById("webRTC-selectMicrophone").hidden ||
> > +        !document.getElementById("webrtc-selectAudioCapture").hidden)
> 
> Err... your patch is missing this? That is, where is this element?

Yeah this was old cruft I forgot to remove from a very early version of this.

Comment 61

[Paul Adenot (:padenot)]

Attachment: Part 10 - Implement the minimum frontend to play with the feature

Comment 62

[:Gijs (he/him)]

Comment on attachment 8632814 [details] [diff] [review]
Part 10 - Implement the minimum frontend to play with the feature

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

What happens with the indicator icons in this case? (both the ones in the URL bar, and the global mac/non-mac indicators)

Comment 63

[Florian Quèze [:florian]]

Comment on attachment 8632150 [details] [diff] [review]
Part 10 - Implement the minimum frontend to play with the feature

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

::: browser/modules/ContentWebRTC.jsm
@@ +94,5 @@
>    for (let device of aDevices) {
>      device = device.QueryInterface(Ci.nsIMediaDevice);
>      switch (device.type) {
>        case "audio":
> +        if (audio && (device.mediaSource == "microphone") != sharingAudio) {

The same test in the video case has a comment explaining what the test does.

Comment 64

[:Gijs (he/him)]

Comment on attachment 8632814 [details] [diff] [review]
Part 10 - Implement the minimum frontend to play with the feature

r+ with a followup for addressing the indicator icons before preffing this on.

Comment 65

[Andrea Marchesini [:baku]]

Comment on attachment 8631683 [details] [diff] [review]
Part 1 - Allow to capture all HTMLMediaElements and AudioContexts for a document. r=

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

I rewrote this patch. I'm going to submit a new version in a couple of minutes.

::: dom/audiochannel/AudioChannelService.h
@@ +217,5 @@
>    RefreshAgentsVolumeEnumerator(AudioChannelAgent* aAgent,
>                                  AudioChannelAgentData* aUnused,
>                                  void *aPtr);
> +  static PLDHashOperator
> +  RefreshAgentsCaptureEnumerator(AudioChannelAgent* aAgent,

This is changed.

::: dom/audiochannel/nsIAudioChannelAgent.idl
@@ +27,5 @@
> +
> +  /**
> +   * Notified when the capture state is changed.
> +   */
> +  void windowAudioCaptureChanged();

change the UUID of this idl.

@@ +146,5 @@
> +
> +  /**
> +   * Retrieve whether the audio from this tab is captured to a MediaStream.
> +   */
> +  readonly attribute bool windowAudioCaptured;

and this idl too.

::: dom/audiochannel/tests/TestAudioChannelService.cpp
@@ +116,5 @@
>    {
>      return NS_OK;
>    }
>  
> +  NS_IMETHODIMP WindowAudioCaptureChanged() override

this file is gone.

::: dom/base/nsGlobalWindow.cpp
@@ +3772,5 @@
>  }
>  
> +bool
> +nsPIDOMWindow::GetAudioCaptured() const
> +{

FORWARD_TO_INNER(GetAudioCapture, (), false);
return mAudioCaptured;

@@ +3783,5 @@
> +
> +nsresult
> +nsPIDOMWindow::SetAudioCapture(bool aCapture)
> +{
> +  if (!IsInnerWindow()) {

FORWARD_TO_INNER(SetAudioCapture, (aCapture), NS_ERROR_FAILURE);

mAudioCaptured = aCapture;
...

::: dom/html/HTMLMediaElement.cpp
@@ +4504,5 @@
>        }
>        mAudioChannelAgent->SetVisibilityState(!OwnerDoc()->Hidden());
>      }
>  
> +    // Immediatly check if this should go to the MSG instead of the normal media

Immediately

Comment 66

[Andrea Marchesini [:baku]]

Attachment: Part 1 - Allow to capture all HTMLMediaElements and AudioContexts for a document.

Comment 67

[Andrea Marchesini [:baku]]

Attachment: Part 1 - Allow to capture all HTMLMediaElements and AudioContexts for a document.

We recently moved the audio values in the outerwindow. I did the same here.

Comment 68

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8632787 [details] [diff] [review]
Part 6 - Allow to un-capture an HTMLMediaElement. r=

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +3156,5 @@
> +      // Start again the audio sink
> +      self->mStreamStartTime = 0;
> +      self->mAudioEndTime = -1;
> +      self->mAudioCaptured = false;
> +      self->MaybeStartPlayback();

It is wrong to call MaybeStartPlayback() here which returns quickly when IsPlaying() is true. So AudioSink will not be created correctly if IsPlaying() is already true.

You should say:

if (self->IsPlaying()) {
  self->StartAudioThread();
}

Comment 69

[Andreas Pehrson [:pehrsons]]

I'd still like to hear on comment 59.

Comment 70

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8631690 [details] [diff] [review]
Part 3 - Implement AudioCaptureStream. r=

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

::: dom/media/AudioCaptureStream.h
@@ +21,5 @@
> +class AudioCaptureStream : public ProcessedMediaStream,
> +                           public MixerCallbackReceiver
> +{
> +public:
> +  enum { AUDIO_TRACK = 1 };

I'd really like to see this declared in the cpp file instead, so no one else ends up relying on it in the future.

Comment 71

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #70)
> > +class AudioCaptureStream : public ProcessedMediaStream,
> > +                           public MixerCallbackReceiver
> > +{
> > +public:
> > +  enum { AUDIO_TRACK = 1 };
> 
> I'd really like to see this declared in the cpp file instead, so no one else
> ends up relying on it in the future.

(Drive-by comment) How about private: ? The problem with file scope is that our unified-build approach (which speeds up full builds) essentially collapses all source files in the same module, so file scope becomes module scope. This has driven static's and other stuff into classes.

Comment 72

[Paul Adenot (:padenot)]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #71)
> (In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #70)
> > > +class AudioCaptureStream : public ProcessedMediaStream,
> > > +                           public MixerCallbackReceiver
> > > +{
> > > +public:
> > > +  enum { AUDIO_TRACK = 1 };
> > 
> > I'd really like to see this declared in the cpp file instead, so no one else
> > ends up relying on it in the future.
> 
> (Drive-by comment) How about private: ? The problem with file scope is that
> our unified-build approach (which speeds up full builds) essentially
> collapses all source files in the same module, so file scope becomes module
> scope. This has driven static's and other stuff into classes.

Yes, private sounds better for us, considering our strange way of building C++.

Comment 73

[Andrea Marchesini [:baku]]

Attachment: Part 1 - Allow to capture all HTMLMediaElements and AudioContexts for a document.

Comment 74

[Andrea Marchesini [:baku]]

Attachment: Part 1 - Allow to capture all HTMLMediaElements and AudioContexts for a document.

Sorry for the spam, I merged the previous patch wrongly.

Comment 75

[Paul Adenot (:padenot)]

Attachment: Part 6 - Allow to un-capture an HTMLMediaElement. r=

I think it's better to not remove all streams in case the HTMLMediaElement was
already captured by the page.

This also addresses jwwang's comments.

Comment 76

[Andrea Marchesini [:baku]]

Attachment: Part 1 - Allow to capture all HTMLMediaElements and AudioContexts for a document.

Comment 77

[Paul Adenot (:padenot)]

Comment on attachment 8635229 [details] [diff] [review]
Part 6 - Allow to un-capture an HTMLMediaElement. r=

(pehrsons is on PTO next week).

Comment 78

[Andrea Marchesini [:baku]]

Attachment: Part 1 - Allow to capture all HTMLMediaElements and AudioContexts for a document.

Comment 79

[Paul Adenot (:padenot)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ee4379e5ccc

Comment 80

[Maire Reavy [:mreavy]]

FYI, looks like there's still an issue with one of the Audio Channel tests with these patches

Comment 81

[Andrea Marchesini [:baku]]

Rebase all these patches on top of m-i. I recently landed fixes for those failures.

Comment 82

[Maire Reavy [:mreavy]]

Thanks, Baku!  Here's a new (rebased) try with a one-liner change that will probably fix the compile failure on Android:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=d54ec0eaacef

Comment 83

[Maire Reavy [:mreavy]]

My try has a few rebase issues which I'm looking at.  Stay tuned for an updated try link.

Comment 84

[Maire Reavy [:mreavy]]

FYI: Aside from Android& B2G build issues (which need some work figuring out parameters for DownMix), there still appear to be problems to resolve after rebasing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f692c8ef5329

Comment 85

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8635229 [details] [diff] [review]
Part 6 - Allow to un-capture an HTMLMediaElement. r=

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

::: dom/media/MediaDecoder.h
@@ +397,5 @@
>    // The stream is initially blocked. The decoder is responsible for unblocking
>    // it while it is playing back.
>    virtual void AddOutputStream(ProcessedMediaStream* aStream, bool aFinishWhenEnded);
> +  // Remove an output stream added with AddOutputStream.
> +  virtual void RemoveOutputStream(ProcessedMediaStream* aStream);

ProcessedMediaStream* is too much information. Just MediaStream* will do.

::: dom/media/MediaDecoderStateMachine.cpp
@@ +3187,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  DECODER_LOG("RemoveOutputStream=%p!", aStream);
> +  mDecodedStream->Remove(aStream);
> +  if (mDecodedStream->HasConsumers()) {

Should it be |!mDecodedStream->HasConsumers()|?

Comment 86

[Paul Adenot (:padenot)]

New try with a bunch of fixes for all the above issues:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5fa7e7ed6395

Comment 87

[Paul Adenot (:padenot)]

Attachment: Part 16 - Null check the window, because it can be different during the window's shutdown

This applies on top of your patch.

Comment 88

[Paul Adenot (:padenot)]

Attachment: Part 14 - Make necessary adjustments for integer audio. r=

This is what we talked about, just some templating and use of AudioDataValue
instead of hardcoding floats. Of course, this is big because the template needs
to go in the header file...

Comment 89

[Andrea Marchesini [:baku]]

Comment on attachment 8637301 [details] [diff] [review]
Part 16 - Null check the window, because it can be different during the window's shutdown

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

If you write an extensive comment explaining why can happen, I'm ok for this.
Check also if we leak something and if we receive outer-window-destroyed correctly for that window.

Comment 90

[Maire Reavy [:mreavy]]

Paul -- Your latest try looks good except it's burning B2G emulator.  I suggest we turn off the test that's failing on B2G (since we're landing the feature pref'd off) and make it a blocker to pref'ing the feature on *on B2G*.  Also, I think we can pref the feature on on other platforms before we pref it on for B2G.  We'd just want a separate meta bug for B2G pref on.

Comment 91

[Maire Reavy [:mreavy]]

Paul and I talked on irc, and he pointed out that the feature (audio capture) works on B2G for him.  The test is failing due to slowness of the emulator.  So he's going to disable the test on B2G, land the code (pref'd off) and *not* make the test on B2G block pref'ing on the feature on B2G.  I'm just capturing our  discussion (and its outcome) in the bug for continuity/clarity.

Comment 92

[Paul Adenot (:padenot)]

Attachment: Part 5 - Add MediaEngineWebRTCAudioCaptureSource as a new audio source, and "audioCapture" as a new MediaSource

smaug, Peter, this is landing behind a pref (media.getusermedia.audioCapture)
that is not set to true for now, and the API might change, I just want to land
this so interested people can try it, and I avoid being bitrot.

Comment 93

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8638515 [details] [diff] [review]
Part 5 - Add MediaEngineWebRTCAudioCaptureSource as a new audio source, and "audioCapture" as a new MediaSource

(you might get review faster from baku)

Comment 94

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8638515 [details] [diff] [review]
Part 5 - Add MediaEngineWebRTCAudioCaptureSource as a new audio source, and "audioCapture" as a new MediaSource

Can you please add a link to the relevant spec to this file?

Also, the name of this IDL file is ... a bit generic given what it's for.  Please file a followup to rename it to something that's clearer about what it's about, since this is not named after any particular interface?

r=me

Comment 95

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/05acb71cf981
https://hg.mozilla.org/integration/mozilla-inbound/rev/13730e7c5997
https://hg.mozilla.org/integration/mozilla-inbound/rev/8210276a98ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/af1638279785
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a60ff1149a1
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8502deeed33
https://hg.mozilla.org/integration/mozilla-inbound/rev/66479dd9eed9
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbf59fbb5875
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c98d7beaea7
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c588a5eaaec
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b1e6069b598
https://hg.mozilla.org/integration/mozilla-inbound/rev/03598139f39a
https://hg.mozilla.org/integration/mozilla-inbound/rev/306d02e17081
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ddbf85a42c0

Comment 96

[Carsten Book [:Tomcat]]

sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=12095095&repo=mozilla-inbound

Comment 97

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1829935d87e

Comment 98

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9819fa56caa1
https://hg.mozilla.org/integration/mozilla-inbound/rev/d477cfba6e1d
https://hg.mozilla.org/integration/mozilla-inbound/rev/28c03c98cb2b
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f565685e20a
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ab4f16eaf0a
https://hg.mozilla.org/integration/mozilla-inbound/rev/65bbfc1546af
https://hg.mozilla.org/integration/mozilla-inbound/rev/99ef8e436a7f
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2f0062a1f67
https://hg.mozilla.org/integration/mozilla-inbound/rev/12805598e6fa
https://hg.mozilla.org/integration/mozilla-inbound/rev/4824d9874663
https://hg.mozilla.org/integration/mozilla-inbound/rev/abd4e47887f7
https://hg.mozilla.org/integration/mozilla-inbound/rev/2dd83ac00bf9
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f5bec4c05ba
https://hg.mozilla.org/integration/mozilla-inbound/rev/deec8eb18346

Comment 99

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

And back out for b2g emulator m6 failures https://treeherder.mozilla.org/logviewer.html#?job_id=12097860&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a28170cf9a2

Comment 100

[Paul Adenot (:padenot)]

Grrr, forgot to disable the test.

Comment 101

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/90696b162030
https://hg.mozilla.org/integration/mozilla-inbound/rev/778457f7bae7
https://hg.mozilla.org/integration/mozilla-inbound/rev/865d6aa9cc67
https://hg.mozilla.org/integration/mozilla-inbound/rev/244d8d88808e
https://hg.mozilla.org/integration/mozilla-inbound/rev/2964352ce228
https://hg.mozilla.org/integration/mozilla-inbound/rev/946e320a1776
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8324bb0ab33
https://hg.mozilla.org/integration/mozilla-inbound/rev/864fff8bbf2a
https://hg.mozilla.org/integration/mozilla-inbound/rev/235175205cde
https://hg.mozilla.org/integration/mozilla-inbound/rev/092dcf4d57f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/a34909cbfb37
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c62be998cb4
https://hg.mozilla.org/integration/mozilla-inbound/rev/4351b1e432dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d3b6dcae7fe
https://hg.mozilla.org/integration/mozilla-inbound/rev/8541ce2730c0

Comment 102

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/90696b162030
https://hg.mozilla.org/mozilla-central/rev/778457f7bae7
https://hg.mozilla.org/mozilla-central/rev/865d6aa9cc67
https://hg.mozilla.org/mozilla-central/rev/244d8d88808e
https://hg.mozilla.org/mozilla-central/rev/2964352ce228
https://hg.mozilla.org/mozilla-central/rev/946e320a1776
https://hg.mozilla.org/mozilla-central/rev/c8324bb0ab33
https://hg.mozilla.org/mozilla-central/rev/864fff8bbf2a
https://hg.mozilla.org/mozilla-central/rev/235175205cde
https://hg.mozilla.org/mozilla-central/rev/092dcf4d57f2
https://hg.mozilla.org/mozilla-central/rev/a34909cbfb37
https://hg.mozilla.org/mozilla-central/rev/4c62be998cb4
https://hg.mozilla.org/mozilla-central/rev/4351b1e432dd
https://hg.mozilla.org/mozilla-central/rev/7d3b6dcae7fe
https://hg.mozilla.org/mozilla-central/rev/8541ce2730c0

Comment 103

[gtk2k]

Can I get both screen capture video and audio by just executing gUM() once?

audio only constraints success.
navigator.mediaDevices.getUserMedia({
  audio: {
    mediaSource: 'audioCapture'
  }
}).then(stream => /*...*/);

but, video and audio constraints 'NotAllowedError'
navigator.mediaDevices.getUserMedia({
  video: {
    mediaSource: 'screen'
  },
  audio: {
    mediaSource: 'audioCapture'
  }
}).then(stream => /*...*/);


One more question.
mediaSource: 'audioCapture' is tab only.
Can I capture system audio like Chrome?

Comment 104

[Paul Adenot (:padenot)]

Our implementation is a prototype. When/if we decide to work on the Web Extension we'll finish it.