Support browser side thumbnailing for GPU composited pages on Windows.

Support browser side thumbnailing for GPU-composited pages on Windows.

Benchmark results of copying a 1280 x 800 pixels using CopyFromCompositingSurface or CopyFromBackingSurface are as follows.
FromCompositingSurface: 40.00 ms
FromBackingStore: 2.12 ms

I'm prepareing the following CLs and these will come soon,
- Support Mac and Chrome OS (Aura)
- Reduce the number of pixels to copy from the compositing surface for performance improvement

BUG=96351
TEST=Manual

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=126870

[mazda, 2012-03-02 13:16:13]

Hi,
Could you review this CL?

I'll update the design doc to reflect this change later
https://docs.google.com/a/google.com/document/d/1249IA-l13GY1teWX-tbVBMdK877z...

[brettw, 2012-03-02 17:15:55]

Normally the platform-specific stuff would go on the RWHView. I think you should
be able to do that here.

[vangelis, 2012-03-05 06:20:54]

I believe that in the case of accelerated compositing we'll want to be picking
up the pixels directly from the accelerated surface.  apatrick should be able to
point you to the best spot to do that.

http://codereview.chromium.org/9582003/diff/3009/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_win.cc (right):

http://codereview.chromium.org/9582003/diff/3009/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_win.cc:28: return
BitBlt(temp_dc, 0, 0, bounds.width(), bounds.height(), window_hdc,
I think ideally we'll want to get the bits out of the compositing surfaces'
accelerated_surface here.  I'm also worried about possible race conditions as
calling GetPlatformSurface() will actually even create a window if one doesn't
yet exist so there are not guarantees that we'll get fresh output here.

[mazda, 2012-03-05 15:54:14]

Thank you for the comment.

apatrick,
Could you suggest the best way to retrieve bits from AcceleratedSurface?
I tried IDirect3DSurface9::GetDC to obtain HDC for BitBlt, but it failed because
the surface is created with D3DFMT_A8R8G8B8 and the format is not supported by
GetDC.
I also tried IDirect3DSurface9::GetRenderTargetData to copy bits from GPU, but
it doesn't seem to be fast as using BitBlt with compositor_host_window_.

Thanks,

http://codereview.chromium.org/9582003/diff/3009/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_win.cc (right):

http://codereview.chromium.org/9582003/diff/3009/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_win.cc:28: return
BitBlt(temp_dc, 0, 0, bounds.width(), bounds.height(), window_hdc,
On 2012/03/05 06:20:55, vangelis wrote:
> I think ideally we'll want to get the bits out of the compositing surfaces'
> accelerated_surface here.  I'm also worried about possible race conditions as
> calling GetPlatformSurface() will actually even create a window if one doesn't
> yet exist so there are not guarantees that we'll get fresh output here.

Did you mean GetCompositingSurface()?
I plan to move these code to RenderWidgetHostViewWin as brettw suggested, where
we can check the existence of |compositor_host_window_|.

[mazda, 2012-03-06 15:27:59]

On 2012/03/02 17:15:55, brettw wrote:
> Normally the platform-specific stuff would go on the RWHView. I think you
should
> be able to do that here.

I changed the CL to delegate CopyFromCompositingSurface to RWHView.

[mazda, 2012-03-07 18:01:08]

I change the CL again to copy the surface data via GpuSurfaceTracker.

Alastair,
Could you check if this change follows what you intended in the design doc?
Once the interface of copying bits from the surface is finalized, I'll add
implementations for the other platforms.

Thanks!

[apatrick_chromium, 2012-03-07 20:39:37]

+piman for GpuSurfaceTracker changes.

I think this will mostly work, except for a potential thread safety issue. Some
comments below.

http://codereview.chromium.org/9582003/diff/12002/chrome/browser/tab_contents...
File chrome/browser/tab_contents/thumbnail_generator.cc (right):

http://codereview.chromium.org/9582003/diff/12002/chrome/browser/tab_contents...
chrome/browser/tab_contents/thumbnail_generator.cc:83: if (view)
nit: braces around multiline body.

http://codereview.chromium.org/9582003/diff/12002/content/browser/gpu/gpu_sur...
File content/browser/gpu/gpu_surface_reader_win.cc (right):

http://codereview.chromium.org/9582003/diff/12002/content/browser/gpu/gpu_sur...
content/browser/gpu/gpu_surface_reader_win.cc:20: if
(!surface_info.handle.accelerated_surface)
Alternatively, given how simple it is, why not just move this into
GpuSurfaceTracker, perhaps with a platform specific gpu_surface_tracker_win.cc
if this ever becomes non-trivial?

http://codereview.chromium.org/9582003/diff/12002/content/browser/gpu/gpu_sur...
File content/browser/gpu/gpu_surface_reader_win.h (right):

http://codereview.chromium.org/9582003/diff/12002/content/browser/gpu/gpu_sur...
content/browser/gpu/gpu_surface_reader_win.h:12: class GpuSurfaceReader {
Do you envision this class ever having state? I don't see why CopySurface is not
just a free function.

http://codereview.chromium.org/9582003/diff/12002/content/browser/gpu/gpu_sur...
File content/browser/gpu/gpu_surface_tracker.cc (right):

http://codereview.chromium.org/9582003/diff/12002/content/browser/gpu/gpu_sur...
content/browser/gpu/gpu_surface_tracker.cc:17: reader_.reset(new
GpuSurfaceReader);
And if CopySurface was a free function, this would not be needed.

http://codereview.chromium.org/9582003/diff/12002/content/browser/gpu/gpu_sur...
File content/browser/gpu/gpu_surface_tracker.h (right):

http://codereview.chromium.org/9582003/diff/12002/content/browser/gpu/gpu_sur...
content/browser/gpu/gpu_surface_tracker.h:31: struct SurfaceInfo {
If it weren't for GpuSurfaceReader, this could remain private.

http://codereview.chromium.org/9582003/diff/12002/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

http://codereview.chromium.org/9582003/diff/12002/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_impl.cc:511: &pixels))
Is it possible to copy the pixels directly into the SkBitmap rather than copy
them from the vector into the SkBitmap later?

http://codereview.chromium.org/9582003/diff/12002/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_impl.cc:517:
bitmap.setPixels(&pixels[0]);
This won't work if the area of the surface is zero because pixels will not have
an element at index zero to take the address of and will assert in debug builds.

http://codereview.chromium.org/9582003/diff/12002/ui/gfx/surface/accelerated_...
File ui/gfx/surface/accelerated_surface_win.cc (right):

http://codereview.chromium.org/9582003/diff/12002/ui/gfx/surface/accelerated_...
ui/gfx/surface/accelerated_surface_win.cc:270:
base::win::ScopedComPtr<IDirect3DSurface9> render_target;
Make the first line of this function:
base::AutoLock locked(lock_);

Otherwise it might try to access the surface concurrently with a present thread.

http://codereview.chromium.org/9582003/diff/12002/ui/gfx/surface/accelerated_...
ui/gfx/surface/accelerated_surface_win.cc:271: HRESULT hr =
device_->GetRenderTarget(0, render_target.Receive());
I just changed this unfortunately but you should be able to call GetBackBuffer
on the newly added swap_chain_ with the same effect.

http://codereview.chromium.org/9582003/diff/12002/ui/gfx/surface/accelerated_...
ui/gfx/surface/accelerated_surface_win.cc:291: hr =
device_->StretchRect(render_target, NULL, temp_render_target, NULL,
If the downsampling reduces the size by more than half on either axis, this will
introduce aliasing. The solution would be to call this multiple times, each by a
factor of no more than two.

http://codereview.chromium.org/9582003/diff/12002/ui/gfx/surface/accelerated_...
ui/gfx/surface/accelerated_surface_win.cc:303: NULL);
HANDLE handle = reinterpret_cast<HANDLE>(&buf[0]);  // but fix the buf[0] issue
for zero area buffers
hr = device_->CreateOffscreenPlainSurface(
    size.width(),
    size.height(),
    D3DFMT_A8R8G8B8,
    D3DPOOL_SYSTEMMEM,
    temp_surface.Receive(),
    &handle);

Will create an offscreen system memory surface backed by the bits in buf...

http://codereview.chromium.org/9582003/diff/12002/ui/gfx/surface/accelerated_...
ui/gfx/surface/accelerated_surface_win.cc:318: D3DLOCKED_RECT locked_rect;
... thereby eliminating the need for lines 318 through 330 and lots of redundant
memory copies. See
http://msdn.microsoft.com/en-us/library/windows/desktop/bb219800(v=vs.85).asp...
(Texture Creation in System Memory).

http://codereview.chromium.org/9582003/diff/12002/ui/gfx/surface/accelerated_...
ui/gfx/surface/accelerated_surface_win.cc:321:
temp_surface->LockRect(&locked_rect, NULL, kLockFlags);
If these lines remain, I would not assume LockRect always succeeds and returns a
value locked_rect.pBits. The device might be lost.

[piman, 2012-03-07 21:22:27]

Overall, it makes me sad that anything that needs to access the
GLSurfaceHandle::accelerated_surface ends up having to be implemented in the
GpuSurfaceTracker because of race conditions otherwise.
I'm afraid that instead of being a trivial thread-safe registry (what it was
meant to be at first), it will become more and more stateful and reasoning about
correctness will become harder and harder.
Also adding platform-specific stuff there bothers me.
Can we try to find a better solution?

http://codereview.chromium.org/9582003/diff/12002/content/browser/gpu/gpu_sur...
File content/browser/gpu/gpu_surface_tracker.cc (right):

http://codereview.chromium.org/9582003/diff/12002/content/browser/gpu/gpu_sur...
content/browser/gpu/gpu_surface_tracker.cc:101: #if defined(OS_WIN)
&& !defined(USE_AURA)

[piman, 2012-03-07 21:29:59]

On Wed, Mar 7, 2012 at 1:22 PM, <piman@chromium.org> wrote:

> Overall, it makes me sad that anything that needs to access the
> GLSurfaceHandle::accelerated_**surface ends up having to be implemented
> in the
> GpuSurfaceTracker because of race conditions otherwise.
> I'm afraid that instead of being a trivial thread-safe registry (what it
> was
> meant to be at first), it will become more and more stateful and reasoning
> about
> correctness will become harder and harder.
> Also adding platform-specific stuff there bothers me.
> Can we try to find a better solution?


Something I'm thinking of is, instead of having the accelerated_surface
directly in the GLSurfaceHandle (which was meant to be a copiable value
type), keep it in a separate win32-only thread-safe registry, and only have
its id inside of the GLSurfaceHandle. Then you can implement logic about
the accelerated_surface there. It's an extra indirection, but it would make
this more scalable I think.
Of course this is not directly related to this patch, but it reinforces the
worries I had when accelerated_surface was added to GLSurfaceHandle.

Antoine


>
>
> http://codereview.chromium.**org/9582003/diff/12002/**
>
content/browser/gpu/gpu_**surface_tracker.cc<http://codereview.chromium.org/9582003/diff/12002/content/browser/gpu/gpu_surface_tracker.cc>
> File content/browser/gpu/gpu_**surface_tracker.cc (right):
>
> http://codereview.chromium.**org/9582003/diff/12002/**
>
content/browser/gpu/gpu_**surface_tracker.cc#newcode101<http://codereview.chromium.org/9582003/diff/12002/content/browser/gpu/gpu_surface_tracker.cc#newcode101>
> content/browser/gpu/gpu_**surface_tracker.cc:101: #if defined(OS_WIN)
> && !defined(USE_AURA)
>
>
http://codereview.chromium.**org/9582003/<http://codereview.chromium.org/9582...
>

[apatrick_chromium, 2012-03-07 21:37:50]

For the purposes of this patch, since this code runs on the browser process UI
thread, the AcceleratedSurface can be retrieved via the RenderWidgetHostView.
The AcceleratedSurface already has sufficient thread synchronization to be
accessed directly on the UI thread.

[mazda, 2012-03-08 13:14:28]

Thank you for the comments.
Could you take another look at the change?

I reverted the change of GpuSurfaceTracker and moved the platform specific code
to RenderWidgetHostView*.
If there are any better ideas, please let me know.

Thanks,

http://codereview.chromium.org/9582003/diff/12002/chrome/browser/tab_contents...
File chrome/browser/tab_contents/thumbnail_generator.cc (right):

http://codereview.chromium.org/9582003/diff/12002/chrome/browser/tab_contents...
chrome/browser/tab_contents/thumbnail_generator.cc:83: if (view)
On 2012/03/07 20:39:37, apatrick_chromium wrote:
> nit: braces around multiline body.

Done.

http://codereview.chromium.org/9582003/diff/12002/content/browser/gpu/gpu_sur...
File content/browser/gpu/gpu_surface_reader_win.cc (right):

http://codereview.chromium.org/9582003/diff/12002/content/browser/gpu/gpu_sur...
content/browser/gpu/gpu_surface_reader_win.cc:20: if
(!surface_info.handle.accelerated_surface)
On 2012/03/07 20:39:37, apatrick_chromium wrote:
> Alternatively, given how simple it is, why not just move this into
> GpuSurfaceTracker, perhaps with a platform specific gpu_surface_tracker_win.cc
> if this ever becomes non-trivial?

Moved the code to RenderWidgetHostViewWin.

http://codereview.chromium.org/9582003/diff/12002/content/browser/gpu/gpu_sur...
File content/browser/gpu/gpu_surface_reader_win.h (right):

http://codereview.chromium.org/9582003/diff/12002/content/browser/gpu/gpu_sur...
content/browser/gpu/gpu_surface_reader_win.h:12: class GpuSurfaceReader {
On 2012/03/07 20:39:37, apatrick_chromium wrote:
> Do you envision this class ever having state? I don't see why CopySurface is
not
> just a free function.

I thought it might be useful to be able to switch the implementation easily. 
Now I moved the code to RenderWidgetHostViewWin

http://codereview.chromium.org/9582003/diff/12002/content/browser/gpu/gpu_sur...
File content/browser/gpu/gpu_surface_tracker.cc (right):

http://codereview.chromium.org/9582003/diff/12002/content/browser/gpu/gpu_sur...
content/browser/gpu/gpu_surface_tracker.cc:17: reader_.reset(new
GpuSurfaceReader);
On 2012/03/07 20:39:37, apatrick_chromium wrote:
> And if CopySurface was a free function, this would not be needed.

Now I moved the code in CopySurface to RenderWidgetHostViewWin, so reverted the
change in this file.

http://codereview.chromium.org/9582003/diff/12002/content/browser/gpu/gpu_sur...
content/browser/gpu/gpu_surface_tracker.cc:101: #if defined(OS_WIN)
On 2012/03/07 21:22:27, piman wrote:
> && !defined(USE_AURA)

Now I moved the code in CopySurface to RenderWidgetHostViewWin, so reverted the
change in this file.

http://codereview.chromium.org/9582003/diff/12002/content/browser/gpu/gpu_sur...
File content/browser/gpu/gpu_surface_tracker.h (right):

http://codereview.chromium.org/9582003/diff/12002/content/browser/gpu/gpu_sur...
content/browser/gpu/gpu_surface_tracker.h:31: struct SurfaceInfo {
On 2012/03/07 20:39:37, apatrick_chromium wrote:
> If it weren't for GpuSurfaceReader, this could remain private.

Done.

http://codereview.chromium.org/9582003/diff/12002/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

http://codereview.chromium.org/9582003/diff/12002/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_impl.cc:511: &pixels))
On 2012/03/07 20:39:37, apatrick_chromium wrote:
> Is it possible to copy the pixels directly into the SkBitmap rather than copy
> them from the vector into the SkBitmap later?

Actually SkBitmap::setPixels does not copy the data to SkBitmap and just sets
the pointer, so there should be little difference (I also noticed that
allocPixels is not necessary).
SkCanvas::writePixels does copy the data, but there's no way to write the pixels
directly to SkCanvas with DirectX as far as I know.

http://codereview.chromium.org/9582003/diff/12002/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_impl.cc:517:
bitmap.setPixels(&pixels[0]);
On 2012/03/07 20:39:37, apatrick_chromium wrote:
> This won't work if the area of the surface is zero because pixels will not
have
> an element at index zero to take the address of and will assert in debug
builds.

I added a check for |size| and also made AcceleratorSurface::CopyTo fail if the
area of surface is zero.

http://codereview.chromium.org/9582003/diff/12002/ui/gfx/surface/accelerated_...
File ui/gfx/surface/accelerated_surface_win.cc (right):

http://codereview.chromium.org/9582003/diff/12002/ui/gfx/surface/accelerated_...
ui/gfx/surface/accelerated_surface_win.cc:270:
base::win::ScopedComPtr<IDirect3DSurface9> render_target;
On 2012/03/07 20:39:37, apatrick_chromium wrote:
> Make the first line of this function:
> base::AutoLock locked(lock_);
> 
> Otherwise it might try to access the surface concurrently with a present
thread.

Done.

http://codereview.chromium.org/9582003/diff/12002/ui/gfx/surface/accelerated_...
ui/gfx/surface/accelerated_surface_win.cc:271: HRESULT hr =
device_->GetRenderTarget(0, render_target.Receive());
On 2012/03/07 20:39:37, apatrick_chromium wrote:
> I just changed this unfortunately but you should be able to call GetBackBuffer
> on the newly added swap_chain_ with the same effect.

Done.

http://codereview.chromium.org/9582003/diff/12002/ui/gfx/surface/accelerated_...
ui/gfx/surface/accelerated_surface_win.cc:291: hr =
device_->StretchRect(render_target, NULL, temp_render_target, NULL,
On 2012/03/07 20:39:37, apatrick_chromium wrote:
> If the downsampling reduces the size by more than half on either axis, this
will
> introduce aliasing. The solution would be to call this multiple times, each by
a
> factor of no more than two.

Done.

http://codereview.chromium.org/9582003/diff/12002/ui/gfx/surface/accelerated_...
ui/gfx/surface/accelerated_surface_win.cc:303: NULL);
On 2012/03/07 20:39:37, apatrick_chromium wrote:
> HANDLE handle = reinterpret_cast<HANDLE>(&buf[0]);  // but fix the buf[0]
issue
> for zero area buffers
> hr = device_->CreateOffscreenPlainSurface(
>     size.width(),
>     size.height(),
>     D3DFMT_A8R8G8B8,
>     D3DPOOL_SYSTEMMEM,
>     temp_surface.Receive(),
>     &handle);
> 
> Will create an offscreen system memory surface backed by the bits in buf...

Thanks. This is much simpler and cleaner.

http://codereview.chromium.org/9582003/diff/12002/ui/gfx/surface/accelerated_...
ui/gfx/surface/accelerated_surface_win.cc:318: D3DLOCKED_RECT locked_rect;
On 2012/03/07 20:39:37, apatrick_chromium wrote:
> ... thereby eliminating the need for lines 318 through 330 and lots of
redundant
> memory copies. See
>
http://msdn.microsoft.com/en-us/library/windows/desktop/bb219800%28v=vs.85%29...
> (Texture Creation in System Memory).

Done.

http://codereview.chromium.org/9582003/diff/12002/ui/gfx/surface/accelerated_...
ui/gfx/surface/accelerated_surface_win.cc:321:
temp_surface->LockRect(&locked_rect, NULL, kLockFlags);
On 2012/03/07 20:39:37, apatrick_chromium wrote:
> If these lines remain, I would not assume LockRect always succeeds and returns
a
> value locked_rect.pBits. The device might be lost.

I deleted this code block following your comment above.

[vangelis, 2012-03-08 17:34:34]

Looks good overall!  Just a couple of comments.

http://codereview.chromium.org/9582003/diff/20002/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/9582003/diff/20002/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:930:
output->writePixels(bitmap, 0, 0);
Is it possible to call accelerated_surface_->CopyTo() directly onto the output
canvas' pixels to avoid this copy?

http://codereview.chromium.org/9582003/diff/20002/ui/gfx/surface/accelerated_...
File ui/gfx/surface/accelerated_surface_win.cc (right):

http://codereview.chromium.org/9582003/diff/20002/ui/gfx/surface/accelerated_...
ui/gfx/surface/accelerated_surface_win.cc:176: hr =
present_thread_->device()->CreateRenderTarget(
Can we avoid creating render targets for all intermediate resolutions? 
StretchRect takes a source and dest rect so it should in theory be possible to
do it. I suspect that we'll need one surface that's half the size of the buffer
and one that's at the target resulution (for the final GetRenderTargetData)

[apatrick_chromium, 2012-03-08 20:15:47]

A few more thoughts...

http://codereview.chromium.org/9582003/diff/12002/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

http://codereview.chromium.org/9582003/diff/12002/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_impl.cc:511: &pixels))
On 2012/03/08 13:14:28, mazda wrote:
> On 2012/03/07 20:39:37, apatrick_chromium wrote:
> > Is it possible to copy the pixels directly into the SkBitmap rather than
copy
> > them from the vector into the SkBitmap later?
> 
> Actually SkBitmap::setPixels does not copy the data to SkBitmap and just sets
> the pointer, so there should be little difference (I also noticed that
> allocPixels is not necessary).
> SkCanvas::writePixels does copy the data, but there's no way to write the
pixels
> directly to SkCanvas with DirectX as far as I know.

I think you could call SkBitmap::allocPixels before CopySurface, call
SkBitmap::getPixels to get the address of the allocated buffer and pass that as
the pixels argument to CopySurface. That would mean changing the the type of the
pixels argument from std::vector* to void* and the code in AcceleratedPresenter
would just assume that the output buffer was exacly the size implied by the size
argument.

http://codereview.chromium.org/9582003/diff/20002/ui/gfx/surface/accelerated_...
File ui/gfx/surface/accelerated_surface_win.cc (right):

http://codereview.chromium.org/9582003/diff/20002/ui/gfx/surface/accelerated_...
ui/gfx/surface/accelerated_surface_win.cc:154: HRESULT hr =
swap_chain_->GetBackBuffer(0,
swap_chain_ will be null if either no surface has been presented yet or the
surface has been suspended because it has not been visible for some number of
tab switches. I think just return false if swap_chain_ is null. Check whether it
is null after locking lock_ though because it is suspended on the present
thread.

http://codereview.chromium.org/9582003/diff/20002/ui/gfx/surface/accelerated_...
ui/gfx/surface/accelerated_surface_win.cc:172: do {
I believe it is possible to call GetRenderTargetData directly on a swap chain
surface since a swap chain surface is a render target. Therefore I think this
loop should iterate zero times if the swap chain happens to be exactly the
requested size.

[mazda, 2012-03-10 07:51:36]

http://codereview.chromium.org/9582003/diff/12002/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

http://codereview.chromium.org/9582003/diff/12002/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_impl.cc:511: &pixels))
On 2012/03/08 20:15:47, apatrick_chromium wrote:
> On 2012/03/08 13:14:28, mazda wrote:
> > On 2012/03/07 20:39:37, apatrick_chromium wrote:
> > > Is it possible to copy the pixels directly into the SkBitmap rather than
> copy
> > > them from the vector into the SkBitmap later?
> > 
> > Actually SkBitmap::setPixels does not copy the data to SkBitmap and just
sets
> > the pointer, so there should be little difference (I also noticed that
> > allocPixels is not necessary).
> > SkCanvas::writePixels does copy the data, but there's no way to write the
> pixels
> > directly to SkCanvas with DirectX as far as I know.
> 
> I think you could call SkBitmap::allocPixels before CopySurface, call
> SkBitmap::getPixels to get the address of the allocated buffer and pass that
as
> the pixels argument to CopySurface. That would mean changing the the type of
the
> pixels argument from std::vector* to void* and the code in
AcceleratedPresenter
> would just assume that the output buffer was exacly the size implied by the
size
> argument.

Thanks. I changed the AcceleratedSurface::CopyTo to take void*.

http://codereview.chromium.org/9582003/diff/20002/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/9582003/diff/20002/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:930:
output->writePixels(bitmap, 0, 0);
On 2012/03/08 17:34:35, vangelis wrote:
> Is it possible to call accelerated_surface_->CopyTo() directly onto the output
> canvas' pixels to avoid this copy? 

I changed AccceleratedSurface::CopyTo to take void* as an argument following
apatrick's comment, so now it's possible to pass the raw pointer of SkBitmap in
this PlatformCanvas. I modified the code that way.

http://codereview.chromium.org/9582003/diff/20002/ui/gfx/surface/accelerated_...
File ui/gfx/surface/accelerated_surface_win.cc (right):

http://codereview.chromium.org/9582003/diff/20002/ui/gfx/surface/accelerated_...
ui/gfx/surface/accelerated_surface_win.cc:154: HRESULT hr =
swap_chain_->GetBackBuffer(0,
On 2012/03/08 20:15:47, apatrick_chromium wrote:
> swap_chain_ will be null if either no surface has been presented yet or the
> surface has been suspended because it has not been visible for some number of
> tab switches. I think just return false if swap_chain_ is null. Check whether
it
> is null after locking lock_ though because it is suspended on the present
> thread.

Done.

http://codereview.chromium.org/9582003/diff/20002/ui/gfx/surface/accelerated_...
ui/gfx/surface/accelerated_surface_win.cc:172: do {
On 2012/03/08 20:15:47, apatrick_chromium wrote:
> I believe it is possible to call GetRenderTargetData directly on a swap chain
> surface since a swap chain surface is a render target. Therefore I think this
> loop should iterate zero times if the swap chain happens to be exactly the
> requested size.

Done.

http://codereview.chromium.org/9582003/diff/20002/ui/gfx/surface/accelerated_...
ui/gfx/surface/accelerated_surface_win.cc:176: hr =
present_thread_->device()->CreateRenderTarget(
On 2012/03/08 17:34:35, vangelis wrote:
> Can we avoid creating render targets for all intermediate resolutions? 
> StretchRect takes a source and dest rect so it should in theory be possible to
> do it. I suspect that we'll need one surface that's half the size of the
buffer
> and one that's at the target resulution (for the final GetRenderTargetData)  

Done.

[apatrick_chromium, 2012-03-12 20:25:06]

http://codereview.chromium.org/9582003/diff/33001/ui/gfx/surface/accelerated_...
File ui/gfx/surface/accelerated_surface_win.cc (right):

http://codereview.chromium.org/9582003/diff/33001/ui/gfx/surface/accelerated_...
ui/gfx/surface/accelerated_surface_win.cc:233: const gfx::Size quater_size =
GetHalfSizeNoLessThan(half_size, size);
quater -> quarter

http://codereview.chromium.org/9582003/diff/33001/ui/gfx/surface/accelerated_...
ui/gfx/surface/accelerated_surface_win.cc:253: RECT read_rect = {0, 0,
read_size.width() - 1, read_size.height() - 1};
The Windows convention is that right and bottom are exclusive, i.e. width ==
right - left and height == bottom - top. So I don't think you want to subtract 1
here. See http://msdn.microsoft.com/en-us/library/dd162897(VS.85).aspx .

http://codereview.chromium.org/9582003/diff/33001/ui/gfx/surface/accelerated_...
ui/gfx/surface/accelerated_surface_win.cc:254: RECT write_rect = {0, 0,
write_size.width() - 1, write_size.height() - 1};
Ditto.

[mazda, 2012-03-12 23:20:42]

http://codereview.chromium.org/9582003/diff/33001/ui/gfx/surface/accelerated_...
File ui/gfx/surface/accelerated_surface_win.cc (right):

http://codereview.chromium.org/9582003/diff/33001/ui/gfx/surface/accelerated_...
ui/gfx/surface/accelerated_surface_win.cc:233: const gfx::Size quater_size =
GetHalfSizeNoLessThan(half_size, size);
On 2012/03/12 20:25:06, apatrick_chromium wrote:
> quater -> quarter

Done.

http://codereview.chromium.org/9582003/diff/33001/ui/gfx/surface/accelerated_...
ui/gfx/surface/accelerated_surface_win.cc:253: RECT read_rect = {0, 0,
read_size.width() - 1, read_size.height() - 1};
On 2012/03/12 20:25:06, apatrick_chromium wrote:
> The Windows convention is that right and bottom are exclusive, i.e. width ==
> right - left and height == bottom - top. So I don't think you want to subtract
1
> here. See http://msdn.microsoft.com/en-us/library/dd162897%28VS.85%29.aspx .

Fixed. Thanks.

http://codereview.chromium.org/9582003/diff/33001/ui/gfx/surface/accelerated_...
ui/gfx/surface/accelerated_surface_win.cc:254: RECT write_rect = {0, 0,
write_size.width() - 1, write_size.height() - 1};
On 2012/03/12 20:25:06, apatrick_chromium wrote:
> Ditto.

Done.

[apatrick_chromium, 2012-03-12 23:22:23]

lgtm

[mazda, 2012-03-13 17:26:41]

Vangelis:
Does my change of the parts you commented on look good?

Owners of the following directories: Please make owners' review.
content/browser: Brett
ui/gfx/surface: Jonathan
content/public/browser: Randy

Thanks,

[Randy Smith (Not in Mondays), 2012-03-13 19:02:11]

I'm the wrong person for generic content/browser/public reviews--I'm just in
there for expediting downloads refactors.  I believe new interfaces are to be
reviewed by John Abd-El Malek; adding him.

[jam, 2012-03-13 19:40:10]

http://codereview.chromium.org/9582003/diff/42003/content/public/browser/rend...
File content/public/browser/render_widget_host_view.h (right):

http://codereview.chromium.org/9582003/diff/42003/content/public/browser/rend...
content/public/browser/render_widget_host_view.h:144: virtual bool
CopyFromCompositingSurface(const gfx::Size& size,
this method doesn't need to be exposed to embedders (i.e. chrome), so instead it
should be on the RenderWidgetHostViewPort interface

[vangelis, 2012-03-13 19:40:49]

Sorry for the late response. It looks good, thanks!

http://codereview.chromium.org/9582003/diff/42003/chrome/browser/tab_contents...
File chrome/browser/tab_contents/thumbnail_generator.cc (right):

http://codereview.chromium.org/9582003/diff/42003/chrome/browser/tab_contents...
chrome/browser/tab_contents/thumbnail_generator.cc:84: // whole view size.
Don't we want to use the desired_width/height here to do the shrinking on the
gpu since it's already implemented?

[jam, 2012-03-13 19:41:36]

another note: do we really want the embedder code to all have

if (backing_store)
  copy from backing store
else if (compositing_surface)
  copy from..

why not just have one method on RenderWidgetHost that hides this?

[mazda, 2012-03-14 12:07:45]

Thank you for the comments.
Please take another look.

John
> why not just have one method on RenderWidgetHost that hides this?
I merged RenderWidgetHostImpl::CopyFromCompositingSurface into
RenderWidgetHostImpl::CopyFromBackingStore.

Thanks,

http://codereview.chromium.org/9582003/diff/42003/chrome/browser/tab_contents...
File chrome/browser/tab_contents/thumbnail_generator.cc (right):

http://codereview.chromium.org/9582003/diff/42003/chrome/browser/tab_contents...
chrome/browser/tab_contents/thumbnail_generator.cc:84: // whole view size.
On 2012/03/13 19:40:49, vangelis wrote:
> Don't we want to use the desired_width/height here to do the shrinking on the
> gpu since it's already implemented? 

Can I resolve the TODO in another CL? Since this size affects performance and
the quality of thumbnail, I'd like to choose optimal value with more
experiments. Just setting desired_width/height can cause the aspect ratio to be
changed. The copied image also needs to be clipped if the aspect ratio is not
equal to that of thumbnail image.

http://codereview.chromium.org/9582003/diff/42003/content/public/browser/rend...
File content/public/browser/render_widget_host_view.h (right):

http://codereview.chromium.org/9582003/diff/42003/content/public/browser/rend...
content/public/browser/render_widget_host_view.h:144: virtual bool
CopyFromCompositingSurface(const gfx::Size& size,
On 2012/03/13 19:40:11, John Abd-El-Malek wrote:
> this method doesn't need to be exposed to embedders (i.e. chrome), so instead
it
> should be on the RenderWidgetHostViewPort interface

Done.

[mazda, 2012-03-14 14:56:56]

+kbr
Could you do owner's review of ui/gfx/surface because it looks backer@ is out
for the week?

[jam, 2012-03-14 17:56:42]

http://codereview.chromium.org/9582003/diff/47001/content/public/browser/rend...
File content/public/browser/render_widget_host.h (right):

http://codereview.chromium.org/9582003/diff/47001/content/public/browser/rend...
content/public/browser/render_widget_host.h:186: virtual bool
CopyFromBackingStore(const gfx::Size& dest_size,
you're not setting this anywhere now, so why are you adding it?

[mazda, 2012-03-14 18:19:04]

http://codereview.chromium.org/9582003/diff/47001/content/public/browser/rend...
File content/public/browser/render_widget_host.h (right):

http://codereview.chromium.org/9582003/diff/47001/content/public/browser/rend...
content/public/browser/render_widget_host.h:186: virtual bool
CopyFromBackingStore(const gfx::Size& dest_size,
On 2012/03/14 17:56:42, John Abd-El-Malek wrote:
> you're not setting this anywhere now, so why are you adding it?

I'll set it soon in ThumbnailGenerator::GetBitmapForRenderWidgetHost after I
find out an optimal size with more experiments.

[Ken Russell (switch to Gerrit), 2012-03-14 19:00:38]

The changes in ui/gfx/surface LGTM but apatrick should make the final call. I'm
adding him as an OWNER in that directory now.

[jam, 2012-03-14 19:03:07]

On 2012/03/14 18:19:04, mazda wrote:
>
http://codereview.chromium.org/9582003/diff/47001/content/public/browser/rend...
> File content/public/browser/render_widget_host.h (right):
> 
>
http://codereview.chromium.org/9582003/diff/47001/content/public/browser/rend...
> content/public/browser/render_widget_host.h:186: virtual bool
> CopyFromBackingStore(const gfx::Size& dest_size,
> On 2012/03/14 17:56:42, John Abd-El-Malek wrote:
> > you're not setting this anywhere now, so why are you adding it?
> 
> I'll set it soon in ThumbnailGenerator::GetBitmapForRenderWidgetHost after I
> find out an optimal size with more experiments.

lgtm

when you start setting that, please send the owners review to brettw as he
originally wrote that

[apatrick_chromium, 2012-03-14 20:00:45]

I reviewed this already and it still looks okay. LGTM.

[mazda, 2012-03-15 04:10:48]

Thank you all for reviewing the CL.

> when you start setting that, please send the owners review
> to brettw as he originally wrote that
Sure, will do.

[vangelis, 2012-03-15 05:28:55]

http://codereview.chromium.org/9582003/diff/42003/chrome/browser/tab_contents...
> chrome/browser/tab_contents/thumbnail_generator.cc:84: // whole view size.
> On 2012/03/13 19:40:49, vangelis wrote:
> > Don't we want to use the desired_width/height here to do the shrinking on
the
> > gpu since it's already implemented? 
> 
> Can I resolve the TODO in another CL? Since this size affects performance and
> the quality of thumbnail, I'd like to choose optimal value with more
> experiments. Just setting desired_width/height can cause the aspect ratio to
be
> changed. The copied image also needs to be clipped if the aspect ratio is not
> equal to that of thumbnail image.
 
Sounds good, thanks!

[commit-bot: I haz the power, 2012-03-15 06:45:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mazda@chromium.org/9582003/49008

[commit-bot: I haz the power, 2012-03-15 09:29:39]

Change committed as 126870