Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
543243003:
Remote Assistance on Chrome OS Part I - Aura Desktop Capturer (Closed)
DescriptionRemote Assistance on Chrome OS - Aura Desktop Capturer
This CL implements a WebRTC Desktop Capturer that captures from
the Aura Shell root window.
The capturer is used by the It2MeHost on Chrome OS.
|DesktopCaptureDeviceAura| cannot be re-used in our scenario
because is timer-driven as opposed to caller driven, which is
required by WebRTC.
The current implementation uses the layer API
desktop_window_->layer()->RequestCopyOfOutput(request.Pass())
to request the layer and its subtree to be rendered to a
|SkiaBitmap|. It then copies the pixels to a WebRTC |DesktopFrame|.
BUG=411530
Committed: https://crrev.com/278b065b519617b1e9231bd53e6502706d20787c
Cr-Commit-Position: refs/heads/master@{#295187}
Committed: https://crrev.com/0215564c6c0225e22419c85e7854c5ce103414c2
Cr-Commit-Position: refs/heads/master@{#295214}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Address feedback and add unittest #Patch Set 3 : Rebase on ToT #Patch Set 4 : Fix linker errors on build bots #
Total comments: 26
Patch Set 5 : Address wez's feedback #
Total comments: 10
Patch Set 6 : Address sergey's feedback #Patch Set 7 #
Total comments: 12
Patch Set 8 : Address Reed's feedback #
Total comments: 8
Patch Set 9 : Address sky's feedback #Patch Set 10 : rebase on ToT #Patch Set 11 : Fix gclient runhooks on Windows and Mac #Patch Set 12 : Fix Build bot failures on Official Chrome builds #
Messages
Total messages: 70 (28 generated)
kelvinp@chromium.org changed reviewers: + jamiewalch@chromium.org, sergeyu@chromium.org
kelvinp@chromium.org changed reviewers: + dcaiafa@chromium.org
PTAL
https://codereview.chromium.org/543243003/diff/1/remoting/host/chromeos/aura_... File remoting/host/chromeos/aura_desktop_capturer.cc (right): https://codereview.chromium.org/543243003/diff/1/remoting/host/chromeos/aura_... remoting/host/chromeos/aura_desktop_capturer.cc:61: uint8_t* bitmapData = reinterpret_cast<uint8_t*>(bitmap->getPixels()); bitmap_data https://codereview.chromium.org/543243003/diff/1/remoting/host/chromeos/aura_... remoting/host/chromeos/aura_desktop_capturer.cc:65: frame->CopyPixelsFrom(bitmapData, bitmap->rowBytes(), rect); Ideally we want to avoid copying pixels. You can achieve that by adding DesktopFrame implementation that wraps SkBitmap. E.g. take a look at DesktopFrameWin that wraps HDC on windows. https://codereview.chromium.org/543243003/diff/1/remoting/host/chromeos/aura_... File remoting/host/chromeos/aura_desktop_capturer.h (right): https://codereview.chromium.org/543243003/diff/1/remoting/host/chromeos/aura_... remoting/host/chromeos/aura_desktop_capturer.h:17: // A webrtc desktop capturer that captures pixels from the root window of the s/webrtc desktop capturer/webrtc::DesktopCapturer/ https://codereview.chromium.org/543243003/diff/1/remoting/host/chromeos/aura_... remoting/host/chromeos/aura_desktop_capturer.h:24: // Called at the beginning of a capturing session. |callback| must remain This duplicates comments from the interface definition. For overridden interfaces it's enough to have the following comment // webrtc::DesktopCapturer implementation. All interesting details specific to this particular implementation can go to the class description. https://codereview.chromium.org/543243003/diff/1/remoting/host/chromeos/aura_... remoting/host/chromeos/aura_desktop_capturer.h:25: // valid until capturer is destroyed. Must be called on the browser UI This doesn't look right. normally we use DesktopCapturer on the capturer thread. In this case I think the implementation will need to jump to jump between the capturer and UI thread. Or do you plan to map capturer thread to the UI thread in case of ChromeOS? https://codereview.chromium.org/543243003/diff/1/remoting/remoting_host.gypi File remoting/remoting_host.gypi (right): https://codereview.chromium.org/543243003/diff/1/remoting/remoting_host.gypi#... remoting/remoting_host.gypi:24: ['enable_remoting_host==1 or enable_it2me_chromeos==1', { I think ideally we want to have enable_remoting_me2me_host and enable_remoting_it2me_host and compile common host targets when either of them is enabled.
jamiewalch@chromium.org changed reviewers: - jamiewalch@chromium.org
kelvinp@chromium.org changed reviewers: + darin@chromium.org
Patchset #2 (id:20001) has been deleted
PTAL https://codereview.chromium.org/543243003/diff/1/remoting/host/chromeos/aura_... File remoting/host/chromeos/aura_desktop_capturer.cc (right): https://codereview.chromium.org/543243003/diff/1/remoting/host/chromeos/aura_... remoting/host/chromeos/aura_desktop_capturer.cc:61: uint8_t* bitmapData = reinterpret_cast<uint8_t*>(bitmap->getPixels()); On 2014/09/06 02:03:25, Sergey Ulanov wrote: > bitmap_data Done. https://codereview.chromium.org/543243003/diff/1/remoting/host/chromeos/aura_... remoting/host/chromeos/aura_desktop_capturer.cc:65: frame->CopyPixelsFrom(bitmapData, bitmap->rowBytes(), rect); On 2014/09/06 02:03:25, Sergey Ulanov wrote: > Ideally we want to avoid copying pixels. You can achieve that by adding > DesktopFrame implementation that wraps SkBitmap. E.g. take a look at > DesktopFrameWin that wraps HDC on windows. Done. https://codereview.chromium.org/543243003/diff/1/remoting/host/chromeos/aura_... File remoting/host/chromeos/aura_desktop_capturer.h (right): https://codereview.chromium.org/543243003/diff/1/remoting/host/chromeos/aura_... remoting/host/chromeos/aura_desktop_capturer.h:17: // A webrtc desktop capturer that captures pixels from the root window of the On 2014/09/06 02:03:25, Sergey Ulanov wrote: > s/webrtc desktop capturer/webrtc::DesktopCapturer/ Done. https://codereview.chromium.org/543243003/diff/1/remoting/host/chromeos/aura_... remoting/host/chromeos/aura_desktop_capturer.h:24: // Called at the beginning of a capturing session. |callback| must remain On 2014/09/06 02:03:25, Sergey Ulanov wrote: > This duplicates comments from the interface definition. For overridden > interfaces it's enough to have the following comment > // webrtc::DesktopCapturer implementation. > > All interesting details specific to this particular implementation can go to the > class description. Done. https://codereview.chromium.org/543243003/diff/1/remoting/host/chromeos/aura_... remoting/host/chromeos/aura_desktop_capturer.h:25: // valid until capturer is destroyed. Must be called on the browser UI On 2014/09/06 02:03:25, Sergey Ulanov wrote: > This doesn't look right. normally we use DesktopCapturer on the capturer thread. > In this case I think the implementation will need to jump to jump between the > capturer and UI thread. Or do you plan to map capturer thread to the UI thread > in case of ChromeOS? Yes, the capturer thread will be mapped to the UI thread in the case of ChromeOS. https://codereview.chromium.org/543243003/diff/1/remoting/remoting_host.gypi File remoting/remoting_host.gypi (right): https://codereview.chromium.org/543243003/diff/1/remoting/remoting_host.gypi#... remoting/remoting_host.gypi:24: ['enable_remoting_host==1 or enable_it2me_chromeos==1', { On 2014/09/06 02:03:25, Sergey Ulanov wrote: > I think ideally we want to have > enable_remoting_me2me_host and enable_remoting_it2me_host and compile common > host targets when either of them is enabled. Done.
wez@chromium.org changed reviewers: + wez@chromium.org
Drive-by on the capture implementation. Mostly looks good. :) https://codereview.chromium.org/543243003/diff/80001/remoting/host/chromeos/a... File remoting/host/chromeos/aura_desktop_capturer.cc (right): https://codereview.chromium.org/543243003/diff/80001/remoting/host/chromeos/a... remoting/host/chromeos/aura_desktop_capturer.cc:19: class SkiaBitmapDestkopFrame : public webrtc::DesktopFrame { typo: Destkop->Desktop https://codereview.chromium.org/543243003/diff/80001/remoting/host/chromeos/a... remoting/host/chromeos/aura_desktop_capturer.cc:19: class SkiaBitmapDestkopFrame : public webrtc::DesktopFrame { This class, and the implementations of its methods, should be in an anonymous namespace. https://codereview.chromium.org/543243003/diff/80001/remoting/host/chromeos/a... remoting/host/chromeos/aura_desktop_capturer.cc:22: static SkiaBitmapDestkopFrame* Create(SkBitmap* bitmap); nit: static methods should appear before ctor/dtor. https://codereview.chromium.org/543243003/diff/80001/remoting/host/chromeos/a... remoting/host/chromeos/aura_desktop_capturer.cc:22: static SkiaBitmapDestkopFrame* Create(SkBitmap* bitmap); scoped_ptr<SkBitmap>, not SkBitmap* https://codereview.chromium.org/543243003/diff/80001/remoting/host/chromeos/a... remoting/host/chromeos/aura_desktop_capturer.cc:49: &AuraDesktopCapturer::OnFrameCaptured, weak_factory_.GetWeakPtr())); nit: I think this'll read better if you wrap before base::Bind and then again before weak_factory, even though it adds an extra line. https://codereview.chromium.org/543243003/diff/80001/remoting/host/chromeos/a... remoting/host/chromeos/aura_desktop_capturer.cc:56: DCHECK(desktop_window_); No need to DCHECK this here - you're about to deref it, which will go boom anyway if it's NULL. You should DCHECK it at the point where it is set. https://codereview.chromium.org/543243003/diff/80001/remoting/host/chromeos/a... remoting/host/chromeos/aura_desktop_capturer.cc:62: DCHECK(result->HasBitmap()); Is there a situation in which it might not have a bitmap, in a real system? If so then consider handling that case by returning a NULL frame to the caller. https://codereview.chromium.org/543243003/diff/80001/remoting/host/chromeos/a... remoting/host/chromeos/aura_desktop_capturer.cc:91: DCHECK(bitmap->bytesPerPixel() == 4) DCHECK_EQ(4, bitmap->bytesPerPixel()) Is there a way that this might not be true in real systems? If so then we should error out instead of DCHECKing; what will the rest of the function do if bytesPerPixel != 4? https://codereview.chromium.org/543243003/diff/80001/remoting/host/chromeos/a... remoting/host/chromeos/aura_desktop_capturer.cc:100: result->mutable_updated_region()->SetRect(rect); nit: This makes more logical sense to do in OnFrameCaptured, immediately before returning the frame. https://codereview.chromium.org/543243003/diff/80001/remoting/host/chromeos/a... File remoting/host/chromeos/aura_desktop_capturer.h (right): https://codereview.chromium.org/543243003/diff/80001/remoting/host/chromeos/a... remoting/host/chromeos/aura_desktop_capturer.h:32: // operations are cancelled when DesktopCapturer is deleted. I'd suggest removing this comment and moving any relevant details either to the class-level comment, or into the implementation itself. In particular the cancellation on deletion is part of the interface contract, and the Capture(region) API is only required to guarantee that |region| is valid; it's not a problem if _more_ than region is valid. https://codereview.chromium.org/543243003/diff/80001/remoting/host/chromeos/a... remoting/host/chromeos/aura_desktop_capturer.h:36: // Capture calls. Not supported in Aura. Similarly, remove this comment; you can document it with NOTIMPLEMENTED() in the implementation. :) https://codereview.chromium.org/543243003/diff/80001/remoting/host/chromeos/a... remoting/host/chromeos/aura_desktop_capturer.h:49: aura::Window* desktop_window_; Where does this ever get set? https://codereview.chromium.org/543243003/diff/80001/remoting/host/chromeos/a... remoting/host/chromeos/aura_desktop_capturer.h:52: }; DISALLOW_COPY_AND_ASSIGN
PTAL https://codereview.chromium.org/543243003/diff/80001/remoting/host/chromeos/a... File remoting/host/chromeos/aura_desktop_capturer.cc (right): https://codereview.chromium.org/543243003/diff/80001/remoting/host/chromeos/a... remoting/host/chromeos/aura_desktop_capturer.cc:19: class SkiaBitmapDestkopFrame : public webrtc::DesktopFrame { On 2014/09/10 01:41:10, Wez wrote: > typo: Destkop->Desktop Done. https://codereview.chromium.org/543243003/diff/80001/remoting/host/chromeos/a... remoting/host/chromeos/aura_desktop_capturer.cc:19: class SkiaBitmapDestkopFrame : public webrtc::DesktopFrame { On 2014/09/10 01:41:10, Wez wrote: > typo: Destkop->Desktop Done. https://codereview.chromium.org/543243003/diff/80001/remoting/host/chromeos/a... remoting/host/chromeos/aura_desktop_capturer.cc:22: static SkiaBitmapDestkopFrame* Create(SkBitmap* bitmap); On 2014/09/10 01:41:10, Wez wrote: > nit: static methods should appear before ctor/dtor. Done. https://codereview.chromium.org/543243003/diff/80001/remoting/host/chromeos/a... remoting/host/chromeos/aura_desktop_capturer.cc:22: static SkiaBitmapDestkopFrame* Create(SkBitmap* bitmap); On 2014/09/10 01:41:10, Wez wrote: > scoped_ptr<SkBitmap>, not SkBitmap* Done. https://codereview.chromium.org/543243003/diff/80001/remoting/host/chromeos/a... remoting/host/chromeos/aura_desktop_capturer.cc:49: &AuraDesktopCapturer::OnFrameCaptured, weak_factory_.GetWeakPtr())); On 2014/09/10 01:41:10, Wez wrote: > nit: I think this'll read better if you wrap before base::Bind and then again > before weak_factory, even though it adds an extra line. Done. https://codereview.chromium.org/543243003/diff/80001/remoting/host/chromeos/a... remoting/host/chromeos/aura_desktop_capturer.cc:56: DCHECK(desktop_window_); On 2014/09/10 01:41:10, Wez wrote: > No need to DCHECK this here - you're about to deref it, which will go boom > anyway if it's NULL. You should DCHECK it at the point where it is set. Done. https://codereview.chromium.org/543243003/diff/80001/remoting/host/chromeos/a... remoting/host/chromeos/aura_desktop_capturer.cc:62: DCHECK(result->HasBitmap()); On 2014/09/10 01:41:10, Wez wrote: > Is there a situation in which it might not have a bitmap, in a real system? > > If so then consider handling that case by returning a NULL frame to the caller. No, that should never happen. https://codereview.chromium.org/543243003/diff/80001/remoting/host/chromeos/a... remoting/host/chromeos/aura_desktop_capturer.cc:91: DCHECK(bitmap->bytesPerPixel() == 4) On 2014/09/10 01:41:10, Wez wrote: > DCHECK_EQ(4, bitmap->bytesPerPixel()) > > Is there a way that this might not be true in real systems? If so then we should > error out instead of DCHECKing; what will the rest of the function do if > bytesPerPixel != 4? No, the current implementation of both the gl renderer and the software renderer has a 32-bit pixel format https://codereview.chromium.org/543243003/diff/80001/remoting/host/chromeos/a... remoting/host/chromeos/aura_desktop_capturer.cc:100: result->mutable_updated_region()->SetRect(rect); On 2014/09/10 01:41:10, Wez wrote: > nit: This makes more logical sense to do in OnFrameCaptured, immediately before > returning the frame. Done. https://codereview.chromium.org/543243003/diff/80001/remoting/host/chromeos/a... File remoting/host/chromeos/aura_desktop_capturer.h (right): https://codereview.chromium.org/543243003/diff/80001/remoting/host/chromeos/a... remoting/host/chromeos/aura_desktop_capturer.h:32: // operations are cancelled when DesktopCapturer is deleted. On 2014/09/10 01:41:11, Wez wrote: > I'd suggest removing this comment and moving any relevant details either to the > class-level comment, or into the implementation itself. > > In particular the cancellation on deletion is part of the interface contract, > and the Capture(region) API is only required to guarantee that |region| is > valid; it's not a problem if _more_ than region is valid. Done. https://codereview.chromium.org/543243003/diff/80001/remoting/host/chromeos/a... remoting/host/chromeos/aura_desktop_capturer.h:36: // Capture calls. Not supported in Aura. On 2014/09/10 01:41:11, Wez wrote: > Similarly, remove this comment; you can document it with NOTIMPLEMENTED() in the > implementation. :) Done. https://codereview.chromium.org/543243003/diff/80001/remoting/host/chromeos/a... remoting/host/chromeos/aura_desktop_capturer.h:49: aura::Window* desktop_window_; On 2014/09/10 01:41:10, Wez wrote: > Where does this ever get set? Good catch. It should be set in Start, which got accidentally removed in my last iteration. https://codereview.chromium.org/543243003/diff/80001/remoting/host/chromeos/a... remoting/host/chromeos/aura_desktop_capturer.h:52: }; On 2014/09/10 01:41:10, Wez wrote: > DISALLOW_COPY_AND_ASSIGN Done.
https://codereview.chromium.org/543243003/diff/100001/remoting/host/chromeos/... File remoting/host/chromeos/aura_desktop_capturer.cc (right): https://codereview.chromium.org/543243003/diff/100001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer.cc:44: webrtc::DesktopSize size(width, height); nit: size(bitmap->width(), bitmap->height()) and remove two lines above. https://codereview.chromium.org/543243003/diff/100001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer.cc:115: frame->mutable_updated_region()->SetRect(rect); I think we also need to set frame DPI according to the screen resolution. cc::Layer::contents_scale_(x|y)() may be useful for that, but I'm not sure. https://codereview.chromium.org/543243003/diff/100001/remoting/host/chromeos/... File remoting/host/chromeos/aura_desktop_capturer.h (right): https://codereview.chromium.org/543243003/diff/100001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer.h:19: // be rendered to a given data structure. |Start| and |Capture| must be called For function names add () at the end instead of | on the sides, e.g. "Start() and Capture()..." https://codereview.chromium.org/543243003/diff/100001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer.h:29: virtual void SetExcludedWindow(webrtc::WindowId window) OVERRIDE; There is a default no-op implementation of this method in DesktopCapturer, so you don't need to override it here. https://codereview.chromium.org/543243003/diff/100001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer.h:43: base::WeakPtrFactory<AuraDesktopCapturer> weak_factory_; please include base/memory/weak_ptr.h https://codereview.chromium.org/543243003/diff/100001/remoting/host/chromeos/... File remoting/host/chromeos/aura_desktop_capturer_unittest.cc (right): https://codereview.chromium.org/543243003/diff/100001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer_unittest.cc:6: #include "remoting/host/chromeos/aura_desktop_capturer.h" this should be the first include in this file with an empty line after it https://codereview.chromium.org/543243003/diff/100001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer_unittest.cc:66: webrtc::DesktopFrame* captured_frame = NULL; This needs to be deleted at the end. OnCaptureCompleted() passes ownership of the frame to OnCaptureCompleted() (we don't use scoped_ptr<> there because webrtc cannot use chromium's scoped_ptr<> and the one in webrtc/base cannot be passed as argument). https://codereview.chromium.org/543243003/diff/100001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer_unittest.cc:79: frame_data, captured_data, sizeof(uint8_t) * arraysize(frame_data))); sizeof(frame_data) would work here just fine and is more readable.
lgtm when my comments are addressed
The CQ bit was checked by kelvinp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/543243003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/54389) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/59327) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
jamescook@chromium.org changed reviewers: + jamescook@chromium.org
I'm not very familiar with It2Me. How does this feature interact with multi-monitor support? Ash has multiple root windows, one per display. (Not for this CL, but will you eventually need to support cursor capture? I seem to remember hshi@ doing some work on that for Chromecast support.)
On 2014/09/11 00:40:50, James Cook wrote: > I'm not very familiar with It2Me. How does this feature interact with > multi-monitor support? Ash has multiple root windows, one per display. Good to know. Chrome Remote Desktop supports multiple monitor capturing. For now, this CL only implement the single monitor use case. Eventually we will extend the support to multiple monitors. > (Not for this CL, but will you eventually need to support cursor capture? I seem > to remember hshi@ doing some work on that for Chromecast support.) For It2me, we don't capture the cursor of of the host. Instead, we move the cursor on the host to mirror the cursor of the client (It support).
reed@google.com changed reviewers: + reed@google.com
https://codereview.chromium.org/543243003/diff/140001/remoting/host/chromeos/... File remoting/host/chromeos/aura_desktop_capturer_unittest.cc (right): https://codereview.chromium.org/543243003/diff/140001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer_unittest.cc:20: // Test frame data. can/should frame_data be declared const? https://codereview.chromium.org/543243003/diff/140001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer_unittest.cc:21: unsigned char frame_data[] = { This data is slightly fragile if passed directly to a bitmap. The "N" in MakeN32Premul stands for Native. There are two public 32it colortypes : RGBA and BGRA. Native is just an alias for whichever one is the current default (set at compile time). Safer would be to either specify the explicit colortype in your SkImageInfo factory (as a parameter), or you could specify your data using a macro that orders the bytes correctly for the given native colortype. SkPackARGB32(a, r, g, b) in SkColorPriv.h https://codereview.chromium.org/543243003/diff/140001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer_unittest.cc:51: bitmap->setInfo(info); replace setInfo and setPixels with bitmap->installPixels(info, frame_data, rowbytes(?))
DEPS on ash/ lgtm https://codereview.chromium.org/543243003/diff/140001/remoting/remoting_host.... File remoting/remoting_host.gypi (right): https://codereview.chromium.org/543243003/diff/140001/remoting/remoting_host.... remoting/remoting_host.gypi:26: 'enable_it2me_host': 0, Does this section need an entry for enable_remoting_host?
danakj@chromium.org changed reviewers: + danakj@chromium.org
DEPS LGTM
sky@chromium.org changed reviewers: + sky@chromium.org
https://codereview.chromium.org/543243003/diff/140001/remoting/host/chromeos/... File remoting/host/chromeos/aura_desktop_capturer.cc (right): https://codereview.chromium.org/543243003/diff/140001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer.cc:75: desktop_window_ = ash::Shell::GetPrimaryRootWindow(); Why are you using GetprimaryRootWindow and not the active, or maybe all roots?
https://codereview.chromium.org/543243003/diff/140001/remoting/host/chromeos/... File remoting/host/chromeos/aura_desktop_capturer_unittest.cc (right): https://codereview.chromium.org/543243003/diff/140001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer_unittest.cc:21: unsigned char frame_data[] = { On 2014/09/11 13:48:15, reed1 wrote: > This data is slightly fragile if passed directly to a bitmap. > > The "N" in MakeN32Premul stands for Native. > > There are two public 32it colortypes : RGBA and BGRA. Native is just an alias > for whichever one is the current default (set at compile time). > > Safer would be to either specify the explicit colortype in your SkImageInfo > factory (as a parameter), or you could specify your data using a macro that > orders the bytes correctly for the given native colortype. SkPackARGB32(a, r, > g, b) in SkColorPriv.h Alternately you could use a .png file off the disk. cc_unittests do this for example.
PTAL https://codereview.chromium.org/543243003/diff/100001/remoting/host/chromeos/... File remoting/host/chromeos/aura_desktop_capturer_unittest.cc (right): https://codereview.chromium.org/543243003/diff/100001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer_unittest.cc:6: #include "remoting/host/chromeos/aura_desktop_capturer.h" On 2014/09/10 23:08:52, Sergey Ulanov wrote: > this should be the first include in this file with an empty line after it Done. https://codereview.chromium.org/543243003/diff/100001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer_unittest.cc:66: webrtc::DesktopFrame* captured_frame = NULL; On 2014/09/10 23:08:52, Sergey Ulanov wrote: > This needs to be deleted at the end. OnCaptureCompleted() passes ownership of > the frame to OnCaptureCompleted() (we don't use scoped_ptr<> there because > webrtc cannot use chromium's scoped_ptr<> and the one in webrtc/base cannot be > passed as argument). Done. https://codereview.chromium.org/543243003/diff/140001/remoting/host/chromeos/... File remoting/host/chromeos/aura_desktop_capturer.cc (right): https://codereview.chromium.org/543243003/diff/140001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer.cc:75: desktop_window_ = ash::Shell::GetPrimaryRootWindow(); On 2014/09/11 19:34:24, sky wrote: > Why are you using GetprimaryRootWindow and not the active, or maybe all roots? For remote assistance, I am trying to capture the main window (The one with the launcher) so I think GetPrimaryRootWindow will suffice. As I implement multiple window support, I will call GetAllRootWindows. https://codereview.chromium.org/543243003/diff/140001/remoting/host/chromeos/... File remoting/host/chromeos/aura_desktop_capturer_unittest.cc (right): https://codereview.chromium.org/543243003/diff/140001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer_unittest.cc:20: // Test frame data. On 2014/09/11 13:48:15, reed1 wrote: > can/should frame_data be declared const? Done. https://codereview.chromium.org/543243003/diff/140001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer_unittest.cc:21: unsigned char frame_data[] = { On 2014/09/11 13:48:15, reed1 wrote: > This data is slightly fragile if passed directly to a bitmap. > > The "N" in MakeN32Premul stands for Native. > > There are two public 32it colortypes : RGBA and BGRA. Native is just an alias > for whichever one is the current default (set at compile time). > > Safer would be to either specify the explicit colortype in your SkImageInfo > factory (as a parameter), or you could specify your data using a macro that > orders the bytes correctly for the given native colortype. SkPackARGB32(a, r, > g, b) in SkColorPriv.h Done. https://codereview.chromium.org/543243003/diff/140001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer_unittest.cc:51: bitmap->setInfo(info); On 2014/09/11 13:48:15, reed1 wrote: > replace setInfo and setPixels with > > bitmap->installPixels(info, frame_data, rowbytes(?)) Done. https://codereview.chromium.org/543243003/diff/140001/remoting/remoting_host.... File remoting/remoting_host.gypi (right): https://codereview.chromium.org/543243003/diff/140001/remoting/remoting_host.... remoting/remoting_host.gypi:26: 'enable_it2me_host': 0, On 2014/09/11 15:52:34, James Cook wrote: > Does this section need an entry for enable_remoting_host? No, the condition above should already set enable_remoting_host
https://codereview.chromium.org/543243003/diff/140001/remoting/host/chromeos/... File remoting/host/chromeos/aura_desktop_capturer.cc (right): https://codereview.chromium.org/543243003/diff/140001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer.cc:75: desktop_window_ = ash::Shell::GetPrimaryRootWindow(); On 2014/09/11 20:54:17, kelvinp wrote: > On 2014/09/11 19:34:24, sky wrote: > > Why are you using GetprimaryRootWindow and not the active, or maybe all roots? > > For remote assistance, I am trying to capture the main window (The one with the > launcher) so I think GetPrimaryRootWindow will suffice. As I implement multiple > window support, I will call GetAllRootWindows. Add a TODO then. https://codereview.chromium.org/543243003/diff/160001/remoting/host/chromeos/... File remoting/host/chromeos/aura_desktop_capturer.cc (right): https://codereview.chromium.org/543243003/diff/160001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer.cc:91: request->set_area(window_rect); It seems like set_area only needs a size, should it take that instead of bounds? Assuming it does in fact want a bounds, then you can replace 87-89 with: gfx::Rect window_rect(desktop_window_->bounds().size()). https://codereview.chromium.org/543243003/diff/160001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer.cc:112: callback_->OnCaptureCompleted(frame.release()); Might callback_ be NULL here? Also, what happens if Start() is called multiple times? Are there any expectations as to how which callback_ should be notified? Seems like Start() should only be invoked once, in which case Init would be a better name for start. https://codereview.chromium.org/543243003/diff/160001/remoting/host/chromeos/... File remoting/host/chromeos/aura_desktop_capturer.h (right): https://codereview.chromium.org/543243003/diff/160001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer.h:10: #include "ui/aura/window.h" nit: forward declare this. https://codereview.chromium.org/543243003/diff/160001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer.h:28: virtual void Start(webrtc::DesktopCapturer::Callback* callback) OVERRIDE; nit: IMO naming this Callback is very confusing given we have base::Callback, which is an entirely different thing, with very different ownership semantics.
https://codereview.chromium.org/543243003/diff/160001/remoting/host/chromeos/... File remoting/host/chromeos/aura_desktop_capturer.cc (right): https://codereview.chromium.org/543243003/diff/160001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer.cc:91: request->set_area(window_rect); On 2014/09/11 21:22:47, sky wrote: > It seems like set_area only needs a size, should it take that instead of bounds? > Assuming it does in fact want a bounds, then you can replace 87-89 with: > gfx::Rect window_rect(desktop_window_->bounds().size()). Done. https://codereview.chromium.org/543243003/diff/160001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer.cc:112: callback_->OnCaptureCompleted(frame.release()); On 2014/09/11 21:22:47, sky wrote: > Might callback_ be NULL here? Also, what happens if Start() is called multiple > times? Are there any expectations as to how which callback_ should be notified? > Seems like Start() should only be invoked once, in which case Init would be a > better name for start. Callback may not be NULL here. As Start() must be called before Capture(). I added a DCHECK on Start() to make sure that it is only invoked once with a non-NULL callback. I agree that Init() would be a better name. However, I don't own the Webrtc::DesktopFrame interface and renaming it in this change would be out of scope. https://codereview.chromium.org/543243003/diff/160001/remoting/host/chromeos/... File remoting/host/chromeos/aura_desktop_capturer.h (right): https://codereview.chromium.org/543243003/diff/160001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer.h:10: #include "ui/aura/window.h" On 2014/09/11 21:22:47, sky wrote: > nit: forward declare this. Done. https://codereview.chromium.org/543243003/diff/160001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer.h:28: virtual void Start(webrtc::DesktopCapturer::Callback* callback) OVERRIDE; On 2014/09/11 21:22:47, sky wrote: > nit: IMO naming this Callback is very confusing given we have base::Callback, > which is an entirely different thing, with very different ownership semantics. I agree. However, renaming methods in the existing interface is out of the scope of this change.
PTAL
LGTM
skbitmap lgtm
Thank you everyone for the code review. Kelvin On Mon, Sep 15, 2014 at 10:51 AM, <reed@google.com> wrote: > skbitmap lgtm > > https://codereview.chromium.org/543243003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by kelvinp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/543243003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/56063) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/60935) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by kelvinp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/543243003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/56299) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/61152) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by kelvinp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/543243003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/56340) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/61199) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by kelvinp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/543243003/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/56356) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/61217) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by kelvinp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/543243003/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/56624) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/61479) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by kelvinp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/543243003/220001
The CQ bit was unchecked by kelvinp@chromium.org
The CQ bit was checked by kelvinp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/543243003/220001
Message was sent while issue was closed.
Committed patchset #11 (id:220001) as dd34e77cf1f8aec617750578360143a76ff8f422
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/278b065b519617b1e9231bd53e6502706d20787c Cr-Commit-Position: refs/heads/master@{#295187}
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:220001) has been created in https://codereview.chromium.org/579583003/ by acolwell@chromium.org. The reason for reverting is: This patch appears to cause a Chrome OS bot to break. http://build.chromium.org/p/chromium.chrome/buildstatus?builder=Google%20Chro....
The CQ bit was checked by kelvinp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/543243003/240001
The CQ bit was unchecked by kelvinp@chromium.org
The CQ bit was checked by kelvinp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/543243003/240001
Message was sent while issue was closed.
Committed patchset #12 (id:240001) as 7c3741869f23912954d77f5af90701a3b6323021
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/0215564c6c0225e22419c85e7854c5ce103414c2 Cr-Commit-Position: refs/heads/master@{#295214} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
