1038926 - Add support for window sharing for getUserMedia

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Matthew Miller [:m_and_m]]

Support sharing a single window from the user's desktop screen.

Comment 1

[Matthew Miller [:m_and_m]]

Attachment: implement window sharing

Comment 2

[Matthew Miller [:m_and_m]]

Attachment: -help

Comment 3

[Matthew Miller [:m_and_m]]

Attachment: v2 implement window sharing

Comment 4

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

Comment on attachment 8457486 [details] [diff] [review]
v2 implement window sharing

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

still need gcp's review, as I wrote a bunch of this

::: dom/webidl/Constraints.webidl
@@ +17,5 @@
>  enum MediaSourceEnum {
>      "camera",
>      "screen",
> +    "application",
> +    "window"

Note that this will also be found in my mods to support florian's UI

::: media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_device_info.cc
@@ +7,4 @@
>  
>  #include <cstddef>
>  #include <cstring>
> +#include <sstream>

see below

@@ +243,5 @@
> +      pWinDevice->setDeviceName(itr->title.c_str());
> +
> +      std::ostringstream outs;
> +      outs << "\\win\\" << pWinDevice->getScreenId();
> +      pWinDevice->setUniqueIdName(outs.str().c_str());

can we do this without ostringstream?  The only use other than tests I can find is in a single logging function (in the webrtc.org codebase).  We generally avoid it (though I'm not sure why; perhaps bloat as we have many other things do similar stuff).  How are uniqueId's generated elsewhere?

::: media/webrtc/trunk/webrtc/modules/desktop_capture/mac/desktop_device_info_mac.mm
@@ +35,5 @@
>    }
>  #endif
> +
> +  initializeWindowList();
> +  

trailing spaces

::: media/webrtc/trunk/webrtc/video_engine/desktop_capture_impl.cc
@@ +215,4 @@
>    RefCountImpl<DesktopCaptureImpl>* capture = new RefCountImpl<DesktopCaptureImpl>(id);
>  
>    //create real screen capturer.
> +  if (capture->Init(uniqueId,type)!=0) {

spaces around !=
In fact, eliminate != 0 -- if (capture->Init(...)) {

@@ +226,5 @@
> +WindowDeviceInfoImpl::WindowDeviceInfoImpl(const int32_t id) : _id(id) {
> +}
> +
> +WindowDeviceInfoImpl::~WindowDeviceInfoImpl(void) {
> +}

this can likely be eliminated and add a {} in the .h file

Comment 5

[Matthew Miller [:m_and_m]]

(In reply to Randell Jesup [:jesup] from comment #4)
> ::: media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_device_info.cc
> @@ +7,4 @@
> >  
> >  #include <cstddef>
> >  #include <cstring>
> > +#include <sstream>
> 
> see below
> 
> @@ +243,5 @@
> > +      pWinDevice->setDeviceName(itr->title.c_str());
> > +
> > +      std::ostringstream outs;
> > +      outs << "\\win\\" << pWinDevice->getScreenId();
> > +      pWinDevice->setUniqueIdName(outs.str().c_str());
> 
> can we do this without ostringstream?  The only use other than tests I can
> find is in a single logging function (in the webrtc.org codebase).  We
> generally avoid it (though I'm not sure why; perhaps bloat as we have many
> other things do similar stuff).  How are uniqueId's generated elsewhere?
> 

Other parts of the code take advantage of window Id being in there.  I can change it to use snprintf() if that's better.

Comment 6

[Matthew Miller [:m_and_m]]

Attachment: Patch 2. Interdiff for review comments from Jesup

Comment 7

[Matthew Miller [:m_and_m]]

Attachment: Patch 1. v3 implement window sharing

Comment 8

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

Comment on attachment 8457778 [details] [diff] [review]
Patch 2. Interdiff for review comments from Jesup

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

::: media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_device_info.cc
@@ +242,5 @@
>        pWinDevice->setScreenId(itr->id);
>        pWinDevice->setDeviceName(itr->title.c_str());
>  
> +      char idStr[BUFSIZ];
> +      snprintf(idStr, BUFSIZ, "\\win\\%ld", pWinDevice->getScreenId());

two things: BUFSIZ is way too big. and snprintf is (amazingly) not portable (enough) - and doesn't always leave it null-terminated 

In this case: [64], sizeof(idStr) instead of BUFSIZ.  Make it _snprintf_s() for XP_WIN, snprintf for the others.

Comment 9

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8457780 [details] [diff] [review]
Patch 1. v3 implement window sharing

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

::: media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_device_info.cc
@@ +7,2 @@
>  
> +#include <stdio.h>

cstdio for consistency

@@ +150,5 @@
> +  std::map<intptr_t, DesktopDisplayDevice *>::iterator itrWindow;
> +  for (itrWindow = desktop_window_list_.begin(); itrWindow != desktop_window_list_.end(); itrWindow++) {
> +    DesktopDisplayDevice *pWindow = itrWindow->second;
> +    if (pWindow) {
> +      delete pWindow;

If is not needed

Comment 10

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

Comment on attachment 8457780 [details] [diff] [review]
Patch 1. v3 implement window sharing

r+ for the .webidl change.

Comment 11

[Matthew Miller [:m_and_m]]

Attachment: Patch 3. Interdiff for review comments from Jesup and GCP

Comment 12

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

Comment on attachment 8458024 [details] [diff] [review]
Patch 3. Interdiff for review comments from Jesup and GCP

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

::: media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_device_info.cc
@@ +244,1 @@
>        snprintf(idStr, BUFSIZ, "\\win\\%ld", pWinDevice->getScreenId());

sizeof(idStr)

Comment 13

[Matthew Miller [:m_and_m]]

Attachment: Patch 1. v4 implement window sharing

Rolls up all the previous interdiffs to addresses latest review comments.

NOTE: No changes to Constraints.webidl over what attachment 8457780 [details] [diff] [review] already had.

Comment 14

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

Comment on attachment 8458057 [details] [diff] [review]
Patch 1. v4 implement window sharing

I gave r+ to that .webidl change already.

Comment 15

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

Comment on attachment 8458057 [details] [diff] [review]
Patch 1. v4 implement window sharing

carry forward r+ gcp, plus my r+ is enough

Comment 16

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f969f1ac011
Thanks Matt!

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0f969f1ac011