Extend content::DesktopMediaID to allow Aura windows as capture sources.

Extend content::DesktopMediaID to allow Aura windows as capture sources.

The new DesktopCaptureID::TYPE_AURA_WINDOW will be used for
Desktop Capture API to tell the content layer to capture Aura windows
instead of native screen/windows.

Also moved DesktopCaptureID to content/public/browser from
content/public/common - it doesn't make sense to use that class in the renderer.

BUG=295102
R=ben@chromium.org, cpu@chromium.org, hshi@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=241031

[jam, 2013-12-04 18:29:23]

it seems odd that this API would have to differentiate between a normal window
and an aura window, even ignoring the fact that an aura window is only available
on some platforms and not all. In C++ code, we have the gfx::NativeWindow
typedef to abstract this. Can the API use this?

[Sergey Ulanov, 2013-12-04 20:35:54]

On 2013/12/04 18:29:23, jam wrote:
> it seems odd that this API would have to differentiate between a normal window
> and an aura window, even ignoring the fact that an aura window is only
available
> on some platforms and not all. 

Desktop Capture API itself will not allow to differentiate between two types of
windows. But in the implementation we do need to differentiate between them
because on Windows with Ash we will want to support capturing Aura windows and
native windows from other apps (i.e. window picker dialog will show both types
of windows). Ash and native windows will also be captured differently.
DesktopMediaID is used to store the id of the screen/window selected by the
user.

> In C++ code, we have the gfx::NativeWindow
> typedef to abstract this. Can the API use this?

With Aura gfx::NativeWindow is aura::Window*, but with DesktopMediaID we want to
be able to reference native windows from other apps (which don't have
corresponding aura::Window object). Second problem is that even when referencing
Aura window we need to be able to detect when the window is destroyed. That
won't be possible if we were storing raw aura::Window pointer in DesktopMediaID.

[hshi1, 2013-12-04 21:15:39]

https://codereview.chromium.org/100743003/diff/40001/content/browser/renderer...
File content/browser/renderer_host/media/desktop_capture_device_ash.h (right):

https://codereview.chromium.org/100743003/diff/40001/content/browser/renderer...
content/browser/renderer_host/media/desktop_capture_device_ash.h:24: class
CONTENT_EXPORT DesktopCaptureDeviceAsh
If I understand it correctly, this class will be used when USE_AURA is true but
USE_ASH is not, is this correct?

You may want to update the comments here and probably rename the class name to
DesktopCaptureDeviceAura.

[Sergey Ulanov, 2013-12-04 21:41:07]

https://codereview.chromium.org/100743003/diff/40001/content/browser/renderer...
File content/browser/renderer_host/media/desktop_capture_device_ash.h (right):

https://codereview.chromium.org/100743003/diff/40001/content/browser/renderer...
content/browser/renderer_host/media/desktop_capture_device_ash.h:24: class
CONTENT_EXPORT DesktopCaptureDeviceAsh
On 2013/12/04 21:15:39, hshi1 wrote:
> If I understand it correctly, this class will be used when USE_AURA is true
but
> USE_ASH is not, is this correct?
> 
> You may want to update the comments here and probably rename the class name to
> DesktopCaptureDeviceAura.

Good point. I'll rename it in a separate CL.

[hshi1, 2013-12-04 21:45:50]

On 2013/12/04 21:41:07, Sergey Ulanov wrote:
>
https://codereview.chromium.org/100743003/diff/40001/content/browser/renderer...
> File content/browser/renderer_host/media/desktop_capture_device_ash.h (right):
> 
>
https://codereview.chromium.org/100743003/diff/40001/content/browser/renderer...
> content/browser/renderer_host/media/desktop_capture_device_ash.h:24: class
> CONTENT_EXPORT DesktopCaptureDeviceAsh
> On 2013/12/04 21:15:39, hshi1 wrote:
> > If I understand it correctly, this class will be used when USE_AURA is true
> but
> > USE_ASH is not, is this correct?
> > 
> > You may want to update the comments here and probably rename the class name
to
> > DesktopCaptureDeviceAura.
> 
> Good point. I'll rename it in a separate CL.

Actually it seems that renaming is necessary for this CL to build properly for
platforms that has USE_AURA but not USE_ASH. The problem is that the default
filename rules (see src/build/filename_rules.gypi) would exclude source files
matching the pattern of "(^|/)ash/" when use_ash is 0.

[Sergey Ulanov, 2013-12-04 23:41:25]

Renamed it in https://codereview.chromium.org/104653007/. PTAL

[jam, 2013-12-05 00:20:08]

I'm adding cpu and ben for their thoughts on this, I'll defer to them as they're
more familiar with aura/ash on windows.

[hshi1, 2013-12-05 01:36:09]

https://codereview.chromium.org/100743003/diff/70001/content/browser/renderer...
File content/browser/renderer_host/media/desktop_capture_device_aura.cc (right):

https://codereview.chromium.org/100743003/diff/70001/content/browser/renderer...
content/browser/renderer_host/media/desktop_capture_device_aura.cc:195:
desktop_window_->bounds().height()));
Why not use desktop_window_->bounds() directly here?

