Closed Bug 1037997 Opened 10 years ago Closed 9 years ago

Support multiple monitors for getUserMedia

Categories

(Core :: WebRTC, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed
relnote-firefox --- 43+

People

(Reporter: m_and_m, Assigned: ffcontribute)

References

Details

Attachments

(1 file, 4 obsolete files)

Support the ability to choose which monitor to stream.
Depends on: 983504
+1. Agreed, support for multiple monitors is necessary. The application should be able to offer the user a list of attached "Screens" in addition to "Entire Screen."

One my system with three monitors connected that would be:

- Screen 1
- Screen 2
- Screen 3
- Entire Screen
backlog: --- → webRTC+
Rank: 25
Priority: -- → P2
Pavlo (from Clearslide) is planning to fix this bug.  Thank you, Pavlo!
Assignee: nobody → ffcontribute
@Reavy this bug is in P2, can you please tell us the tentative time frame for this to be work in DeveloperEdition/Nightly.
Flags: needinfo?(mreavy)
Comment on attachment 8634709 [details] [diff] [review]
Bug 1037997 - Added possibility to choose monitors during screen capturing on Windows (UI options are: Entire Screen, Screen 1, Screen 2, ...)

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

::: browser/modules/webrtcUI.jsm
@@ +348,4 @@
>          for (let i = 0; i < devices.length; ++i) {
>            let name;
>            // Screen has a special treatment because we currently only support
>            // sharing the primary screen and want to display a localized string.

this comment will need to change

::: media/webrtc/trunk/webrtc/modules/desktop_capture/win/desktop_device_info_win.cc
@@ +57,5 @@
> +    char idStr[64];
> +    _snprintf_s(idStr, sizeof(idStr), sizeof(idStr) - 1, "%ld", pDesktopDeviceInfo->getScreenId());
> +    pDesktopDeviceInfo->setUniqueIdName(idStr);
> +    desktop_display_list_[pDesktopDeviceInfo->getScreenId()] = pDesktopDeviceInfo;
> +  }

if the part is the same as the part above the #else, move the ifdef to inside the function (i.e. the part above is invariant, the part below is in an ifdef).  Try to avoid duplicate code where reasonable - and at the minimum this function now always exists

@@ +61,5 @@
> +  }
> +
> +  ScreenCapturer::ScreenList screens;
> +  GetScreenList(&screens);
> +  uint32_t num_of_screens = screens.size();

auto, or ScreenCapturer::ScreenList::size_type (or typedef something to that)

