Closed Bug 1039529 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: WebRTC, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: florian, Assigned: jesup)

References

Details

Attachments

(1 file, 2 obsolete files)

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).
Summary: Add an API to know which kind video streams a web page is using → Add an API to know which kind of video streams a web page is using
Attachment #8456936 - Flags: review?(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.
Attachment #8456936 - Flags: review?(pkerr) → review+
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.
Attachment #8457736 - Flags: review?(gpascutto)
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?
Attachment #8457736 - Flags: review?(gpascutto) → review+
(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.
Attachment #8456936 - Attachment is obsolete: true
Comment on attachment 8458013 [details] [diff] [review]
Add screen and window sharing booleans to MediaCaptureWindowState

carry forward r+=pkerr,gcp
Attachment #8458013 - Flags: review+
Attachment #8457736 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/d00f111241a8

In 34, we'll replace this with a list of what's being shared
Target Milestone: --- → mozilla33
https://hg.mozilla.org/mozilla-central/rev/d00f111241a8
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
(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.
You need to log in before you can comment on or make changes to this bug.