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

RESOLVED FIXED in Firefox 42

Status

()

defect
P1
normal
Rank:
15
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: mreavy, Assigned: padenot)

Tracking

(Depends on 4 bugs)

unspecified
mozilla42
x86_64
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox42 fixed)

Details

(Whiteboard: [capture])

Attachments

(16 attachments, 22 obsolete attachments)

1.44 KB, patch
roc
: review+
Details | Diff | Splinter Review
934 bytes, patch
Details | Diff | Splinter Review
11.67 KB, patch
Details | Diff | Splinter Review
19.64 KB, patch
Details | Diff | Splinter Review
8.04 KB, patch
Details | Diff | Splinter Review
21.45 KB, patch
jesup
: review+
Details | Diff | Splinter Review
15.73 KB, patch
pehrsons
: review+
Details | Diff | Splinter Review
13.42 KB, patch
Details | Diff | Splinter Review
1.42 KB, patch
roc
: review+
Details | Diff | Splinter Review
9.07 KB, patch
Details | Diff | Splinter Review
19.38 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
7.62 KB, patch
jwwang
: review+
Details | Diff | Splinter Review
13.29 KB, patch
padenot
: review+
Details | Diff | Splinter Review
1.14 KB, patch
baku
: review+
Details | Diff | Splinter Review
16.75 KB, patch
jesup
: review+
Details | Diff | Splinter Review
21.54 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
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.
Assignee

Comment 1

4 years ago
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
Flags: needinfo?(mreavy)
Assignee

Comment 2

