Make GUM able to pull screen into a MediaStream

RESOLVED FIXED in mozilla33

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ehugg, Assigned: m_and_m)

Tracking

(Depends on 1 bug, Blocks 1 bug)

Trunk
mozilla33
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(17 attachments, 34 obsolete attachments)

10.71 KB, patch
ted
: review+
Details | Diff | Splinter Review
14.05 KB, patch
gcp
: review+
Details | Diff | Splinter Review
1.03 KB, patch
ted
: review+
Details | Diff | Splinter Review
989 bytes, patch
ted
: review+
Details | Diff | Splinter Review
17.00 KB, patch
Dolske
: review-
Details | Diff | Splinter Review
9.62 KB, patch
jesup
: review+
Details | Diff | Splinter Review
5.99 KB, patch
jesup
: review+
Details | Diff | Splinter Review
4.75 KB, patch
jib
: review+
Details | Diff | Splinter Review
12.61 KB, patch
bholley
: review+
Details | Diff | Splinter Review
4.30 KB, patch
Details | Diff | Splinter Review
18.56 KB, patch
jesup
: review+
Details | Diff | Splinter Review
12.46 KB, patch
Details | Diff | Splinter Review
34.07 KB, patch
jesup
: review+
Details | Diff | Splinter Review
9.69 KB, patch
Details | Diff | Splinter Review
46.64 KB, patch
jesup
: review+
Details | Diff | Splinter Review
1.55 KB, patch
mshal
: review+
jesup
: feedback+
Details | Diff | Splinter Review
1.16 KB, patch
jesup
: review+
Details | Diff | Splinter Review
Reporter

Description

5 years ago
Add a new constraint (perhaps screen:true) to pull the screen in GetUserMedia into a WebRTC MediaStream.  This is a first step to getting screensharing working.  This bug does not include the permissions UI needed for this.

Also this is for the desktop platforms and should be ifdef'd out for Android and FFOS for now.
Reporter

Updated

5 years ago
Reporter

Updated

5 years ago
Assignee: nobody → vagouzhou
(In reply to Ethan Hugg [:ehugg] from comment #0)
> Add a new constraint (perhaps screen:true) to pull the screen in
> GetUserMedia into a WebRTC MediaStream.

I think we're using the video-constraint mozMediaSource:'screen|application|camera(default)' at the moment to mirror chromeMediaSource, but I have a question about that on the list http://lists.w3.org/Archives/Public/public-media-capture/2014Apr/0028.html

What's the scope of this feature? Are we planning to allow getting audio and video AND screen?
Reporter

Comment 2

5 years ago
>What's the scope of this feature? Are we planning to allow getting audio and video AND screen?

I consider this bug to just be getting the screen as a video stream.  As you know we are not currently able to do two video streams in the same peerconnection so that will have to be solved in the context of other bugs.

Comment 3

5 years ago
Just update status.

https://github.com/vagouzhou/gecko-dev/commits/master
After this commit fb968a557c531f5ce6184e760617a366c7ef55e4, there are one workable screen sharing and pipeline in Mac/Windows. use https://github.com/vagouzhou/vagouzhou.github.io readme instruction to test it.

Move forward to implement appshare and multi-monitor feature now.

Comment 4

5 years ago
The rough proposal from Martin and I on the API for this is 
http://lists.w3.org/Archives/Public/public-media-capture/2014Apr/att-0189/screensharing.html

Comment 5

5 years ago
current implement is 
https://github.com/vagouzhou/gecko-dev/commit/5ac1d09b6923d15ea968387a8c2fa0e419ad9f50
+enum MozMediaSourceEnum {
 +    "camera",
 +    "screen",
 +    "application"
 +};

can update after finalize.

Comment 6

5 years ago
Code review for current pipeline for screen sharing with single monitor/desktop windows/mac/linux platform. 

To create patch maybe too big , I just share code diff link as below.
https://github.com/vagouzhou/gecko-dev/compare/master@%7B38days%7D...master
https://github.com/vagouzhou/gecko-dev/commit/1b6018fc1b06879c4c8d71463a69505356ca3455
https://github.com/vagouzhou/gecko-dev/commit/0b4ba064c4650ec4dfb5999b2fa8bb31dbce0d8d

I don’t add comments for each line code in github.
To be  convenience for code review, I summary code changed items as below.
I also input these comments into my bug https://bugzilla.mozilla.org/show_bug.cgi?id=983504

	To build WebRTC desktop capture module in Geoko
build/gyp.mozbuild
configure.in
media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_capture.gypi


	To show screen sharing popup windows that can select screen display(multi-monitor) or process(application share)
browser/base/content/popup-notifications.inc
browser/locales/en-US/chrome/browser/browser.dtd
browser/locales/en-US/chrome/browser/browser.properties
browser/modules/webrtcUI.jsm
 


	getUserMedia API constraint update for screen sharing with mMozMediaSource of mandatory. Refer to http://lists.w3.org/Archives/Public/public-media-capture/2014Apr/att-0189/screensharing.html
and also Enumerate source for screen sharing not just audio/video source . 
BTW, it has one bug is that it always Enumerate all source don’t count on input constraint , I told Jan-Ivar, he has one bug to address it. https://bugzilla.mozilla.org/show_bug.cgi?id=916012
	dom/media/nsIDOMNavigatorUserMedia.idl
dom/webidl/MediaStreamTrack.webidl 
dom/media/MediaManager.cpp
dom/media/MediaManager.h

	Implement real appshare and screen source enumerate ,and MediaEngineWebRTCScreenSource and MediaEngineWebRTCApplicationSource
content/media/webrtc/MediaEngine.h
content/media/webrtc/MediaEngineDefault.cpp
content/media/webrtc/MediaEngineDefault.h
content/media/webrtc/MediaEngineWebRTC.cpp
content/media/webrtc/MediaEngineWebRTC.h

	WebRTC Video engine can generate screen capture  device info (for process , display info) ,not just video capture device info depending on config, that is from mMozMediaSource  constraint
media/webrtc/trunk/webrtc/video_engine/vie_input_manager.cc
media/webrtc/trunk/webrtc/video_engine/vie_input_manager.h

	WebRTC Video engine can generate screen capture ,not just video capture depending on config, that is from mMozMediaSource  constraint
media/webrtc/trunk/webrtc/video_engine/include/vie_capture.h
media/webrtc/trunk/webrtc/video_engine/vie_capturer.cc
media/webrtc/trunk/webrtc/video_engine/vie_capturer.h

	To reuse video engine for screen sharing , it one adapter that bridge Video capture API into Desktop Capture interfaces. (WebRTC desktop capture module that is maintained by chrome team ,not WebRTC team, we can notice it that has some different code style and interface style. 
media/webrtc/trunk/webrtc/video_engine/desktop_capture_impl.cc
media/webrtc/trunk/webrtc/video_engine/desktop_capture_impl.h


	These is application sharing framework by refer to current screen/window sharing. Need not review code now, not real implement it. 
media/webrtc/trunk/webrtc/modules/desktop_capture/app_capturer.h
media/webrtc/trunk/webrtc/modules/desktop_capture/app_capturer.cc
media/webrtc/trunk/webrtc/modules/desktop_capture/app_capturer_mac.mm
media/webrtc/trunk/webrtc/modules/desktop_capture/app_capturer_null.cc
media/webrtc/trunk/webrtc/modules/desktop_capture/app_capturer_unittest.cc
media/webrtc/trunk/webrtc/modules/desktop_capture/app_capturer_win.cc
media/webrtc/trunk/webrtc/modules/desktop_capture/app_capturer_x11.cc

	These are for device(process, display) , it is not real implement. Need not review code now, not real implement it.  at present just hardcode to share primary display.
media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_device_info.cc
media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_device_info.h
media/webrtc/trunk/webrtc/modules/desktop_capture/mac/desktop_device_info_mac.h
media/webrtc/trunk/webrtc/modules/desktop_capture/mac/desktop_device_info_mac.mm
media/webrtc/trunk/webrtc/modules/desktop_capture/win/desktop_device_info_win.cc
media/webrtc/trunk/webrtc/modules/desktop_capture/win/desktop_device_info_win.h
media/webrtc/trunk/webrtc/modules/desktop_capture/x11/desktop_device_info_x11.cc
media/webrtc/trunk/webrtc/modules/desktop_capture/x11/desktop_device_info_x11.h

	Most code is merge from latest WebRTC desktop capture code , maybe change few to make building pass , I don’t change logic. Need not review code
media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_and_cursor_composer.cc
media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_and_cursor_composer.h
media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_and_cursor_composer_unittest.cc
media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_capture_options.cc
media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_capture_options.h
media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_capture_types.h
media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_frame.cc
media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_frame.h
media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_geometry.cc
media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_geometry.h
media/webrtc/trunk/webrtc/modules/desktop_capture/differ_block.cc
media/webrtc/trunk/webrtc/modules/desktop_capture/differ_block.h
media/webrtc/trunk/webrtc/modules/desktop_capture/mac/desktop_configuration.h
media/webrtc/trunk/webrtc/modules/desktop_capture/mac/desktop_configuration.mm
media/webrtc/trunk/webrtc/modules/desktop_capture/mac/desktop_configuration_monitor.cc
media/webrtc/trunk/webrtc/modules/desktop_capture/mac/desktop_configuration_monitor.h
media/webrtc/trunk/webrtc/modules/desktop_capture/mac/osx_version.cc
media/webrtc/trunk/webrtc/modules/desktop_capture/mac/osx_version.h
media/webrtc/trunk/webrtc/modules/desktop_capture/mouse_cursor.cc
media/webrtc/trunk/webrtc/modules/desktop_capture/mouse_cursor.h
media/webrtc/trunk/webrtc/modules/desktop_capture/mouse_cursor_monitor.h
media/webrtc/trunk/webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm
media/webrtc/trunk/webrtc/modules/desktop_capture/mouse_cursor_monitor_null.cc
media/webrtc/trunk/webrtc/modules/desktop_capture/mouse_cursor_monitor_unittest.cc
media/webrtc/trunk/webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc
media/webrtc/trunk/webrtc/modules/desktop_capture/mouse_cursor_monitor_x11.cc
media/webrtc/trunk/webrtc/modules/desktop_capture/mouse_cursor_shape.h
media/webrtc/trunk/webrtc/modules/desktop_capture/screen_capturer.cc
media/webrtc/trunk/webrtc/modules/desktop_capture/screen_capturer.h
media/webrtc/trunk/webrtc/modules/desktop_capture/screen_capturer_mac.mm
media/webrtc/trunk/webrtc/modules/desktop_capture/screen_capturer_mock_objects.h
media/webrtc/trunk/webrtc/modules/desktop_capture/screen_capturer_null.cc
media/webrtc/trunk/webrtc/modules/desktop_capture/screen_capturer_unittest.cc
media/webrtc/trunk/webrtc/modules/desktop_capture/screen_capturer_win.cc
media/webrtc/trunk/webrtc/modules/desktop_capture/screen_capturer_x11.cc
media/webrtc/trunk/webrtc/modules/desktop_capture/win/cursor.cc
media/webrtc/trunk/webrtc/modules/desktop_capture/win/cursor.h
media/webrtc/trunk/webrtc/modules/desktop_capture/win/cursor_unittest.cc
media/webrtc/trunk/webrtc/modules/desktop_capture/window_capturer.cc
media/webrtc/trunk/webrtc/modules/desktop_capture/window_capturer.h 
media/webrtc/trunk/webrtc/modules/desktop_capture/ window_capturer_mac.cc
media/webrtc/trunk/webrtc/modules/desktop_capture/window_capturer_null.cc
media/webrtc/trunk/webrtc/modules/desktop_capture/window_capturer_unittest.cc
media/webrtc/trunk/webrtc/modules/desktop_capture/window_capturer_win.cc
media/webrtc/trunk/webrtc/modules/desktop_capture/window_capturer_x11.cc
media/webrtc/trunk/webrtc/modules/desktop_capture/x11/shared_x_display.cc
media/webrtc/trunk/webrtc/modules/desktop_capture/x11/shared_x_display.h

Comment 7

5 years ago
Posted patch patch.diff (obsolete) — Splinter Review
code review for screen sharing pipeline first. 
we will support application sharing and multi-monitor support later.

Then other cross function engineer can do parallel
#  Integration screen sharing  with openh264 codec ,now it is VP8.
#  Support to share individual application, now just share entire desktop/display. And multi-monitor support.
#  openh264 codec also need be optimized for screen content sharing to get clear text.
#  screen sharing indicator GUI in firefox browser. So user know that he/she is sharing screen.
I'll be looking at this in the next day or so.
Attachment #8418448 - Flags: feedback?(rjesup)
Comment on attachment 8418448 [details] [diff] [review]
patch.diff

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

Publishing partial review.  Will continue later.  

It would help a lot if this were re-packaged into coherent groups of changes like "x11 sharing support" or "Add this API".  Even if it means we have 10 patches - with something this large, that's *good* and will make things go faster.

I recommend something like "hg qcrecord ..." (from the crecord mercurial extension)

::: .gitignore
@@ +20,5 @@
>  /config.log
>  /.clang_complete
>  /mach.ini
> +/build.bat
> +/gecko-dev.*

don't touch this file

::: browser/modules/webrtcUI.jsm
@@ +72,4 @@
>      constraints,
>      function (devices) {
>        prompt(contentWindow, aSubject.callID, constraints.audio,
> +             constraints.video || constraints.picture, devices,constraints.videom,constraints.audiom);

All the UI code will need UI team review

::: build/gyp.mozbuild
@@ +18,4 @@
>      'include_tests': 0,
>      'enable_android_opensl': 1,
>      'enable_android_opensl_output': 0,
> +	'use_openssl':1 if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'Android' else 0,

Why is this needed?

::: content/media/webrtc/MediaEngineDefault.cpp
@@ +462,4 @@
>  
>    return;
>  }
> +    

strip trailing spaces from your patches

::: content/media/webrtc/MediaEngineWebRTC.cpp
@@ +69,3 @@
>  void
> +MediaEngineWebRTC::EnumerateScreenDevices(nsTArray<nsRefPtr<MediaEngineVideoSource> >*aVSources){
> +#ifdef MOZ_B2G_CAMERA

MOZ_WIDGET_GONK

@@ +77,5 @@
> +            return;
> +        }
> +    }
> +#endif
> +    EnumerateCommonVideoDevices(aVSources,mScreenEngine,mScreenEngineInit,dom::MozMediaSourceEnum::Screen);

indent?  Could be splinter confusing things
Style: ", foo" not ",foo"; split lines this long

@@ +180,5 @@
>      return;
>    }
>  #endif
> +  /*if (!videoEngine) {
> +    if (!(videoEngine = webrtc::VideoEngine::Create())) {

Why is this commented out?  It should be in or out, or at most disabled with a "fix this when that happens" typically with a bug #

@@ +206,5 @@
>    if (!ptrViEBase) {
>      return;
>    }
>  
> +  if (!bEngineInit) {

mozilla style is that member vars are mFoo and arguments are aFoo, not hungarian-style typeFoo or google-style foo_ (or _foo)

::: content/media/webrtc/MediaEngineWebRTC.h
@@ +395,5 @@
>  private:
>    Mutex mMutex;
> +    //vagouzhou@gmail.com
> +    //TBD ,implement application sharing in future
> +    //vagouzhou>>engine is cache, had to seperate video/screen/application

Comment is confusing.  Also, normally we don't sign comments.  

"todo" items are // XXX or // TODO (usually XXX), and preferably with a bug # to track the issue.

@@ +403,3 @@
>    // protected with mMutex:
> +    webrtc::VideoEngine* mScreenEngine;
> +    webrtc::VideoEngine* mAppEngine;//vagouzhou>>maybe we can merge it with mScreenEngine

ditto about signing comments

::: dom/media/MediaManager.cpp
@@ +675,4 @@
>        return false;
>      }
>    }
> +    // TODO: Add more video-specific constraints

don't change indents

@@ +944,4 @@
>      if (mConstraints.mPicture || mConstraints.mVideo) {
>        ScopedDeletePtr<SourceSet> sources (GetSources(backend,
>            mConstraints.mVideom, &MediaEngine::EnumerateVideoDevices));
> +//vagouzhou@gmail.com >> TBD ,what to do for screen sharing ?

// XXX  What should we do for screen sharing here?  (Bug NNNNNN)

You can leave it as NNNNNN for now until reviews are done, then file the followups and fill in the bug numbers.  Avoids filing bugs you may not end up needing.

@@ +1095,5 @@
>      ScopedDeletePtr<SourceSet> final (GetSources(backend, mConstraints.mVideom,
>                                            &MediaEngine::EnumerateVideoDevices,
>                                            mLoopbackVideoDevice));
> +      {
> +          ScopedDeletePtr<SourceSet> s (GetSources(backend, mConstraints.mVideom,

single-char variables aren't good for stuff like this.  Perhaps "sources"

::: dom/media/MediaManager.h
@@ +449,5 @@
>    nsString mType;
>    nsString mID;
>    bool mHasFacingMode;
> +    dom::VideoFacingModeEnum mFacingMode;
> +    dom::MozMediaSourceEnum mMozMediaSource;

don't change indents (and don't use tabs, which is likely the cause)

::: dom/media/nsIDOMNavigatorUserMedia.idl
@@ +13,4 @@
>    readonly attribute DOMString name;
>    readonly attribute DOMString id;
>    readonly attribute DOMString facingMode;
> +  readonly attribute DOMString mozMediaSource;

The idl/webidl/DOM changes will need DOM peer review

::: media/webrtc/trunk/webrtc/modules/desktop_capture/app_capturer.cc
@@ +1,2 @@
> +/*
> +*  Copyright (c) 2013 The WebRTC project authors. All Rights Reserved.

2014 (and other new files the same)

::: media/webrtc/trunk/webrtc/modules/desktop_capture/app_capturer_mac.mm
@@ +59,5 @@
> +bool AppCapturerMac::SelectApp(ProcessId id){
> +	return true;
> +}
> + bool AppCapturerMac::BringAppToFront() {
> +	return true;

log not implemented yet (for all the not-implemented ones)

::: media/webrtc/trunk/webrtc/modules/desktop_capture/app_capturer_null.cc
@@ +45,5 @@
> +
> +		AppCapturerNull::~AppCapturerNull() {
> +		}
> +
> +		bool AppCapturerNull::GetAppList(AppList* apps){

space before {

::: media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_and_cursor_composer.cc
@@ +23,5 @@
> +// Helper function that blends one image into another. Source image must be
> +// pre-multiplied with the alpha channel. Destination is assumed to be opaque.
> +void AlphaBlend(uint8_t* dest, int dest_stride,
> +                const uint8_t* src, int src_stride,
> +                const DesktopSize& size) {

There's some serious assumptions about GFX formats here....  Probably a GFX function would be preferred.  Also that can avoid any issues with possible out-of-bounds writes you'd have to be careful about

@@ +50,5 @@
> +    dest += dest_stride;
> +  }
> +}
> +
> +// DesktopFrame wrapper that draws mouse on a frame and restores original

I stopped looking at this file around here

::: media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_device_info.cc
@@ +1,1 @@
> +#include "webrtc/modules/desktop_capture/desktop_device_info.h"

no copyright

@@ +8,5 @@
> +        screenId_ = kInvalidScreenId;
> +        deviceUniqueIdUTF8_ = NULL;
> +        deviceNameUTF8_ = NULL;
> +    }
> +    DesktopDisplayDevice::~DesktopDisplayDevice(){

lots of style cleanup needed in this file, comments, etc

::: media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_device_info.h
@@ +1,1 @@
> +#ifndef WEBRTC_MODULES_DESKTOP_CAPTURE_DEVICE_INFO_H_

copyright

@@ +7,5 @@
> +namespace webrtc {
> +    //================================================
> +    //
> +    class DesktopDisplayDevice{
> +        public:

follow indent/style standards of the webrtc code

@@ +58,5 @@
> +    public:
> +        virtual ~DesktopDeviceInfo(){};
> +        
> +        //
> +        virtual int32_t Init() =0 ;

Init()=0 or Init() = 0  (follow standards)

::: media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_frame.cc
@@ +37,5 @@
> +  for (int y = 0; y < dest_rect.height(); ++y) {
> +    memcpy(dest, src_buffer, DesktopFrame::kBytesPerPixel * dest_rect.width());
> +    src_buffer += src_stride;
> +    dest += stride();
> +  }

slight concern that we ensure sanity here.  Are there any additional asserts that can be added?

Platform-specific copy-rectangle functions might be more efficient; consider adding a comment about that and a TODO (even if only "investigate more-efficient platform copy options")

::: media/webrtc/trunk/webrtc/modules/desktop_capture/mac/desktop_configuration.mm
@@ -111,5 @@
>          GetConfigurationForScreen([screens objectAtIndex: i]);
>  
> -    // Handling mixed-DPI is hard, so we only return displays that match the
> -    // "primary" display's DPI.  The primary display is always the first in the
> -    // list returned by [NSScreen screens].

why was this description removed?

@@ -117,5 @@
>        desktop_config.dip_to_pixel_scale = display_config.dip_to_pixel_scale;
> -    } else if (desktop_config.dip_to_pixel_scale !=
> -               display_config.dip_to_pixel_scale) {
> -      continue;
> -    }

Why was the else removed, and what's the impact?  Is it because this is handled correctly by changes below?  (It appears so).  If so, great.

::: media/webrtc/trunk/webrtc/modules/desktop_capture/mac/desktop_configuration_monitor.cc
@@ +20,5 @@
> +static const int64_t kDisplayConfigurationEventTimeoutMs = 10 * 1000;
> +
> +DesktopConfigurationMonitor::DesktopConfigurationMonitor()
> +    : ref_count_(0),
> +      display_configuration_capture_event_(EventWrapper::Create()) {

{ on left edge (style, match norms for webrtc.org code)

@@ +25,5 @@
> +  CGError err = CGDisplayRegisterReconfigurationCallback(
> +      DesktopConfigurationMonitor::DisplaysReconfiguredCallback, this);
> +  if (err != kCGErrorSuccess) {
> +    LOG(LS_ERROR) << "CGDisplayRegisterReconfigurationCallback " << err;
> +    abort();

I realize this is a constructor, but abort should normally not be used; it's a sign the design is wrong.  Move the initialization code that can fail to an Init() function perhaps.

@@ +44,5 @@
> +void DesktopConfigurationMonitor::Lock() {
> +  if (!display_configuration_capture_event_->Wait(
> +              kDisplayConfigurationEventTimeoutMs)) {
> +    LOG_F(LS_ERROR) << "Event wait timed out.";
> +    abort();

We definitely shouldn't be aborting on a timeout.  Either loop (with a warning?) or return an error (and check it)

@@ +73,5 @@
> +      // from accessing display memory until the reconfiguration completes.
> +      if (!display_configuration_capture_event_->Wait(
> +              kDisplayConfigurationEventTimeoutMs)) {
> +        LOG_F(LS_ERROR) << "Event wait timed out.";
> +        abort();

Again, I'd like to avoid abort().  Compare to the other desktop capture versions (and yes, they have a couple of aborts)

::: media/webrtc/trunk/webrtc/modules/desktop_capture/mac/desktop_configuration_monitor.h
@@ +34,5 @@
> +  void Unlock();
> +  // Returns the current desktop configuration. Should only be called when the
> +  // lock has been acquired.
> +  const MacDesktopConfiguration& desktop_configuration() {
> +    return desktop_configuration_;

can we assert the lock is held?

::: media/webrtc/trunk/webrtc/modules/desktop_capture/mac/desktop_device_info_mac.h
@@ +1,1 @@
> +#ifndef WEBRTC_MODULES_DESKTOP_CAPTURE_MAC_DEVICE_INFO_H_

copyright

@@ +8,5 @@
> +    class DesktopDeviceInfoMac : public DesktopDeviceInfoImpl{
> +    public:
> +        DesktopDeviceInfoMac();
> +        ~DesktopDeviceInfoMac();
> +        

Trailing spaces...  Generally make sure you aren't introducing any.

::: media/webrtc/trunk/webrtc/modules/desktop_capture/mac/desktop_device_info_mac.mm
@@ +1,1 @@
> +#include "webrtc/modules/desktop_capture/mac/desktop_device_info_mac.h"

copyright

@@ +14,5 @@
> +    return pDesktopDeviceInfo;
> +}
> +DesktopDeviceInfoMac::DesktopDeviceInfoMac()
> +{
> +    

remove blank line

@@ +18,5 @@
> +    
> +}
> +DesktopDeviceInfoMac::~DesktopDeviceInfoMac()
> +{
> +    

ditto

@@ +36,5 @@
> +    //
> +    DesktopApplication *pDesktopApplication = new DesktopApplication;
> +    if(pDesktopApplication){
> +        pDesktopApplication->setProcessId(9999);
> +        pDesktopApplication->setProcessAppName("WebEx Meeting Center");

Probably not the best choice

@@ +38,5 @@
> +    if(pDesktopApplication){
> +        pDesktopApplication->setProcessId(9999);
> +        pDesktopApplication->setProcessAppName("WebEx Meeting Center");
> +        pDesktopApplication->setProcessPathName("\\screen\\monitor#1");
> +        pDesktopApplication->setUniqueIdName("\\app\\9999");

Is 9999 a good unique number?  static int next_unique = 0;  unique = next_unique++; or some such?  Does it matter?

::: media/webrtc/trunk/webrtc/modules/desktop_capture/mac/osx_version.cc
@@ +43,5 @@
> +
> +  // Verify that the version has been parsed correctly.
> +  if (darwin_version < 6) {
> +    LOG_F(LS_ERROR) << "Invalid Darwin version: " << darwin_version;
> +    abort();

abort is not reasonable here

::: media/webrtc/trunk/webrtc/modules/desktop_capture/mouse_cursor.h
@@ +1,2 @@
> +/*
> + *  Copyright (c) 2013 The WebRTC project authors. All Rights Reserved.

2014?

::: media/webrtc/trunk/webrtc/modules/desktop_capture/mouse_cursor_monitor.h
@@ +1,2 @@
> +/*
> + *  Copyright (c) 2013 The WebRTC project authors. All Rights Reserved.

2014?

::: media/webrtc/trunk/webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm
@@ +1,2 @@
> +/*
> + *  Copyright (c) 2013 The WebRTC project authors. All Rights Reserved.

2014?

@@ +112,5 @@
> +    }
> +  }
> +  // If we are capturing cursor for a specific window then we need to figure out
> +  // if the current mouse position is covered by another window and also adjust
> +  // |position| to make it relative to the window origin.

stopping here in this file

::: media/webrtc/trunk/webrtc/modules/desktop_capture/mouse_cursor_monitor_null.cc
@@ +1,2 @@
> +/*
> + *  Copyright (c) 2013 The WebRTC project authors. All Rights Reserved.

2014?

::: media/webrtc/trunk/webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc
@@ +60,5 @@
> +}
> +
> +MouseCursorMonitorWin::~MouseCursorMonitorWin() {
> +  if (desktop_dc_)
> +    ReleaseDC(NULL, desktop_dc_);

braces

::: media/webrtc/trunk/webrtc/modules/desktop_capture/mouse_cursor_monitor_x11.cc
@@ +7,5 @@
> + *  in the file PATENTS.  All contributing project authors may
> + *  be found in the AUTHORS file in the root of the source tree.
> + */
> +
> +#include "webrtc/modules/desktop_capture/mouse_cursor_monitor.h"

