Make RenderWidgetHostViewAura::CopyFromCompositingSurface readback layer

aura: Make RenderWidgetHostViewAura::CopyFromCompositingSurface readback layer

Currently CopyFromCompositingSurface does an async texture readback of
current_surface_ which depends on the renderer compositor sending
textures to the browser compositor. Under ubercomp, however, the
current_surface_ will not exist.

The cc/ compositor now supports async readback of layers. This uses the
new mechanism to get the contents of the RWHVA's ui::Layer instead which
is always available. The result of this readback can be a texture or a
bitmap (when using software compositing). The result read by the
compositor will already be cropped.

If the result is a texture, we use the GLHelper to scale the result to
the desired size. If it is a bitmap, we use skia::ImageOperations::Resize
to do the same.

R=piman
BUG=162291
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=210106

[danakj, 2013-06-26 23:31:03]

Not sure if there are tests for this. If there is and I broke things, I'll find
out shortly.

[piman, 2013-06-27 00:49:24]

LGTM!

[danakj, 2013-06-27 16:02:46]

On Wed, Jun 26, 2013 at 8:49 PM, <piman@chromium.org> wrote:

> LGTM!
>

Thanks! I updated the CL to use skia::ImageOperations::Resize
in RenderWidgetHostViewAura::PrepareBitmapCopyOutputResult instead of
SkCanvas to resize the bitmap output, thanks to jbroman@'s pointing out the
method.


>
>
https://codereview.chromium.**org/17971002/<https://codereview.chromium.org/1...
>

[piman, 2013-06-27 19:39:12]

lgtm

[danakj, 2013-06-28 02:53:21]

Ok, I added 4 browser tests for this that are pixel tests. Woo. They check
cropping and scaling, separate and together.

Also fixed the IsSurfaceAvailableForCopy() method, had to add a method to
ui::Layer for that, I think.

PTAL!

(We'll need another CL for the CopyFromCompositingSurfaceToVideoFrame method I
guess.)

[danakj, 2013-06-28 02:54:09]

On 2013/06/28 02:53:21, danakj wrote:
> Ok, I added 4 browser tests for this that are pixel tests. Woo. They check
> cropping and scaling, separate and together.

Oh, the tests only check hardware compositing mode. Browser tests aren't set up
for software compositing at the moment.. I guess that's okay for now?

[piman, 2013-06-28 03:23:13]

Thanks for the tests! I have 2 things.

https://codereview.chromium.org/17971002/diff/27001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_browsertest.cc
(right):

https://codereview.chromium.org/17971002/diff/27001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:459:
base::MessageLoop::current()->Quit();
You can pass in the RunLoop's QuitClosure, so that it's safe even in case of
nested message loops for any reason (--flakiness).