I'll probably manually test this on a device, but the desktop_window_->bounds()
does not necessarily have origin at (0,0) in a multi-monitor configuration,
although on some platforms maybe the primary desktop always have origin at
(0,0).

[Sergey Ulanov, 2013-12-05 01:41:50]

https://codereview.chromium.org/100743003/diff/70001/content/browser/renderer...
File content/browser/renderer_host/media/desktop_capture_device_aura.cc (right):

https://codereview.chromium.org/100743003/diff/70001/content/browser/renderer...
content/browser/renderer_host/media/desktop_capture_device_aura.cc:195:
desktop_window_->bounds().height()));
On 2013/12/05 01:36:10, hshi1 wrote:
> Why not use desktop_window_->bounds() directly here?
> 
> I'll probably manually test this on a device, but the
desktop_window_->bounds()
> does not necessarily have origin at (0,0) in a multi-monitor configuration,
> although on some platforms maybe the primary desktop always have origin at
> (0,0).

Area is set relative to the window layer, while bounds() are relative to the
parent window. I think separate monitors have separate layers. Previously this
code didn't work correctly for regular windows which have origin different from
(0,0).

[hshi1, 2013-12-05 01:51:44]

LGTM with one more comment.

https://codereview.chromium.org/100743003/diff/70001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/100743003/diff/70001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.cc:28: #if
defined(OS_CHROMEOS)
this should now be #if defined(USE_AURA).

[cpu_(ooo_6.6-7.5), 2013-12-06 19:23:28]

I am fairly confused about the use of the ifdefs on this CL. 
Let me tell you my understanding

- If you are doing an ash specific piece you should use USE_ASH, it is an
unfortunate situation that we have two flags but existing code uses them.

- On Windows aura implies ash, 
- On Linux you can have aura but not ash

In jan 14, we start removing USE_AURA, if by that point we compile ash on linux
(even if we don't use it) then we can remove them both.

[Sergey Ulanov, 2013-12-06 19:36:47]

On 2013/12/06 19:23:28, cpu wrote:
> I am fairly confused about the use of the ifdefs on this CL. 
> Let me tell you my understanding
> 
> - If you are doing an ash specific piece you should use USE_ASH, it is an
> unfortunate situation that we have two flags but existing code uses them.
> 
> - On Windows aura implies ash, 
> - On Linux you can have aura but not ash
> 
> In jan 14, we start removing USE_AURA, if by that point we compile ash on
linux
> (even if we don't use it) then we can remove them both.

The new TYPE_AURA_WINDOW is not Ash-specific. Initially it will be used only
with ASH builds, but in future I plan to use it for Aura builds without Ash too.
In Chrome we can capture any Aura window directly without going through the OS,
and that would be more efficient.

[Sergey Ulanov, 2013-12-06 19:51:38]

https://codereview.chromium.org/100743003/diff/70001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/100743003/diff/70001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.cc:28: #if
defined(OS_CHROMEOS)
On 2013/12/05 01:51:44, hshi1 wrote:
> this should now be #if defined(USE_AURA).

Done.

[cpu_(ooo_6.6-7.5), 2013-12-07 02:14:37]

https://codereview.chromium.org/100743003/diff/90001/content/public/browser/d...
File content/public/browser/desktop_media_id.cc (right):

https://codereview.chromium.org/100743003/diff/90001/content/public/browser/d...
content/public/browser/desktop_media_id.cc:60: std::map<int, aura::Window*>
id_to_window_map_;
I am not sure this is the best practice here, we already have lists of all the
windows, other cases I've seen we just stick stuff in a property bag associated
with each window.

Let's see Ben's opinion here. Maybe this way is better.

Also is there a doc one can read to understand how this works?

[Sergey Ulanov, 2013-12-07 02:29:33]

https://codereview.chromium.org/100743003/diff/90001/content/public/browser/d...
File content/public/browser/desktop_media_id.cc (right):

https://codereview.chromium.org/100743003/diff/90001/content/public/browser/d...
content/public/browser/desktop_media_id.cc:60: std::map<int, aura::Window*>
id_to_window_map_;
On 2013/12/07 02:14:37, cpu wrote:
> I am not sure this is the best practice here, we already have lists of all the
> windows, other cases I've seen we just stick stuff in a property bag
associated
> with each window.

I don't think there is a place where we store list of all existing windows.
BTW, I first tried adding it in aura layer:
https://codereview.chromium.org/100603002/

Basically the problem that we need a way to pass a window from chrome layer to
content. Passing raw aura::Window* won't work because it may be destroyed on UI
thread while being passed on IO thread.

> 
> Let's see Ben's opinion here. Maybe this way is better.
> 
> Also is there a doc one can read to understand how this works?

Here is the documentation for the API:
http://developer.chrome.com/extensions/desktopCapture.html .

[Sergey Ulanov, 2013-12-09 19:42:03]

cpu, ben: ping

[Ben Goodger (Google), 2013-12-10 17:43:00]

On 2013/12/09 19:42:03, Sergey Ulanov wrote:
> cpu, ben: ping

I will get to this today.

[hshi1, 2013-12-10 21:24:02]

https://codereview.chromium.org/100743003/diff/90001/chrome/browser/media/med...
File chrome/browser/media/media_capture_devices_dispatcher.cc (right):

https://codereview.chromium.org/100743003/diff/90001/chrome/browser/media/med...
chrome/browser/media/media_capture_devices_dispatcher.cc:354:
screen_id.ToString(), "Screen"));
Any reason why the above change is only applicable to
ProcessScreenCaptureRequest but not ProcessDesktopCaptureRequest?