4 years ago
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.
Flags: needinfo?(rjesup)
(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>)
Flags: needinfo?(rjesup)
Paul -- yes, this sounds reasonable.  The primary usecase here is capturing audio along with video from a tab via getUserMedia.
Flags: needinfo?(mreavy)
(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.
Assignee

Updated

4 years ago
Depends on: 1166181

Updated

4 years ago
backlog: --- → webRTC+
Rank: 15
Priority: -- → P1
Whiteboard: [capture]
Assignee

Updated

4 years ago
Depends on: 1177399
Assignee

Updated

4 years ago
Assignee

Comment 6

4 years ago
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.
Attachment #8630025 - Flags: review?(amarchesini)
Assignee

Comment 7

4 years ago
There are now two different possible audio source, so this was getting confusing.
Attachment #8630026 - Flags: review?(rjesup)
Assignee

Comment 8

4 years ago
It is a ProcessMediaStream that simply mixes its inputs into a stereo stream,
up/down mixing appropriately.
Attachment #8630027 - Flags: review?(roc)
Assignee

Comment 10

4 years ago
This contains a lot of printfs, I'll make sure to remove them before landing.
Attachment #8630029 - Flags: review?(rjesup)
Assignee

Comment 11

4 years ago
Attachment #8630030 - Flags: review?(jwwang)
Assignee

Comment 14

4 years ago
This is probably gonna land via baku's patches.
Assignee

Comment 19

4 years ago
Attachment #8630038 - Flags: review?(pehrsons)
Assignee

Updated

4 years ago
Attachment #8630027 - Flags: review?(rjesup)
Assignee

Updated

4 years ago
Attachment #8630028 - Flags: review?(rjesup)
Assignee

Comment 20

4 years ago
Also, this is behind a pref: `media.getusermedia.audiocapture.enabled`.
Assignee

Comment 21

4 years ago
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?
Attachment #8630036 - Flags: review?(roc)
Assignee

Comment 22

4 years ago
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 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.
Attachment #8630036 - Flags: review?(martin.thomson) → review-
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 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
Attachment #8630035 - Flags: review?(jib) → review+
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
Attachment #8630026 - Flags: review?(rjesup) → review+
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
Attachment #8630038 - Flags: review?(pehrsons) → review-
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 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.
Attachment #8630030 - Flags: review?(jwwang) → review-
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.
Attachment #8630037 - Flags: review?(jwwang)
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?
Attachment #8630027 - Flags: review?(rjesup) → review+
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();
Attachment #8630028 - Flags: review?(rjesup) → review+
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.
Attachment #8630036 - Flags: review?(roc) → review-
Assignee

Comment 34

4 years ago
(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.
Assignee

Comment 35

4 years ago
(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 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
Attachment #8630029 - Flags: review?(rjesup) → review-
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.
Attachment #8630034 - Flags: feedback?(florian) → feedback+
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.
Assignee

Comment 39

4 years ago
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.
Attachment #8631683 - Flags: review?(amarchesini)
Assignee

Updated

4 years ago
Attachment #8630025 - Attachment is obsolete: true
Attachment #8630025 - Flags: review?(amarchesini)
Assignee

Comment 40

4 years ago
There are now two different possible audio source, so this was getting confusing.
Assignee

Updated

4 years ago
Attachment #8630026 - Attachment is obsolete: true
Assignee

Comment 41

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

Updated

4 years ago
Attachment #8630027 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8630028 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8630029 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8630031 - Attachment is obsolete: true
Assignee

Comment 45

4 years ago
That is indeed better. I'm not sure about the NO_FAKE_DEVICE thing, it's a bit
ugly.
Attachment #8631699 - Flags: review?(pehrsons)
Assignee

Updated

4 years ago
Attachment #8630038 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8630035 - Attachment is obsolete: true
Assignee

Comment 47

4 years ago
Yeah I was being stupid, this works just fine.
Attachment #8630036 - Attachment is obsolete: true
Attachment #8631703 - Flags: review?(martin.thomson)
Assignee

Updated

4 years ago
Attachment #8631703 - Attachment description: Bug 1156472 - Part 13 - Allow to pipe the AudioCaptureStream into an AudioContext. r= → Part 13 - Allow to pipe the AudioCaptureStream into an AudioContext. r=
Attachment #8631703 - Flags: review?(roc)
Attachment #8631703 - Flags: review?(martin.thomson) → review+
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).
Attachment #8631693 - Flags: review?(rjesup) → review+
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?
Attachment #8631699 - Flags: review?(pehrsons) → review+
Assignee

Comment 50

4 years ago
(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.
Assignee

Comment 51

4 years ago
Florian, do you think this is good enough? We'd like to land this preffed off,
and then we can do the polish.
Attachment #8632150 - Flags: review?(florian)
Assignee

Updated

4 years ago
Attachment #8630034 - Attachment is obsolete: true
Assignee

Comment 52

4 years ago
That should only take the monitor on the right threads.
Attachment #8632152 - Flags: review?(jwwang)
Assignee

Updated

4 years ago
Attachment #8630030 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8632152 - Flags: review?(pehrsons)
Assignee

Comment 53

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

(This got folded into part 6)
Attachment #8630037 - Attachment is obsolete: true
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.
Attachment #8632152 - Flags: review?(jwwang) → review-
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.
Attachment #8632152 - Flags: review?(pehrsons) → review-
Assignee

Updated

4 years ago
Attachment #8631695 - Attachment is obsolete: true
Assignee

Comment 57

4 years ago
This hopefully addresses the comments. Clearing the stream in the
HTMLMediaElement happens in an other patch.
Assignee

Updated

4 years ago
Attachment #8632152 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8632787 - Flags: review?(pehrsons)
Attachment #8632787 - Flags: review?(jwwang)

Comment 58

4 years ago
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
Attachment #8632150 - Flags: review?(florian) → feedback+
(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.
Flags: needinfo?(padenot)
Assignee

Comment 60

4 years ago
(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.
Assignee

Updated

4 years ago
Attachment #8632150 - Attachment is obsolete: true

Comment 62

4 years ago
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 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

4 years ago
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.
Attachment #8632814 - Flags: review?(gijskruitbosch+bugs) → review+
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
Attachment #8631683 - Flags: review?(amarchesini) → review-
Assignee

Updated

4 years ago
Attachment #8632878 - Attachment is patch: true
Flags: needinfo?(padenot)
We recently moved the audio values in the outerwindow. I did the same here.
Attachment #8632878 - Attachment is obsolete: true
Attachment #8632878 - Flags: review?(padenot)
Attachment #8632907 - Flags: review?(padenot)
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();
}
Attachment #8632787 - Flags: review?(jwwang) → review-
I'd still like to hear on comment 59.
Flags: needinfo?(padenot)
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.
Depends on: 1183510
(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.
Assignee

Comment 72

4 years ago
(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++.
Flags: needinfo?(padenot)
Attachment #8632907 - Attachment is obsolete: true
Attachment #8632907 - Flags: review?(padenot)
Attachment #8634808 - Flags: review?(padenot)
Sorry for the spam, I merged the previous patch wrongly.
Attachment #8634808 - Attachment is obsolete: true
Attachment #8634808 - Flags: review?(padenot)
Attachment #8634836 - Flags: review?(padenot)
Assignee

Comment 75

4 years ago
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.
Attachment #8635229 - Flags: review?(pehrsons)
Assignee

Updated

4 years ago
Attachment #8632787 - Attachment is obsolete: true
Attachment #8632787 - Flags: review?(pehrsons)
Attachment #8634836 - Attachment is obsolete: true
Attachment #8634836 - Flags: review?(padenot)
Attachment #8635280 - Flags: review?(padenot)
Attachment #8635280 - Attachment is patch: true
Assignee

Comment 77

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

(pehrsons is on PTO next week).
Attachment #8635229 - Flags: review?(pehrsons) → review?(jwwang)
Attachment #8635337 - Attachment is patch: true
FYI, looks like there's still an issue with one of the Audio Channel tests with these patches
Flags: needinfo?(padenot)
Flags: needinfo?(amarchesini)
Rebase all these patches on top of m-i. I recently landed fixes for those failures.
Flags: needinfo?(amarchesini)
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
Flags: needinfo?(padenot)
My try has a few rebase issues which I'm looking at.  Stay tuned for an updated try link.
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
Flags: needinfo?(padenot)
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()|?
Attachment #8635229 - Flags: review?(jwwang) → review+
Assignee

Comment 86

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5fa7e7ed6395
Flags: needinfo?(padenot)
Assignee

Comment 87

4 years ago
This applies on top of your patch.
Attachment #8637301 - Flags: review?(amarchesini)
Assignee

Comment 88

4 years ago
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...
Attachment #8637303 - Flags: review?(rjesup)
Attachment #8637303 - Flags: review?(rjesup) → review+
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.
Attachment #8637301 - Flags: review?(amarchesini) → review+
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.
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.
Assignee

Comment 92

4 years ago
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.
Attachment #8638515 - Flags: review?(bugs)
Assignee

Updated

4 years ago
Attachment #8638515 - Flags: review?(peterv)
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)
Attachment #8638515 - Flags: review?(bugs) → review?(amarchesini)
Assignee

Updated

4 years ago
Attachment #8638515 - Flags: review?(peterv)
Attachment #8638515 - Flags: review?(bzbarsky)
Attachment #8638515 - Flags: review?(amarchesini)
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
Attachment #8638515 - Flags: review?(bzbarsky) → review+
Assignee

Updated

4 years ago
Depends on: 901633
Assignee

Updated

4 years ago
No longer depends on: 901633
sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=12095095&repo=mozilla-inbound
Flags: needinfo?(padenot)
Assignee

Comment 100

4 years ago
Grrr, forgot to disable the test.
Flags: needinfo?(padenot)
Assignee

Updated

4 years ago
Attachment #8635337 - Flags: review?(padenot) → review+
Depends on: 1189171
Depends on: 1190101
Depends on: 1189982

Updated

4 years ago
Depends on: 1191298

Comment 103

Last year
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?
Assignee

Comment 104

Last year
Our implementation is a prototype. When/if we decide to work on the Web Extension we'll finish it.
You need to log in before you can comment on or make changes to this bug.