Closed Bug 1041369 Opened 10 years ago Closed 10 years ago

Rescan window list on each getUserMedia window/screensharing request

Categories

(Core :: WebRTC: Audio/Video, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla34
Tracking Status
firefox33 --- verified
firefox34 --- verified

People

(Reporter: jesup, Assigned: rskalish)

References

Details

(Keywords: qawanted, verifyme, Whiteboard: p=5)

Attachments

(2 files, 8 obsolete files)

Right now the MediaEngineWebRTC code scans the window list once the first time it's called for use with window/app/screen sharing.  The list(s) should be scanned on each request, for obvious reasons.  (This would include the list of screens, since we plan to support multiple screens).
Roman -- This is the bug I was talking about on Friday in #media.
It looks like we need to re-create WinEngine,AppEngine and ScreenEngine every time when we use getUserMedia() or we can extend VideoEngine by adding function like refresh() (but it will does the same as re-creating object).
Not sure what means '(This would include the list of screens, since we plan to support multiple screens)'. Do you mean that in scope of this bug we need also display a list of screens?
Flags: needinfo?(rjesup)
Work to support multi-monitor screenshare (ie. select which monitor you want to share) is being done; it wasn't ready so it was disabled for the moment.  Best to make your solution work for that case (monitors can appear/go away).

One thing to be careful of is to not stop any existing users - but since they already have a reference, it should be ok.  Just keep in mind there can be multiple users, and multiple requests.
Flags: needinfo?(rjesup)
Maire could you please assign this bug to me?
I am not familiar Bugzilla points, but as I am new in mozilla dev, I'll give 5.
Flags: needinfo?(mreavy)
Flags: needinfo?(mreavy)
Thanks Roman!
Assignee: nobody → rskalish
Whiteboard: p=5, ft:screenshare
Attached patch RescanWindowList.patch (obsolete) — Splinter Review
Attachment #8461562 - Flags: review+
Comment on attachment 8461562 [details] [diff] [review]
RescanWindowList.patch

You can't approve your own patch, see: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch
Attachment #8461562 - Flags: review+ → review?(rjesup)
Whiteboard: p=5, ft:screenshare → p=5, [sceensharing-uplift]
Comment on attachment 8461562 [details] [diff] [review]
RescanWindowList.patch

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

r+ if the ifdef is swapped - please make the change, and also update the patch description to remove the file list (if using mercurial queues, "hg qref -e" and edit it.  Then upload it and mark it checkin-needed in the Keywords field.

::: mozilla-central/media/webrtc/trunk/webrtc/modules/desktop_capture/win/desktop_device_info_win.cc
@@ +47,5 @@
>    return 0;
>  }
>  
> +int32_t DesktopDeviceInfoWin::Refresh() {
> +#if !defined(MULTI_MONITOR_SCREENSHARE)

This should be #if defined(MULTI_MONITOR_SCREENSHARE) (so it works once that code lands).  Until then it's a fixed list anyways
Attachment #8461562 - Flags: review?(rjesup) → review+
And thanks!
(In reply to Randell Jesup [:jesup] from comment #8)
> Comment on attachment 8461562 [details] [diff] [review]
> RescanWindowList.patch
> 
> Review of attachment 8461562 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ if the ifdef is swapped - please make the change, and also update the
> patch description to remove the file list (if using mercurial queues, "hg
> qref -e" and edit it.  Then upload it and mark it checkin-needed in the
> Keywords field.
> 
> :::
> mozilla-central/media/webrtc/trunk/webrtc/modules/desktop_capture/win/
> desktop_device_info_win.cc
> @@ +47,5 @@
> >    return 0;
> >  }
> >  
> > +int32_t DesktopDeviceInfoWin::Refresh() {
> > +#if !defined(MULTI_MONITOR_SCREENSHARE)
> 
> This should be #if defined(MULTI_MONITOR_SCREENSHARE) (so it works once that
> code lands).  Until then it's a fixed list anyways

Actually in base code it was #if !defined(MULTI_MONITOR_SCREENSHARE), so I've left it as is. If we change it to #if defined(MULTI_MONITOR_SCREENSHARE), then screensharing will not be available from default build and then we need define locally MULTI_MONITOR_SCREENSHARE to make screensharing available.
carrying forward r+
Attachment #8463380 - Flags: review+
Attachment #8463380 - Flags: checkin+
Keywords: checkin-needed
Attachment #8463380 - Flags: review+
Attachment #8463380 - Flags: checkin+
Attachment #8461562 - Attachment is obsolete: true
Comment on attachment 8463380 [details] [diff] [review]
RescanWindowList_removed_list_of_files.patch

Corry forward r+; will submit Try
Attachment #8463380 - Flags: review+
Priority: -- → P1
Target Milestone: --- → mozilla34
(In reply to Randell Jesup [:jesup] from comment #12)
> Comment on attachment 8463380 [details] [diff] [review]
> RescanWindowList_removed_list_of_files.patch
> 
> Corry forward r+; will submit Try

Hi, do you have the try link ? :)
Keywords: checkin-needed
(In reply to Carsten Book [:Tomcat] from comment #13)
> (In reply to Randell Jesup [:jesup] from comment #12)
> > Comment on attachment 8463380 [details] [diff] [review]
> > RescanWindowList_removed_list_of_files.patch
> > 
> > Corry forward r+; will submit Try
> 
> Hi, do you have the try link ? :)

Jesup told yesterday that all Try links are red, it happened because I've attached git patch not hg (I didn't know that it should be hg :) )
Going to attach hg patch file asap.
Attached patch Bug1041369_Fixed.patch (obsolete) — Splinter Review
carrying forward r+
p.s. This patch is generated by hg, so it should be ok.
Attachment #8463380 - Attachment is obsolete: true
Attachment #8463946 - Flags: review?(rjesup)
Keywords: checkin-needed
Comment on attachment 8463946 [details] [diff] [review]
Bug1041369_Fixed.patch

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

If you carry forward then it would be r+, not r?. In any case, please check your editor settings and clean up the indentation in the file.

::: media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_device_info.cc
@@ +254,5 @@
> +int32_t DesktopDeviceInfoImpl::RefreshWindowList() {
> +    desktop_window_list_.clear();
> +    initializeWindowList();
> +    
> +     return 0;

Inconsistent & spurious whitespace. The original files use 2 space, this code uses a mixture of 4 and 5 spaces throughout the file. There's also empty lines with whitespace.

::: media/webrtc/trunk/webrtc/modules/desktop_capture/win/desktop_device_info_win.cc
@@ +31,5 @@
> +        pDesktopDeviceInfo->setUniqueIdName("\\screen\\monitor#1");
> +
> +        desktop_display_list_[pDesktopDeviceInfo->getScreenId()] = pDesktopDeviceInfo;
> +    }
> +    return 0;    

nit: trailing whitespace

::: media/webrtc/trunk/webrtc/modules/desktop_capture/win/desktop_device_info_win.h
@@ +20,5 @@
> +
> +  virtual int32_t Refresh();
> +
> +private:
> + 

nit: spurious whitespace

::: media/webrtc/trunk/webrtc/video_engine/desktop_capture_impl.cc
@@ +44,5 @@
>  
> +int32_t ScreenDeviceInfoImpl::Refresh() {
> +    desktop_device_info_->Refresh();
> +    return 0;
> +} 

Trailing whitespace.
Attachment #8463946 - Flags: review-
Attached patch Bug1041369_Fixed.patch (obsolete) — Splinter Review
Attachment #8463946 - Attachment is obsolete: true
Attachment #8463946 - Flags: review?(rjesup)
Attachment #8463958 - Flags: review?(rjesup)
Attachment #8463958 - Flags: review?(gpascutto)
Comment on attachment 8463958 [details] [diff] [review]
Bug1041369_Fixed.patch

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

::: media/webrtc/trunk/webrtc/video_engine/desktop_capture_impl.cc
@@ +240,5 @@
>  
> +int32_t WindowDeviceInfoImpl::Refresh() {
> +  desktop_device_info_->Refresh();
> +  return 0;
> + }

nit: indentation mistake
Attachment #8463958 - Flags: review?(gpascutto) → review+
Attachment #8463958 - Attachment is obsolete: true
Attachment #8463958 - Flags: review?(rjesup)
Attachment #8464000 - Flags: review?(rjesup)
Attachment #8464000 - Flags: review?(gpascutto)
Attachment #8464000 - Flags: review?(gpascutto) → review+
Attachment #8464000 - Flags: review?(rjesup) → review+
Keywords: checkin-needed
Attached patch Bug104139.patch (obsolete) — Splinter Review
fix diff error; carrying forward r+=jesup,gcp
Attachment #8464000 - Attachment is obsolete: true
Attachment #8464081 - Flags: review+
(In reply to Randell Jesup [:jesup] from comment #21)
>  https://tbpl.mozilla.org/?tree=Try&rev=9162d53b6a95

there seems to be a bustage:

/builds/slave/try-l64-asan-00000000000000000/build/media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc:41:25: error: allocating an object of abstract class type 'videocapturemodule::DeviceInfoLinux'
Keywords: checkin-needed
I've fixed build error. Tested on Win 7 x64 and OS X 10.9.4 x64
Attachment #8464081 - Attachment is obsolete: true
Attachment #8464627 - Flags: review?(rjesup)
Comment on attachment 8464627 [details] [diff] [review]
Bug1041369_fixed_build_errors.patch

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

This patch makes almost all files executable. There's no reason for this...

diff --git a/media/webrtc/trunk/webrtc/modules/desktop_capture/OWNERS b/media/webrtc/trunk/webrtc/modules/desktop_capture/OWNER
old mode 100644
new mode 100755

It also contains Windows style line endings mixed with Unix style ones...
Attachment #8464627 - Flags: review?(rjesup) → review-
New try push, because it looks like jesup's was missing the actual patch, oopsie.
https://tbpl.mozilla.org/?tree=Try&rev=7dafbb381e25
Attached patch Bug1041369.patch (obsolete) — Splinter Review
Fixed Linux build fail
Attachment #8464627 - Attachment is obsolete: true
Attachment #8465259 - Flags: review?(rjesup)
Attachment #8465259 - Flags: review?(gpascutto)
Comment on attachment 8465259 [details] [diff] [review]
Bug1041369.patch

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

::: media/webrtc/trunk/webrtc/modules/desktop_capture/mac/desktop_device_info_mac.h
@@ +20,5 @@
> +  virtual int32_t Refresh();
> +
> +private:
> +#if !defined(MULTI_MONITOR_SCREENSHARE)
> +    int32_t MultiMonitorScreenshare();

nit: inconsistent indentation again

::: media/webrtc/trunk/webrtc/modules/desktop_capture/win/desktop_device_info_win.h
@@ +20,5 @@
> +  virtual int32_t Refresh();
> +
> +private:
> +#if !defined(MULTI_MONITOR_SCREENSHARE)
> +    int32_t MultiMonitorScreenshare();

nit: indentation

::: media/webrtc/trunk/webrtc/modules/desktop_capture/x11/desktop_device_info_x11.cc
@@ +24,5 @@
>  
> +#if !defined(MULTI_MONITOR_SCREENSHARE)
> +int32_t DesktopDeviceInfoX11::MultiMonitorScreenshare()
> +{
> +    DesktopDisplayDevice *pDesktopDeviceInfo = new DesktopDisplayDevice;

nit: indentation is wrong, 4 spaces instead of 2

::: media/webrtc/trunk/webrtc/modules/desktop_capture/x11/desktop_device_info_x11.h
@@ +20,5 @@
> +  virtual int32_t Refresh();
> +
> +private:
> +#if !defined(MULTI_MONITOR_SCREENSHARE)
> +    int32_t MultiMonitorScreenshare();

nit: indentation

::: media/webrtc/trunk/webrtc/video_engine/desktop_capture_impl.cc
@@ +44,5 @@
>  
> +int32_t ScreenDeviceInfoImpl::Refresh() {
> +  desktop_device_info_->Refresh();
> +  return 0;
> +} 

nit: spurious whitespace
Attachment #8465259 - Flags: review?(gpascutto) → review+
Comment on attachment 8465259 [details] [diff] [review]
Bug1041369.patch

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

::: media/webrtc/trunk/webrtc/modules/desktop_capture/x11/desktop_device_info_x11.cc
@@ +52,5 @@
> +    desktop_display_list_.clear();
> +    MultiMonitorScreenshare();
> +#endif
> +
> +    RefreshWindowList();

indentation here too
Attachment #8465259 - Flags: review?(rjesup) → review+
Attached patch Bug1041369.patchSplinter Review
Ready for TRY
Attachment #8465259 - Attachment is obsolete: true
Attachment #8466072 - Flags: review?(rjesup)
Attachment #8466072 - Flags: review?(rjesup) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/db13171100fa

Please use your full name in your hg commit information in the future :)
Keywords: checkin-needed
Backed out for LSAN failures. You can ignore the inputmethod failure in that log - it's another unrelated issue.
https://hg.mozilla.org/integration/mozilla-inbound/rev/bffb1d563196

https://tbpl.mozilla.org/php/getParsedLog.php?id=45187268&tree=Mozilla-Inbound
Nils is this something you might want to test? I'm not sure how to triage it, so, over to you.
Flags: needinfo?(drno)
Attachment #8468946 - Flags: review?(gpascutto)
Comment on attachment 8468946 [details] [diff] [review]
fix LSAN leaks in window and screen capturing in window/screen rescan

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

::: media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_device_info.cc
@@ +223,5 @@
>    return 0;
>  }
>  
>  int32_t DesktopDeviceInfoImpl::initializeWindowList() {
> +  scoped_ptr<WindowCapturer> pWinCap(WindowCapturer::Create());

*mumble* deprecated *mumble* const UniquePtr *mumble*

I guess this code is semi-shared so using Chrome's stuff is ok.

::: media/webrtc/trunk/webrtc/modules/desktop_capture/win/desktop_device_info_win.cc
@@ +48,5 @@
>  
>  int32_t DesktopDeviceInfoWin::Refresh() {
>  #if !defined(MULTI_MONITOR_SCREENSHARE)
> +  std::map<intptr_t,DesktopDisplayDevice*>::iterator iterDevice;
> +  for (iterDevice=desktop_display_list_.begin(); iterDevice!=desktop_display_list_.end(); iterDevice++){

itr, iter, spaces, no spaces, the coding style consistency is not strong with this one.

::: media/webrtc/trunk/webrtc/video_engine/desktop_capture_impl.cc
@@ +393,5 @@
>  
>      std::string idStr(uniqueId);
>      const std::string prefix("\\win\\");
>      if (idStr.substr(0, prefix.size()) != prefix) {
> +      delete pWindowCapturer;

You could've used a smart pointer here too and explicitly transfer ownership at the end, thereby avoiding future versions of this leak.
Attachment #8468946 - Flags: review?(gpascutto) → review+
Comment on attachment 8468946 [details] [diff] [review]
fix LSAN leaks in window and screen capturing in window/screen rescan

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

::: media/webrtc/trunk/webrtc/modules/desktop_capture/win/desktop_device_info_win.cc
@@ +48,5 @@
>  
>  int32_t DesktopDeviceInfoWin::Refresh() {
>  #if !defined(MULTI_MONITOR_SCREENSHARE)
> +  std::map<intptr_t,DesktopDisplayDevice*>::iterator iterDevice;
> +  for (iterDevice=desktop_display_list_.begin(); iterDevice!=desktop_display_list_.end(); iterDevice++){

> itr, iter, spaces, no spaces, the coding style consistency is not strong with this one

Just cut-and-pasting the code used to clean these up elsewhere.  At least they match.

I cleaned up the iter/itr mismatches and spaces around *'s some for the checkin.  The rest I left; I want to avoid extra pain on merge, and this is largely upstream code I hope to merge up after the crunch
Flags: needinfo?(drno)
This says 33 is affected, but there seems to be no uplift request and no landing in aurora. Consequently it is also not fixed in Aurora. Can you update the status flag or request uplift?
Flags: needinfo?(rjesup)
Flags: needinfo?(rjesup)
Whiteboard: p=5, [sceensharing-uplift] → p=5, [screensharing-uplift]
Comment on attachment 8466072 [details] [diff] [review]
Bug1041369.patch

Approval Request Comment
[Feature/regressing bug #]: screensharing

[User impact if declined]: Window sharing has low utility as the window list will be fixed on the first usage.

[Describe test coverage new/current, TBPL]: Mostly covered by normal screen/window sharing tests; manual testing is needed to confirm new windows show up properly (tbpl has no ability to manipulate things to actually verify it in automation).  Tested manually for some time now.

[Risks and why]: moderately low risk; contained to a new feature.  Invokes the existing scan mechanism again; highest risk is of a leak (which happened, and has been fixed - this request is for both patches).

[String/UUID change made/needed]: none
Attachment #8466072 - Flags: approval-mozilla-aurora?
Attachment #8466072 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This is hitting a conflict on the uplift due to bug 1041493 not being on Aurora.
Hi Nils I've noticed that you verified this fix on FF34 do you have the necessary time to verify it on FF33?
Flags: needinfo?(drno)
Flags: needinfo?(drno)
Status: RESOLVED → VERIFIED
See Also: → 1396990
You need to log in before you can comment on or make changes to this bug.