Skipping this file for now

::: media/webrtc/trunk/webrtc/modules/desktop_capture/screen_capturer.h
@@ +11,2 @@
>  #ifndef WEBRTC_MODULES_DESKTOP_CAPTURE_SCREEN_CAPTURER_H_
>  #define WEBRTC_MODULES_DESKTOP_CAPTURE_SCREEN_CAPTURER_H_

stopped here

Comment 10

5 years ago
Comment on attachment 8418448 [details] [diff] [review]
patch.diff

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

::: build/gyp.mozbuild
@@ +18,4 @@
>      'include_tests': 0,
>      'enable_android_opensl': 1,
>      'enable_android_opensl_output': 0,
> +	'use_openssl':1 if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'Android' else 0,

Ted Mielczarek help update this building script in order to fix building fail in Linux platform.

>Hello,
> 
>Sorry about the delay, I kept getting bogged down with other things.
>The good news is that I pulled your repository and was able to fix that
>OpenSSL/NSS error with a one line patch (attached). There are still
>some build errors, but they look like simple things to fix in the new
>code so I didn't attempt to address them. Let me know if there's
>anything else I can do to help.
> 
>Best regards,
>-Ted

::: content/media/webrtc/MediaEngineWebRTC.cpp
@@ +77,5 @@
> +            return;
> +        }
> +    }
> +#endif
> +    EnumerateCommonVideoDevices(aVSources,mScreenEngine,mScreenEngineInit,dom::MozMediaSourceEnum::Screen);

indent issue, it display good in XCode.
will change code to split long line.

