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?

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.

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.

Thanks Roman!

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

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

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+

Comment on attachment 8463380 [details] [diff] [review] RescanWindowList_removed_list_of_files.patch Corry forward r+; will submit Try

(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 ? :)

(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.

carrying forward r+ p.s. This patch is generated by hg, so it should be ok.

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.

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

fix diff error; carrying forward r+=jesup,gcp

https://tbpl.mozilla.org/?tree=Try&rev=9162d53b6a95

(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'

I've fixed build error. Tested on Win 7 x64 and OS X 10.9.4 x64

https://tbpl.mozilla.org/?tree=Try&rev=5678ac79b865

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...

New try push, because it looks like jesup's was missing the actual patch, oopsie. https://tbpl.mozilla.org/?tree=Try&rev=7dafbb381e25

Fixed Linux build fail

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

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

Ready for TRY

https://hg.mozilla.org/integration/mozilla-inbound/rev/db13171100fa Please use your full name in your hg commit information in the future :)

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.

And ASAN for leaks (duh) https://tbpl.mozilla.org/?tree=Try&rev=9a351c5fcac6

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.

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

Bustage fixes: (1-liners for unified builds and b2g was missing the Refresh() impl for the null capture device) https://hg.mozilla.org/integration/mozilla-inbound/rev/546ac81a104a https://hg.mozilla.org/integration/mozilla-inbound/rev/2ba42fa44ce3 https://hg.mozilla.org/integration/mozilla-inbound/rev/48ffc7f64c9b

Bump the max number of asserts to 3 (tracked in bug 1047842): https://hg.mozilla.org/integration/mozilla-inbound/rev/5c73b82d7723

https://hg.mozilla.org/mozilla-central/rev/2fbe18cb6f08 https://hg.mozilla.org/mozilla-central/rev/546ac81a104a https://hg.mozilla.org/mozilla-central/rev/2ba42fa44ce3 https://hg.mozilla.org/mozilla-central/rev/48ffc7f64c9b https://hg.mozilla.org/mozilla-central/rev/5c73b82d7723

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?

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

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?