Alternatively, make the RunLoop a member of this (since you're passing it), and
Quit() it explicitly. That should work too.

https://codereview.chromium.org/17971002/diff/27001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:508:
EXPECT_EQ(gfx::Rect(766, 506).ToString(), bounds.ToString());
Is this size expected to be stable across platforms (or even runs)?

[danakj, 2013-06-28 16:50:12]

https://codereview.chromium.org/17971002/diff/27001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_browsertest.cc
(right):

https://codereview.chromium.org/17971002/diff/27001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:459:
base::MessageLoop::current()->Quit();
On 2013/06/28 03:23:13, piman wrote:
> You can pass in the RunLoop's QuitClosure, so that it's safe even in case of
> nested message loops for any reason (--flakiness).
> 
> Alternatively, make the RunLoop a member of this (since you're passing it),
and
> Quit() it explicitly. That should work too.

Ah! Ok, I was going to use the QuitClosure, but then I was like "What for?".
Thanks for the explanation.

https://codereview.chromium.org/17971002/diff/27001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:508:
EXPECT_EQ(gfx::Rect(766, 506).ToString(), bounds.ToString());
On 2013/06/28 03:23:13, piman wrote:
> Is this size expected to be stable across platforms (or even runs)?

Ya I wasn't sure, so I put this to see and run the bots.

However, I think I could just use the top-left WxH pixels where WxH fits inside
the page for all platforms.

[danakj, 2013-06-28 16:51:59]

It looks like the bot doesn't like me using the real compositor. I wonder what I
need to do to make this work?

[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from CompositingRenderWidgetHostViewBrowserTestTabCapture,
where TypeParam = 
[ RUN      ]
CompositingRenderWidgetHostViewBrowserTestTabCapture.CopyFromCompositingSurface_WholeSurface_Unscaled
Xlib:  extension "RANDR" missing on display ":9".
[27169:27169:0627/212309:1369302399415:ERROR:nss_util.cc(444)] Error
initializing NSS with a persistent database (sql:/etc/fake_root_ca/nssdb): NSS
error code: -8174
[27169:27169:0627/212309:1369302402694:ERROR:gl_surface_glx.cc(327)]
glxQueryVersion failed
[27169:27169:0627/212309:1369302402731:ERROR:gl_surface_x11.cc(58)]
GLSurfaceGLX::InitializeOneOff failed.
[27169:27169:0627/212309:1369302407310:WARNING:sandbox_linux.cc(36)] Activated
seccomp-bpf sandbox for process type: gpu-process.
[27135:27135:0627/212309:1369302414034:FATAL:image_transport_factory.cc(850)]
Failed to initialize UI shared context.
[27135:27135:0627/212309:1369302414034:FATAL:image_transport_factory.cc(850)]
Failed to initialize UI shared context.
Received signal 6
 [0x7f2669e7235e] base::debug::StackTrace::StackTrace()
 [0x7f2669e72888] base::debug::(anonymous namespace)::StackDumpSignalHandler()
 [0x7f266564dcb0] <unknown>
 [0x7f2664a82425] gsignal
 [0x7f2664a85b8b] abort
 [0x7f2669e71d09] base::debug::BreakDebugger()
 [0x7f2669e98884] logging::LogMessage::~LogMessage()
 [0x7f26663b349c] content::(anonymous
namespace)::GpuProcessTransportFactory::CreateOutputSurface()
 [0x7f2669209dd5] ui::Compositor::CreateOutputSurface()

[piman, 2013-06-28 16:56:05]

I think you're running in the same problems as hubbe@ in
https://codereview.chromium.org/17904008/

The solution would be to explicitly force using osmesa, see
https://codereview.chromium.org/17904008/diff/33001/content/browser/gpu/gpu_i...


On Fri, Jun 28, 2013 at 9:51 AM, <danakj@chromium.org> wrote:

> It looks like the bot doesn't like me using the real compositor. I wonder
> what I
> need to do to make this work?
>
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from CompositingRenderWidgetHostVie**
> wBrowserTestTabCapture,
> where TypeParam =
> [ RUN      ]
> CompositingRenderWidgetHostVie**wBrowserTestTabCapture.**
> CopyFromCompositingSurface_**WholeSurface_Unscaled
> Xlib:  extension "RANDR" missing on display ":9".
> [27169:27169:0627/212309:**1369302399415:ERROR:nss_util.**cc(444)] Error
> initializing NSS with a persistent database (sql:/etc/fake_root_ca/nssdb):
> NSS
> error code: -8174
> [27169:27169:0627/212309:**1369302402694:ERROR:gl_**surface_glx.cc(327)]
> glxQueryVersion failed
> [27169:27169:0627/212309:**1369302402731:ERROR:gl_**surface_x11.cc(58)]
> GLSurfaceGLX::InitializeOneOff failed.
> [27169:27169:0627/212309:**1369302407310:WARNING:sandbox_**linux.cc(36)]
> Activated
> seccomp-bpf sandbox for process type: gpu-process.
> [27135:27135:0627/212309:**1369302414034:FATAL:image_**
> transport_factory.cc(850)]
> Failed to initialize UI shared context.
> [27135:27135:0627/212309:**1369302414034:FATAL:image_**
> transport_factory.cc(850)]
> Failed to initialize UI shared context.
> Received signal 6
>  [0x7f2669e7235e] base::debug::StackTrace::**StackTrace()
>  [0x7f2669e72888] base::debug::(anonymous namespace)::**
> StackDumpSignalHandler()
>  [0x7f266564dcb0] <unknown>
>  [0x7f2664a82425] gsignal
>  [0x7f2664a85b8b] abort
>  [0x7f2669e71d09] base::debug::BreakDebugger()
>  [0x7f2669e98884] logging::LogMessage::~**LogMessage()
>  [0x7f26663b349c] content::(anonymous
> namespace)::**GpuProcessTransportFactory::**CreateOutputSurface()
>  [0x7f2669209dd5] ui::Compositor::**CreateOutputSurface()
>
>
https://codereview.chromium.**org/17971002/<https://codereview.chromium.org/1...
>

[danakj, 2013-06-28 17:58:38]

Thanks, trying out the command line flag for the bots.

https://codereview.chromium.org/17971002/diff/27001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_browsertest.cc
(right):

https://codereview.chromium.org/17971002/diff/27001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:459:
base::MessageLoop::current()->Quit();
On 2013/06/28 16:50:12, danakj wrote:
> On 2013/06/28 03:23:13, piman wrote:
> > You can pass in the RunLoop's QuitClosure, so that it's safe even in case of
> > nested message loops for any reason (--flakiness).
> > 
> > Alternatively, make the RunLoop a member of this (since you're passing it),
> and
> > Quit() it explicitly. That should work too.
> 
> Ah! Ok, I was going to use the QuitClosure, but then I was like "What for?".
> Thanks for the explanation.

Done.

https://codereview.chromium.org/17971002/diff/27001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:508:
EXPECT_EQ(gfx::Rect(766, 506).ToString(), bounds.ToString());
On 2013/06/28 16:50:12, danakj wrote:
> On 2013/06/28 03:23:13, piman wrote:
> > Is this size expected to be stable across platforms (or even runs)?
> 
> Ya I wasn't sure, so I put this to see and run the bots.
> 
> However, I think I could just use the top-left WxH pixels where WxH fits
inside
> the page for all platforms.

Using just the top left 400x300 now. We'll see what the bots think.

[piman, 2013-06-28 19:46:51]

LGTM3

[danakj, 2013-06-28 21:33:12]

Hm, my tests are all reading white pixels.. I must need to wait for something,
but I thought the NavigateToURL would already block until load finished..

[danakj, 2013-06-28 23:05:41]

Added dom automation events to the html files that are loaded, and waiting on
them in the browser before grabbing pixels. I hope this works!

[danakj, 2013-06-29 01:19:40]

On 2013/06/28 23:05:41, danakj wrote:
> Added dom automation events to the html files that are loaded, and waiting on
> them in the browser before grabbing pixels. I hope this works!

Mmm.. chromeos version of these tests are still reading white on the bots.. I
dunno..
http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds...

[piman, 2013-06-29 01:35:35]

On Fri, Jun 28, 2013 at 6:19 PM, <danakj@chromium.org> wrote:

> On 2013/06/28 23:05:41, danakj wrote:
>
>> Added dom automation events to the html files that are loaded, and
>> waiting on
>> them in the browser before grabbing pixels. I hope this works!
>>
>
> Mmm.. chromeos version of these tests are still reading white on the
> bots.. I
> dunno..
> http://build.chromium.org/p/**tryserver.chromium/builders/**
> linux_chromeos/builds/131096/**steps/content_browsertests/**logs/**
>
CopyFromCompositingSurface_**Cropped_Scaled<http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds/131096/steps/content_browsertests/logs/CopyFromCompositingSurface_Cropped_Scaled>
>
>
chromeos uses threaded compositor by default in the browser (linux aura
doesn't) - maybe that's the difference?


>
https://codereview.chromium.**org/17971002/<https://codereview.chromium.org/1...
>

[danakj, 2013-06-29 03:08:37]

On Fri, Jun 28, 2013 at 9:35 PM, Antoine Labour <piman@chromium.org> wrote:

>
>
>
> On Fri, Jun 28, 2013 at 6:19 PM, <danakj@chromium.org> wrote:
>
>> On 2013/06/28 23:05:41, danakj wrote:
>>
>>> Added dom automation events to the html files that are loaded, and
>>> waiting on
>>> them in the browser before grabbing pixels. I hope this works!
>>>
>>
>> Mmm.. chromeos version of these tests are still reading white on the
>> bots.. I
>> dunno..
>> http://build.chromium.org/p/**tryserver.chromium/builders/**
>> linux_chromeos/builds/131096/**steps/content_browsertests/**logs/**
>>
CopyFromCompositingSurface_**Cropped_Scaled<http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds/131096/steps/content_browsertests/logs/CopyFromCompositingSurface_Cropped_Scaled>
>>
>>
> chromeos uses threaded compositor by default in the browser (linux aura
> doesn't) - maybe that's the difference?
>

Hrm, checked and the thread is also on for my local runs. Maybe I need to
force the renderer to draw and wait for it somehow after the js runs.


>
>
>>
https://codereview.chromium.**org/17971002/<https://codereview.chromium.org/1...
>>
>
>

[danakj, 2013-06-29 17:30:39]

Ok I did something more drastic and this seems to have worked. PTAL and let me
know how to do it better or if you like it.

I added RendererFrameCount() to RWHVPort interface, and RWHVBase implements it
and keeps a count of how often DidReceiveRendererFrame() is called by its
subclasses. Then each RWHV_ instance calls the function when it gets a frame.

Then the test can call ScheduleComposite() and wait for the RendererFrameCount()
to increment..

I could only call DidReceveRendererFrame from RWHVAura perhaps, idk what you'll
think!

[piman, 2013-07-02 01:49:08]

https://codereview.chromium.org/17971002/diff/64001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/17971002/diff/64001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_android.cc:609:
DidReceiveRendererFrame();
I don't know if we need to handle the "old"
AcceleratedSurfaceBuffersSwapped/PostSubBuffer here for consistency.

https://codereview.chromium.org/17971002/diff/64001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/17971002/diff/64001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:1635:
DidReceiveRendererFrame();
For consistency, it's also needed in SwapDelegatedFrame, SwapSoftwareFrame and
DidUpdateBackingStore.

https://codereview.chromium.org/17971002/diff/64001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_browsertest.cc
(right):

https://codereview.chromium.org/17971002/diff/64001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:560:
RenderWidgetHostImpl::From(rwh)->ScheduleComposite();
So, ScheduleComposite early-outs if we're not in compositing mode (or not
yet?)... Not sure what to do about that...
Our infrastructure is terrible :(

https://codereview.chromium.org/17971002/diff/64001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_gtk.cc (right):

https://codereview.chromium.org/17971002/diff/64001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_gtk.cc:1076:
DidReceiveRendererFrame();
Also needed in DidUpdateBackingStore

https://codereview.chromium.org/17971002/diff/64001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_guest.cc (right):

https://codereview.chromium.org/17971002/diff/64001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_guest.cc:171:
DidReceiveRendererFrame();
I suspect it's also needed in DidUpdateBackingStore.
Or maybe not, I don't know how the UpdateRect path works for the <webview> tag,
but I think it's supposed to work.

https://codereview.chromium.org/17971002/diff/64001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

https://codereview.chromium.org/17971002/diff/64001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_win.cc:777:
DidReceiveRendererFrame();
So, on Windows, the accelerated swap doesn't get to the UI thread (it's routed
to a "presenter" thread). I'm not sure how this would work there, short of
posting a task to update the frame count. Maybe we can do that?

[danakj, 2013-07-03 01:08:44]

https://codereview.chromium.org/17971002/diff/64001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_browsertest.cc
(right):

https://codereview.chromium.org/17971002/diff/64001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:560:
RenderWidgetHostImpl::From(rwh)->ScheduleComposite();
On 2013/07/02 01:49:09, piman wrote:
> So, ScheduleComposite early-outs if we're not in compositing mode (or not
> yet?)... Not sure what to do about that...
> Our infrastructure is terrible :(

I think that's okay. We already waited until we received a frame from the
compositor - ie the ui::layer has a texture set on it.

[danakj, 2013-07-03 18:05:39]

https://codereview.chromium.org/17971002/diff/64001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/17971002/diff/64001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_android.cc:609:
DidReceiveRendererFrame();
On 2013/07/02 01:49:09, piman wrote:
> I don't know if we need to handle the "old"
> AcceleratedSurfaceBuffersSwapped/PostSubBuffer here for consistency.

They have a NOTREACHED() in them, so I figured not, but I can add it there
anyhow.

https://codereview.chromium.org/17971002/diff/64001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/17971002/diff/64001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:1635:
DidReceiveRendererFrame();
On 2013/07/02 01:49:09, piman wrote:
> For consistency, it's also needed in SwapDelegatedFrame, SwapSoftwareFrame and
> DidUpdateBackingStore.

Ah, ok I moved it out to the entry point methods instead of in here.

https://codereview.chromium.org/17971002/diff/64001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_gtk.cc (right):

https://codereview.chromium.org/17971002/diff/64001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_gtk.cc:1076:
DidReceiveRendererFrame();
On 2013/07/02 01:49:09, piman wrote:
> Also needed in DidUpdateBackingStore

Done.

https://codereview.chromium.org/17971002/diff/64001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_guest.cc (right):

https://codereview.chromium.org/17971002/diff/64001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_guest.cc:171:
DidReceiveRendererFrame();
On 2013/07/02 01:49:09, piman wrote:
> I suspect it's also needed in DidUpdateBackingStore.
> Or maybe not, I don't know how the UpdateRect path works for the <webview>
tag,
> but I think it's supposed to work.

The function is empty except for NOTREACHED() so I think I'll leave it out?

https://codereview.chromium.org/17971002/diff/64001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

https://codereview.chromium.org/17971002/diff/64001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_win.cc:777:
DidReceiveRendererFrame();
On 2013/07/02 01:49:09, piman wrote:
> So, on Windows, the accelerated swap doesn't get to the UI thread (it's routed
> to a "presenter" thread). I'm not sure how this would work there, short of
> posting a task to update the frame count. Maybe we can do that?

Hm. The GpuProcessHost calls the RWVImpl to tell it FrameSwapped. It could call
the RWHV->DidReceiveRendererFrame()?

[piman, 2013-07-03 20:31:52]

https://codereview.chromium.org/17971002/diff/95001/content/browser/gpu/gpu_p...
File content/browser/gpu/gpu_process_host_ui_shim.cc (right):

https://codereview.chromium.org/17971002/diff/95001/content/browser/gpu/gpu_p...
content/browser/gpu/gpu_process_host_ui_shim.cc:371:
view->AcceleratedSurfacePostSubBuffer(params, host_id_);
also needed here...

[piman, 2013-07-03 20:32:05]

LGTM4 after that

[danakj, 2013-07-03 20:34:08]

https://codereview.chromium.org/17971002/diff/95001/content/browser/gpu/gpu_p...
File content/browser/gpu/gpu_process_host_ui_shim.cc (right):

https://codereview.chromium.org/17971002/diff/95001/content/browser/gpu/gpu_p...
content/browser/gpu/gpu_process_host_ui_shim.cc:371:
view->AcceleratedSurfacePostSubBuffer(params, host_id_);
On 2013/07/03 20:31:53, piman wrote:
> also needed here...

Oops, ya, thanks.

[commit-bot: I haz the power, 2013-07-03 20:36:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/17971002/94006

[commit-bot: I haz the power, 2013-07-04 01:04:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/17971002/110002

[commit-bot: I haz the power, 2013-07-04 02:23:09]

Change committed as 210106