@@ +180,5 @@
>      return;
>    }
>  #endif
> +  /*if (!videoEngine) {
> +    if (!(videoEngine = webrtc::VideoEngine::Create())) {

EnumerateApplicationDevices and EnumerateVideoDevices did it.
and they call this function EnumerateCommonVideoDevices

@@ +206,5 @@
>    if (!ptrViEBase) {
>      return;
>    }
>  
> +  if (!bEngineInit) {

I will update code to follow mozilla style

::: content/media/webrtc/MediaEngineWebRTC.h
@@ +395,5 @@
>  private:
>    Mutex mMutex;
> +    //vagouzhou@gmail.com
> +    //TBD ,implement application sharing in future
> +    //vagouzhou>>engine is cache, had to seperate video/screen/application

just personal mark.
I removed it ,appshare is ready now

@@ +403,3 @@
>    // protected with mMutex:
> +    webrtc::VideoEngine* mScreenEngine;
> +    webrtc::VideoEngine* mAppEngine;//vagouzhou>>maybe we can merge it with mScreenEngine

just persona mark,
remove it. we need 2 engines now

::: dom/media/MediaManager.cpp
@@ +675,4 @@
>        return false;
>      }
>    }
> +    // TODO: Add more video-specific constraints

it indeed is one issue, I will try to adjust unified indents my IDE tools cross visual studio, xcode and vim.

@@ +944,4 @@
>      if (mConstraints.mPicture || mConstraints.mVideo) {
>        ScopedDeletePtr<SourceSet> sources (GetSources(backend,
>            mConstraints.mVideom, &MediaEngine::EnumerateVideoDevices));
> +//vagouzhou@gmail.com >> TBD ,what to do for screen sharing ?

update comments

@@ +1095,5 @@
>      ScopedDeletePtr<SourceSet> final (GetSources(backend, mConstraints.mVideom,
>                                            &MediaEngine::EnumerateVideoDevices,
>                                            mLoopbackVideoDevice));
> +      {
> +          ScopedDeletePtr<SourceSet> s (GetSources(backend, mConstraints.mVideom,

I just follow up origin code style.
This looks like it needs re-review. Since Jesup is busy, perhaps we can find
someone else?
Flags: needinfo?(mreavy)
My apologies for not getting back to this sooner.  I think there was a bit of a misunderstanding.

As I indicated before, the patch as written is simply too large and too wide-ranging for a coherent, solid review.  

It needs to broken up into a set of coherent patches that implement an aspect of the overall feature.  Right now it's "Screen sharing" with a single monster patch that hits the code all over, and it's hard to even find the related parts that work together to understand and review; all I can really do is look at individual diff segments for obvious problems and style issues - which are good to fix, but don't solve the problem of reviewing.

Also, we do our reviews in mercurial; so changing git isn't enough.  (Some reviewers, at least for small patches, may review on git, but most don't, and we want the reviews (and patches being reviewed) captured here anyways.)

There actually are studies that show that review quality (based on how likely a bug is to be missed in the review) gets worse as the size of the code to review goes up.

I can continue the feedback on this patch, but it needs to be broken up to be reviewed - and honestly, it's not a great use of time to be going over the rest of the current code line-by-line as one huge blob.  I think I've highlighted the classes of style/etc issues that need to be fixed across the entire patch in the feedback I did earlier.  Looking at the (almost) exact same code multiple times also makes reviews worse - your eye starts skipping over stuff it saw before, and can miss changes; that's why once *real* review starts, it's often best to build a response to the review as a patch, and then upload both that (an "interdiff") and the merged complete "clean" patch.  That lets the reviewer focus on what you changed as a response to the review and not have to a/b the two patches.

So, the path forward there is:
a) break up the patch into smaller patches
b) resolve the style/etc stuff already flagged (for the entire set of patches).
c) put up the patches for review
d) respond to reviews of a patch by posting both an interdiff and an updated patch
e) If updating patch N causes bitrot in a later patch in the queue, update those patches (bitrot changes generally do not require re-review if they're "obvious"; if they're already up for review just move the r? to the new patch).

You may want to upload the first patch you separate out from the mega-patch before/while finishing the decomposition, to verify you're on the right path and are producing reviewable patches.  That first patch could be put up for feedback or review.

When decomposing, do frequent compiles and/or tests to verify you haven't broken something.

I don't know the git equivalent, but in mercurial, "hg qcrecord" is great for taking a big patch and splitting it into two.  (hg qref -X .; hg qcrecord piece_1; hg qnew remainder) - this creates a new smaller patch pulled out of the current one, and leaves everything else in remainder.  (Note you end up with an empty patch in the queue as well, after being sure it's empty you can hg qdelete it.)

I also recommend if doing a lot of work in mercurial queues to turn on a repository for queue changes: "hg init --mq", and add this to ~/.hgrc 

[mqext]
mqcommit = auto

So any change to your queue causes an auto-commit.  Alternatively, you can "hg commit --mq" periodically at "good" points, but (like in git) you have to remember to do so.
Flags: needinfo?(mreavy)

Comment 14

5 years ago
I think if you want it broken up, Mozilla, not Cisco needs to do that. It's not that I disagree with your comments but this is a large block of new functionality that does require a bunch of changes to happen simultaneously to get it to all work. How to deal with all that and the review systems is pretty hard for us. We've written the code - help us get it landed.
So, the first thing to do to turn this into something reviewable and then landable is to list the different areas/features being changed.  So let's start that here in this bug, and then we can figure out how to split the patch up into these buckets.

(also note that splitting is also because different parts need to be reviewed by different people/groups - UI changes by UI/Firefox team; core webrtc changes by me or other webrtc people; etc).

Also, wherever possible, we want to keep webrtc.org upliftable changes separate from mozilla-specific changes.

So, off a quick look at the files:
X11 sharing support
UI (probably needs more splitting)
base desktop capture support
  mouse/cursor capture support
  multi-monitor support
    mac/windows/x11 (*may not need separate patches)
app capture support
  UI for app capture
getUserMedia support
  app capture

Since you know the patch better, how does this sound? Please feel free to propose improvements to that.
Flags: needinfo?(vagouzhou)

Comment 16

5 years ago
Sorry, busy on company project ,and don't contribute it 1+ month :(
https://github.com/vagouzhou/gecko-dev/commits/master
I will start code again for this the week after next. 

I already group source code files in Comment 6: https://bugzilla.mozilla.org/show_bug.cgi?id=983504#c6
Each file is for what.

mercurial sounds great tool,but I am not familiar with it yet. It is possible to review code in github? 

Little increment patch is great . But for source code dependent reason, initial screen share pipeline patch need all of these. it indeed is little huge :)
We can deliver little increment patch in future.
Flags: needinfo?(vagouzhou)

Comment 17

5 years ago
In addition , in order to deliver this function to user ASAP , cross function engineer can do parallel as Comment 7 :

#  Support to share individual application, now just share entire desktop/display. And multi-monitor support. [I will handle it]
     - list running applications mac/windows/linux
     - application sharing mac/windows/linux
     - mulit-monitor support mac/windows/linux, maybe can deliver next version. 
#  openh264 codec also need be optimized for screen content sharing to get clear text. [OpenH264 ??]
#  screen sharing indicator GUI in firefox browser. So user know that he/she is sharing screen.[UI/Firefox team ??]
#  and application list GUI. I just write simple GUI in order to I can do testing,maybe need redesign/rewrite.[UI/Firefox team ??]
#  Integration screen sharing  with openh264 codec ,now it is VP8. [GMP?, maybe just testing]
Unfortunately the patches seem to be based on a 3 month old copy of the Gecko source and haven't been rebased. I did a merge to todays mozilla-central, and I think I got most of it but there's been a load of changes to WebRTC in those 3 months and the result will need some fixes to even build.

My working repo, including the merge, is here:
https://github.com/gcp/gecko-dev/tree/screenshare
Patch for those who prefer Mercurial.
>Most code is merge from latest WebRTC desktop capture code , maybe change few to make building 

FWIW, because m-c has updated webrtc.org code compared to the 3-months old version that was the base, the patch almost halved in size after the rebase, so that's good news.
Still doesn't link.
Attachment #8448036 - Attachment is obsolete: true

Comment 22

5 years ago
Hi Gian, it seems that you incorrect merge some code in your fork. 
I merge latest mozilla/gecko-dev into my fork,including some your changed code. please refer to my fork 
https://github.com/vagouzhou/gecko-dev/commits/master.

In addition, it seems that "media constraint" logic is re-design in latest code.
It cause that webrtcUI.jsm logic is incorrect, cannot get mozMediaSource. 
Try update some logic, screen sharing is not workable yet. I will check it tomorrow. 

Hi Gian,to speed up process , if possible would you ask engineer who refactoring this part code to help it.
I noticed you had merge fixes in your branch and already pulled them.

There's a fix wrt the logging and an attempt at fixing the constraints stuff on my github, but indeed it's hard without knowing much about this subsystem, and I'm not sure if my fixes are correct. I'll see who can assist with this.

Comment 24

5 years ago
Just review latest code , Jan-Ivar want to remove mandatory for "Bug 997365 - end support for backwards-compatible facingMode constraint on mobile"
====================================================
dictionary MediaTrackConstraints : MediaTrackConstraintSet {
    sequence<DOMString> require;
    sequence<MediaTrackConstraintSet> advanced;

    // mobile-only backwards-compatibility for facingMode
    MobileLegacyMediaTrackConstraintSet mandatory;
    sequence<MobileLegacyMediaTrackConstraintSet> _optional;
};

// TODO(jib): Remove in 6+ weeks (Bug 997365)
dictionary MobileLegacyMediaTrackConstraintSet {
    VideoFacingModeEnum facingMode;
};
==================================================== 
we still need mandatory  for screen sharing.

mediaConstraints = {
video: {
mandatory:{mozMediaSource:'screen',
maxWidth:screen.availWidth,
maxHeight:screen.availHeight},
optional:[]
}
};


Hi Jan-Ivar, would you help check it. 
Change MobileLegacyMediaTrackConstraintSet  into MandatoryMediaTrackConstraintSet , and don’t remove it ?

Comment 25

5 years ago
I add mozMediaSource in mandatory constraint. 

// TODO(jib): Remove in 6+ weeks (Bug 997365)
dictionary MobileLegacyMediaTrackConstraintSet {
    VideoFacingModeEnum facingMode;
    MozMediaSourceEnum mozMediaSource;
};

Now , screen sharing merge with latest mozilla/gecko-dev code is workable now.

Comment 26

5 years ago
Hi, Gian-Carlo
I tested on windows/mac/linux , all are workable. You can pull changes to your fork and help create code diff patch for code-reviewing, Thanks.
Assignee

Comment 27

5 years ago
I can appreciate the machinations that standards bodies go through, and will not condemn or condone the removal of "mandatory".

I have a working copy that supports both the now-legacy 'mandatory' and closer-to-spec variants.  Will next help work on breaking this apart into digestible diffs.
(In reply to Matthew Miller [:linuxwolf] from comment #27)
> I can appreciate the machinations that standards bodies go through, and will
> not condemn or condone the removal of "mandatory".

There's still time to comment on it http://lists.w3.org/Archives/Public/public-media-capture

> I have a working copy that supports both the now-legacy 'mandatory' and
> closer-to-spec variants.  Will next help work on breaking this apart into
> digestible diffs.

Great to hear!

(In reply to vagouzhou from comment #25)
> I add mozMediaSource in mandatory constraint. 
> 
> // TODO(jib): Remove in 6+ weeks (Bug 997365)
> dictionary MobileLegacyMediaTrackConstraintSet {
>     VideoFacingModeEnum facingMode;
>     MozMediaSourceEnum mozMediaSource;
> };

I haven't seen the rest of the patch but note that this particular dictionary is called "MobileLegacy" and that mandatory for facingMode here is only supported on mobile platforms, not desktop. Just letting you know.

As I mentioned in email, what's currently in firefox (until Bug 1033885 is fixed) is the early-May spec:

var constraints = 
{ 
   require: ["mozMediaSource"], 
   mozMediaSource: "screen", 
};
Some testing shows that on Linux screen capture works, but application capture just shows the list of cameras. Is this expected?
Assignee

Comment 30

5 years ago
Application capture is still a work in progress, as far as I know.

However, it should have filtered out all the cameras.  My branch at https://github.com/linuxwolf/gecko-dev definitely does that.  I think I found some logic hiccups, and fixed those in my branch.
Thanks, I confirm it's working (screen sharing) from your branch, and have merged with mine.
Posted patch Patch 1. Build system changes (obsolete) — Splinter Review
Attachment #8418448 - Attachment is obsolete: true
Attachment #8448844 - Attachment is obsolete: true
Attachment #8418448 - Flags: feedback?(rjesup)
Fixed:
- OpenSSL workaround is no longer needed, was properly fixed in m-c already
- Don't compile on Android and don't try to instantiate Screen Capture devices. (I noticed there are null implementations. Obviously they don't work properly)
- Style fixes

Observation:
- Some of the files moved into conditionals probably don't need to be due to gyp's automatic platform selection thing...though I certainly don't mind not relying on it.
Attachment #8451118 - Attachment is obsolete: true
Attachment #8451690 - Flags: review?(ted)
Attachment #8451120 - Attachment is obsolete: true
Attachment #8451701 - Flags: review?(dolske)
Attachment #8451121 - Attachment is obsolete: true
Attachment #8451709 - Flags: review?(martin.thomson)
Attachment #8451709 - Flags: feedback?(jib)
Attachment #8451123 - Attachment is obsolete: true
Attachment #8451727 - Flags: review?(rjesup)
Attachment #8451125 - Attachment is obsolete: true
Attachment #8451733 - Flags: review?(gpascutto)
Attachment #8451733 - Flags: feedback?(rjesup)
Posted patch Patch 9. Newly added files (obsolete) — Splinter Review
Not cleaned or split up yet.
>To reuse video engine for screen sharing , it one adapter that bridge Video capture API into Desktop 
>Capture interfaces. (WebRTC desktop capture module that is maintained by chrome team ,not WebRTC 
>team, we can notice it that has some different code style and interface style. 

I am not sure I understand what this is saying. Does this mean those files originate from the Chrome source? (I couldn't find them, but my tree is a little out of date)

>These are for device(process, display) , it is not real implement. Need not review code now, 

All code that needs to land needs to be reviewed. Some files are empty shells, others have some real code in it even if it's null.

I think I need to split the desktop_capture_impl things from the "empty" parts of that patch.
Comment on attachment 8451709 [details] [diff] [review]
Patch 3. v2 Modify constraints for screen sharing

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

Firstly, for this patch:

I'm not comfortable that the interaction between the UI and MediaManager.cpp has been properly articulated here.  The code is confused about whether it is one of three different types of source (screen, application, or camera) and whether it's just a video source.  That needs to be much clearer.  I see elements of similar confusion in the other patches as well.

I think that if we are planning to treat sharing as a video source, then we treat them as a video source and the only relevant distinctions are: what sources can be selected; and how those sources are connected up.  For example, this patch seems to enumerate all three sources for selection unnecessarily, relying on constraint filtering to ensure that they don't appear.  The drawback with this is that you can end up in a situation where users are presented with a long list of applications intermingled with cameras and desktops.  That's going to be a confusing experience (though I'll concede that a better user experience is one thing that this patch does not need to address.)

On a broader scale:

It's not clear that desktop sharing is a desirable feature.  We should consider the addition of application sharing and desktop sharing separately.  Particularly when Firefox is visible on most desktops.

I had a quick look through most of the patches and it doesn't seem like Firefox being blacklisted from application sharing.  That's a security problem and - as the spec we've informally followed says - we should not permit that.  

We have tab-sharing code that can be used for Firefox and we should be using that when Firefox is selected.  That seems like separable work for the moment, particularly since it has far more serious security consequences.

::: dom/media/MediaManager.cpp
@@ +1031,5 @@
>        VideoTrackConstraintsN constraints(GetInvariant(mConstraints.mVideo));
>        ScopedDeletePtr<SourceSet> sources (GetSources(backend, constraints,
>            &MediaEngine::EnumerateVideoDevices));
>  
> +      // XXX  What should we do for screen sharing here?  (Bug NNNNNN)

This seems like a problem.  The entire point of this is to link what the user selects to actually choosing the matching source for the media stream.

Since it seems like you are relying on SetVideoDevice for this purpose (and here I'm not sure that this is the right way to integrate with the UX), the enumerated sources need to consider the other source types.

Here again, it's not clear that this is the right architecture.

@@ +1206,5 @@
> +      {
> +        ScopedDeletePtr<SourceSet> s(GetSources(backend, constraints,
> +          &MediaEngine::EnumerateApplicationDevices));
> +        final->MoveElementsFrom(*s);
> +      }

This shouldn't be enumerating all the devices, it should be looking at the value of the source type constraint and having that be used as the basis for enumeration.

Having separate functions on MediaEngine for enumeration here (as opposed to using the source type) doesn't seem right to me.

::: dom/media/nsIDOMNavigatorUserMedia.idl
@@ +12,5 @@
>    readonly attribute DOMString type;
>    readonly attribute DOMString name;
>    readonly attribute DOMString id;
>    readonly attribute DOMString facingMode;
> +  readonly attribute DOMString mozMediaSource;

Prefixing this seems unnecessary.

::: dom/webidl/Constraints.webidl
@@ +17,5 @@
> +enum MozMediaSourceEnum {
> +    "camera",
> +    "screen",
> +    "application"
> +};

This will need review from a DOM peer.

::: dom/webidl/MediaStreamTrack.webidl
@@ +21,5 @@
>  
>  // TODO(jib): Remove in 6+ weeks (Bug 997365)
>  dictionary MobileLegacyMediaTrackConstraintSet {
>      VideoFacingModeEnum facingMode;
> +    MozMediaSourceEnum mozMediaSource;

As above.
Attachment #8451709 - Flags: review?(martin.thomson) → review-
(In reply to Gian-Carlo Pascutto [:gcp] from comment #40)
> Created attachment 8451701 [details] [diff] [review]
> Patch 2. v2. Add Screen Sharing UI

How does this intersect with the planning in bug 1031424?
Flags: needinfo?(gpascutto)
Assignee

Comment 48

5 years ago
(In reply to Justin Dolske [:Dolske] from comment #47)
> (In reply to Gian-Carlo Pascutto [:gcp] from comment #40)
> > Created attachment 8451701 [details] [diff] [review]
> > Patch 2. v2. Add Screen Sharing UI
> 
> How does this intersect with the planning in bug 1031424?

The code is an initial prototype; enough to write screen sharing demo code against.
Comment on attachment 8451709 [details] [diff] [review]
Patch 3. v2 Modify constraints for screen sharing

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

::: dom/webidl/MediaTrackConstraintSet.webidl
@@ +25,5 @@
>      ConstrainLongRange width;
>      ConstrainLongRange height;
>      ConstrainDoubleRange frameRate;
>      ConstrainVideoFacingMode facingMode;
> +    ConstrainMediaSource mozMediaSource;

Make this:

ConstrainMediaSource mozMediaSource = "camera";

and I think this could work with your other selection code.

A default value here means all uses to date as well as all uses that don't explicitly provide a different value will get mediaSource: "camera" implicitly, which will filter out the screensharing devices by default, which is what we want.

This means there'll effectively be two "sub-pools" of devices here that users will never see a union of, which is what is desired.

So the thing that Martin worries about where both kinds of devices appear in the same UX list should never happen.

This is the idea anyway. I'll look at the rest of the patch with more scrutiny in a little bit.
Comment on attachment 8451709 [details] [diff] [review]
Patch 3. v2 Modify constraints for screen sharing

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

Looks good with some critical nits.

::: content/media/webrtc/MediaTrackConstraints.h
@@ +87,1 @@
>      // Reminder: add handling for new constraints both here & SatisfyConstraintSet

You're missing the constraint-test itself in SatisfyConstraintSet http://mxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp#716

::: dom/media/MediaManager.cpp
@@ +1031,5 @@
>        VideoTrackConstraintsN constraints(GetInvariant(mConstraints.mVideo));
>        ScopedDeletePtr<SourceSet> sources (GetSources(backend, constraints,
>            &MediaEngine::EnumerateVideoDevices));
>  
> +      // XXX  What should we do for screen sharing here?  (Bug NNNNNN)

If we add a webidl "camera" default then the inherent separation logic it produces should provide the desired selection automatically here. So I would remove this comment unless there is something I'm missing.

@@ +1206,5 @@
> +      {
> +        ScopedDeletePtr<SourceSet> s(GetSources(backend, constraints,
> +          &MediaEngine::EnumerateApplicationDevices));
> +        final->MoveElementsFrom(*s);
> +      }

Again should be OK with a default.

::: dom/media/nsIDOMNavigatorUserMedia.idl
@@ +12,5 @@
>    readonly attribute DOMString type;
>    readonly attribute DOMString name;
>    readonly attribute DOMString id;
>    readonly attribute DOMString facingMode;
> +  readonly attribute DOMString mozMediaSource;

I agree. We'll get internal pushback on prefixing now, so better to remove it.

::: dom/webidl/MediaStreamTrack.webidl
@@ +21,5 @@
>  
>  // TODO(jib): Remove in 6+ weeks (Bug 997365)
>  dictionary MobileLegacyMediaTrackConstraintSet {
>      VideoFacingModeEnum facingMode;
> +    MozMediaSourceEnum mozMediaSource;

This dictionary is for legacy only on mobile, specifically FacingMode only. We should not be adding anything here.

::: dom/webidl/MediaTrackConstraintSet.webidl
@@ +39,2 @@
>  // TODO: Bug 767924 sequences in unions
>  //typedef (VideoFacingModeEnum or sequence<VideoFacingModeEnum>) ConstrainVideoFacingMode;

Please add the following here:
//typedef (MediaSourceEnum or sequence<MediaSourceEnum>) ConstrainMediaSource;
Attachment #8451709 - Flags: feedback?(jib) → feedback+
I'll note (for the record) that we want to have a a pref governing this for now.  This seems like critical infrastructure pieces for later work to enable application sharing.  Landing that without some of the things I mentioned earlier would be bad, but we can manage that if it is preffed off.

You might also like to look at https://wiki.mozilla.org/WebAPI/ExposureGuidelines#Intent_to_Implement as well, just to get all the procedural stuff out of the way.

Comment 52

5 years ago
(In reply to Gian-Carlo Pascutto [:gcp] from comment #45)
> >To reuse video engine for screen sharing , it one adapter that bridge Video capture API into Desktop 
> >Capture interfaces. (WebRTC desktop capture module that is maintained by chrome team ,not WebRTC 
> >team, we can notice it that has some different code style and interface style. 
> 
> I am not sure I understand what this is saying. Does this mean those files
> originate from the Chrome source? (I couldn't find them, but my tree is a
> little out of date)
> 
[Vagou] current code implement is to reuse video engine pipeline for sharing and video, screen capture interface is different with video capture interface, so we need wrapper screen capture interface as video capture.  desktop_capture_impl just a interface adaptor.

For Chrome, screen capture module just is added in webrtc.org. all other screen sharing code implement in browser layer ,don't reuse video engine pipeline.
in my opinion , webrtc.org code should include screen-sharing engine code logic as audio/video. or reuse video engine.

> >These are for device(process, display) , it is not real implement. Need not review code now, 
> 
> All code that needs to land needs to be reviewed. Some files are empty
> shells, others have some real code in it even if it's null.
[Vagou] application sharing/multi display support are not ready yet. 

> 
> I think I need to split the desktop_capture_impl things from the "empty"
> parts of that patch.
[Vagou] yes , we can break it into 2 patch , one for application sharing , another is for desktop/dispaly sharing.

Comment 53

5 years ago
"
>For
> example, this patch seems to enumerate all three sources for selection
> unnecessarily, relying on constraint filtering to ensure that they don't
> appear. 
"
[Vagou]enumerate all source include video/audio in old code base, we need refactoring it.
refer to comment #6 , "getUserMedia API constraint update for screen sharing with mMozMediaSource of mandatory. Refer to http://lists.w3.org/Archives/Public/public-media-capture/2014Apr/att-0189/screensharing.html
and also Enumerate source for screen sharing not just audio/video source . 
BTW, it has one bug is that it always Enumerate all source don’t count on input constraint , I told Jan-Ivar, he has one bug to address it. https://bugzilla.mozilla.org/show_bug.cgi?id=916012
"

(In reply to Martin Thomson [:mt] from comment #46)
> Comment on attachment 8451709 [details] [diff] [review]
> Patch 3. v2 Modify constraints for screen sharing
> 
> Review of attachment 8451709 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Firstly, for this patch:
> 
> I'm not comfortable that the interaction between the UI and MediaManager.cpp
> has been properly articulated here.  The code is confused about whether it
> is one of three different types of source (screen, application, or camera)
> and whether it's just a video source.  That needs to be much clearer.  I see
> elements of similar confusion in the other patches as well.
> 
> I think that if we are planning to treat sharing as a video source, then we
> treat them as a video source and the only relevant distinctions are: what
> sources can be selected; and how those sources are connected up.  For
> example, this patch seems to enumerate all three sources for selection
> unnecessarily, relying on constraint filtering to ensure that they don't
> appear.  The drawback with this is that you can end up in a situation where
> users are presented with a long list of applications intermingled with
> cameras and desktops.  That's going to be a confusing experience (though
> I'll concede that a better user experience is one thing that this patch does
> not need to address.)
> 
> On a broader scale:
> 
> It's not clear that desktop sharing is a desirable feature.  We should
> consider the addition of application sharing and desktop sharing separately.
> Particularly when Firefox is visible on most desktops.
> 
> I had a quick look through most of the patches and it doesn't seem like
> Firefox being blacklisted from application sharing.  That's a security
> problem and - as the spec we've informally followed says - we should not
> permit that.  
> 
> We have tab-sharing code that can be used for Firefox and we should be using
> that when Firefox is selected.  That seems like separable work for the
> moment, particularly since it has far more serious security consequences.
> 
> ::: dom/media/MediaManager.cpp
> @@ +1031,5 @@
> >        VideoTrackConstraintsN constraints(GetInvariant(mConstraints.mVideo));
> >        ScopedDeletePtr<SourceSet> sources (GetSources(backend, constraints,
> >            &MediaEngine::EnumerateVideoDevices));
> >  
> > +      // XXX  What should we do for screen sharing here?  (Bug NNNNNN)
> 
> This seems like a problem.  The entire point of this is to link what the
> user selects to actually choosing the matching source for the media stream.
> 
> Since it seems like you are relying on SetVideoDevice for this purpose (and
> here I'm not sure that this is the right way to integrate with the UX), the
> enumerated sources need to consider the other source types.
> 
> Here again, it's not clear that this is the right architecture.
> 
> @@ +1206,5 @@
> > +      {
> > +        ScopedDeletePtr<SourceSet> s(GetSources(backend, constraints,
> > +          &MediaEngine::EnumerateApplicationDevices));
> > +        final->MoveElementsFrom(*s);
> > +      }
> 
> This shouldn't be enumerating all the devices, it should be looking at the
> value of the source type constraint and having that be used as the basis for
> enumeration.
> 
> Having separate functions on MediaEngine for enumeration here (as opposed to
> using the source type) doesn't seem right to me.
> 
> ::: dom/media/nsIDOMNavigatorUserMedia.idl
> @@ +12,5 @@
> >    readonly attribute DOMString type;
> >    readonly attribute DOMString name;
> >    readonly attribute DOMString id;
> >    readonly attribute DOMString facingMode;
> > +  readonly attribute DOMString mozMediaSource;
> 
> Prefixing this seems unnecessary.
> 
> ::: dom/webidl/Constraints.webidl
> @@ +17,5 @@
> > +enum MozMediaSourceEnum {
> > +    "camera",
> > +    "screen",
> > +    "application"
> > +};
> 
> This will need review from a DOM peer.
> 
> ::: dom/webidl/MediaStreamTrack.webidl
> @@ +21,5 @@
> >  
> >  // TODO(jib): Remove in 6+ weeks (Bug 997365)
> >  dictionary MobileLegacyMediaTrackConstraintSet {
> >      VideoFacingModeEnum facingMode;
> > +    MozMediaSourceEnum mozMediaSource;
> 
> As above.
(In reply to vagouzhou from comment #53)
> BTW, it has one bug is that it always Enumerate all source don’t count on
> input constraint , I told Jan-Ivar, he has one bug to address it.
> https://bugzilla.mozilla.org/show_bug.cgi?id=916012
> "

Sorry I didn't catch this in comment 6. The bug you reference is something else. I think the fix here is what I suggest in comment 49:

> ConstrainMediaSource mozMediaSource = "camera";

I think if we add that then we'll see the enumeration filter things out correctly.
Attachment #8451727 - Flags: review?(rjesup) → review+
Matthew already clarified that the UI is temporary/there to allow testing.
Flags: needinfo?(gpascutto)
Attachment #8451126 - Attachment is obsolete: true
Attachment #8452267 - Flags: review?(rjesup)
Attachment #8452267 - Flags: feedback?(gpascutto)
Comment on attachment 8452267 [details] [diff] [review]
Patch 6. v2 WebRTC ViE Capturer changes

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

r+, modulo .gitignore and please consider if the CaptureDeviceType should just return an enum (none, screen, application, window).

::: .gitignore
@@ +20,5 @@
>  /config.log
>  /.clang_complete
>  /mach.ini
> +/build.bat
> +/gecko-dev.*

Did you mean to change this??

::: media/webrtc/trunk/webrtc/video_engine/include/vie_capture.h
@@ +54,5 @@
> +    CaptureDeviceType()
> +      : isScreenDevice(false), isApplication(false){
> +    }
> +    CaptureDeviceType(bool isScreenDevice, bool isApplication)
> +      : isScreenDevice(isScreenDevice), isApplication(isApplication){

What about window capture?  Should this instead be an enum?

::: media/webrtc/trunk/webrtc/video_engine/vie_capturer.cc
@@ +168,5 @@
>  int32_t ViECapturer::Init(const char* device_unique_idUTF8,
>                            uint32_t device_unique_idUTF8Length) {
>    assert(capture_module_ == NULL);
> +  bool is_screen_sharing = config_.Get<CaptureDeviceType>().isScreenDevice;
> +  bool is_application = config_.Get<CaptureDeviceType>().isApplication;

Should this be an enum?
Attachment #8452267 - Flags: review?(rjesup) → review+
Assignee

Comment 58

5 years ago
This is an initial cut at addressing Martin's concerns.  Is this approach acceptable?
Attachment #8452339 - Flags: feedback?
Attachment #8452339 - Flags: feedback? → feedback?(martin.thomson)
Attachment #8451709 - Attachment is obsolete: true
Comment on attachment 8452339 [details] [diff] [review]
Patch 3. v3 Modify constraints for screen sharing

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

::: content/media/webrtc/MediaTrackConstraints.h
@@ +86,5 @@
>      Triage(Kind::Height).mHeight = mHeight;
>      Triage(Kind::FrameRate).mFrameRate = mFrameRate;
> +
> +    // mozMediaSource is a little different, as it's always considered required
> +    mRequired.mMozMediaSource = mMozMediaSource;

This violates the constraints model, and seems unnecessary. With the "camera" as WebIDL default there WILL be a required key, no worries, so no need for code exceptions here.

In fact, if someone also provides an optional MediaSource in the advanced constraints array to constrain more narrowly than the initial required constraint (which could be an array of multiple-choice values) then that MUST work, and currently wont the way it is here. Please change to work like all other constraint.
Attachment #8452339 - Flags: review-
Assignee

Comment 60

5 years ago
(In reply to Jan-Ivar Bruaroey [:jib] from comment #59)
> Comment on attachment 8452339 [details] [diff] [review]
> Patch 3. v3 Modify constraints for screen sharing
> 
> Review of attachment 8452339 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/webrtc/MediaTrackConstraints.h
> @@ +86,5 @@
> >      Triage(Kind::Height).mHeight = mHeight;
> >      Triage(Kind::FrameRate).mFrameRate = mFrameRate;
> > +
> > +    // mozMediaSource is a little different, as it's always considered required
> > +    mRequired.mMozMediaSource = mMozMediaSource;
> 
> This violates the constraints model, and seems unnecessary. With the
> "camera" as WebIDL default there WILL be a required key, no worries, so no
> need for code exceptions here.
> 
> In fact, if someone also provides an optional MediaSource in the advanced
> constraints array to constrain more narrowly than the initial required
> constraint (which could be an array of multiple-choice values) then that
> MUST work, and currently wont the way it is here. Please change to work like
> all other constraint.


My concern is that a given video device only has one source (at least today), and the implementation filters devices by source before reaching SatisfyConstraintSet.  This means that an API user *must* include a "require" field, or they get an empty list.  It feels very awkward and error-prone to me.

I can change it to get all devices, and rely on filtering to get what the user intended.  However, this feels like it will be overkill when app-capture is fully implemented (potentially dozens of "devices").

The other suggestion is to skip mediaSource filtering in SatisfyConstraintSet.
Comment on attachment 8452339 [details] [diff] [review]
Patch 3. v3 Modify constraints for screen sharing

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

::: dom/media/MediaManager.cpp
@@ +1039,5 @@
>        VideoTrackConstraintsN constraints(GetInvariant(mConstraints.mVideo));
> +      ScopedDeletePtr<SourceSet> sources;
> +
> +      switch (constraints.mMozMediaSource) {
> +        case dom::MozMediaSourceEnum::Camera:

Change this case to default and your optimization here should be ok. No need for the required hack.

Personally I'd prefer gathering all devices and let the constraints do the filter, for easier code-maintenance, but I could see there perhaps being some overhead involved with gathering devices from different subsystems that this may avoid.
Attachment #8451128 - Attachment is obsolete: true
Attachment #8452412 - Flags: review?(gpascutto)
Comment on attachment 8452339 [details] [diff] [review]
Patch 3. v3 Modify constraints for screen sharing

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

::: dom/media/MediaManager.cpp
@@ +1231,5 @@
> +              &MediaEngine::EnumerateApplicationDevices);
> +          break;
> +        default:
> +          s = new SourceSet;
> +      }

I'd push this mess down another level:

s = GetSources(backend, constraints, &MediaEngine::EnumerateVideoDevices, constraints.mMozMediaSource, mLoopbackVideoDevice.get());

Where MediaEngine::EnumerateVideoDevices is changed to permit a new source type argument.

::: dom/webidl/MediaTrackConstraintSet.webidl
@@ +25,5 @@
>      ConstrainLongRange width;
>      ConstrainLongRange height;
>      ConstrainDoubleRange frameRate;
>      ConstrainVideoFacingMode facingMode;
> +    ConstrainMediaSource mozMediaSource = "camera";

I'd put this at the same level as "audio", "video" and "peerIdentity" in I'd put this at the same level as "audio", "video" and "peerIdentity", as I described in my email.  Then it's not a constraint., as I described in my email.  Then it's not a constraint with all that baggage and you can get greater determinism.

See dom/webidl/MediaStream.webidl
Attachment #8452339 - Flags: feedback?(martin.thomson) → feedback-
Attachment #8451738 - Attachment is obsolete: true
Attachment #8452414 - Flags: review?(rjesup)
Attachment #8452414 - Flags: feedback?(gpascutto)
Attachment #8452417 - Flags: review?(gpascutto)
Attachment #8452417 - Flags: feedback?(rjesup)
Assignee

Comment 66

5 years ago
This refactor is a result of refactoring the constraints logic.

Have not addressed all the other comments yet, but this was a significant amount of work in and of itself.
Attachment #8451727 - Attachment is obsolete: true
Attachment #8452751 - Flags: review?(rjesup)
Attachment #8452751 - Flags: review?(martin.thomson)
Assignee

Comment 67

5 years ago
Refactors selection and enumeration to match discussion via IRC and email. See attachment 8452751 [details] [diff] [review] for the bulk of the work.

This also adds a pref guard for screen sharing.

Still need to de-prefix (moz)MediaSource.
Attachment #8452339 - Attachment is obsolete: true
Attachment #8452754 - Flags: review?(martin.thomson)
Attachment #8452754 - Flags: review?(jib)
Comment on attachment 8452414 [details] [diff] [review]
Patch 9. v2 DesktopCaptureImpl

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

r+ (with lots of nits!) if-and-only-if the cs/cs2 locks aren't needed where I pointed it out.  If a follow-on is needed, just post the interdiff please against this set of patches.

::: media/webrtc/trunk/webrtc/video_engine/desktop_capture_impl.cc
@@ +28,5 @@
> +#include "webrtc/modules/desktop_capture/desktop_capture_options.h"
> +
> +namespace webrtc {
> +
> +ScreenDeviceInfoImpl::ScreenDeviceInfoImpl(const int32_t id) {

Isn't this missing " : _id(id)" ?

@@ +52,5 @@
> +                                            char* productUniqueIdUTF8,
> +                                            uint32_t productUniqueIdUTF8Length) {
> +
> +  DesktopDisplayDevice desktopDisplayDevice;
> +  if(desktop_device_info_->getDesktopDisplayDeviceInfo(deviceNumber,

space after if's

@@ +55,5 @@
> +  DesktopDisplayDevice desktopDisplayDevice;
> +  if(desktop_device_info_->getDesktopDisplayDeviceInfo(deviceNumber,
> +                                                       desktopDisplayDevice) == 0){
> +
> +    const char * deviceName = desktopDisplayDevice.getDeivceName();

const char *deviceName or const char* deviceName (I prefer the first)
Also: getDeviceName()

@@ +60,5 @@
> +    if(deviceNameLength > 0 && deviceNameUTF8 && deviceName){
> +      memset(deviceNameUTF8,0,deviceNameLength);
> +      memcpy(deviceNameUTF8,
> +             deviceName,
> +             strlen(deviceName));

This sequence is rather problematic.  There are better ways to copy a null-terminated string.  If it's not, then comment on what the requirement is and implement to that requirement.  What if strlen(deviceName) is == deviceNameLength?  Or greater than it!

@@ +68,5 @@
> +    if(deviceUniqueIdUTF8Length > 0 && deviceUniqueIdUTF8 && deviceUniqueId){
> +      memset(deviceUniqueIdUTF8, 0, deviceUniqueIdUTF8Length);
> +      memcpy(deviceUniqueIdUTF8,
> +             deviceUniqueId,
> +             strlen(deviceUniqueId));

Ditto

@@ +107,5 @@
> +  return 0;
> +}
> +
> +int32_t ScreenDeviceInfoImpl::GetOrientation(
> +                                             const char* deviceUniqueIdUTF8,

Again, fix all these extraneous line insertions

@@ +112,5 @@
> +                                             VideoCaptureRotation& orientation) {
> +  return 0;
> +}
> +
> +AppDeviceInfoImpl::AppDeviceInfoImpl(const int32_t id) {

missing " : _id(id)" ?

@@ +143,5 @@
> +    if(deviceNameLength > 0 && deviceNameUTF8 && deviceName){
> +      memset(deviceNameUTF8, 0, deviceNameLength);
> +      memcpy(deviceNameUTF8,
> +             deviceName,
> +             strlen(deviceName));

Same comments.  Perhaps make a static function you can use for all these?

@@ +167,5 @@
> +                                                           const char* dialogTitleUTF8,
> +                                                           void* parentWindow,
> +                                                           uint32_t positionX,
> +                                                           uint32_t positionY) {
> +  return 0;

Purposely empty?

@@ +241,5 @@
> +int32_t DesktopCaptureImpl::Init(const char* uniqueId,
> +                                 const bool bIsApp) {
> +  if(bIsApp) {
> +    AppCapturer *pAppCapturer =AppCapturer::Create();
> +    if(pAppCapturer==nullptr) return -1;

if possible, make that "if (pAppCapturer)".  ALso: braces are mandatory, and don't 1-line it.

@@ +243,5 @@
> +  if(bIsApp) {
> +    AppCapturer *pAppCapturer =AppCapturer::Create();
> +    if(pAppCapturer==nullptr) return -1;
> +
> +    ProcessId processid = 0;//uniqueId

space(s) before //
Apply this across the patch(es)

@@ +247,5 @@
> +    ProcessId processid = 0;//uniqueId
> +    pAppCapturer->SelectApp(processid);
> +
> +    //desktop_capturer_.reset(pAppCapturer);
> +    //mouse_cursor_monitor_.reset(MouseCursorMonitor::create);

If there's a reason for commented out code, include the reason in a comment

@@ +249,5 @@
> +
> +    //desktop_capturer_.reset(pAppCapturer);
> +    //mouse_cursor_monitor_.reset(MouseCursorMonitor::create);
> +    MouseCursorMonitor * pMouseCursorMonitor = MouseCursorMonitor::CreateForScreen(webrtc::DesktopCaptureOptions::CreateDefault(), webrtc::kFullDesktopScreenId);
> +    desktop_capturer_cursor_composer_.reset(new DesktopAndCursorComposer(pAppCapturer,pMouseCursorMonitor));

space after commas
Apply across patch(es)

@@ +252,5 @@
> +    MouseCursorMonitor * pMouseCursorMonitor = MouseCursorMonitor::CreateForScreen(webrtc::DesktopCaptureOptions::CreateDefault(), webrtc::kFullDesktopScreenId);
> +    desktop_capturer_cursor_composer_.reset(new DesktopAndCursorComposer(pAppCapturer,pMouseCursorMonitor));
> +  } else {
> +    ScreenCapturer *pScreenCapturer = ScreenCapturer::Create();
> +    if (pScreenCapturer == nullptr) return -1;

if possible, make that "if (pScreenCapturer)".  ALso: braces are mandatory, and don't 1-line it.

@@ +259,5 @@
> +    pScreenCapturer->SelectScreen(screenid);
> +    pScreenCapturer->SetMouseShapeObserver(this);
> +
> +    //desktop_capturer_.reset(pScreenCapturer);
> +    MouseCursorMonitor * pMouseCursorMonitor = MouseCursorMonitor::CreateForScreen(webrtc::DesktopCaptureOptions::CreateDefault(), screenid);

space on one side of *, not both

@@ +262,5 @@
> +    //desktop_capturer_.reset(pScreenCapturer);
> +    MouseCursorMonitor * pMouseCursorMonitor = MouseCursorMonitor::CreateForScreen(webrtc::DesktopCaptureOptions::CreateDefault(), screenid);
> +    desktop_capturer_cursor_composer_.reset(new DesktopAndCursorComposer(pScreenCapturer,pMouseCursorMonitor));
> +  }
> +  //desktop_capturer_->Start(this);

Why is this commented out?

@@ +329,5 @@
> +    _captureAlarm(Cleared),
> +    _setCaptureDelay(0),
> +    _dataCallBack(NULL),
> +    _captureCallBack(NULL),
> +  _lastProcessFrameCount(TickTime::Now()),

indent change

@@ +335,5 @@
> +  last_capture_time_(TickTime::MillisecondTimestamp()),
> +  delta_ntp_internal_ms_(
> +                         Clock::GetRealTimeClock()->CurrentNtpInMilliseconds() -
> +                         TickTime::MillisecondTimestamp()),
> +  capturer_thread_(*ThreadWrapper::CreateThread(Run, this, kHighPriority, "ScreenCaptureThread")){

space before {, and indent code following
(The confusing layout here is part of why I personally strongly prefer left-edge function braces)

@@ +354,5 @@
> +  delete &_callBackCs;
> +  delete &_apiCs;
> +
> +  if (_deviceUniqueId)
> +    delete[] _deviceUniqueId;

braces

@@ +407,5 @@
> +
> +  const bool callOnCaptureDelayChanged = _setCaptureDelay != _captureDelay;
> +  // Capture delay changed
> +  if (_setCaptureDelay != _captureDelay) {
> +    _setCaptureDelay = _captureDelay;

Is this safe?  ::CaptureDelay() and SetCaptureDelay() above lock 'cs' before accessing these

@@ +475,5 @@
> +      target_width = abs(height);
> +      target_height = width;
> +    }
> +    // TODO(mikhal): Update correct aligned stride values.
> +    //Calc16ByteAlignedStride(target_width, &stride_y, &stride_uv);

Why is this commented out?  Is this code copied from somewhere else?

@@ +510,5 @@
> +
> +  const uint32_t processTime =
> +    (uint32_t)(TickTime::Now() - startProcessTime).Milliseconds();
> +
> +  // If the process time is too long MJPG will not work well.

Pretty unlikely that a screen/desktop/app capture will appear as Motion-JPEG

@@ +589,5 @@
> +    // first no shift
> +  } else {
> +    // shift
> +    for (int i = (kFrameRateCountHistorySize - 2); i >= 0; i--) {
> +      _incomingFrameTimes[i + 1] = _incomingFrameTimes[i];

SO this is just an overlapping memmove....

@@ +610,5 @@
> +  }
> +  if (num > 1) {
> +    int64_t diff = (now - _incomingFrameTimes[num - 1]).Milliseconds();
> +    if (diff > 0) {
> +      return uint32_t((nrOfFrames * 1000.0f / diff) + 0.5f);

Can be more efficient, but doesn't matter.  Comment it's round-to-nearest

@@ +619,5 @@
> +}
> +
> +int32_t DesktopCaptureImpl::StartCapture(const VideoCaptureCapability& capability) {
> +  _requestedCapability = capability;
> +  //desktop_capturer_->Start(this);

more commented out code?

::: media/webrtc/trunk/webrtc/video_engine/desktop_capture_impl.h
@@ +1,2 @@
> +/*
> + *  Copyright (c) 2012 The WebRTC project authors. All Rights Reserved.

Copyright date is wrong

@@ +33,5 @@
> +class CriticalSectionWrapper;
> +class VideoCaptureEncodeInterface;
> +
> +
> +//simulate deviceInfo interface for video engine, bridge screen/applicaiton and real screen/application device info

application

@@ +52,5 @@
> +                                char* productUniqueIdUTF8,
> +                                uint32_t productUniqueIdUTF8Length);
> +
> +  virtual int32_t DisplayCaptureSettingsDialogBox(
> +                                                  const char* deviceUniqueIdUTF8,

don't split lines here (and in later functions)

@@ +113,5 @@
> +                                 const char* deviceUniqueIdUTF8,
> +                                 VideoCaptureRotation& orientation);
> +protected:
> +  int32_t _id;
> +  scoped_ptr<DesktopDeviceInfo> desktop_device_info_;

inconsistent naming (_id vs desktop_device_info_)

@@ +117,5 @@
> +  scoped_ptr<DesktopDeviceInfo> desktop_device_info_;
> +};
> +
> +//we reuse video engine pipeline for screen sharing.
> +//As video did , DesktopCaptureImpl will be proxy for screen sharing , also follow video pipeline design

Try to clean up the comment

@@ +120,5 @@
> +//we reuse video engine pipeline for screen sharing.
> +//As video did , DesktopCaptureImpl will be proxy for screen sharing , also follow video pipeline design
> +class DesktopCaptureImpl: public VideoCaptureModule,
> +                          public VideoCaptureExternal,
> +                          public ScreenCapturer::Callback ,

remove space before comma

@@ +124,5 @@
> +                          public ScreenCapturer::Callback ,
> +                          public ScreenCapturer::MouseShapeObserver
> +{
> +public:
> +  /* Create a screen capture modules object

Please add OVERRIDE indicators to (most) functions here

@@ +187,5 @@
> +  char* _deviceUniqueId; // current Device unique name;
> +  CriticalSectionWrapper& _apiCs;
> +  int32_t _captureDelay; // Current capture delay. May be changed of platform dependent parts.
> +  VideoCaptureCapability _requestedCapability; // Should be set by platform dependent code in StartCapture.
> +private:

blank line before private:
Attachment #8452414 - Flags: review?(rjesup) → review+
Attachment #8452751 - Flags: review?(rjesup) → review+
Assignee

Comment 69

5 years ago
This patch updates attachment 8452754 [details] [diff] [review] to change "mozMediaSource" to "mediaSource".

This should address all review comments from Martin and Jan-Ivar.
Attachment #8453220 - Flags: review?(martin.thomson)
Attachment #8453220 - Flags: review?(jib)
Assignee

Comment 70

5 years ago
This interdiff update attachment 8452751 [details] [diff] [review] to change "mozMediaSource" to "mediaSource".
Attachment #8453224 - Flags: review?(rjesup)
Assignee

Comment 71

5 years ago
This interdiff updates attachment 8451701 [details] [diff] [review] to change "mozMediaSource" to "mediaSource".

It also protects against there being no sources when checking "always share" permissions when application/screen devices are present.
Attachment #8453226 - Flags: review?(dolske)
Attachment #8453220 - Flags: review?(jib) → review+
Comment on attachment 8452754 [details] [diff] [review]
Patch 3. v4 Modify constraints for screen sharing

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

r=me with changes below.

::: content/media/webrtc/MediaTrackConstraints.h
@@ +84,5 @@
>      // Reminder: add handling for new constraints both here & SatisfyConstraintSet
>      Triage(Kind::Width).mWidth = mWidth;
>      Triage(Kind::Height).mHeight = mHeight;
>      Triage(Kind::FrameRate).mFrameRate = mFrameRate;
> +    Triage(Kind::MozMediaSource).mMozMediaSource = mMozMediaSource;

You need to put this back the way you had it where MediaSource is always required, as that's the only way to prevent screensharing sources from leaking into camera list of existing users. My bad.

::: modules/libpref/src/init/all.js
@@ +339,5 @@
>  #endif
>  
> +// do not enable screensharing before addressing security concerns: Bug XXXXXX
> +// do not enable screensharing before implementing app/window sharing: Bug XXXXXX
> +// do not enable screensharing before source constraints are finalized: Bug XXXXXX

Please fill in the XXXXXX before landing.
Attachment #8452754 - Flags: review?(jib) → review+
Attachment #8453224 - Flags: review?(rjesup) → review+
Comment on attachment 8452751 [details] [diff] [review]
Patch 4. v3 Device Enumeration & MediaEngine

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

A few comments if I didn't make them before

::: content/media/webrtc/MediaEngineWebRTC.cpp
@@ +71,5 @@
>  }
>  
>  void
> +MediaEngineWebRTC::EnumerateVideoDevices(dom::MozMediaSourceEnum aMediaSource,
> +                                         nsTArray<nsRefPtr<MediaEngineVideoSource> >* aVSources){

{ on a new line, left-edge

@@ +109,5 @@
>      if (mVideoSources.Get(uuid, getter_AddRefs(vSource))) {
>        // We've already seen this device, just append.
>        aVSources->AppendElement(vSource.get());
>      } else {
> +      if(mozMediaSourceType == dom::MozMediaSourceEnum::Screen){

spaces after if (throughout)

@@ +111,5 @@
>        aVSources->AppendElement(vSource.get());
>      } else {
> +      if(mozMediaSourceType == dom::MozMediaSourceEnum::Screen){
> +        vSource = new MediaEngineWebRTCScreenSource(i);
> +      } else if(mozMediaSourceType == dom::MozMediaSourceEnum::Application){

space before {

@@ +126,5 @@
>  #else
>    ScopedCustomReleasePtr<webrtc::ViEBase> ptrViEBase;
>    ScopedCustomReleasePtr<webrtc::ViECapture> ptrViECapture;
> +  webrtc::VideoEngine *videoEngine = NULL;
> +  bool *videoEngineInit = NULL;

nullptr

@@ +258,5 @@
> +        case dom::MozMediaSourceEnum::Camera:
> +          // fall through
> +        default:
> +        vSource = new MediaEngineWebRTCVideoSource(videoEngine, i);
> +        break;

fix indent for the default case
Attachment #8453220 - Flags: review?(martin.thomson) → review+
Comment on attachment 8452751 [details] [diff] [review]
Patch 4. v3 Device Enumeration & MediaEngine

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

::: content/media/webrtc/MediaEngineDefault.cpp
@@ +483,3 @@
>    MutexAutoLock lock(mMutex);
>  
> +  // aMediaSource is ignored for audio devices.

I think that the right answer here might be different to that, but this is OK for now.

@@ +503,5 @@
> +  // only supports camera sources
> +  if (aMediaSource != dom::MozMediaSourceEnum::Camera)
> +  {
> +    return;
> +  }

As above.

::: content/media/webrtc/MediaEngineWebRTC.cpp
@@ +141,5 @@
> +
> +  switch (aMediaSource) {
> +    case dom::MozMediaSourceEnum::Application:
> +      mConfigApplication.Set<webrtc::CaptureDeviceType>(
> +          new webrtc::CaptureDeviceType(true,true));

Why is mConfigApplication/mConfigScreen a member variable and not just something that is created on the stack and passed to VideoEngine::Create ?  I can't see any need to hold onto these.

@@ +258,5 @@
> +        case dom::MozMediaSourceEnum::Camera:
> +          // fall through
> +        default:
> +        vSource = new MediaEngineWebRTCVideoSource(videoEngine, i);
> +        break;

See below comment regarding unnecessary inheritance.

::: content/media/webrtc/MediaEngineWebRTC.h
@@ +296,5 @@
> +#endif
> +    virtual dom::MozMediaSourceEnum GetMozMediaSource() {
> +      return dom::MozMediaSourceEnum::Application;
> +    }
> +};

I think that inheritance here is overkill.  The value for GetMozMediaSource() could more easily come from a member variable.  Extend MediaEngineWebRTCVideoSource to add that and it's nice.
Attachment #8452751 - Flags: review?(martin.thomson) → review+
Assignee

Comment 75

5 years ago
(In reply to Martin Thomson [:mt] from comment #74)
> Comment on attachment 8452751 [details] [diff] [review]
> Patch 4. v3 Device Enumeration & MediaEngine
> 
> Review of attachment 8452751 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/webrtc/MediaEngineDefault.cpp
> @@ +483,3 @@
> >    MutexAutoLock lock(mMutex);
> >  
> > +  // aMediaSource is ignored for audio devices.
> 
> I think that the right answer here might be different to that, but this is
> OK for now.
> 

I had meant to append "(for now)" to that comment (and the one below).  Would that help?

> @@ +503,5 @@
> > +  // only supports camera sources
> > +  if (aMediaSource != dom::MozMediaSourceEnum::Camera)
> > +  {
> > +    return;
> > +  }
> 
> As above.
> 
> ::: content/media/webrtc/MediaEngineWebRTC.cpp
> @@ +141,5 @@
> > +
> > +  switch (aMediaSource) {
> > +    case dom::MozMediaSourceEnum::Application:
> > +      mConfigApplication.Set<webrtc::CaptureDeviceType>(
> > +          new webrtc::CaptureDeviceType(true,true));
> 
> Why is mConfigApplication/mConfigScreen a member variable and not just
> something that is created on the stack and passed to VideoEngine::Create ? 
> I can't see any need to hold onto these.
> 

I think this is just something I missed in refactoring MediaEngine.

> @@ +258,5 @@
> > +        case dom::MozMediaSourceEnum::Camera:
> > +          // fall through
> > +        default:
> > +        vSource = new MediaEngineWebRTCVideoSource(videoEngine, i);
> > +        break;
> 
> See below comment regarding unnecessary inheritance.
> 
> ::: content/media/webrtc/MediaEngineWebRTC.h
> @@ +296,5 @@
> > +#endif
> > +    virtual dom::MozMediaSourceEnum GetMozMediaSource() {
> > +      return dom::MozMediaSourceEnum::Application;
> > +    }
> > +};
> 
> I think that inheritance here is overkill.  The value for
> GetMozMediaSource() could more easily come from a member variable.  Extend
> MediaEngineWebRTCVideoSource to add that and it's nice.

That makes sense.
Comment on attachment 8451733 [details] [diff] [review]
Patch 5. v2 WebRTC ViE input changes

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

::: media/webrtc/trunk/webrtc/video_engine/vie_input_manager.cc
@@ +416,5 @@
> +// Create different DeviceInfo by _config;
> +VideoCaptureModule::DeviceInfo* ViEInputManager::GetDeviceInfo() {
> +  bool use_video_deviceinfo = true;
> +  bool use_app_deviceinfo = false;
> +  if(config_.Get<CaptureDeviceType>().isScreenDevice) {

spaces after if and around conditionals (<)
Attachment #8451733 - Flags: feedback?(rjesup) → feedback+
Comment on attachment 8452417 [details] [diff] [review]
Patch 10. Null implementations for deviceinfo

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

Copyright dates all say 2013... Probably needs to be 2014 for all new files.

Not critical, but try to be consistent about indents - some files are 2, others are 4.  Most code in webrtc.org and mozilla is 2.

::: media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_device_info.cc
@@ +1,2 @@
> +#include "webrtc/modules/desktop_capture/desktop_device_info.h"
> +

copyright

::: media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_device_info.h
@@ +1,2 @@
> +#ifndef WEBRTC_MODULES_DESKTOP_CAPTURE_DEVICE_INFO_H_
> +#define WEBRTC_MODULES_DESKTOP_CAPTURE_DEVICE_INFO_H_

copyright

::: media/webrtc/trunk/webrtc/modules/desktop_capture/mac/desktop_device_info_mac.h
@@ +1,1 @@
> +#ifndef WEBRTC_MODULES_DESKTOP_CAPTURE_MAC_DEVICE_INFO_H_

copyright

::: media/webrtc/trunk/webrtc/modules/desktop_capture/mac/desktop_device_info_mac.mm
@@ +1,2 @@
> +#include "webrtc/modules/desktop_capture/mac/desktop_device_info_mac.h"
> +#include <Cocoa/Cocoa.h>

copyright

@@ +6,5 @@
> +#define MULTI_MONITOR_NO_SUPPORT 1
> +
> +DesktopDeviceInfo * DesktopDeviceInfoImpl::Create() {
> +  DesktopDeviceInfoMac * pDesktopDeviceInfo = new DesktopDeviceInfoMac();
> +  if (pDesktopDeviceInfo && pDesktopDeviceInfo->Init() != 0){

normal comments about spaces

::: media/webrtc/trunk/webrtc/modules/desktop_capture/win/desktop_device_info_win.cc
@@ +15,5 @@
> +#define MULTI_MONITOR_NO_SUPPORT 1
> +
> +DesktopDeviceInfo * DesktopDeviceInfoImpl::Create() {
> +  DesktopDeviceInfoWin * pDesktopDeviceInfo = new DesktopDeviceInfoWin();
> +  if(pDesktopDeviceInfo && pDesktopDeviceInfo->Init() != 0){

standard "spaces after 'if' and before {

::: media/webrtc/trunk/webrtc/modules/desktop_capture/win/desktop_device_info_win.h
@@ +1,3 @@
> +#ifndef WEBRTC_MODULES_DESKTOP_CAPTURE_WIN_DEVICE_INFO_H_
> +#define WEBRTC_MODULES_DESKTOP_CAPTURE_WIN_DEVICE_INFO_H_
> +

copyright

::: media/webrtc/trunk/webrtc/modules/desktop_capture/window_capturer_mac.mm
@@ +76,5 @@
> +  CFArrayRef window_array = CGWindowListCopyWindowInfo(
> +      kCGWindowListOptionOnScreenOnly | kCGWindowListExcludeDesktopElements,
> +      kCGNullWindowID);
> +  if (!window_array)
> +    return false;

braces for all if's

@@ +95,5 @@
> +      // Skip windows with layer=0 (menu, dock).
> +      int layer;
> +      CFNumberGetValue(window_layer, kCFNumberIntType, &layer);
> +      if (layer != 0)
> +        continue;

braces

@@ +136,5 @@
> +}
> +
> +bool WindowCapturerMac::BringSelectedWindowToFront() {
> +  if (!window_id_)
> +    return false;

braces

@@ +139,5 @@
> +  if (!window_id_)
> +    return false;
> +
> +  CGWindowID ids[1];
> +  ids[0] = window_id_;

If this is just so we can use ** below, add a 1-liner comment

::: media/webrtc/trunk/webrtc/modules/desktop_capture/x11/desktop_device_info_x11.cc
@@ +1,2 @@
> +#include "webrtc/modules/desktop_capture/x11/desktop_device_info_x11.h"
> +

copyright

@@ +5,5 @@
> +#define MULTI_MONITOR_NO_SUPPORT 1
> +
> +DesktopDeviceInfo * DesktopDeviceInfoImpl::Create() {
> +  DesktopDeviceInfoX11 * pDesktopDeviceInfo = new DesktopDeviceInfoX11();
> +  if (pDesktopDeviceInfo && pDesktopDeviceInfo->Init() != 0){

spaces before {

::: media/webrtc/trunk/webrtc/modules/desktop_capture/x11/desktop_device_info_x11.h
@@ +1,2 @@
> +#ifndef WEBRTC_MODULES_DESKTOP_CAPTURE_X11_DEVICE_INFO_H_
> +#define WEBRTC_MODULES_DESKTOP_CAPTURE_X11_DEVICE_INFO_H_

Need copyright info
Attachment #8452417 - Flags: feedback?(rjesup) → feedback+
Comment on attachment 8452412 [details] [diff] [review]
Patch 7. v2 Desktop capture code changes

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

rs=me because this is an update from upstream

::: media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_capture_types.h
@@ +38,5 @@
>  const ScreenId kInvalidScreenId = -2;
>  
> +
> +typedef intptr_t ProcessId;
> +const ProcessId DesktopProcessId = 0;

All of this patch seems to be an update from upstream/Chromium, except for this. And as far as I can  tell, it's unused? So remove it.
Attachment #8452412 - Flags: review?(gpascutto) → review+
Comment on attachment 8451701 [details] [diff] [review]
Patch 2. v2. Add Screen Sharing UI

Dolske, can you look at this? I know this isn't final UI, but we need to know if the platform bits interacting with this code need changes or not. Also, we'd like to put the test UI into an add-on or similar - if parts of this code can't go into an addon, they need a review as well.
Attachment #8451701 - Flags: review?(dolske) → feedback?(dolske)
Patch stack:
ssh://hg.mozilla.org/users/linuxwolf_outer-planes.net/bug-983504-patches

Try run of the current patch stack:
https://tbpl.mozilla.org/?tree=Try&rev=5411f449fef6
Comment on attachment 8451733 [details] [diff] [review]
Patch 5. v2 WebRTC ViE input changes

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

::: media/webrtc/trunk/webrtc/video_engine/vie_input_manager.h
@@ +77,5 @@
>    int CreateExternalCaptureDevice(ViEExternalCapture*& external_capture,
>                                    int& capture_id);
>    int DestroyCaptureDevice(int capture_id);
> + protected:
> +  VideoCaptureModule::DeviceInfo* GetDeviceInfo();

Is this likely to be overriden in a sane manner? Doesn't look like it, so I'd make it private.
Attachment #8451733 - Flags: review?(gpascutto) → review+
Comment on attachment 8452754 [details] [diff] [review]
Patch 3. v4 Modify constraints for screen sharing

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

::: dom/media/MediaManager.cpp
@@ +1487,5 @@
>    }
>  
> +  // deny screensharing request if support is disabled
> +  if (c.mVideo.IsMediaTrackConstraints() &&
> +      !Preferences::GetBool("media.getusermedia.screensharing.enabled", false)) {

The guard in webidl should suffice.

::: dom/webidl/MediaTrackConstraintSet.webidl
@@ +12,5 @@
>      "facingMode",
>      "width",
>      "height",
>      "frameRate",
> +    "mozMediaSource"

I was going to suggest that you decorate this, but that doesn't work, so don't bother.

@@ +25,5 @@
>      ConstrainLongRange width;
>      ConstrainLongRange height;
>      ConstrainDoubleRange frameRate;
>      ConstrainVideoFacingMode facingMode;
> +    ConstrainMediaSource mozMediaSource = "camera";

This, on the other hand, can be decorated with a pref check (unless I've misunderstood something).  That will be a better guard than the one you have.

@@ +39,3 @@
>  // TODO: Bug 767924 sequences in unions
>  //typedef (VideoFacingModeEnum or sequence<VideoFacingModeEnum>) ConstrainVideoFacingMode;
> +//typedef (MozMediaSourceEnum or sequence<MozMediaSourceEnum>) ConstrainMediaSource;
\ No newline at end of file

\ No newline at end of file

:jib isn't this redundant now that you are going to DOMString in place of enumerations, or is that just a spec convenience?
Attachment #8452754 - Flags: review?(martin.thomson) → review+
Assignee

Comment 83

5 years ago
Posted patch Add missing license headers (obsolete) — Splinter Review
Add license headers to files that did not have them previously.

Using MPLv2 for these new files.
Attachment #8453864 - Flags: review?(fluffy)
Assignee

Comment 84

5 years ago
(In reply to Martin Thomson [:mt] from comment #82)
> > +  // deny screensharing request if support is disabled
> > +  if (c.mVideo.IsMediaTrackConstraints() &&
> > +      !Preferences::GetBool("media.getusermedia.screensharing.enabled", false)) {
> 
> The guard in webidl should suffice.

When I tried to apply [Pref='...'] in the webidl, I got compile errors.  Presumably because MediaConstraintSet is a dictionary and not an interface.  My (admittedly limited) understanding of IDL is such things can only be applied to interfaces their members, not to more primitive types.

I put it where I felt it had the most impact.

> 
> ::: dom/webidl/MediaTrackConstraintSet.webidl
> @@ +12,5 @@
> >      "facingMode",
> >      "width",
> >      "height",
> >      "frameRate",
> > +    "mozMediaSource"
> 
> I was going to suggest that you decorate this, but that doesn't work, so
> don't bother.
> 
> @@ +25,5 @@
> >      ConstrainLongRange width;
> >      ConstrainLongRange height;
> >      ConstrainDoubleRange frameRate;
> >      ConstrainVideoFacingMode facingMode;
> > +    ConstrainMediaSource mozMediaSource = "camera";
> 
> This, on the other hand, can be decorated with a pref check (unless I've
> misunderstood something).  That will be a better guard than the one you have.
> 

See above.
Comment on attachment 8451690 [details] [diff] [review]
Patch 8. Buildsystem changes

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

::: configure.in
@@ +8133,5 @@
>  
>      MOZ_CAIRO_OSLIBS='${CAIRO_FT_OSLIBS}'
>  
>      if test "$MOZ_X11"; then
> +        MOZ_CAIRO_OSLIBS="$MOZ_CAIRO_OSLIBS $XLDFLAGS -lXext -lXdamage -lXfixes -lXcomposite -lXrender"

This doesn't actually change our runtime requirements does it? (As in these are already present with X11 anyway.)
Attachment #8451690 - Flags: review?(ted) → review+
> >  // TODO: Bug 767924 sequences in unions
> >  //typedef (VideoFacingModeEnum or sequence<VideoFacingModeEnum>) ConstrainVideoFacingMode;
> > +//typedef (MozMediaSourceEnum or sequence<MozMediaSourceEnum>) ConstrainMediaSource;
> \ No newline at end of file
> 
> \ No newline at end of file
> 
> :jib isn't this redundant now that you are going to DOMString in place of
> enumerations, or is that just a spec convenience?

Yes, good catch. But then we should use DOMString here as well for MediaSource. It can be this patch or later at the same time we fix facingMode, which we haven't done yet.
Comment on attachment 8452412 [details] [diff] [review]
Patch 7. v2 Desktop capture code changes

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

::: media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_capture_types.h
@@ +38,5 @@
>  const ScreenId kInvalidScreenId = -2;
>  
> +
> +typedef intptr_t ProcessId;
> +const ProcessId DesktopProcessId = 0;

Oops, seems my grepping missed the usage in the newly added files. I would think that the type is potentially something that is platform specific and is defined lower down, but other stuff like ScreenId is also using intptr_t as a catch-all so this is OK for now.
Comment on attachment 8452417 [details] [diff] [review]
Patch 10. Null implementations for deviceinfo

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

r- due to the window_capturer_mac.cc/mm weirdness. Mostly small nits otherwise.

::: media/webrtc/trunk/webrtc/modules/desktop_capture/app_capturer.cc
@@ +5,5 @@
> +*  that can be found in the LICENSE file in the root of the source
> +*  tree. An additional intellectual property rights grant can be found
> +*  in the file PATENTS.  All contributing project authors may
> +*  be found in the AUTHORS file in the root of the source tree.
> +*/