FYI I have a re-factor CL that would cause these 2 functions to share code (see
https://codereview.chromium.org/98233009/)

[Sergey Ulanov, 2013-12-10 22:29:56]

https://codereview.chromium.org/100743003/diff/90001/chrome/browser/media/med...
File chrome/browser/media/media_capture_devices_dispatcher.cc (right):

https://codereview.chromium.org/100743003/diff/90001/chrome/browser/media/med...
chrome/browser/media/media_capture_devices_dispatcher.cc:354:
screen_id.ToString(), "Screen"));
On 2013/12/10 21:24:02, hshi1 wrote:
> Any reason why the above change is only applicable to
> ProcessScreenCaptureRequest but not ProcessDesktopCaptureRequest?
> 
> FYI I have a re-factor CL that would cause these 2 functions to share code
(see
> https://codereview.chromium.org/98233009/)

ProcessDesktopCaptureAccessRequest() get MediaID that comes from
DesktopMediaList interface. Ash-specific implementation of DesktopMediaList will
be returning media IDs of type TYPE_AURA_WINDOW, see
https://codereview.chromium.org/100833002/ . On the other hand,
ProcessScreenCaptureAccessRequest() is called when screen capture is requested
directly - in that case DesktopMediaList is not involved, and we need to create
correct DesktopMediaID that refers to either Ash or Native screen.

[Ben Goodger (Google), 2013-12-10 23:09:52]

Is this for capturing any window on the ash desktop or just the content area?

[Sergey Ulanov, 2013-12-11 00:14:45]

On 2013/12/10 23:09:52, Ben Goodger (Google) wrote:
> Is this for capturing any window on the ash desktop or just the content area?

It's for capturing whole windows, not only content area. Capturing of Ash
windows will be enabled with this CL: https://codereview.chromium.org/100833002/

[Sergey Ulanov, 2013-12-11 19:41:46]

ben: ping

[Ben Goodger (Google), 2013-12-13 00:55:44]

lgtm

[cpu_(ooo_6.6-7.5), 2013-12-14 01:22:39]

lgtm

[commit-bot: I haz the power, 2013-12-14 06:31:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/100743003/170001

[commit-bot: I haz the power, 2013-12-14 06:31:37]

Failed to apply patch for content/renderer/media/media_stream_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/renderer/media/media_stream_impl.cc
  Hunk #2 FAILED at 73.
  1 out of 2 hunks FAILED -- saving rejects to file
content/renderer/media/media_stream_impl.cc.rej

Patch:       content/renderer/media/media_stream_impl.cc
Index: content/renderer/media/media_stream_impl.cc
diff --git a/content/renderer/media/media_stream_impl.cc
b/content/renderer/media/media_stream_impl.cc
index
a1dac1d6403b08ebc9a801eac19ccabd58015dd1..594b1135df5c6c28f214ac4c08c2559d672b034d
100644
--- a/content/renderer/media/media_stream_impl.cc
+++ b/content/renderer/media/media_stream_impl.cc
@@ -10,7 +10,6 @@
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
-#include "content/public/common/desktop_media_id.h"
 #include "content/renderer/media/media_stream_audio_renderer.h"
 #include "content/renderer/media/media_stream_dependency_factory.h"
 #include "content/renderer/media/media_stream_dispatcher.h"
@@ -74,8 +73,8 @@ void UpdateRequestOptions(
           kMediaStreamSourceId, true);
     } else if (video_stream_source == kMediaStreamSourceScreen) {
       options->video_type = content::MEDIA_DESKTOP_VIDEO_CAPTURE;
-      options->video_device_id =
-          DesktopMediaID(DesktopMediaID::TYPE_SCREEN, 0).ToString();
+      // Set empty video device ID when screen capture is requested.
+      options->video_device_id.clear();
     } else if (video_stream_source == kMediaStreamSourceDesktop) {
       options->video_type = content::MEDIA_DESKTOP_VIDEO_CAPTURE;
       options->video_device_id = GetStreamConstraint(

[commit-bot: I haz the power, 2013-12-14 08:19:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/100743003/190001

[commit-bot: I haz the power, 2013-12-14 09:16:24]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-12-16 09:36:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/100743003/190001

[commit-bot: I haz the power, 2013-12-16 11:11:33]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[Sergey Ulanov, 2013-12-16 21:53:28]

Committed patchset #8 manually as r241031 (presubmit successful).