1039529 - Add an API to know which kind of video streams a web page is using

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Florian Quèze [:florian]]

The current API http://mxr.mozilla.org/mozilla-central/source/dom/media/nsIMediaManager.idl#22 :
void mediaCaptureWindowState(in nsIDOMWindow aWindow, out boolean aVideo, out boolean aAudio);

is no longer enough now that we are adding support for screen/window sharing (bug 983504).

Comment 1

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

Attachment: Add screen and window sharing booleans to MediaCaptureWindowState

Comment 2

[Paul Kerr [:pkerr]]

Comment on attachment 8456936 [details] [diff] [review]
Add screen and window sharing booleans to MediaCaptureWindowState

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

::: content/media/webrtc/MediaEngineWebRTC.h
@@ +329,5 @@
>  
> +  virtual const dom::MediaSourceEnum GetMediaSource() {
> +    return dom::MediaSourceEnum::Microphone;
> +  }
> +

I am assuming, since I do not see a matching method on MediaEngineWebRTCVideoSource, that the audio source is always a dom::MediaSourceEnum::Microphone but that the GetMediaSource() for video is sorted out in the subclass.

Comment 3

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

Attachment: interdiffs for changes to avoid over-reuse of dom::MediaSourceEnum

Comment 4

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

Comment on attachment 8457736 [details] [diff] [review]
interdiffs for changes to avoid over-reuse of dom::MediaSourceEnum

The MediaSourceEnum dom type is for Video constraints, so adding microphone to it wouldn't be reasonable or match the proposal; use our own internal enum that's a superset of that.

Comment 5

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8457736 [details] [diff] [review]
interdiffs for changes to avoid over-reuse of dom::MediaSourceEnum

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

This reeks of "we're going to have to replace it all eventually".

::: content/media/webrtc/MediaEngine.h
@@ +44,5 @@
> +enum MediaSourceType {
> +  Camera = (int) dom::MediaSourceEnum::Camera,
> +  Screen = (int) dom::MediaSourceEnum::Screen,
> +  Application = (int) dom::MediaSourceEnum::Application,
> +  Window, // = (int) dom::MediaSourceEnum::Window,

Too subtle. Need at least a big XXX and a bug nr to remove the commented part, which I assume is the goal.

@@ +45,5 @@
> +  Camera = (int) dom::MediaSourceEnum::Camera,
> +  Screen = (int) dom::MediaSourceEnum::Screen,
> +  Application = (int) dom::MediaSourceEnum::Application,
> +  Window, // = (int) dom::MediaSourceEnum::Window,
> +  //Browser = (int) dom::MediaSourceEnum::Browser,

Same remark.

@@ +46,5 @@
> +  Screen = (int) dom::MediaSourceEnum::Screen,
> +  Application = (int) dom::MediaSourceEnum::Application,
> +  Window, // = (int) dom::MediaSourceEnum::Window,
> +  //Browser = (int) dom::MediaSourceEnum::Browser,
> +  Microphone

So dom::MediaSourceEnum is really dom::VideoSourceEnum?

Comment 6

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

(In reply to Gian-Carlo Pascutto [:gcp] from comment #5)

> This reeks of "we're going to have to replace it all eventually".

We'll see where the WG goes; I think having an internal type separate from theirs is good in any case.

> 
> ::: content/media/webrtc/MediaEngine.h
> @@ +44,5 @@
> > +enum MediaSourceType {
> > +  Camera = (int) dom::MediaSourceEnum::Camera,
> > +  Screen = (int) dom::MediaSourceEnum::Screen,
> > +  Application = (int) dom::MediaSourceEnum::Application,
> > +  Window, // = (int) dom::MediaSourceEnum::Window,
> 
> Too subtle. Need at least a big XXX and a bug nr to remove the commented
> part, which I assume is the goal.

Bug 1038926 (window-sharing bug)

> 
> @@ +45,5 @@
> > +  Camera = (int) dom::MediaSourceEnum::Camera,
> > +  Screen = (int) dom::MediaSourceEnum::Screen,
> > +  Application = (int) dom::MediaSourceEnum::Application,
> > +  Window, // = (int) dom::MediaSourceEnum::Window,
> > +  //Browser = (int) dom::MediaSourceEnum::Browser,
> 
> Same remark.
> 
> @@ +46,5 @@
> > +  Screen = (int) dom::MediaSourceEnum::Screen,
> > +  Application = (int) dom::MediaSourceEnum::Application,
> > +  Window, // = (int) dom::MediaSourceEnum::Window,
> > +  //Browser = (int) dom::MediaSourceEnum::Browser,

Browser is in the proposal today, but we don't support it and it's unclear if it's going to stay in the proposal or handle that a different way.  (I suspect Browser will go away)

> > +  Microphone
> 
> So dom::MediaSourceEnum is really dom::VideoSourceEnum?

In the Proposal: yes.

Comment 7

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

Attachment: Add screen and window sharing booleans to MediaCaptureWindowState

merged patch for checkin

Comment 8

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

Comment on attachment 8458013 [details] [diff] [review]
Add screen and window sharing booleans to MediaCaptureWindowState

carry forward r+=pkerr,gcp

Comment 9

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/d00f111241a8

In 34, we'll replace this with a list of what's being shared

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d00f111241a8

Comment 11

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

(In reply to Randell Jesup [:jesup] PTO until Jan 5 from comment #6)
> (In reply to Gian-Carlo Pascutto [:gcp] from comment #5)
> 
> > This reeks of "we're going to have to replace it all eventually".
> 
> We'll see where the WG goes; I think having an internal type separate from
> theirs is good in any case.

I ran across MediaSourceType in the code and traced it back to this bug which I missed when it went by, sorry.

Unless I hear objections, I'd like to merge this back to using the pure dom enums in Bug 1006707. That patch will make MediaSource a regular DOMString instead of a webidl enum - to match the spec - so adding "Microphone" will no longer have any behavioral impact.