Question: these files have WebRTC license headers. But are they actually from the WebRTC project?

@@ +14,5 @@
> +namespace webrtc {
> +
> +// static
> +AppCapturer* AppCapturer::Create() {
> +    return Create(DesktopCaptureOptions::CreateDefault());

Need to check indentation vs what the default in the webrtc code is, iirc it's 2.

::: media/webrtc/trunk/webrtc/modules/desktop_capture/app_capturer.h
@@ +32,5 @@
> +        std::string title;
> +    };
> +    typedef std::vector<App> AppList;
> +
> +    //

nit: clean up

@@ +43,5 @@
> +    //AppCapturer Interfaces
> +    virtual bool GetAppList(AppList* apps) = 0;
> +    virtual bool SelectApp(ProcessId id) = 0;
> +    virtual bool BringAppToFront() {
> +        return true;

Isn't it more sensible to also make this a pure virtual?

::: media/webrtc/trunk/webrtc/modules/desktop_capture/app_capturer_mac.mm
@@ +28,5 @@
> +  AppCapturerMac();
> +  virtual ~AppCapturerMac();
> +
> +  // AppCapturer interface.
> +  virtual bool GetAppList(AppList* apps);

OVERRIDE

@@ +73,5 @@
> +  callback_ = callback;
> +}
> +
> +void AppCapturerMac::Capture(const DesktopRegion& region) {
> +  // callback_->OnCaptureCompleted(frame);

Why is it commented out?

::: media/webrtc/trunk/webrtc/modules/desktop_capture/app_capturer_null.cc
@@ +45,5 @@
> +AppCapturerNull::~AppCapturerNull() {
> +}
> +
> +bool AppCapturerNull::GetAppList(AppList* apps) {
> +  // Not implemented yet.

File bugs in Bugzilla describing what is missing and what needs to be done, mark the number here.

::: media/webrtc/trunk/webrtc/modules/desktop_capture/app_capturer_win.cc
@@ +20,5 @@
> +namespace webrtc {
> +
> +namespace {
> +
> +std::string Utf16ToUtf8(const WCHAR* str) {

Already exists in
media/webrtc/trunk/webrtc/modules/desktop_capture/window_capturer_win.cc

so maybe try to pull it out.

@@ +41,5 @@
> +  AppCapturerWin();
> +  virtual ~AppCapturerWin();
> +
> +  // AppCapturer interface.
> +  virtual bool GetAppList(AppList* apps);

OVERRIDE

::: media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_device_info.cc
@@ +44,5 @@
> +
> +void DesktopDisplayDevice::setUniqueIdName(char* deviceUniqueIdUTF8) {
> +  if (deviceUniqueIdUTF8 == NULL) return;
> +
> +  if (deviceUniqueIdUTF8_) {

Deleting a NULL ptr is safe, this check is unneeded.

@@ +49,5 @@
> +    delete [] deviceUniqueIdUTF8_;
> +    deviceUniqueIdUTF8_ = NULL;
> +  }
> +
> +  int nBufLen = strlen(deviceUniqueIdUTF8) + 1;

size_t not int

@@ +78,5 @@
> +}
> +
> +
> +DesktopApplication::DesktopApplication() {
> +  processId_ =0;

nit: spacing

@@ +101,5 @@
> +  }
> +
> +  int nBufLen = strlen(appPathNameUTF8) + 1;
> +  processPathNameUTF8_ = new char[nBufLen];
> +  memset(processPathNameUTF8_, 0, nBufLen);

This kind of code is repeated all over, pull it into a function. (Which can then avoid the memset by adding the 0 terminator at the right position)

@@ +163,5 @@
> +DesktopDeviceInfoImpl::DesktopDeviceInfoImpl() {
> +}
> +
> +DesktopDeviceInfoImpl::~DesktopDeviceInfoImpl() {
> +  std::map<intptr_t,DesktopDisplayDevice*>::iterator iterDevice;

nit:spacing

@@ +201,5 @@
> +  if(pDesktopDisplayDevice) {
> +    desktopDisplayDevice = (*pDesktopDisplayDevice);
> +  }
> +
> +  return 0;

Wouldn't pDesktopDisplayDevice being NULL warrant an error?

::: media/webrtc/trunk/webrtc/modules/desktop_capture/window_capturer_mac.mm
@@ +21,5 @@
> +namespace webrtc {
> +
> +namespace {
> +
> +bool CFStringRefToUtf8(const CFStringRef string, std::string* str_utf8) {

Exists in:
http://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/modules/desktop_capture/window_capturer_mac.cc

which upon closer inspection is the same file, but with a different extension?
Attachment #8452417 - Flags: review?(gpascutto) → review-
Attachment #8452267 - Flags: feedback?(gpascutto) → feedback+
Comment on attachment 8452414 [details] [diff] [review]
Patch 9. v2 DesktopCaptureImpl

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

Going to defer mostly to jesup's comments, but I'd really like to see the final version here given the amount of comments.

::: media/webrtc/trunk/webrtc/video_engine/desktop_capture_impl.cc
@@ +197,5 @@
> +
> +VideoCaptureModule* DesktopCaptureImpl::Create(const int32_t id,
> +                                               const char* uniqueId,
> +                                               const bool bIsApp) {
> +  // TODO(tommi): Use Media Foundation implementation for Vista and up.

What's the origin of this code?
Attachment #8452414 - Flags: feedback?(gpascutto) → feedback-
Assignee

Comment 91

5 years ago
(In reply to Gian-Carlo Pascutto [:gcp] from comment #89)

> ::: media/webrtc/trunk/webrtc/modules/desktop_capture/window_capturer_mac.mm
> @@ +21,5 @@
> > +namespace webrtc {
> > +
> > +namespace {
> > +
> > +bool CFStringRefToUtf8(const CFStringRef string, std::string* str_utf8) {
> 
> Exists in:
> http://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/
> modules/desktop_capture/window_capturer_mac.cc
> 
> which upon closer inspection is the same file, but with a different
> extension?

There is one difference -- window_capturer_mac.mm calls a Cocoa API (Objective-C), whereas the previous did not; that requires switching from .cc to .mm.

Note that window_capturer_mac.mm (and not window_capturer_mac.cc) exists in webrtc.org trunk.
>that requires switching from .cc to .mm.

Let's delete the old file in the patch then. (Clarified on IRC that deleting the old file got lost in reworking the patches)
Assignee

Comment 93

5 years ago
(In reply to Gian-Carlo Pascutto [:gcp] from comment #90)
> Comment on attachment 8452414 [details] [diff] [review]
> Patch 9. v2 DesktopCaptureImpl
> 
> Review of attachment 8452414 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Going to defer mostly to jesup's comments, but I'd really like to see the
> final version here given the amount of comments.
> 
> ::: media/webrtc/trunk/webrtc/video_engine/desktop_capture_impl.cc
> @@ +197,5 @@
> > +
> > +VideoCaptureModule* DesktopCaptureImpl::Create(const int32_t id,
> > +                                               const char* uniqueId,
> > +                                               const bool bIsApp) {
> > +  // TODO(tommi): Use Media Foundation implementation for Vista and up.
> 
> What's the origin of this code?

This was copied from a near-identical function in video_capture_factory_windows.cc in this tree.
Assignee

Comment 94

5 years ago
(In reply to Randell Jesup [:jesup] from comment #68)
> Comment on attachment 8452414 [details] [diff] [review]
> Patch 9. v2 DesktopCaptureImpl
> 
> Review of attachment 8452414 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ (with lots of nits!) if-and-only-if the cs/cs2 locks aren't needed where
> I pointed it out.  If a follow-on is needed, just post the interdiff please
> against this set of patches.
> 
> ::: media/webrtc/trunk/webrtc/video_engine/desktop_capture_impl.cc
> @@ +407,5 @@
> > +
> > +  const bool callOnCaptureDelayChanged = _setCaptureDelay != _captureDelay;
> > +  // Capture delay changed
> > +  if (_setCaptureDelay != _captureDelay) {
> > +    _setCaptureDelay = _captureDelay;
> 
> Is this safe?  ::CaptureDelay() and SetCaptureDelay() above lock 'cs' before
> accessing these


The code in question is only accessed after the locks are established, and mirrors video_capture_impl.cc.
Assignee

Comment 95

5 years ago
Interdiff over attachment 8452267 [details] [diff] [review] to change CaptureDeviceType from a struct to an enum.

Because of how webrtc::Config works, wrapping it in a struct seems the safest approach.
Assignee

Comment 96

5 years ago
This interdiff over attachement 8452267 changes CaptureDeviceType from a struct to an enum, but creates a struct to wrap it for use in webrtc::Config (which expects allocated objects).
Attachment #8454048 - Attachment is obsolete: true
Attachment #8454105 - Flags: feedback?(rjesup)
Comment on attachment 8451701 [details] [diff] [review]
Patch 2. v2. Add Screen Sharing UI

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

Technical r- for some confusing things to either fix or explain.

However... The mockups in bug 1031424 are apparently done (although I just asked another question there). This bug certainly doesn't need to deal with implementing the global "you're sharing your screen/app" notification. But given that we seem to have converged on desired UI for the doorhanger(s), I would expect this bug to implement what's intended reasonably closely. I don't want to land a bunch of changes in this bug, and then have someone redoing it all again next week (with other intermediate changes further complicating things). I'd obviously feel different if the "final UI" was going to take a long time to figure out, or if this bug was just adding a trivial temporary checkbox. But since we have a spec and this isn't a trivial change, we shouldn't spin our wheels implementing the wrong thing.

Finally, just to confirm, since it's important: looks like attachment 8452754 [details] [diff] [review] adds a disabled-by-default pref controlling if it's possible to do screensharing? Accidentally sharing your screen without realizing it (due to a lack of indicators) is a pretty big deal, even on Nightly, so this pref should stay disabled until we finish proper notification UI (whatever we do here). Please add the blocking bug numbers to that patch's "XXX" placeholders in firefox.js.

::: browser/modules/webrtcUI.jsm
@@ +103,5 @@
> +  let applicationDevices = [];
> +
> +  // determine which sources are desired
> +  let mozMediaSourceMandatory = "";
> +  if (typeof(aVideoRequested) == "object") {

Ugh. I am not at all a fan of complex type overloading like this -- sometimes aVideoRequested is a an object, sometimes a string. Sometimes mozMediaSourceMandatory is set to a string, sometimes it's not (?). It makes code difficult to follow and is bug-prone.

Can the calling code be changed to make this more consistent? (EG, for the existing camera-only case, make aVideoRequested be a object with .mozMediaSource set appropriately.)

Do we anticipate adding other media types to getUserMedia? (ie, in addition to mic/cam/app/screen?)

Aside: What's a "mandatory" source? That term and how it's used here seem confusing.

And why is this adding new code to implement a "legacy" form?

@@ +151,5 @@
>    }
>  
>    let requestType;
> +
> +  if (audioDevices.length && (screenDevices.length) && mozMediaSourceMandatory == "screen") {

The extra parens in the conditional are not needed.

@@ +354,5 @@
>        this.mainAction.callback = function(aRemember) {
>          let allowedDevices = Cc["@mozilla.org/supports-array;1"]
>                                 .createInstance(Ci.nsISupportsArray);
>          let perms = Services.perms;
> +        if (videoDevices.length || screenDevices.length || applicationDevices.length) {

This seems inverted, or I don't understand what you're trying to do.

The original code is basically says "of the available video devices we offered, which (if any) did the user select, and add it to allowedDevices". Which is repeated twice, once for videoDevices and once for audioDevices.

Why isn't this just further extending that pattern? E.G. Add two more loops, for screenDevices and applicationDevices?

@@ +357,5 @@
>          let perms = Services.perms;
> +        if (videoDevices.length || screenDevices.length || applicationDevices.length) {
> +          let allowCamera = false ;
> +
> +          if(mozMediaSourceMandatory=="screen") {

Nit: spacing -- "if (x == y) {"
Attachment #8451701 - Flags: feedback?(dolske) → review-
Comment on attachment 8453226 [details] [diff] [review]
Interdiff over webrtcUI to remove "moz" prefix

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

Mechanical changes like this are fine. Roll into the next version of the patch.
Attachment #8453226 - Flags: review?(dolske) → feedback+

Comment 99

5 years ago
Posted file patch_MacOSX_application_share.diff (obsolete) —
Application Sharing is workable on MacOSX platform after 4 commits 

https://github.com/vagouzhou/gecko-dev/commits/master

git diff 4109c4367469ece7613dcdfb3bb2cb7bd80a0eeb e759267817756046519f83a9e1976445a801be16 > patch_MacOSX_application_share.diff
(In reply to Justin Dolske [:Dolske] from comment #97)
> > +  if (typeof(aVideoRequested) == "object") {
> 
> Ugh. I am not at all a fan of complex type overloading like this --
> sometimes aVideoRequested is a an object, sometimes a string.

I may be able to answer this one.

aVideoRequested predates these patches and is passed in from elsewhere in the same file for its truthy/falsy property of indicating whether video was requested by content. It's a video constraints argument of type (boolean or MediaTrackConstraints) as defined in http://dev.w3.org/2011/webrtc/editor/getusermedia.html#mediastreamconstraints - This patch exploits that the full thing is passed in rather than just booleans. Perhaps renaming to aVideoConstraints would help?

> Do we anticipate adding other media types to getUserMedia? (ie, in addition
> to mic/cam/app/screen?)

Yes, but which way to extend - as proper top-level media types next to audio/video, or as mediaSource constraints under the existing (audio/)video - is being debated on the media-capture list. This seems the most actionable short-term solution right now, though perhaps not the right long-term solution (Google is doing the same).

> Aside: What's a "mandatory" source? That term and how it's used here seem confusing.

Agree. It's a legacy constraint term we should avoid using here. Besides, we agreed to treat MediaSource as always required (aka mandatory) here for this to work, so I would remove mention of this.

> And why is this adding new code to implement a "legacy" form?

Yeah I don't like this part. It's trying to straddle Bug 1033885. Please implement it the old way and I'll deal with it when we update gUM. 

> The original code is basically says "of the available video devices we
> offered, which (if any) did the user select, and add it to allowedDevices".
> Which is repeated twice, once for videoDevices and once for audioDevices.
> 
> Why isn't this just further extending that pattern? E.G. Add two more loops,
> for screenDevices and applicationDevices?

Because we cheat and implement screensharing right now as sub-types of video, using a constraint as selector. This happens to match current limitations (it reuses our sole webrtc video pipeline. e.g. you can't screenshare and camera share at the same time). Long term we'll want to fix this, but the spec hasn't caught up.
(In reply to vagouzhou from comment #99)
> Created attachment 8454173 [details]
> patch_MacOSX_application_share.diff
> 
> Application Sharing is workable on MacOSX platform after 4 commits 

Err, this really needs to go into a separate bug. There's already a large set of patches undergoing review here, and it's impossible to track a bug's status if new things keep being added to it. (For example, it certainly wasn't clear to me that OS X wasn't working (?) with the patches here.)

Also, who is the proper assignee for this bug? There are patches from 3 different authors.

Comment 102

5 years ago
M&M just told me that create another bug for "application sharing" https://bugzilla.mozilla.org/show_bug.cgi?id=1036653.
"application sharing" patchs will be put in this bug. 

This bug is indeed too big :)

(In reply to Justin Dolske [:Dolske] from comment #101)
> (In reply to vagouzhou from comment #99)
> > Created attachment 8454173 [details]
> > patch_MacOSX_application_share.diff
> > 
> > Application Sharing is workable on MacOSX platform after 4 commits 
> 
> Err, this really needs to go into a separate bug. There's already a large
> set of patches undergoing review here, and it's impossible to track a bug's
> status if new things keep being added to it. (For example, it certainly
> wasn't clear to me that OS X wasn't working (?) with the patches here.)
> 
> Also, who is the proper assignee for this bug? There are patches from 3
> different authors.

Updated

5 years ago
Attachment #8454173 - Attachment is patch: false

Updated

5 years ago
Attachment #8454173 - Attachment is obsolete: true
Comment on attachment 8454105 [details] [diff] [review]
Interdiff to change CaptureDeviceType to enum

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

::: .gitignore
@@ -20,5 @@
>  /config.log
>  /.clang_complete
>  /mach.ini
> -/build.bat
> -/gecko-dev.*

these patches shouldn't touch .gitignore!
Attachment #8454105 - Flags: feedback?(rjesup) → feedback+
>these patches shouldn't touch .gitignore!

It's an interdiff and it's undoing the (incorrect) add of those files in a previous patch.
Attachment #8454367 - Flags: review?(ted) → review+
Assignee

Comment 106

5 years ago
(In reply to Justin Dolske [:Dolske] from comment #97)
> Comment on attachment 8451701 [details] [diff] [review]
> Patch 2. v2. Add Screen Sharing UI
> 
> Review of attachment 8451701 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Technical r- for some confusing things to either fix or explain.
> 
> However... The mockups in bug 1031424 are apparently done (although I just
> asked another question there). This bug certainly doesn't need to deal with
> implementing the global "you're sharing your screen/app" notification. But
> given that we seem to have converged on desired UI for the doorhanger(s), I
> would expect this bug to implement what's intended reasonably closely. I
> don't want to land a bunch of changes in this bug, and then have someone
> redoing it all again next week (with other intermediate changes further
> complicating things). I'd obviously feel different if the "final UI" was
> going to take a long time to figure out, or if this bug was just adding a
> trivial temporary checkbox. But since we have a spec and this isn't a
> trivial change, we shouldn't spin our wheels implementing the wrong thing.
> 
> Finally, just to confirm, since it's important: looks like attachment
> 8452754 [details] [diff] [review] adds a disabled-by-default pref
> controlling if it's possible to do screensharing? Accidentally sharing your
> screen without realizing it (due to a lack of indicators) is a pretty big
> deal, even on Nightly, so this pref should stay disabled until we finish
> proper notification UI (whatever we do here). Please add the blocking bug
> numbers to that patch's "XXX" placeholders in firefox.js.
> 

I agree we shouldn't be spending too much time on something not permanent, but something is needed to demo and test with until the real UI is ready.  From email conversations, it looks like there are people just about to start that work.

I've added the relevant bug ids to the patch, creating any that did not already exist.

> ::: browser/modules/webrtcUI.jsm
> @@ +103,5 @@
> > +  let applicationDevices = [];
> > +
> > +  // determine which sources are desired
> > +  let mozMediaSourceMandatory = "";
> > +  if (typeof(aVideoRequested) == "object") {
> 
> Ugh. I am not at all a fan of complex type overloading like this --
> sometimes aVideoRequested is a an object, sometimes a string. Sometimes
> mozMediaSourceMandatory is set to a string, sometimes it's not (?). It makes
> code difficult to follow and is bug-prone.
> 
> Can the calling code be changed to make this more consistent? (EG, for the
> existing camera-only case, make aVideoRequested be a object with
> .mozMediaSource set appropriately.)
> 
> Do we anticipate adding other media types to getUserMedia? (ie, in addition
> to mic/cam/app/screen?)
> 
> Aside: What's a "mandatory" source? That term and how it's used here seem
> confusing.
> 
> And why is this adding new code to implement a "legacy" form?
> 

The discussion around constraints moved quicker than this patch did; my apologies for not keeping it up and will make the changes suggested by you and Jan-Ivar.
Depends on: 1037424
Assignee

Comment 107

5 years ago
This interdiff updates attachment 8451701 [details] [diff] [review] to address code review comments.
Assignee

Updated

5 years ago
Attachment #8454454 - Flags: feedback?(dolske)
(In reply to Jan-Ivar Bruaroey [:jib] from comment #100)

> > Ugh. I am not at all a fan of complex type overloading like this --
> > sometimes aVideoRequested is a an object, sometimes a string.
> 
> I may be able to answer this one.
> 
> aVideoRequested predates these patches and is passed in from elsewhere in
> the same file for its truthy/falsy property of indicating whether video was
> requested by content. It's a video constraints argument of type (boolean or
> MediaTrackConstraints) as defined in
> http://dev.w3.org/2011/webrtc/editor/getusermedia.
> html#mediastreamconstraints - This patch exploits that the full thing is
> passed in rather than just booleans. Perhaps renaming to aVideoConstraints
> would help?

It... might. "Constraints" is a term in the spec, so it's reasonable to use it when referring to such.

But it's still pretty confusing, and I don't _think_ this is actually being used in the way the spec is talking about. Staring at it again, it seems like this should just be named something like requestedVideoType or requestedVideoSource. The caller should sort this out, and pass it in as an argument (equal to "camera", "screen", "application", or empty-string). In other words, change the aVideoRequested arg from a boolean to a string.

Bug 1031424 comment 37 helpfully clarifies that we're only supporting requesting 1 type of video source (i.e., you can't request a camera and screen at the same time), so making that clear in the API would be helpful.

If this sounds like crazy-talk, it's possible I'm still missing something here. :)

> > Do we anticipate adding other media types to getUserMedia? (ie, in addition
> > to mic/cam/app/screen?)
> 
> Yes, but which way to extend - as proper top-level media types next to
> audio/video, or as mediaSource constraints under the existing (audio/)video
> - is being debated on the media-capture list.

Ah, fair enough. Just wanted to know if there was a clear future direction this should start adapting to, but if things are in flux we shouldn't worry about it now.

> > The original code is basically says "of the available video devices we
> > offered, which (if any) did the user select, and add it to allowedDevices".
> > Which is repeated twice, once for videoDevices and once for audioDevices.
> > 
> > Why isn't this just further extending that pattern? E.G. Add two more loops,
> > for screenDevices and applicationDevices?
> 
> Because we cheat and implement screensharing right now as sub-types of
> video, using a constraint as selector. This happens to match current
> limitations (it reuses our sole webrtc video pipeline. e.g. you can't
> screenshare and camera share at the same time). Long term we'll want to fix
> this, but the spec hasn't caught up.

Ok, this plus bug 1031424 comment 37 makes me think that for now, the code here shouldn't add the extra |screenDevices| and |applicationDevices| arrays. The existing (and now generic) |videoDevices| array should just end up being filled with the list of devices matching the new |requestedVideoType| arg (cam/screen/app).

Further, it might help with clarity/layering if the aDevices was prefiltered in handleRequest() and split into aAudioDevices and aVideoDevices. (Which mostly eliminates the need for the aVideoRequested and aAudioRequested args; it may or may not make sense to compute and pass in requestType too.) That lets prompt() focus on one task. OTOH, I think the changes in the my previous paragraph will simplify things enough that this not strictly needed, and could be handled in a separate bug (or not at all).
Comment on attachment 8451701 [details] [diff] [review]
Patch 2. v2. Add Screen Sharing UI

One more thing I just noticed:

>+          switch (device.mozMediaSource) {
>+            case "application":
>+              applicationDevices.push(device);
>+              break;
>+            case "screen":
>+              screenDevices.push(device);
>+              break;
>+            case "camera":
>+            default:
>+              videoDevices.push(device);
>+              break;
>+          }

This shouldn't handle unknown types as "camera". The |default| case should result in the device being ignored / reported as an error.

If we new device types show up before the UI is ready for them, I don't think we want them available as cameras. (E.G. if the existing code worked this way, we wouldn't want screen/application types to be shared as cameras while we worked on this bug!)
Comment on attachment 8454454 [details] [diff] [review]
Interdiff addressing webrtcUI review comments

I'd actually prefer the full patch (instead of just the the interdiff). See also last comment.

But looks headed in the right direction!
Attachment #8454454 - Flags: feedback?(dolske)
(In reply to Justin Dolske [:Dolske] from comment #97)

> However... The mockups in bug 1031424 are apparently done (although I just
> asked another question there). This bug certainly doesn't need to deal with
> implementing the global "you're sharing your screen/app" notification. But
> given that we seem to have converged on desired UI for the doorhanger(s), I
> would expect this bug to implement what's intended reasonably closely.

Given bug 1031424 comment 37, I'm no longer concerned about this.
Attachment #8454510 - Flags: review?(linuxwolf)
(In reply to Justin Dolske [:Dolske] from comment #108)
> Bug 1031424 comment 37 helpfully clarifies that we're only supporting
> requesting 1 type of video source (i.e., you can't request a camera and
> screen at the same time), so making that clear in the API would be helpful.

Only one set of video constraints may be pased to gUM and the MediaSource constraint controls the type, so the syntax is inherently limited.

> If this sounds like crazy-talk, it's possible I'm still missing something
> here. :)

No worries, constraints are a bit confusing for sure, not to mention a moving target still.
(In reply to Justin Dolske [:Dolske] from comment #109)
> This shouldn't handle unknown types as "camera". The |default| case should
> result in the device being ignored / reported as an error.

I agree. It should fire the failure callback.
I filed bug 1037405 to work on the doorhanger UI. I intend to start with the current patch, make it match the UX mockup from bug 1031424 and get it in a landable state next week.
Comment on attachment 8454454 [details] [diff] [review]
Interdiff addressing webrtcUI review comments

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

::: browser/modules/webrtcUI.jsm
@@ +86,5 @@
>    contentWindow.navigator.mozGetUserMediaDevices(
>      constraints,
>      function (devices) {
>        prompt(contentWindow, aSubject.callID, constraints.audio,
> +             videoConstraints, devices, secure);

Instead of all the code added above, why not:

prompt(contentWindow, aSubject.callID, constraints.audio,
       videoConstraints === true ? { mediaSource: "camera" } : videoConstraints,
       devices, secure);

@@ +110,5 @@
>                  aDevices, aSecure) {
>    let audioDevices = [];
>    let videoDevices = [];
>    let screenDevices = [];
>    let applicationDevices = [];

I think Justin's point was that you don't need screenDevices and applicationDevices here. Just add the one possible video device to videoDevices and then use mediaSource to determine what it is.

The video devices are guaranteed to all be of the same type since the constraint is required and comes with a default.

@@ +113,5 @@
>    let screenDevices = [];
>    let applicationDevices = [];
>  
> +  // filter devices into types, then sources
> +  let mediaSource = (aVideoConstraints && aVideoConstraints.mediaSource) || "";

Don't need the || ""

Updated

5 years ago
Depends on: 1037589
Attachment #8454512 - Flags: review?(ted) → review+
Assignee

Updated

5 years ago
Attachment #8454510 - Flags: review?(linuxwolf) → review+
Attachment #8453864 - Flags: review?(rjesup)
Attachment #8453864 - Flags: review?(rjesup) → review+
Comment on attachment 8453864 [details] [diff] [review]
Add missing license headers

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

Just to verify - these were all new-code, not cut-and-paste-and-modified from webrtc.org code, correct?
Assignee

Comment 119

5 years ago
(In reply to Randell Jesup [:jesup] from comment #118)
> Comment on attachment 8453864 [details] [diff] [review]
> Add missing license headers
> 
> Review of attachment 8453864 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just to verify - these were all new-code, not cut-and-paste-and-modified
> from webrtc.org code, correct?

I confirmed with the individual that originally did this work.  These are completely original.

Comment 120

5 years ago
Comment on attachment 8453864 [details] [diff] [review]
Add missing license headers

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

Cisco is fine with licensing the contribution we made to these files as MPL2.
Attachment #8453864 - Flags: review?(fluffy) → review+
Assignee

Comment 121

5 years ago
Rolls up all posted interdiffs.

Note that, as discussed in IRC, the final work will be done as part of bug 1037405 using this this patch as the starting point.
Attachment #8451701 - Attachment is obsolete: true
Attachment #8453226 - Attachment is obsolete: true
Attachment #8454454 - Attachment is obsolete: true
Attachment #8454954 - Flags: review?(dolske)
Assignee

Comment 122

5 years ago
Rolls up all previous interdiffs, and addresses outstanding nits.
Attachment #8452754 - Attachment is obsolete: true
Attachment #8453220 - Attachment is obsolete: true
Assignee

Comment 123

5 years ago
Rolls up all previous interdiffs, and addresses nits.
Attachment #8452751 - Attachment is obsolete: true
Attachment #8453224 - Attachment is obsolete: true
Attachment #8454956 - Flags: review?(rjesup)
Attachment #8454956 - Flags: review?(martin.thomson)
Assignee

Updated

5 years ago
Attachment #8454955 - Flags: review?(martin.thomson)
Attachment #8454955 - Flags: review?(jib)
Assignee

Comment 124

5 years ago
Rolls up all previous interdiffs, and addresses nits.
Attachment #8451733 - Attachment is obsolete: true
Attachment #8454105 - Attachment is obsolete: true
Attachment #8454957 - Flags: review?(rjesup)
Assignee

Comment 125

5 years ago
Addresses nits.
Attachment #8452267 - Attachment is obsolete: true
Attachment #8454958 - Flags: review?(rjesup)
Assignee

Comment 126

5 years ago
Rolls up previous interdiffs and addresses nits.
Attachment #8452414 - Attachment is obsolete: true
Attachment #8453864 - Attachment is obsolete: true
Attachment #8454510 - Attachment is obsolete: true
Attachment #8454960 - Flags: review?(rjesup)
Attachment #8454960 - Flags: feedback?(gpascutto)
Assignee

Comment 127

5 years ago
Addresses nits.
Attachment #8452417 - Attachment is obsolete: true
Attachment #8454961 - Flags: review?(gpascutto)
Attachment #8454961 - Flags: feedback?(rjesup)
Assignee

Updated

5 years ago
Blocks: 1036653
Assignee

Updated

5 years ago
Blocks: 1037997
Comment on attachment 8454956 [details] [diff] [review]
Patch 4. v4 Device Enumeration & MediaEngine

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

Conditional on fixes below.

::: content/media/webrtc/MediaEngineDefault.cpp
@@ +501,5 @@
>    int32_t len = mASources.Length();
>  
> +  // only supports camera sources (for now).
> +  if (aMediaSource != dom::MediaSourceEnum::Camera)
> +  {

brace style here is back to front: new line for function definition, end of line for control statements.  I know, it's kinda screwy, but that's how it's done here apparently.

::: content/media/webrtc/MediaEngineWebRTC.cpp
@@ +137,5 @@
>  #endif
> +
> +  switch (aMediaSource) {
> +    case dom::MediaSourceEnum::Application:
> +      mAppEngineConfig.Set<webrtc::CaptureDeviceInfo>(

mAppEngineConfig is still a member, did you discover that it needed to be somehow?

::: content/media/webrtc/MediaEngineWebRTC.h
@@ +416,5 @@
>    webrtc::VoiceEngine* mVoiceEngine;
>  
> +  // specialized configurations
> +  webrtc::Config mAppEngineConfig;
> +  webrtc::Config mScreenEngineConfig;

See above.
Attachment #8454956 - Flags: review?(martin.thomson) → review+
Assignee

Comment 129

5 years ago
(In reply to Martin Thomson [:mt] from comment #128)
> Comment on attachment 8454956 [details] [diff] [review]
> Patch 4. v4 Device Enumeration & MediaEngine
> 
> Review of attachment 8454956 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Conditional on fixes below.
> 
> ::: content/media/webrtc/MediaEngineDefault.cpp
> @@ +501,5 @@
> >    int32_t len = mASources.Length();
> >  
> > +  // only supports camera sources (for now).
> > +  if (aMediaSource != dom::MediaSourceEnum::Camera)
> > +  {
> 
> brace style here is back to front: new line for function definition, end of
> line for control statements.  I know, it's kinda screwy, but that's how it's
> done here apparently.
> 

Grr ... missed one.  Will fix.

> ::: content/media/webrtc/MediaEngineWebRTC.cpp
> @@ +137,5 @@
> >  #endif
> > +
> > +  switch (aMediaSource) {
> > +    case dom::MediaSourceEnum::Application:
> > +      mAppEngineConfig.Set<webrtc::CaptureDeviceInfo>(
> 
> mAppEngineConfig is still a member, did you discover that it needed to be
> somehow?
> 

See below.

> ::: content/media/webrtc/MediaEngineWebRTC.h
> @@ +416,5 @@
> >    webrtc::VoiceEngine* mVoiceEngine;
> >  
> > +  // specialized configurations
> > +  webrtc::Config mAppEngineConfig;
> > +  webrtc::Config mScreenEngineConfig;
> 
> See above.

webrtc::VideoEngine takes references to webrtc::Config.  The configs still need to be members since the lifetime of the App- and ScreenEngines are longer than the function's scope.

Thought this was a more acceptable solution than refactoring VideoEngine and its descendants or playing funny tricks with memory addressing.
Comment on attachment 8454955 [details] [diff] [review]
Patch 3. v5 Modify constraints for screen sharing

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

r=me with comments addressed. I'm open to opinions on the advanced array problem.

::: content/media/webrtc/MediaTrackConstraints.h
@@ +86,5 @@
>      Triage(Kind::Height).mHeight = mHeight;
>      Triage(Kind::FrameRate).mFrameRate = mFrameRate;
> +
> +    // treat MediaSource special because it's always required
> +    mRequired.mMediaSource = mMediaSource;

Just a general comment that non-camera users are going to have a hard time using the advanced array for anything. They'll have to know to repeat the mediaSource in EVERY entry in the advanced array, or the default "camera" will kick in and make their entries no-ops. :-(

The only way I see around that would be to try to detect such clashes and avoid them. The best place to do this might be here. Specifically, if a non-camera mMediaSource is required, then make sure to copy it to each entry in the advanced array (at least to those that have the default "camera"), otherwise they are dead meat.

::: dom/media/MediaManager.cpp
@@ +758,5 @@
>  static SourceSet *
>    GetSources(MediaEngine *engine,
>               ConstraintsType &aConstraints,
> +             void (MediaEngine::* aEnumerate)(dom::MediaSourceEnum, nsTArray<nsRefPtr<SourceType> >*),
> +             dom::MediaSourceEnum aMediaSource = dom::MediaSourceEnum::Camera,

Why aMediaSource instead of aConstraints.mMediaSource ?

@@ +1206,5 @@
>      if (IsOn(mConstraints.mAudio)) {
>        AudioTrackConstraintsN constraints(GetInvariant(mConstraints.mAudio));
>        ScopedDeletePtr<SourceSet> s (GetSources(backend, constraints,
>            &MediaEngine::EnumerateAudioDevices,
> +          dom::MediaSourceEnum::Camera,

Same as constraints.mMediaSource, so again the extra arg seems redundant.

@@ +1487,5 @@
>    }
>  
> +  // deny screensharing request if support is disabled
> +  if (c.mVideo.IsMediaTrackConstraints() &&
> +      !Preferences::GetBool("media.getusermedia.screensharing.enabled", false)) {

Quibble: I can imagine rationales for checking the pref first or last, but in the middle seems odd. YMMV.
Attachment #8454955 - Flags: review?(jib) → review+
Comment on attachment 8454960 [details] [diff] [review]
Patch 9. v3 DesktopCaptureImpl

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

::: media/webrtc/trunk/webrtc/video_engine/desktop_capture_impl.cc
@@ +57,5 @@
> +                                                       desktopDisplayDevice) == 0) {
> +
> +    const char * deviceName = desktopDisplayDevice.getDeviceName();
> +    if (deviceNameLength > 0 && deviceNameUTF8 && deviceName) {
> +      memset(deviceNameUTF8,0,deviceNameLength);

nit: spacing

@@ +126,5 @@
> +}
> +
> +int32_t AppDeviceInfoImpl::GetDeviceName(uint32_t deviceNumber,
> +                                         char* deviceNameUTF8,
> +                                         uint32_t deviceNameLength,

nit: Given the naming of the other variables, I'd expect this to be called deviceNameUTF8Length.

@@ +198,5 @@
> +
> +  //create real screen capturer.
> +  if (capture->Init(uniqueId,bIsApp)!=0) {
> +    delete capture;
> +    return capture;

Return a reference counted pointer that we just called delete on? This is strange. Should this just return NULL?

@@ +235,5 @@
> +
> +int32_t DesktopCaptureImpl::Init(const char* uniqueId,
> +                                 const bool bIsApp) {
> +  if (bIsApp) {
> +    AppCapturer *pAppCapturer =AppCapturer::Create();

nit: spacing

@@ +240,5 @@
> +    if (!pAppCapturer) {
> +      return -1;
> +    }
> +
> +    ProcessId processid = 0;//uniqueId

uniqueId what?

@@ +251,5 @@
> +    if (!pScreenCapturer) {
> +      return -1;
> +    }
> +
> +    ScreenId screenid = webrtc::kFullDesktopScreenId;//uniqueId

same remark
Attachment #8454960 - Flags: feedback?(gpascutto) → feedback+
Comment on attachment 8454961 [details] [diff] [review]
Patch 10. v2 Null Implementations for deviceinfo

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

::: media/webrtc/trunk/webrtc/modules/desktop_capture/app_capturer.h
@@ +24,5 @@
> +class DesktopCaptureOptions;
> +
> +class AppCapturer : public DesktopCapturer {
> +public:
> +    typedef webrtc::ProcessId ProcessId;

indentation for these files should be 2 spaces, like the other stuff in the same dir

::: media/webrtc/trunk/webrtc/modules/desktop_capture/app_capturer_mac.mm
@@ +73,5 @@
> +  callback_ = callback;
> +}
> +
> +void AppCapturerMac::Capture(const DesktopRegion& region) {
> +  // callback_->OnCaptureCompleted(frame);

Add an explanation, a bug number if it's todo, etc... or just delete it if neither applies.

::: media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_device_info.cc
@@ +34,5 @@
> +
> +DesktopDisplayDevice::~DesktopDisplayDevice() {
> +  screenId_ = kInvalidScreenId;
> +
> +  if (deviceUniqueIdUTF8_){

Not needed, deleting null is fine.

@@ +38,5 @@
> +  if (deviceUniqueIdUTF8_){
> +    delete [] deviceUniqueIdUTF8_;
> +  }
> +
> +  if (deviceNameUTF8_){

Same.

@@ +137,5 @@
> +DesktopDeviceInfoImpl::~DesktopDeviceInfoImpl() {
> +  std::map<intptr_t,DesktopDisplayDevice*>::iterator iterDevice;
> +  for (iterDevice=desktop_display_list_.begin(); iterDevice!=desktop_display_list_.end(); iterDevice++){
> +    DesktopDisplayDevice * pDesktopDisplayDevice = iterDevice->second;
> +    if (pDesktopDisplayDevice) {

Same.

@@ +147,5 @@
> +
> +  std::map<intptr_t,DesktopApplication*>::iterator iterApp;
> +  for (iterApp=desktop_application_list_.begin(); iterApp!=desktop_application_list_.end(); iterApp++){
> +    DesktopApplication * pDesktopApplication = iterApp->second;
> +    if (pDesktopApplication) {

etc.
Attachment #8454961 - Flags: review?(gpascutto) → review+
(In reply to Jan-Ivar Bruaroey [:jib] from comment #130)
> Same as constraints.mMediaSource, so again the extra arg seems redundant.

AudioTrackConstraintsN is derived from MediaTrackConstraintsN which is derived from MediaTrackConstraints so it should still have the mediaSource = "camera" default.
Comment on attachment 8454955 [details] [diff] [review]
Patch 3. v5 Modify constraints for screen sharing

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

I'll note that a lot of this code is likely to change almost immediately, based on the public-media-capture@w3.org discussions.  But that's no reason not to make progress. Ship it.
Attachment #8454955 - Flags: review?(martin.thomson) → review+
Comment on attachment 8454956 [details] [diff] [review]
Patch 4. v4 Device Enumeration & MediaEngine

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

The proliferation of subtypes for sources implies we should templatize it or add another class layer - but that can be for future cleanup work.

::: content/media/webrtc/MediaEngineDefault.cpp
@@ +500,4 @@
>    MutexAutoLock lock(mMutex);
>    int32_t len = mASources.Length();
>  
> +  // only supports camera sources (for now).

File followup bug to provide fake:true screen/etc shares

::: content/media/webrtc/MediaEngineWebRTC.h
@@ +165,5 @@
>    virtual bool IsFake() {
>      return false;
>    }
>  
> +  virtual dom::MediaSourceEnum GetMediaSource(){

space before {
Also: I presume this is const (we never change a source's type)

@@ +388,5 @@
> +  virtual void EnumerateVideoDevices(dom::MediaSourceEnum,
> +                                    nsTArray<nsRefPtr<MediaEngineVideoSource> >*);
> +  virtual void EnumerateAudioDevices(dom::MediaSourceEnum,
> +                                    nsTArray<nsRefPtr<MediaEngineAudioSource> >*);
> +  

trailing spaces

@@ +391,5 @@
> +                                    nsTArray<nsRefPtr<MediaEngineAudioSource> >*);
> +  
> + protected:
> +   void EnumerateCommonVideoDevices(nsTArray<nsRefPtr<MediaEngineVideoSource> >*aVSources,
> +                                   webrtc::VideoEngine* videoEngine,

indent fix; aVideoEngine

@@ +393,5 @@
> + protected:
> +   void EnumerateCommonVideoDevices(nsTArray<nsRefPtr<MediaEngineVideoSource> >*aVSources,
> +                                   webrtc::VideoEngine* videoEngine,
> +                                    bool& aEngineInit,
> +                                    dom::MediaSourceEnum);

aType (or aSource)
Attachment #8454956 - Flags: review?(rjesup) → review+
Comment on attachment 8454957 [details] [diff] [review]
Patch 5. v3 WebRTCViE Input changes

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

::: media/webrtc/trunk/webrtc/video_engine/vie_input_manager.cc
@@ +434,5 @@
> +      case CaptureDeviceType::Camera:
> +        capture_device_info_ = VideoCaptureFactory::CreateDeviceInfo(ViEModuleId(engine_id_));
> +        break;
> +      default:
> +        // Don't try to build anything for unknown/unsupported types

Perhaps add a TRACE call here to log it
Attachment #8454957 - Flags: review?(rjesup) → review+
Comment on attachment 8454958 [details] [diff] [review]
Patch 6. v3 WebRTC ViE Capturer changes

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

::: media/webrtc/trunk/webrtc/video_engine/vie_capturer.cc
@@ +181,5 @@
> +        capture_module_ = DesktopCaptureImpl::Create(
> +            ViEModuleId(engine_id_, capture_id_), device_unique_idUTF8, true);
> +        break;
> +      default:
> +        // all other non-camera types are not supported

add a WEBRTC_TRACE to log the error

::: media/webrtc/trunk/webrtc/video_engine/vie_capturer.h
@@ +191,5 @@
>  
>    CaptureCapability requested_capability_;
>  
>    scoped_ptr<OveruseFrameDetector> overuse_detector_;
> +  const Config & config_;

remove space on one side or the other
Attachment #8454958 - Flags: review?(rjesup) → review+
Assignee

Updated

5 years ago
Assignee: vagouzhou → linuxwolf
Status: NEW → ASSIGNED
Assignee

Updated

5 years ago
Attachment #8455438 - Attachment description: Patch 14. interdiff for review comments [ → Patch 14. interdiff for review comments from Jan-Ivar on Constraints
Assignee

Updated

5 years ago
Attachment #8455473 - Flags: review?(jib)
Assignee

Updated

5 years ago
Attachment #8454955 - Attachment is obsolete: true
Comment on attachment 8455438 [details] [diff] [review]
Patch 14. interdiff for review comments from Jan-Ivar on Constraints

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

lgtm. For future reference I also prefer the patch over an interdiff unless the patch is huge. I generally only ask for interdiffs if Bugzilla's built-in diff-by-revision fails (which isn't never).
Attachment #8455438 - Flags: review+
Attachment #8455438 - Flags: feedback?(martin.thomson)
Assignee

Comment 142

5 years ago
interdiff over attachement 8454956 to address review comments
Comment on attachment 8454960 [details] [diff] [review]
Patch 9. v3 DesktopCaptureImpl

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

::: media/webrtc/trunk/webrtc/video_engine/desktop_capture_impl.cc
@@ +55,5 @@
> +  DesktopDisplayDevice desktopDisplayDevice;
> +  if (desktop_device_info_->getDesktopDisplayDeviceInfo(deviceNumber,
> +                                                       desktopDisplayDevice) == 0) {
> +
> +    const char * deviceName = desktopDisplayDevice.getDeviceName();

nit: * on one side or the other

@@ +57,5 @@
> +                                                       desktopDisplayDevice) == 0) {
> +
> +    const char * deviceName = desktopDisplayDevice.getDeviceName();
> +    if (deviceNameLength > 0 && deviceNameUTF8 && deviceName) {
> +      memset(deviceNameUTF8,0,deviceNameLength);

Move the memset()s out of "if (deviceNameLength > 0" - if it doesn't return a name, don't return with random garbage in the output buffers.  It'll need an if around it

@@ +65,5 @@
> +    }
> +
> +    const char * deviceUniqueId = desktopDisplayDevice.getUniqueIdName();
> +    if (deviceUniqueIdUTF8Length > 0 && deviceUniqueIdUTF8 && deviceUniqueId) {
> +      memset(deviceUniqueIdUTF8, 0, deviceUniqueIdUTF8Length);

ditto

@@ +73,5 @@
> +    }
> +
> +    if (productUniqueIdUTF8Length > 0 && productUniqueIdUTF8) {
> +      memset(productUniqueIdUTF8, 0, productUniqueIdUTF8Length);
> +    }

Perhaps move this (and the other memset()s) to the start of the function.

@@ +138,5 @@
> +
> +    const char * deviceName = desktopApplication.getProcessAppName();
> +    size_t len = deviceName ? strlen(deviceName) : 0;
> +    if (deviceNameLength > 0 && deviceNameUTF8 && deviceName && len <= deviceNameLength) {
> +      memset(deviceNameUTF8, 0, deviceNameLength);

ditto above about memset, etc

@@ +244,5 @@
> +    ProcessId processid = 0;//uniqueId
> +    pAppCapturer->SelectApp(processid);
> +
> +    MouseCursorMonitor * pMouseCursorMonitor = MouseCursorMonitor::CreateForScreen(webrtc::DesktopCaptureOptions::CreateDefault(), webrtc::kFullDesktopScreenId);
> +    desktop_capturer_cursor_composer_.reset(new DesktopAndCursorComposer(pAppCapturer,pMouseCursorMonitor));

spaces after commas

@@ +256,5 @@
> +    pScreenCapturer->SelectScreen(screenid);
> +    pScreenCapturer->SetMouseShapeObserver(this);
> +
> +    MouseCursorMonitor * pMouseCursorMonitor = MouseCursorMonitor::CreateForScreen(webrtc::DesktopCaptureOptions::CreateDefault(), screenid);
> +    desktop_capturer_cursor_composer_.reset(new DesktopAndCursorComposer(pScreenCapturer,pMouseCursorMonitor));

space after comma

@@ +328,5 @@
> +  _lastProcessFrameCount(TickTime::Now()),
> +  _rotateFrame(kRotateNone),
> +  last_capture_time_(TickTime::MillisecondTimestamp()),
> +  delta_ntp_internal_ms_(
> +                         Clock::GetRealTimeClock()->CurrentNtpInMilliseconds() -

remove spurious line break

@@ +353,5 @@
> +    delete[] _deviceUniqueId;
> +}
> +
> +void DesktopCaptureImpl::RegisterCaptureDataCallback(
> +                                                     VideoCaptureDataCallback& dataCallBack)

spurious line break

@@ +428,5 @@
> +
> +  return 0;
> +}
> +
> +int32_t DesktopCaptureImpl::IncomingFrame(

Note in a comment and followup bug that this is cloned from VideoCaptureImp(), and likely would benefit from being factored out in some way to avoid duplication

@@ +429,5 @@
> +  return 0;
> +}
> +
> +int32_t DesktopCaptureImpl::IncomingFrame(
> +                                          uint8_t* videoFrame,

spurious break

@@ +514,5 @@
> +  return 0;
> +}
> +
> +int32_t DesktopCaptureImpl::IncomingI420VideoFrame(I420VideoFrame* video_frame, int64_t captureTime) {
> +

Ditto

@@ +602,5 @@
> +  if (num > 1) {
> +    int64_t diff = (now - _incomingFrameTimes[num - 1]).Milliseconds();
> +    if (diff > 0) {
> +      // round to nearest value rather than return minimumid_
> +      return uint32_t((nrOfFrames * 1000.0f / diff) + 0.5f);

Since the result is int, likely this can be done as integer math - doesn't make a big difference though

::: media/webrtc/trunk/webrtc/video_engine/desktop_capture_impl.h
@@ +122,5 @@
> +   */
> +  static VideoCaptureModule* Create(const int32_t id, const char* uniqueId, const bool bIsApp);
> +  static VideoCaptureModule::DeviceInfo* CreateDeviceInfo(const int32_t id, const bool bIsApp);
> +
> +  int32_t Init(const char* uniqueId,const bool bIsApp);

As commented in IRC, we'll want to change those for window sharing; but let's make that a separate bug

@@ +160,5 @@
> +  virtual int32_t StartCapture(const VideoCaptureCapability& capability) OVERRIDE;
> +  virtual int32_t StopCapture() OVERRIDE;
> +  virtual bool CaptureStarted() OVERRIDE;
> +  virtual int32_t CaptureSettings(VideoCaptureCapability& /*settings*/) OVERRIDE;
> +  VideoCaptureEncodeInterface* GetEncodeInterface(const VideoCodec& /*codec*/) OVERRIDE {return NULL;}

why are those commented out?
Attachment #8454960 - Flags: review?(rjesup) → review+
Assignee

Comment 144

5 years ago
Attachment #8454956 - Attachment is obsolete: true
Attachment #8455480 - Flags: review?(rjesup)
Attachment #8455480 - Flags: review?(martin.thomson)
Comment on attachment 8454954 [details] [diff] [review]
Patch 2. v3. Add Screen Sharing UI

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

AIUI Florian was going to take over the UI patch, and proceed in bug 1037405.
Attachment #8454954 - Flags: review?(dolske) → review-
Comment on attachment 8455438 [details] [diff] [review]
Patch 14. interdiff for review comments from Jan-Ivar on Constraints

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

::: content/media/webrtc/MediaTrackConstraints.h
@@ +45,5 @@
> +
> +    // treat MediaSource special because it's always required
> +    mRequired.mMediaSource = mMediaSource;
> +
> +    if (mMediaSource != dom::MediaSourceEnum::Camera && mAdvanced.WasPassed()) {

These are two highly unrelated conditions, which makes this code suspect.  A more loquacious comment might be needed here.

I'm inclined to defer this sort of fixup given the high probability of having to throw all this work out soon.
Attachment #8455438 - Flags: feedback?(martin.thomson)
Comment on attachment 8455480 [details] [diff] [review]
Patch 4. v5 Device Enumeration & MediaEngine

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

I thought that I'd already given you an r+ on this one.

::: content/media/webrtc/MediaEngineDefault.cpp
@@ +503,4 @@
>    MutexAutoLock lock(mMutex);
>    int32_t len = mASources.Length();
>  
> +  // aMediaSource is ignored for audio devices (for now).

Thinking on this more, why not return if source != Camera ?
Attachment #8455480 - Flags: review?(martin.thomson) → review+
Comment on attachment 8454961 [details] [diff] [review]
Patch 10. v2 Null Implementations for deviceinfo

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

r- only because I want to see the gyp changes I suggested

::: media/webrtc/trunk/webrtc/modules/desktop_capture/app_capturer.cc
@@ +14,5 @@
> +namespace webrtc {
> +
> +// static
> +AppCapturer* AppCapturer::Create() {
> +    return Create(DesktopCaptureOptions::CreateDefault());

indent 2

::: media/webrtc/trunk/webrtc/modules/desktop_capture/app_capturer.h
@@ +36,5 @@
> +    //
> +    static AppCapturer* Create(const DesktopCaptureOptions& options);
> +    static AppCapturer* Create();
> +
> +    //

comment?

::: media/webrtc/trunk/webrtc/modules/desktop_capture/app_capturer_mac.mm
@@ +61,5 @@
> +  return true;
> +}
> +
> +bool AppCapturerMac::BringAppToFront() {
> +   return true;

// Not implemented yet: See Bug 1036653

::: media/webrtc/trunk/webrtc/modules/desktop_capture/app_capturer_win.cc
@@ +75,5 @@
> +  return true;
> +}
> +
> +bool AppCapturerWin::BringAppToFront() {
> +  return true;

// Not implemented yet: See Bug 1036653

@@ +86,5 @@
> +
> +  callback_ = callback;
> +}
> +void AppCapturerWin::Capture(const DesktopRegion& region) {
> +  //callback_->OnCaptureCompleted(frame.release());

comment as to why it's commented out

::: media/webrtc/trunk/webrtc/modules/desktop_capture/app_capturer_x11.cc
@@ +70,5 @@
> +bool AppCapturerLinux::SelectApp(ProcessId id){
> +  return true;
> +}
> +bool AppCapturerLinux::BringAppToFront(){
> +  return true;

// Not implemented yet: See Bug 1036653

@@ +82,5 @@
> +  callback_ = callback;
> +}
> +
> +void AppCapturerLinux::Capture(const DesktopRegion& region) {
> +  //callback_->OnCaptureCompleted(frame);

commented out for a reason?  if so, say why

::: media/webrtc/trunk/webrtc/modules/desktop_capture/mac/desktop_device_info_mac.mm
@@ +35,5 @@
> +  }
> +#endif
> +
> +  DesktopApplication *pDesktopApplication = new DesktopApplication;
> +  if (pDesktopApplication) {

Should this code be controlled by another define?  Or do we leave it and block it for platforms that don't support it elsewhere?

@@ +39,5 @@
> +  if (pDesktopApplication) {
> +    pDesktopApplication->setProcessId(9999);
> +    pDesktopApplication->setProcessAppName("WebEx Meeting Center");
> +    pDesktopApplication->setProcessPathName("\\screen\\monitor#1");
> +    pDesktopApplication->setUniqueIdName("\\app\\9999");

Likely these shouldn't be hard-coded here

::: media/webrtc/trunk/webrtc/modules/desktop_capture/x11/desktop_device_info_x11.cc
@@ +5,5 @@
> +#include "webrtc/modules/desktop_capture/x11/desktop_device_info_x11.h"
> +
> +namespace webrtc{
> +
> +#define MULTI_MONITOR_NO_SUPPORT 1

I'd prefer to have feature defines be "supports foo" not "foo not supported".

So, #ifdef MULTI_MONITOR_SCREENSHARE perhaps.  And move it to be in a .gyp file ('multimonitor_screenshare%': 0,), and with 
 ['multimonitor_screenshare != 0', {
   'defines': [ 'MULTI_MONITOR_SCREENSHARE' ]
 }],
in a conditional somewhere in the gyp/gypi.

Then, to turn it on: in build/gyp.mozbuild, you can just override the value.  (Add a commented out setting there)
This makes it easier to get the changes upstreamed to webrtc.org.


repeat for all the other instances of this for other platforms.
Attachment #8454961 - Flags: feedback?(rjesup) → feedback-
Comment on attachment 8455480 [details] [diff] [review]
Patch 4. v5 Device Enumeration & MediaEngine

THanks; much easier to review with the interdiffs available
Attachment #8455480 - Flags: review?(rjesup) → review+
Assignee

Comment 151

5 years ago
Attachment #8454960 - Attachment is obsolete: true
Attachment #8455547 - Flags: review?(rjesup)
Attachment #8455547 - Flags: feedback?(gpascutto)
Comment on attachment 8455546 [details] [diff] [review]
Patch 16 - Interdiff for review/feedback comments from Jesup and GCP on DesktopCaptureImpl

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

::: media/webrtc/trunk/webrtc/video_engine/desktop_capture_impl.cc
@@ +159,3 @@
>    if (desktop_device_info_->getApplicationInfo(deviceNumber,desktopApplication) == 0) {
> +    size_t len; 
> +    

trailing spaces on those lines
Attachment #8455547 - Flags: review?(rjesup) → review+
Attachment #8455473 - Flags: review?(jib) → review+
Depends on: 1038799
Assignee

Comment 153

5 years ago
Interdiff over attachment 845961 to address review comments.
Assignee

Comment 154

5 years ago
Attachment #8454961 - Attachment is obsolete: true
Attachment #8456339 - Flags: review?(rjesup)
(In reply to Justin Dolske [:Dolske] from comment #145)
> Comment on attachment 8454954 [details] [diff] [review]
> Patch 2. v3. Add Screen Sharing UI
> 
> Review of attachment 8454954 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> AIUI Florian was going to take over the UI patch, and proceed in bug 1037405.

I attached a simplified version of this patch in bug 1037405.
Assignee

Comment 156

5 years ago
Attachment #8456358 - Flags: review?(ted)
Attachment #8456358 - Flags: feedback?(rjesup)
Attachment #8456339 - Flags: review?(rjesup) → review+
Assignee

Updated

5 years ago
Blocks: 1038926
Attachment #8456358 - Flags: feedback?(rjesup) → feedback+
Comment on attachment 8456358 [details] [diff] [review]
Interdiff for conditional multi-monitor support

Jumping in since :jesup asked for a quick review in IRC.
Attachment #8456358 - Flags: review?(ted) → review+
Comment on attachment 8456583 [details] [diff] [review]
Patch to fix Android emulator build errors

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

I made the same fix for my try...
Attachment #8456583 - Flags: review?(rjesup) → review+
Comment on attachment 8455547 [details] [diff] [review]
Patch 9. v4 DesktopCaptureImpl

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

::: media/webrtc/trunk/webrtc/video_engine/desktop_capture_impl.cc
@@ +157,5 @@
> +  }
> +
> +  if (desktop_device_info_->getApplicationInfo(deviceNumber,desktopApplication) == 0) {
> +    size_t len; 
> +    

nit: spurious whitespace

@@ +366,5 @@
> +  delete &_callBackCs;
> +  delete &_apiCs;
> +
> +  if (_deviceUniqueId)
> +    delete[] _deviceUniqueId;

Don't need the if.
Attachment #8455547 - Flags: feedback?(gpascutto) → feedback+
Cleaned up the patches, incorporated last feedback above, filtered out the GUI patches that aren't done yet and the window sharing:
https://tbpl.mozilla.org/?tree=Try&rev=fd67e825fe16

If this goes green those patches will land ASAP.
remote: ************************** ERROR ****************************
remote: 
remote: WebIDL file dom/webidl/Constraints.webidl altered in changeset a3dd70ee3823 without DOM peer review
remote: WebIDL file dom/webidl/MediaTrackConstraintSet.webidl altered in changeset a3dd70ee3823 without DOM peer review
remote: 
remote: 
Changes to WebIDL files in this repo require review from a DOM peer in the form of r=...
This is to ensure that we behave responsibly with exposing new Web APIs. We appreciate your understanding..
remote: 
remote: *************************************************************

This is add-screenshare-contraints, in the patch queue.
Attachment #8455473 - Flags: review+ → review?(bobbyholley)
Assignee

Comment 164

5 years ago
Note that attachment 8455473 [details] [diff] [review] changes Constraints.webidl and MediaTrackConstraints.webidl to add the mediaSource constraint for getUserMedia().

More information on mediaSource and screen sharing is available at < https://fluffy.github.io/w3c-screen-share/ >.
Comment on attachment 8455473 [details] [diff] [review]
Patch 3. v6 Modify constraints for screen sharing

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

This isn't a very useful review request.

Can you please explain what this patch is about, what web-facing behavior it changes (if any), and what spec specifies that behavior?
Attachment #8455473 - Flags: review?(bobbyholley)
Comment on attachment 8455473 [details] [diff] [review]
Patch 3. v6 Modify constraints for screen sharing

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

Looks like jib r+ed this between comment 152 and comment 153, so I'll defer to his judgement. In the future, please leave his r+ on the patch and flag for "additional review" rather than resetting it.
Attachment #8455473 - Flags: review+
Comment on attachment 8451690 [details] [diff] [review]
Patch 8. Buildsystem changes

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

::: configure.in
@@ +8133,5 @@
>  
>      MOZ_CAIRO_OSLIBS='${CAIRO_FT_OSLIBS}'
>  
>      if test "$MOZ_X11"; then
> +        MOZ_CAIRO_OSLIBS="$MOZ_CAIRO_OSLIBS $XLDFLAGS -lXext -lXdamage -lXfixes -lXcomposite -lXrender"

This is very much a variable abuse, because it's totally unrelated to cairo, and that block is for in-tree cairo only, and that breaks gtk3 builds with pedantic linkers, because they don't get those flags, and fail to link because of the missing symbols (but non pedantic linkers don't care because something else pulls those libraries indirectly).
Depends on: 1039897
Duplicate of this bug: 923227
Depends on: 1040098
The build system change caused some desktop and b2g builds to fail, because it makes the build depend on gmodule, which is supposed to be a gtk dependency but not on some platforms like Ubuntu 12.04 LTS.

I filed bug 1040025, but it was marked invalid because the fault arguably is on the platform's side.

Chromium fixed their build system to add the dependency: http://src.chromium.org/viewvc/chrome/trunk/src/build/linux/system.gyp?r1=126148&r2=126147&pathrev=126148

Updated

5 years ago
Depends on: 1040168
>because it makes the build depend on gmodule

This patch adds X11 libs, not GTK libs. It's actually toolkit-agnostic after bug 1039897 landed. It's a pure Ubuntu bug, you're better off arguing in bug 1040025 that we work around it in our GTK config. 

But it's has absolutely nothing to do with this bug.

Updated

5 years ago
Depends on: 1041268
Depends on: 1042525
Depends on: 1061663

Updated

5 years ago
Blocks: 1063547
No longer blocks: 1063547
Depends on: 1063547
You need to log in before you can comment on or make changes to this bug.