content: Split out VideoCaptureMachine from DesktopCaptureDeviceAura

content: Split out VideoCaptureMachine from DesktopCaptureDeviceAura

This refactoring would allow the creation of capturing devices without having a
DesktopMediaID. This change pulls the DesktopVideoCaptureMachine class out from
DesktopCaptureDeviceAura and renames it to AuraWindowCaptureMachine. The class
is also refactored to take the target window (rather than a DesktopMediaID)
after construction through a SetWindow method.

BUG=425060
TEST=Using Chrome Cast beta extension and capture both screen and window
and observe that the capture works correctly and that histogram values
are updated.

Committed: https://crrev.com/8a244ea4bd4547fa7481b548b6997f3412f6c215
Cr-Commit-Position: refs/heads/master@{#320953}

[robert.bradford, 2015-03-02 18:47:04]

robert.bradford@intel.com changed reviewers:
+ sergeyu@chromium.org

[robert.bradford, 2015-03-02 18:47:05]

robert.bradford@intel.com changed required reviewers:
+ sergeyu@chromium.org

[robert.bradford, 2015-03-02 18:47:05]

Hi Sergey, in follow-up to your comments on my virtual desktop capturing patch
please can you take a look at this CL making the changes you requested.

Thanks!

[Sergey Ulanov, 2015-03-02 21:58:59]

https://codereview.chromium.org/974513002/diff/1/content/browser/media/captur...
File content/browser/media/capture/desktop_capture_device_aura.cc (right):

https://codereview.chromium.org/974513002/diff/1/content/browser/media/captur...
content/browser/media/capture/desktop_capture_device_aura.cc:519: aura::Window*
window = content::DesktopMediaID::GetAuraWindowById(source);
DesktopMediaID::GetAuraWindowById() is not thread-safe and must be called on the
UI thread, while this code is executed on the device thread. Another related
problem is that |window| may be destroyed between Create() and
DesktopVideoCaptureMachine::Start() (note that Start() adds observer for the
window to get notified when it's destroyed). Sorry, I forgot about it when
reviewing your CL for virtual screens.

I suggest adding SetWindow(window) method in DesktopVideoCaptureMachine and
removing the window parameter from the constructor. Then you can post a task
from DesktopCaptureDeviceAura() to get window ID resolved on the UI thread.
After that, to get virtual screen implemented:
 1. Move DesktopVideoCaptureMachine to a separate file, and maybe rename it to
AuraWindowCaptureMachine.
 2. Add VirtualScreenCaptureDevice class that is the same
DesktopCaptureDeviceAura except it creates a virtual screen.

[robert.bradford, 2015-03-03 17:21:09]

Hi Sergey, thanks for the review feedback. I've done as you've requested. In
order to make it easier to review i've put the rename and split out into its own
patch (and avoided running git cl format over everything!)

Thanks.

[Sergey Ulanov, 2015-03-04 01:54:03]

https://codereview.chromium.org/974513002/diff/100001/content/browser/media/c...
File content/browser/media/capture/desktop_capture_device_aura.cc (right):

https://codereview.chromium.org/974513002/diff/100001/content/browser/media/c...
content/browser/media/capture/desktop_capture_device_aura.cc:33:
device->AsWeakPtr(), source));
weak pointers are not thread-safe and shouldn't be used to post tasks from one
thread to another. I believe this code should DCHECK in debug builds. See my
comment below for how you can avoid weak pointers here.