@@ +66,5 @@
> +  if (1 >= num_of_screens) {
> +    return;
> +  }
> +
> +  for (uint32_t i = 0; i < num_of_screens; ++i) {

and here - or for c++11 points, decltype(num_of_screens) i = 0

@@ +71,5 @@
> +    DesktopDisplayDevice *pDesktopDeviceInfo = new DesktopDisplayDevice;
> +    if (pDesktopDeviceInfo) {
> +      pDesktopDeviceInfo->setScreenId(screens[i].id);
> +      char nameStr[64];
> +      _snprintf_s(nameStr, sizeof(nameStr), sizeof(nameStr) - 1, "Screen %ld", i + 1);

default size types are unsigned, barring overrides.  'l' may not always be right as well.
Cast i+1 to unsigned int, and use %u.

@@ +75,5 @@
> +      _snprintf_s(nameStr, sizeof(nameStr), sizeof(nameStr) - 1, "Screen %ld", i + 1);
> +      pDesktopDeviceInfo->setDeviceName(nameStr);
> +
> +      char idStr[64];
> +      _snprintf_s(idStr, sizeof(idStr), sizeof(idStr) - 1, "%ld", pDesktopDeviceInfo->getScreenId());

Similar: getScreenId() -> ScreenId -> intptr_t (an int big enough to hold a ptr).
It could even map to long long.  Likely it's really a small unsigned int, but the types don't guarantee it. (Note this problem exists elsewhere in this file/code)
To be safe, cast to match the %-format - (long), or %lld and (long long), or to be absolutely correct use "%" PRIdPTR, ...->getScreenId()  (PRIdPTR comes from inttypes.h)
Attachment #8634709 - Flags: feedback?(rjesup) → feedback+
(In reply to AjayG from comment #4)
> @Reavy this bug is in P2, can you please tell us the tentative time frame
> for this to be work in DeveloperEdition/Nightly.

Conservatively I expect this to land in Fx44 or before.  The Fx44 Nightly cycle starts Sept 21st and uplifts to DevEdition on Nov 2nd.  However, as you can see, this has an assigned owner who is already making good progress (though he is a new to Firefox);  so it may land well before Sept 21st.  Stay tuned to this bug for updates.  We'll land it as soon as we have r+'d patches.
Flags: needinfo?(mreavy)
In chrome i was able to see all the screen/window images in the pop-up screen selector, 
How firefox is going to provide the screen details in select bar, i think it is bit difficult to find appropriate screen number
Flags: needinfo?(mreavy)
(In reply to AjayG from comment #8)
> In chrome i was able to see all the screen/window images in the pop-up screen selector, 
> How firefox is going to provide the screen details in select bar, i think it is bit difficult to find appropriate screen number

Here's the mock up I have for screensharing: https://bug1058775.bugzilla.mozilla.org/attachment.cgi?id=8485640.  Our UI team is implementing improvements to the UI (based on this mock up) over time.  This bug is about the platform support for multiple monitors.
Flags: needinfo?(mreavy)
@Reavy, Thanks for the provided cgi, its awesome.
I was eagerly waiting for this feature.
Implemented behavior are:
  Windows:
    One Monitor:
      Popup shows options "No Screen" and "Entire screen" (no change)
    Two or more Monitors:
      Extend mode: Popup shows options "No Screen", "Entire screen" (combined picture from all monitors), Screen 1, Screen 2, ...
      Duplicate mode: Popup shows options "No Screen", "Entire screen" (picture from one monitor)
      Show desktop only on N monitor: Popup shows options "No Screen", "Entire screen" (picture from chosen monitor)
    
  Mac:
    One Monitor:
      Popup shows options "No Screen" and "Entire screen" (no change)
    Two or more Monitors:
      Extend mode: Popup shows options "No Screen", Screen 1, Screen 2, ...
      Mirror mode: Popup shows options "No Screen", "Entire screen" (picture from one monitor)
    
  Linux:
    One Monitor:
      Popup shows options "No Screen" and "Entire screen" (no change)
    Two or more Monitors:
      Extend mode: Popup shows options "No Screen", "Entire screen" (combined picture from all monitors)
      Mirror mode: Popup shows options "No Screen", "Entire screen" (picture from one monitor)

So on Mac we have no combined monitor and on Linux all screens combined to the large screen
Attachment #8638012 - Flags: review?(rjesup)
Comment on attachment 8638012 [details] [diff] [review]
Bug 1037997 - Added possibility to choose monitors during screen capturing

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

::: browser/modules/webrtcUI.jsm
@@ +351,2 @@
>            if (type == "screen") {
> +            if (devices[i].name == "Primary Monitor") {

Looks good, but we'll want a Firefox peer (:florian) to review this file

::: media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_capture.gypi
@@ -7,5 @@
>  # be found in the AUTHORS file in the root of the source tree.
>  
>  {
>    'variables': {
> -    'multi_monitor_screenshare%' : 0,

Lets set this in build/gyp.mozbuild to override the default of 0 here

::: media/webrtc/trunk/webrtc/modules/desktop_capture/mac/desktop_device_info_mac.mm
@@ +45,3 @@
>    }
> +#else
> +  const UInt32 kMaxScreens = 256;

Probably overkill... Though if CGDirectDisplayID isn't large, no problem.  Better too large than not large enough

@@ +61,5 @@
> +        desktop_device_info->setDeviceName(nameStr);
> +      }
> +
> +      char idStr[64];
> +      snprintf(idStr, sizeof(idStr), "%" PRIdPTR, desktop_device_info->getScreenId());

snprintf is safe because this file isn't for windows; good
Attachment #8638012 - Flags: review?(rjesup)
Attachment #8638012 - Flags: review?(florian)
Attachment #8638012 - Flags: review+
Comment on attachment 8638012 [details] [diff] [review]
Bug 1037997 - Added possibility to choose monitors during screen capturing

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

Seems reasonable, thanks for improving this! :)

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +566,5 @@
>  getUserMedia.noScreen.label = No Screen
>  getUserMedia.noWindow.label = No Window
>  getUserMedia.noAudio.label = No Audio
>  getUserMedia.shareEntireScreen.label = Entire screen
> +getUserMedia.shareMonitor.label = Screen %S

A localization note above this new string is needed to explain what the %S will be replaced with. See other localization notes in the file for examples of the format to use for them.

::: browser/modules/webrtcUI.jsm
@@ +348,3 @@
>          for (let i = 0; i < devices.length; ++i) {
>            let name;
> +          // Screen list build from Primary Monitor (all monitor screens as one) and separate monitor screens if present.

I don't really understand this sentence, is there a typo in it? Also, please wrap it so that the line length is roughly 80 columns.

@@ +352,5 @@
> +            if (devices[i].name == "Primary Monitor") {
> +              name = stringBundle.getString("getUserMedia.shareEntireScreen.label");
> +            } else {
> +              name = stringBundle.getFormattedString("getUserMedia.shareMonitor.label", [monitorIndex]);
> +              ++monitorIndex;

I don't know how users are expected to figure out what "Screen 1" is vs "Screen 2". I know we have nice designs to highlight the screen when it's being selected, or show a preview before we start sharing, but I don't expect this to come in the near future. I'm wondering if there would be an easy way to give some slightly more useful indication to the user in the short term. How difficult would it be for example to display something like "Screen 1 (1920x1080)"? Or the name (eg. brand & model) of the screen? (If it's not easy, let's not block on this.)
Attachment #8638012 - Flags: review?(florian) → feedback+
Thank's for reviews. I applied all the comments.

And have additional idea about UI improvement, maybe better to remove 'Entire screen' option in case user have several monitors on Windows? ('Entire screen' option shows all combined monitors as one screen.) and leave only 'Screen 1', 'Screen 2', etc.
Agreed; "Entire screen" is confusing to users if the user has more than one screen.  Probably it should say "Entire screen" if you have only one, and "Screen 1" etc if you have more than one.
So now screen selection looks the same on Windows and Mac. 'Entire screen' option present if only one monitor present. In case of multiple monitors 'Screen 1', 'Screen 2', etc. present in dropdown selection.
Attachment #8649932 - Flags: review?(mdeboer)
Attachment #8649932 - Flags: review?(MattN+bmo)
Comment on attachment 8649932 [details] [diff] [review]
Bug 1037997 - In addition remove 'Entire screen' optio' for Windows

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

Apologies, but I don't have enough context to be able to review this work. I'd like to defer this to :flo.
Attachment #8649932 - Flags: review?(mdeboer) → review?(florian)
Hi, what version you expect it'll be integrated (not Nightly)? Thanks.
Comment on attachment 8649932 [details] [diff] [review]
Bug 1037997 - In addition remove 'Entire screen' optio' for Windows

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

Looks good for the browser/ part, thanks! It looks like the rest of the code has some changes since the previous patch that Randell r+'ed, so I'm re-requesting review from him. Note: this patch seems to introduce some trailing whitespace.
Attachment #8649932 - Flags: review?(rjesup)
Attachment #8649932 - Flags: review?(florian)
Attachment #8649932 - Flags: review?(MattN+bmo)
Attachment #8649932 - Flags: review+
If more than one screen is present, then we need a way to identify the shared screen from gUM stream/videoTrack.
We need display name or any other properties which matches with NSScreen(in MAC)/getMonitorInfo(in Windows).
In MAC screenid is always unique for the Screen.

And we need the selected Screen resolution to set maxHeight & maxWidth in getUserMedia constraints.
We are able to get the current screen width/height from window.width/height, but there is a possibility to share other screens.
Flags: needinfo?(rjesup)
Flags: needinfo?(mreavy)
jib - any thoughts about comment 20?
Flags: needinfo?(mreavy) → needinfo?(jib)
(In reply to AjayG from comment #20)
> If more than one screen is present, then we need a way to identify the
> shared screen from gUM stream/videoTrack.

The good news for screensharing is that "Each potential source of capture is treated .. as a discrete media source" [1] aka device, which lets us put some screen-identifying information in the device label, which content can access through track.label [2].

The bad news is we haven't implemented track.label [3] yet (Bug 910249), but it shouldn't be a big deal if we want to break it out into a smaller bug.

> And we need the selected Screen resolution to set maxHeight & maxWidth in
> getUserMedia constraints.

I'm not following. Why not leave it unconstrained and read out element.videoWidth and element.videoHeight? See [3].

Note that maxWidth and maxHeight are non-standard and wont work in Firefox. See [4].

> We are able to get the current screen width/height from window.width/height,
> but there is a possibility to share other screens.

Sorry, I don't think I understand your use case.

[1] http://www.w3.org/TR/screen-capture/#device-identifiers
[2] http://w3c.github.io/mediacapture-main/getusermedia.html#widl-MediaStreamTrack-label
[3] http://jsfiddle.net/9t3szw0L
[4] http://stackoverflow.com/questions/28282385/webrtc-firefox-constraints/28911694#28911694
Flags: needinfo?(jib)
(In reply to Jan-Ivar Bruaroey [:jib] from comment #22)
> (In reply to AjayG from comment #20)
> > And we need the selected Screen resolution to set maxHeight & maxWidth in
> > getUserMedia constraints.
> 
> I'm not following. Why not leave it unconstrained and read out
> element.videoWidth and element.videoHeight? See [3].
> 

Okay i will leave width & height as unconstrained.

> > We are able to get the current screen width/height from window.width/height,
> > but there is a possibility to share other screens.
> 
> Sorry, I don't think I understand your use case.
> 

I was planning to implementing screen-control feature with my App, with single screen it is working fine. If there are more than one screen i was getting conflict.
see: https://groups.google.com/forum/#!topic/discuss-webrtc/7OeXjU2brG8
Flags: needinfo?(jib)
Comment on attachment 8649932 [details] [diff] [review]
Bug 1037997 - In addition remove 'Entire screen' optio' for Windows

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

Just correct the trailing spaces and we're good

::: build/gyp.mozbuild
@@ +44,5 @@
>  
>      'moz_widget_toolkit_gonk': 0,
>      'moz_webrtc_omx': 0,
>      'moz_webrtc_mediacodec': 0,
> +    

get rid of trailing spaces

::: media/webrtc/trunk/webrtc/modules/desktop_capture/mac/desktop_device_info_mac.mm
@@ +37,5 @@
> +  DesktopDisplayDevice* desktop_device_info = new DesktopDisplayDevice;
> +  if (desktop_device_info) {
> +    desktop_device_info->setScreenId(CGMainDisplayID());
> +    desktop_device_info->setDeviceName("Primary Monitor");
> + 

trailing space

::: media/webrtc/trunk/webrtc/modules/desktop_capture/win/desktop_device_info_win.cc
@@ +39,5 @@
> +  DesktopDisplayDevice* desktop_device_info = new DesktopDisplayDevice;
> +  if (desktop_device_info) {
> +    desktop_device_info->setScreenId(webrtc::kFullDesktopScreenId);
> +    desktop_device_info->setDeviceName("Primary Monitor");
> + 

trailing space

::: media/webrtc/trunk/webrtc/modules/desktop_capture/x11/desktop_device_info_x11.cc
@@ +36,5 @@
> +  DesktopDisplayDevice* desktop_device_info = new DesktopDisplayDevice;
> +  if (desktop_device_info) {
> +    desktop_device_info->setScreenId(webrtc::kFullDesktopScreenId);
> +    desktop_device_info->setDeviceName("Primary Monitor");
> + 

trailing space
Attachment #8649932 - Flags: review?(rjesup) → review+
(In reply to AjayG from comment #23)
> I was planning to implementing screen-control feature with my App, with
> single screen it is working fine. If there are more than one screen i was
> getting conflict.
> see: https://groups.google.com/forum/#!topic/discuss-webrtc/7OeXjU2brG8

If I understand the problem correctly, then I think waiting for track.label and making screen-sharing dimensions be part of the device description as I mention in comment 22 is the right approach.

If there are more specific questions I can help with please let me know.
Flags: needinfo?(jib)
Attachment #8634709 - Attachment is obsolete: true
Attachment #8638012 - Attachment is obsolete: true
Attachment #8649932 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/47a89d8276cb
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Release Note Request (optional, but appreciated)
[Why is this notable]: user facing support for a feature added
[Suggested wording]: Multiple monitors supported for WebRTC streaming 
[Links (documentation, blog post, etc)]:

If you have improvements for the release note wording, please comment and needinfo me. Thanks!
Did I understand this correctly? Users will have another option when streaming via webrtc app:

Previously
- browser tabs
- program windows
- entire screen

Now the list also includes an option to select a specific screen.
Yes. "entire screen" option is present if user have only one screen. In case of multi screen environment Screen 1, Screen 2,.. options present instead of "entire screen".
Flags: needinfo?(rjesup)

FYI
I created a new bug to support sharing a single screen in multiscreen setup on linux:
https://bugzilla.mozilla.org/show_bug.cgi?id=1552456

You need to log in before you can comment on or make changes to this bug.