Closed Bug 1038926 Opened 5 years ago Closed 5 years ago

Add support for window sharing for getUserMedia

Categories

(Core :: WebRTC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: m_and_m, Assigned: m_and_m)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 6 obsolete files)

Support sharing a single window from the user's desktop screen.
Depends on: 983504
Attached patch implement window sharing (obsolete) — Splinter Review
Assignee: nobody → linuxwolf
Status: NEW → ASSIGNED
Attached patch -help (obsolete) — Splinter Review
Attachment #8457480 - Attachment is obsolete: true
Attached patch v2 implement window sharing (obsolete) — Splinter Review
Attachment #8457463 - Attachment is obsolete: true
Attachment #8457486 - Flags: review?(rjesup)
Attachment #8457486 - Flags: review?(gpascutto)
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
Attachment #8457486 - Flags: review?(gpascutto) → review+
(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.
Attachment #8457486 - Attachment is obsolete: true
Attachment #8457486 - Flags: review?(rjesup)
Attachment #8457780 - Flags: review?(rjesup)
Attachment #8457780 - Flags: review?(gpascutto)
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.
Attachment #8457780 - Flags: review?(rjesup) → review+
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
Attachment #8457780 - Flags: review?(gpascutto) → review+
Comment on attachment 8457780 [details] [diff] [review]
Patch 1. v3 implement window sharing

r+ for the .webidl change.
Attachment #8457780 - Flags: review?(bugs) → review+
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)
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.
Attachment #8457778 - Attachment is obsolete: true
Attachment #8457780 - Attachment is obsolete: true
Attachment #8458024 - Attachment is obsolete: true
Attachment #8458057 - Flags: review?(rjesup)
Attachment #8458057 - Flags: review?(gpascutto)
Attachment #8458057 - Flags: review?(bugs)
Comment on attachment 8458057 [details] [diff] [review]
Patch 1. v4 implement window sharing

I gave r+ to that .webidl change already.
Attachment #8458057 - Flags: review?(bugs) → review+
Attachment #8458057 - Flags: review?(rjesup) → review+
Comment on attachment 8458057 [details] [diff] [review]
Patch 1. v4 implement window sharing

carry forward r+ gcp, plus my r+ is enough
Attachment #8458057 - Flags: review?(gpascutto)
https://hg.mozilla.org/mozilla-central/rev/0f969f1ac011
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Blocks: 1050930
You need to log in before you can comment on or make changes to this bug.