https://codereview.chromium.org/974513002/diff/100001/content/browser/media/c...
content/browser/media/capture/desktop_capture_device_aura.cc:48: void
DesktopCaptureDeviceAura::SetWindow(const DesktopMediaID& source) {
This doesn't need to be a class member. You can have a static function instead
that takes AuraWindowCaptureMachine* and DesktopMediaID as arguments, resolves
DesktopMediaID and passes it to the AuraWindowCaptureMachine::SetWindow().
Passing raw pointer to AuraWindowCaptureMachine is safe because it's always
destroyed on the UI thread. That way you can avoid the problem mentioned above.

https://codereview.chromium.org/974513002/diff/100001/content/browser/media/c...
File content/browser/media/capture/desktop_capture_device_aura.h (right):

https://codereview.chromium.org/974513002/diff/100001/content/browser/media/c...
content/browser/media/capture/desktop_capture_device_aura.h:28: public
base::SupportsWeakPtr<DesktopCaptureDeviceAura> {
Please use WeakPtrFactory<> as a member instead of inheriting from
SupportsWeakPtr<>. But you don't need weak pointers here at all - see my
comments in .cc

[robert.bradford, 2015-03-04 18:33:06]

Hi Sergey, thanks for the education wrt. WeakPtr. I've also rebased this patch
on the changes miu@ landed yesterday.

If you could take another look that would be appreciated.

https://codereview.chromium.org/974513002/diff/100001/content/browser/media/c...
File content/browser/media/capture/desktop_capture_device_aura.cc (right):

https://codereview.chromium.org/974513002/diff/100001/content/browser/media/c...
content/browser/media/capture/desktop_capture_device_aura.cc:48: void
DesktopCaptureDeviceAura::SetWindow(const DesktopMediaID& source) {

> Passing raw pointer to AuraWindowCaptureMachine is safe because it's always
> destroyed on the UI thread. That way you can avoid the problem mentioned
above.

Ah, this was the insight that I was missing and I also wrongly thought that
WeakPtr was the solution. The existing code makes use of them when posting tasks
however I missed that the request is made from the UI thread so the WeakPtr is
safe.

[Sergey Ulanov, 2015-03-04 18:48:59]

https://codereview.chromium.org/974513002/diff/120001/content/browser/media/c...
File content/browser/media/capture/aura_window_capture_machine.cc (right):

https://codereview.chromium.org/974513002/diff/120001/content/browser/media/c...
content/browser/media/capture/aura_window_capture_machine.cc:448:
DesktopCaptureDeviceAura::DesktopCaptureDeviceAura()
Move this to desktop_capture_device_aura.cc?

https://codereview.chromium.org/974513002/diff/120001/content/browser/media/c...
content/browser/media/capture/aura_window_capture_machine.cc:454:
DesktopCaptureDeviceAura::~DesktopCaptureDeviceAura() {
Move this to desktop_capture_device_aura.cc

https://codereview.chromium.org/974513002/diff/120001/content/browser/media/c...
File content/browser/media/capture/desktop_capture_device_aura.cc (right):

https://codereview.chromium.org/974513002/diff/120001/content/browser/media/c...
content/browser/media/capture/desktop_capture_device_aura.cc:33:
BrowserThread::PostTask(
Move this to DesktopCaptureDeviceAura() constructor, then you will not need
machine_ member.

https://codereview.chromium.org/974513002/diff/120001/content/browser/media/c...
File content/browser/media/capture/desktop_capture_device_aura.h (right):

https://codereview.chromium.org/974513002/diff/120001/content/browser/media/c...
content/browser/media/capture/desktop_capture_device_aura.h:42:
AuraWindowCaptureMachine* machine_;
I don't think you need this.

[robert.bradford, 2015-03-04 20:52:58]

Patchset #8 (id:140001) has been deleted

[robert.bradford, 2015-03-04 20:59:11]

Patchset #8 (id:160001) has been deleted

[robert.bradford, 2015-03-04 21:01:06]

Patchset #8 (id:180001) has been deleted

[robert.bradford, 2015-03-04 21:05:10]

Hi Sergey, thanks for the feedback, I also spotted that if I reordered the
position of the Create method it made the patch easier to read and fixed a
spurious include of desktop_capture_device_aura.h in the
aura_window_capture_machine.cc

https://codereview.chromium.org/974513002/diff/120001/content/browser/media/c...
File content/browser/media/capture/aura_window_capture_machine.cc (right):

https://codereview.chromium.org/974513002/diff/120001/content/browser/media/c...
content/browser/media/capture/aura_window_capture_machine.cc:448:
DesktopCaptureDeviceAura::DesktopCaptureDeviceAura()
On 2015/03/04 18:48:58, Sergey Ulanov wrote:
> Move this to desktop_capture_device_aura.cc?

Oops. I messed that up when I was doing the rebase. I missed this despite
looking over the patch twice.

https://codereview.chromium.org/974513002/diff/120001/content/browser/media/c...
content/browser/media/capture/aura_window_capture_machine.cc:454:
DesktopCaptureDeviceAura::~DesktopCaptureDeviceAura() {
On 2015/03/04 18:48:58, Sergey Ulanov wrote:
> Move this to desktop_capture_device_aura.cc

Done.

https://codereview.chromium.org/974513002/diff/120001/content/browser/media/c...
File content/browser/media/capture/desktop_capture_device_aura.cc (right):

https://codereview.chromium.org/974513002/diff/120001/content/browser/media/c...
content/browser/media/capture/desktop_capture_device_aura.cc:33:
BrowserThread::PostTask(
On 2015/03/04 18:48:58, Sergey Ulanov wrote:
> Move this to DesktopCaptureDeviceAura() constructor, then you will not need
> machine_ member.

Done although I had to make the scoped_ptr for core_ non-const as I was now
setting it in the body of the constructor.

https://codereview.chromium.org/974513002/diff/120001/content/browser/media/c...
File content/browser/media/capture/desktop_capture_device_aura.h (right):

https://codereview.chromium.org/974513002/diff/120001/content/browser/media/c...
content/browser/media/capture/desktop_capture_device_aura.h:42:
AuraWindowCaptureMachine* machine_;
On 2015/03/04 18:48:59, Sergey Ulanov wrote:
> I don't think you need this.

Done.

[Sergey Ulanov, 2015-03-04 22:07:42]

https://codereview.chromium.org/974513002/diff/200001/content/browser/media/c...
File content/browser/media/capture/aura_window_capture_machine.cc (right):

https://codereview.chromium.org/974513002/diff/200001/content/browser/media/c...
content/browser/media/capture/aura_window_capture_machine.cc:124:
DCHECK(desktop_window_);
window may be destroyed between SetWindow() and Start(). You want to handle that
case here by returning false.

https://codereview.chromium.org/974513002/diff/200001/content/browser/media/c...
content/browser/media/capture/aura_window_capture_machine.cc:183:
desktop_window_->GetHost()->compositor()->AddObserver(this);
I think this should be kept in Start(). Otherwise Capture() may be called before
Start()

https://codereview.chromium.org/974513002/diff/200001/content/browser/media/c...
File content/browser/media/capture/desktop_capture_device_aura.cc (right):

https://codereview.chromium.org/974513002/diff/200001/content/browser/media/c...
content/browser/media/capture/desktop_capture_device_aura.cc:32:
scoped_ptr<VideoCaptureMachine>(machine)));
make_scoped_ptr()

https://codereview.chromium.org/974513002/diff/200001/content/browser/media/c...
content/browser/media/capture/desktop_capture_device_aura.cc:33:
BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
Add a comment that |core_| deletes |machine| on the UI thread, so it's safe to
pass raw pointer to UI thread here.

[robert.bradford, 2015-03-05 18:12:11]

Hi Sergey, updated the patch with your review feedback. Please take a look when
you can.

https://codereview.chromium.org/974513002/diff/200001/content/browser/media/c...
File content/browser/media/capture/aura_window_capture_machine.cc (right):

https://codereview.chromium.org/974513002/diff/200001/content/browser/media/c...
content/browser/media/capture/aura_window_capture_machine.cc:124:
DCHECK(desktop_window_);
On 2015/03/04 22:07:42, Sergey Ulanov wrote:
> window may be destroyed between SetWindow() and Start(). You want to handle
that
> case here by returning false.

Done.

https://codereview.chromium.org/974513002/diff/200001/content/browser/media/c...
content/browser/media/capture/aura_window_capture_machine.cc:183:
desktop_window_->GetHost()->compositor()->AddObserver(this);
On 2015/03/04 22:07:42, Sergey Ulanov wrote:
> I think this should be kept in Start(). Otherwise Capture() may be called
before
> Start()

Done.

https://codereview.chromium.org/974513002/diff/200001/content/browser/media/c...
File content/browser/media/capture/desktop_capture_device_aura.cc (right):

https://codereview.chromium.org/974513002/diff/200001/content/browser/media/c...
content/browser/media/capture/desktop_capture_device_aura.cc:32:
scoped_ptr<VideoCaptureMachine>(machine)));
On 2015/03/04 22:07:42, Sergey Ulanov wrote:
> make_scoped_ptr()

Done.

https://codereview.chromium.org/974513002/diff/200001/content/browser/media/c...
content/browser/media/capture/desktop_capture_device_aura.cc:33:
BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
On 2015/03/04 22:07:42, Sergey Ulanov wrote:
> Add a comment that |core_| deletes |machine| on the UI thread, so it's safe to
> pass raw pointer to UI thread here.

Done.

[Sergey Ulanov, 2015-03-16 19:42:32]

LGTM. Sorry about the delay.

[robert.bradford, 2015-03-17 18:51:40]

robert.bradford@intel.com changed reviewers:
+ sievers@chromium.org

[robert.bradford, 2015-03-17 18:51:40]

robert.bradford@intel.com changed required reviewers:
+ sievers@chromium.org

[robert.bradford, 2015-03-17 18:51:41]

Hi sievers@ can you review the changes in content/browser/BUILD.gn

[no sievers, 2015-03-17 19:04:41]

lgtm

[robert.bradford, 2015-03-17 19:06:24]

The CQ bit was checked by robert.bradford@intel.com

[robert.bradford, 2015-03-17 19:06:25]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/974513002/#ps240001
(title: "Fix non-aura mac build")

[commit-bot: I haz the power, 2015-03-17 19:06:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/974513002/240001

[commit-bot: I haz the power, 2015-03-17 19:12:59]

Committed patchset #10 (id:240001)

[commit-bot: I haz the power, 2015-03-17 19:13:52]

Patchset 10 (id:??) landed as
https://crrev.com/8a244ea4bd4547fa7481b548b6997f3412f6c215
Cr-Commit-Position: refs/heads/master@{#320953}