[Android WebView] Fix DevTools Screencast slowness

[Android WebView] Fix DevTools Screencast slowness

Do not scale the destination bitmap by the device screen density if
synchronous compositor is being used.

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

[mnaganov (inactive), 2014-05-19 17:32:52]

Daniel, Pavel, can you please take a look?

I have tested it with Android WebView Shell and with WebView as a system
component, seems to work fine (and much faster!)

Chrome is not affected, as it doesn't use synchronous compositor.

[no sievers, 2014-05-19 18:45:08]

Doesn't that break the API? So it would now return you a bitmap of the wrong
size, i.e. different size compared to what the non-synchronous compositor would
return.

If you want a scaled down version why don't you just pass in a different dst
rect? That's what we do for thumbnails in Chrome.

[mnaganov (inactive), 2014-05-19 20:02:13]

On 2014/05/19 18:45:08, sievers wrote:
> Doesn't that break the API? So it would now return you a bitmap of the wrong
> size, i.e. different size compared to what the non-synchronous compositor
would
> return.
> 
> If you want a scaled down version why don't you just pass in a different dst
> rect? That's what we do for thumbnails in Chrome.

Pre-downscaling the dst rect will create an implicit dependency between the code
in RendererOverridesHandler::InnerSwapCompositorFrame (which calls
CopyFromCompositingSurface) and CopyFromCompositingSurface itself, which doesn't
seem good to me.

I'll let Pavel speak for the API side here. In the meanwhile, I can explain what
I have observed here.

When DevTools sends a request for the bitmap, it provides the size in CSS pixels
of its Screencast widget (I have checked this in
RendererOverridesHandler::InnerSwapCompositorFrame). If we return a bitmap of
this size, then DevTools actually gets what is has asked for. I'm not sure,
whether is it in fact correct that in Chrome, we return a bitmap that is scaled
up by the device screen density -- surely this can provide a sharper image if on
desktop you use a hi-res screen, but technically, this isn't correct.

[no sievers, 2014-05-19 20:07:26]

On 2014/05/19 20:02:13, Mikhail Naganov (Cr) wrote:
> On 2014/05/19 18:45:08, sievers wrote:
> > Doesn't that break the API? So it would now return you a bitmap of the wrong
> > size, i.e. different size compared to what the non-synchronous compositor
> would
> > return.
> > 
> > If you want a scaled down version why don't you just pass in a different dst
> > rect? That's what we do for thumbnails in Chrome.
> 
> Pre-downscaling the dst rect will create an implicit dependency between the
code
> in RendererOverridesHandler::InnerSwapCompositorFrame (which calls
> CopyFromCompositingSurface) and CopyFromCompositingSurface itself, which
doesn't
> seem good to me.
> 
> I'll let Pavel speak for the API side here. In the meanwhile, I can explain
what
> I have observed here.
> 
> When DevTools sends a request for the bitmap, it provides the size in CSS
pixels
> of its Screencast widget (I have checked this in
> RendererOverridesHandler::InnerSwapCompositorFrame). If we return a bitmap of
> this size, then DevTools actually gets what is has asked for. I'm not sure,
> whether is it in fact correct that in Chrome, we return a bitmap that is
scaled
> up by the device screen density -- surely this can provide a sharper image if
on
> desktop you use a hi-res screen, but technically, this isn't correct.

What happens on AURA?

[mnaganov (inactive), 2014-05-19 20:14:33]

On 2014/05/19 20:07:26, sievers wrote:
> On 2014/05/19 20:02:13, Mikhail Naganov (Cr) wrote:
> > On 2014/05/19 18:45:08, sievers wrote:
> > > Doesn't that break the API? So it would now return you a bitmap of the
wrong
> > > size, i.e. different size compared to what the non-synchronous compositor
> > would
> > > return.
> > > 
> > > If you want a scaled down version why don't you just pass in a different
dst
> > > rect? That's what we do for thumbnails in Chrome.
> > 
> > Pre-downscaling the dst rect will create an implicit dependency between the
> code
> > in RendererOverridesHandler::InnerSwapCompositorFrame (which calls
> > CopyFromCompositingSurface) and CopyFromCompositingSurface itself, which
> doesn't
> > seem good to me.
> > 
> > I'll let Pavel speak for the API side here. In the meanwhile, I can explain
> what
> > I have observed here.
> > 
> > When DevTools sends a request for the bitmap, it provides the size in CSS
> pixels
> > of its Screencast widget (I have checked this in
> > RendererOverridesHandler::InnerSwapCompositorFrame). If we return a bitmap
of
> > this size, then DevTools actually gets what is has asked for. I'm not sure,
> > whether is it in fact correct that in Chrome, we return a bitmap that is
> scaled
> > up by the device screen density -- surely this can provide a sharper image
if
> on
> > desktop you use a hi-res screen, but technically, this isn't correct.
> 
> What happens on AURA?

I'm not sure. Does Aura work with Android? Screencast is Android-only feature.

[no sievers, 2014-05-19 20:20:20]

On 2014/05/19 20:14:33, Mikhail Naganov (Cr) wrote:
> On 2014/05/19 20:07:26, sievers wrote:
> > On 2014/05/19 20:02:13, Mikhail Naganov (Cr) wrote:
> > > On 2014/05/19 18:45:08, sievers wrote:
> > > > Doesn't that break the API? So it would now return you a bitmap of the
> wrong
> > > > size, i.e. different size compared to what the non-synchronous
compositor
> > > would
> > > > return.
> > > > 
> > > > If you want a scaled down version why don't you just pass in a different
> dst
> > > > rect? That's what we do for thumbnails in Chrome.
> > > 
> > > Pre-downscaling the dst rect will create an implicit dependency between
the
> > code
> > > in RendererOverridesHandler::InnerSwapCompositorFrame (which calls
> > > CopyFromCompositingSurface) and CopyFromCompositingSurface itself, which
> > doesn't
> > > seem good to me.
> > > 
> > > I'll let Pavel speak for the API side here. In the meanwhile, I can
explain
> > what
> > > I have observed here.
> > > 
> > > When DevTools sends a request for the bitmap, it provides the size in CSS
> > pixels
> > > of its Screencast widget (I have checked this in
> > > RendererOverridesHandler::InnerSwapCompositorFrame). If we return a bitmap
> of
> > > this size, then DevTools actually gets what is has asked for. I'm not
sure,
> > > whether is it in fact correct that in Chrome, we return a bitmap that is
> > scaled
> > > up by the device screen density -- surely this can provide a sharper image
> if
> > on
> > > desktop you use a hi-res screen, but technically, this isn't correct.
> > 
> > What happens on AURA?
> 
> I'm not sure. Does Aura work with Android? Screencast is Android-only feature.

But CopyFromCompositingSurface() is a common API and should behave the same
across platforms.
There are probably even unit tests for this, we might just not be running them
on Android.

[pfeldman, 2014-05-20 13:21:39]

I agree with sievers@ - not sure why I ended up scaling the dst_rect. I thought
I was copying the aura version, but now that I look at it, I don't see dst_rect
being scaled there. I guess I misunderstood the API back then.
We should be able to fix it here + patch the scale calculation at
RendererOverridesHandler::ParseCaptureParameters.

[mnaganov (inactive), 2014-05-20 17:15:24]

On 2014/05/20 13:21:39, pfeldman wrote:
> I agree with sievers@ - not sure why I ended up scaling the dst_rect. I
thought
> I was copying the aura version, but now that I look at it, I don't see
dst_rect
> being scaled there. I guess I misunderstood the API back then.
> We should be able to fix it here + patch the scale calculation at
> RendererOverridesHandler::ParseCaptureParameters.

I think, it wasn't you. It looks like scaling of dst had been applied in this
patch: https://codereview.chromium.org/143803004/
And since it has been committed, Android WebView Screencast is slow.

I have tried pre-scaling output bitmap size in
RendererOverridesHandler::ParseCaptureParameters, which affects both Clank and
WebView, and this makes Screencast images for Clank really low-res. I have a
suspicion that there is another downscaling by device scale factor somewhere in
the capturing path via GPU backread. Daniel, do you think this is what is
happening?

So I have resorted to modifying
RenderWidgetHostViewAndroid::SynchronousCopyContents which uses software
rendering, and is currently only used by WebView.

Pavel, we had some discussion off-line, but we still haven't resolved the API
question. The current situation for Clank is that DevTools asks for a bitmap of
size "X x Y", and it gets in return a bitmap of size "X * device-scale x Y *
device-scale". With this patch applied, for WebView it will be getting a bitmap
of size "X x Y". For DevTools, this actually doesn't matter, as they just scale
the received bitmap to fit into the view.

[no sievers, 2014-05-20 18:50:43]

On 2014/05/20 17:15:24, Mikhail Naganov (Cr) wrote:
> On 2014/05/20 13:21:39, pfeldman wrote:
> > I agree with sievers@ - not sure why I ended up scaling the dst_rect. I
> thought
> > I was copying the aura version, but now that I look at it, I don't see
> dst_rect
> > being scaled there. I guess I misunderstood the API back then.
> > We should be able to fix it here + patch the scale calculation at
> > RendererOverridesHandler::ParseCaptureParameters.
> 
> I think, it wasn't you. It looks like scaling of dst had been applied in this
> patch: https://codereview.chromium.org/143803004/
> And since it has been committed, Android WebView Screencast is slow.
> 

That's weird. The patch doesn't really introduce the scale. It just changed it
from using dip_util.cc/ConvertViewSizeToPixel() to do the scaling to using the
exact same device_scale_factor that we use in other calculations, because it
caused inconsistencies. (I think the dip_util stuff uses discrete scale factors
which doesn't work right on Android.)


> I have tried pre-scaling output bitmap size in
> RendererOverridesHandler::ParseCaptureParameters, which affects both Clank and
> WebView, and this makes Screencast images for Clank really low-res. I have a
> suspicion that there is another downscaling by device scale factor somewhere
in
> the capturing path via GPU backread. Daniel, do you think this is what is
> happening?
> 
> So I have resorted to modifying
> RenderWidgetHostViewAndroid::SynchronousCopyContents which uses software
> rendering, and is currently only used by WebView.
> 
> Pavel, we had some discussion off-line, but we still haven't resolved the API
> question. The current situation for Clank is that DevTools asks for a bitmap
of
> size "X x Y", and it gets in return a bitmap of size "X * device-scale x Y *
> device-scale". With this patch applied, for WebView it will be getting a
bitmap
> of size "X x Y". For DevTools, this actually doesn't matter, as they just
scale
> the received bitmap to fit into the view.

Can you post what values you are passing into CopyFromCompositingSurface(), what
the device_scale_factor is, and what bitmap pixel size you are getting in return
with the current code?

[pfeldman, 2014-05-20 18:52:51]

> I think, it wasn't you. It looks like scaling of dst had been applied in this
> patch: https://codereview.chromium.org/143803004/
> And since it has been committed, Android WebView Screencast is slow.

Phew, you are right!
@aelias: what is the right convention for CopyFromCompositingSurface? What are
dst_size_in_pixel? Why are they using device_scale_factor?

> Pavel, we had some discussion off-line, but we still haven't resolved the API
> question. The current situation for Clank is that DevTools asks for a bitmap
of
> size "X x Y", and it gets in return a bitmap of size "X * device-scale x Y *
> device-scale". With this patch applied, for WebView it will be getting a
bitmap
> of size "X x Y". For DevTools, this actually doesn't matter, as they just
scale
> the received bitmap to fit into the view.

We should be getting what we are requesting, i.e. X x Y. DevTools is aware of
device-scale and can take it into consideration when requesting the image.

[no sievers, 2014-05-20 18:53:58]

On 2014/05/20 18:50:43, sievers wrote:
> On 2014/05/20 17:15:24, Mikhail Naganov (Cr) wrote:
> > On 2014/05/20 13:21:39, pfeldman wrote:
> > > I agree with sievers@ - not sure why I ended up scaling the dst_rect. I
> > thought
> > > I was copying the aura version, but now that I look at it, I don't see
> > dst_rect
> > > being scaled there. I guess I misunderstood the API back then.
> > > We should be able to fix it here + patch the scale calculation at
> > > RendererOverridesHandler::ParseCaptureParameters.
> > 
> > I think, it wasn't you. It looks like scaling of dst had been applied in
this
> > patch: https://codereview.chromium.org/143803004/
> > And since it has been committed, Android WebView Screencast is slow.
> > 
> 
> That's weird. The patch doesn't really introduce the scale. It just changed it
> from using dip_util.cc/ConvertViewSizeToPixel() to do the scaling to using the
> exact same device_scale_factor that we use in other calculations, because it
> caused inconsistencies. (I think the dip_util stuff uses discrete scale
factors
> which doesn't work right on Android.)
> 
> 
> > I have tried pre-scaling output bitmap size in
> > RendererOverridesHandler::ParseCaptureParameters, which affects both Clank
and
> > WebView, and this makes Screencast images for Clank really low-res. I have a
> > suspicion that there is another downscaling by device scale factor somewhere
> in
> > the capturing path via GPU backread. Daniel, do you think this is what is
> > happening?
> > 
> > So I have resorted to modifying
> > RenderWidgetHostViewAndroid::SynchronousCopyContents which uses software
> > rendering, and is currently only used by WebView.
> > 
> > Pavel, we had some discussion off-line, but we still haven't resolved the
API
> > question. The current situation for Clank is that DevTools asks for a bitmap
> of
> > size "X x Y", and it gets in return a bitmap of size "X * device-scale x Y *
> > device-scale". With this patch applied, for WebView it will be getting a
> bitmap
> > of size "X x Y". For DevTools, this actually doesn't matter, as they just
> scale
> > the received bitmap to fit into the view.
> 
> Can you post what values you are passing into CopyFromCompositingSurface(),
what
> the device_scale_factor is, and what bitmap pixel size you are getting in
return
> with the current code?

Btw I don't think callers use pixel sizes with CopyFromCompositingSurface().
This is in view bounds.

[aelias_OOO_until_Jul13, 2014-05-20 20:08:47]

+danakj.  Dana, could you clarify the policy of the async readback code with
respect to device scale factor?

[danakj, 2014-05-20 20:40:55]

On 2014/05/19 20:07:26, sievers wrote:
> On 2014/05/19 20:02:13, Mikhail Naganov (Cr) wrote:
> > On 2014/05/19 18:45:08, sievers wrote:
> > > Doesn't that break the API? So it would now return you a bitmap of the
wrong
> > > size, i.e. different size compared to what the non-synchronous compositor
> > would
> > > return.
> > > 
> > > If you want a scaled down version why don't you just pass in a different
dst
> > > rect? That's what we do for thumbnails in Chrome.
> > 
> > Pre-downscaling the dst rect will create an implicit dependency between the
> code
> > in RendererOverridesHandler::InnerSwapCompositorFrame (which calls
> > CopyFromCompositingSurface) and CopyFromCompositingSurface itself, which
> doesn't
> > seem good to me.
> > 
> > I'll let Pavel speak for the API side here. In the meanwhile, I can explain
> what
> > I have observed here.
> > 
> > When DevTools sends a request for the bitmap, it provides the size in CSS
> pixels
> > of its Screencast widget (I have checked this in
> > RendererOverridesHandler::InnerSwapCompositorFrame). If we return a bitmap
of
> > this size, then DevTools actually gets what is has asked for. I'm not sure,
> > whether is it in fact correct that in Chrome, we return a bitmap that is
> scaled
> > up by the device screen density -- surely this can provide a sharper image
if
> on
> > desktop you use a hi-res screen, but technically, this isn't correct.
> 
> What happens on AURA?

Async readbacks give you back a copy of what would have appeared on the screen.
Device scale factor will be applied, the compositor does no rescaling with the
texture it returns. That's up the caller.

Does that answer the question?

[mnaganov (inactive), 2014-05-20 20:43:24]

On 2014/05/20 18:53:58, sievers wrote:
> On 2014/05/20 18:50:43, sievers wrote:
> > On 2014/05/20 17:15:24, Mikhail Naganov (Cr) wrote:
> > > On 2014/05/20 13:21:39, pfeldman wrote:
> > > > I agree with sievers@ - not sure why I ended up scaling the dst_rect. I
> > > thought
> > > > I was copying the aura version, but now that I look at it, I don't see
> > > dst_rect
> > > > being scaled there. I guess I misunderstood the API back then.
> > > > We should be able to fix it here + patch the scale calculation at
> > > > RendererOverridesHandler::ParseCaptureParameters.
> > > 
> > > I think, it wasn't you. It looks like scaling of dst had been applied in
> this
> > > patch: https://codereview.chromium.org/143803004/
> > > And since it has been committed, Android WebView Screencast is slow.
> > > 
> > 
> > That's weird. The patch doesn't really introduce the scale. It just changed
it
> > from using dip_util.cc/ConvertViewSizeToPixel() to do the scaling to using
the
> > exact same device_scale_factor that we use in other calculations, because it
> > caused inconsistencies. (I think the dip_util stuff uses discrete scale
> factors
> > which doesn't work right on Android.)
> > 
> > 
> > > I have tried pre-scaling output bitmap size in
> > > RendererOverridesHandler::ParseCaptureParameters, which affects both Clank
> and
> > > WebView, and this makes Screencast images for Clank really low-res. I have
a
> > > suspicion that there is another downscaling by device scale factor
somewhere
> > in
> > > the capturing path via GPU backread. Daniel, do you think this is what is
> > > happening?
> > > 
> > > So I have resorted to modifying
> > > RenderWidgetHostViewAndroid::SynchronousCopyContents which uses software
> > > rendering, and is currently only used by WebView.
> > > 
> > > Pavel, we had some discussion off-line, but we still haven't resolved the
> API
> > > question. The current situation for Clank is that DevTools asks for a
bitmap
> > of
> > > size "X x Y", and it gets in return a bitmap of size "X * device-scale x Y
*
> > > device-scale". With this patch applied, for WebView it will be getting a
> > bitmap
> > > of size "X x Y". For DevTools, this actually doesn't matter, as they just
> > scale
> > > the received bitmap to fit into the view.
> > 
> > Can you post what values you are passing into CopyFromCompositingSurface(),
> what
> > the device_scale_factor is, and what bitmap pixel size you are getting in
> return
> > with the current code?
> 
> Btw I don't think callers use pixel sizes with CopyFromCompositingSurface().
> This is in view bounds.

This is the flow of Screencast:

1. content/browser/devtools/renderer_overrides_handler.cc -- here
RendererOverridesHandler receives the command from desktop, in the command,
DevTools specifies dimensions in CSS pixels (e.g. 360 x 480 for a phone) and
scale (which is usually 1). Scale is then adjusted inside
RendererOverridesHandler::ParseCaptureParameters to fit the actual view size (in
CSS pixels) into the dimensions provided.

2. Inside RendererOverridesHandler::InnerSwapCompositorFrame, view's size (in
CSS pixels) is taken as the source size, and the same view size, but scaled by
the scale from step 1. is used as destination size. So, for example, if the
scale remains 1, both source and destination dimensions will be 360 x 480.

3. The source and destination dimensions are passed to
CopyFromCompositingSurface, where they are scaled by device screen density.
Thus, the resulting bitmap has size that corresponds to screen size in physical
pixels.

4. The bitmap is returned to RendererOverridesHandler::ScreencastFrameCaptured,
where it is encoded, (no resizing!) and passed back to the desktop.

5. The hi-res bitmap is then resized on desktop to fit DevTool's Screencast
layout element.


So, for Chrome for Android this only turns to be inefficiency in the amount of
data being passed to desktop. For WebView this inflation of bitmap size results
in significant dropping of framerate and considerable slowdown of the
application.

[mnaganov (inactive), 2014-05-20 20:46:06]

On 2014/05/20 20:40:55, danakj wrote:
> On 2014/05/19 20:07:26, sievers wrote:
> > On 2014/05/19 20:02:13, Mikhail Naganov (Cr) wrote:
> > > On 2014/05/19 18:45:08, sievers wrote:
> > > > Doesn't that break the API? So it would now return you a bitmap of the
> wrong
> > > > size, i.e. different size compared to what the non-synchronous
compositor
> > > would
> > > > return.
> > > > 
> > > > If you want a scaled down version why don't you just pass in a different
> dst
> > > > rect? That's what we do for thumbnails in Chrome.
> > > 
> > > Pre-downscaling the dst rect will create an implicit dependency between
the
> > code
> > > in RendererOverridesHandler::InnerSwapCompositorFrame (which calls
> > > CopyFromCompositingSurface) and CopyFromCompositingSurface itself, which
> > doesn't
> > > seem good to me.
> > > 
> > > I'll let Pavel speak for the API side here. In the meanwhile, I can
explain
> > what
> > > I have observed here.
> > > 
> > > When DevTools sends a request for the bitmap, it provides the size in CSS
> > pixels
> > > of its Screencast widget (I have checked this in
> > > RendererOverridesHandler::InnerSwapCompositorFrame). If we return a bitmap
> of
> > > this size, then DevTools actually gets what is has asked for. I'm not
sure,
> > > whether is it in fact correct that in Chrome, we return a bitmap that is
> > scaled
> > > up by the device screen density -- surely this can provide a sharper image
> if
> > on
> > > desktop you use a hi-res screen, but technically, this isn't correct.
> > 
> > What happens on AURA?
> 
> Async readbacks give you back a copy of what would have appeared on the
screen.
> Device scale factor will be applied, the compositor does no rescaling with the
> texture it returns. That's up the caller.
> 
> Does that answer the question?

danakj@: Thanks! Still not. What happens if the size of destination bitmap
passed to cc::CopyOutputRequest::CreateRequest is in CSS pixels instead of
physical pixels?

[no sievers, 2014-05-20 20:51:48]

On 2014/05/20 20:46:06, Mikhail Naganov (Cr) wrote:
> On 2014/05/20 20:40:55, danakj wrote:
> > On 2014/05/19 20:07:26, sievers wrote:
> > > On 2014/05/19 20:02:13, Mikhail Naganov (Cr) wrote:
> > > > On 2014/05/19 18:45:08, sievers wrote:
> > > > > Doesn't that break the API? So it would now return you a bitmap of the
> > wrong
> > > > > size, i.e. different size compared to what the non-synchronous
> compositor
> > > > would
> > > > > return.
> > > > > 
> > > > > If you want a scaled down version why don't you just pass in a
different
> > dst
> > > > > rect? That's what we do for thumbnails in Chrome.
> > > > 
> > > > Pre-downscaling the dst rect will create an implicit dependency between
> the
> > > code
> > > > in RendererOverridesHandler::InnerSwapCompositorFrame (which calls
> > > > CopyFromCompositingSurface) and CopyFromCompositingSurface itself, which
> > > doesn't
> > > > seem good to me.
> > > > 
> > > > I'll let Pavel speak for the API side here. In the meanwhile, I can
> explain
> > > what
> > > > I have observed here.
> > > > 
> > > > When DevTools sends a request for the bitmap, it provides the size in
CSS
> > > pixels
> > > > of its Screencast widget (I have checked this in
> > > > RendererOverridesHandler::InnerSwapCompositorFrame). If we return a
bitmap
> > of
> > > > this size, then DevTools actually gets what is has asked for. I'm not
> sure,
> > > > whether is it in fact correct that in Chrome, we return a bitmap that is
> > > scaled
> > > > up by the device screen density -- surely this can provide a sharper
image
> > if
> > > on
> > > > desktop you use a hi-res screen, but technically, this isn't correct.
> > > 
> > > What happens on AURA?
> > 
> > Async readbacks give you back a copy of what would have appeared on the
> screen.
> > Device scale factor will be applied, the compositor does no rescaling with
the
> > texture it returns. That's up the caller.
> > 
> > Does that answer the question?
> 
> danakj@: Thanks! Still not. What happens if the size of destination bitmap
> passed to cc::CopyOutputRequest::CreateRequest is in CSS pixels instead of
> physical pixels?

The units used in CopyFromCompositingSurface() need to match the units used in
GetViewBounds,
which is DIP on Android.

When dealing with the layer on Android we are in pixel space. Therefore we
convert DIP to pixels in
RenderWidgetHostViewAndroid::CopyFromCompositingSurface().

On AURA I think the DPI scaling works a bit different.

[mnaganov (inactive), 2014-05-20 21:13:00]

On 2014/05/20 20:51:48, sievers wrote:
> On 2014/05/20 20:46:06, Mikhail Naganov (Cr) wrote:
> > On 2014/05/20 20:40:55, danakj wrote:
> > > On 2014/05/19 20:07:26, sievers wrote:
> > > > On 2014/05/19 20:02:13, Mikhail Naganov (Cr) wrote:
> > > > > On 2014/05/19 18:45:08, sievers wrote:
> > > > > > Doesn't that break the API? So it would now return you a bitmap of
the
> > > wrong
> > > > > > size, i.e. different size compared to what the non-synchronous
> > compositor
> > > > > would
> > > > > > return.
> > > > > > 
> > > > > > If you want a scaled down version why don't you just pass in a
> different
> > > dst
> > > > > > rect? That's what we do for thumbnails in Chrome.
> > > > > 
> > > > > Pre-downscaling the dst rect will create an implicit dependency
between
> > the
> > > > code
> > > > > in RendererOverridesHandler::InnerSwapCompositorFrame (which calls
> > > > > CopyFromCompositingSurface) and CopyFromCompositingSurface itself,
which
> > > > doesn't
> > > > > seem good to me.
> > > > > 
> > > > > I'll let Pavel speak for the API side here. In the meanwhile, I can
> > explain
> > > > what
> > > > > I have observed here.
> > > > > 
> > > > > When DevTools sends a request for the bitmap, it provides the size in
> CSS
> > > > pixels
> > > > > of its Screencast widget (I have checked this in
> > > > > RendererOverridesHandler::InnerSwapCompositorFrame). If we return a
> bitmap
> > > of
> > > > > this size, then DevTools actually gets what is has asked for. I'm not
> > sure,
> > > > > whether is it in fact correct that in Chrome, we return a bitmap that
is
> > > > scaled
> > > > > up by the device screen density -- surely this can provide a sharper
> image
> > > if
> > > > on
> > > > > desktop you use a hi-res screen, but technically, this isn't correct.
> > > > 
> > > > What happens on AURA?
> > > 
> > > Async readbacks give you back a copy of what would have appeared on the
> > screen.
> > > Device scale factor will be applied, the compositor does no rescaling with
> the
> > > texture it returns. That's up the caller.
> > > 
> > > Does that answer the question?
> > 
> > danakj@: Thanks! Still not. What happens if the size of destination bitmap
> > passed to cc::CopyOutputRequest::CreateRequest is in CSS pixels instead of
> > physical pixels?
> 
> The units used in CopyFromCompositingSurface() need to match the units used in
> GetViewBounds,
> which is DIP on Android.
> 
> When dealing with the layer on Android we are in pixel space. Therefore we
> convert DIP to pixels in
> RenderWidgetHostViewAndroid::CopyFromCompositingSurface().
> 
> On AURA I think the DPI scaling works a bit different.

I agree with what you say about units. Let me state what problems we do have
currently with Screencast:

1. For WebView, we must use small destination bitmap (with dimension values in
DIP pixels), as we do software rasterization, so using a destination bitmap with
dimensions in physical pixels on Nexus 5, for example, increases the amount of
work and slows down screencasting by 9x times, which is not acceptable.

2. For non-WebView (Chrome, Content shell, etc) we must scale the resulting
bitmap back to DIP pixels before sending it to desktop.

The problem #1 is very important to be solved now, because Screencast is
currently unusable on WebView. The patch is solving it. I agree, it can violate
the contract of CopyFromCompositingSurface, as the resulting bitmap will be in
DIP pixels, not in physical pixels. But we don't use those bitmaps for anything
but WebView Screencast, so it's fine, and will work as a temporary solution that
can be merged into M36.

For solving the problem #2 a more generic solution is needed.

If you can provide a better working solution at least for the problem #1 right
now, I'm glad to see it.

[no sievers, 2014-05-20 22:05:21]

On 2014/05/20 21:13:00, Mikhail Naganov (Cr) wrote:
> On 2014/05/20 20:51:48, sievers wrote:
> > On 2014/05/20 20:46:06, Mikhail Naganov (Cr) wrote:
> > > On 2014/05/20 20:40:55, danakj wrote:
> > > > On 2014/05/19 20:07:26, sievers wrote:
> > > > > On 2014/05/19 20:02:13, Mikhail Naganov (Cr) wrote:
> > > > > > On 2014/05/19 18:45:08, sievers wrote:
> > > > > > > Doesn't that break the API? So it would now return you a bitmap of
> the
> > > > wrong
> > > > > > > size, i.e. different size compared to what the non-synchronous
> > > compositor
> > > > > > would
> > > > > > > return.
> > > > > > > 
> > > > > > > If you want a scaled down version why don't you just pass in a
> > different
> > > > dst
> > > > > > > rect? That's what we do for thumbnails in Chrome.
> > > > > > 
> > > > > > Pre-downscaling the dst rect will create an implicit dependency
> between
> > > the
> > > > > code
> > > > > > in RendererOverridesHandler::InnerSwapCompositorFrame (which calls
> > > > > > CopyFromCompositingSurface) and CopyFromCompositingSurface itself,
> which
> > > > > doesn't
> > > > > > seem good to me.
> > > > > > 
> > > > > > I'll let Pavel speak for the API side here. In the meanwhile, I can
> > > explain
> > > > > what
> > > > > > I have observed here.
> > > > > > 
> > > > > > When DevTools sends a request for the bitmap, it provides the size
in
> > CSS
> > > > > pixels
> > > > > > of its Screencast widget (I have checked this in
> > > > > > RendererOverridesHandler::InnerSwapCompositorFrame). If we return a
> > bitmap
> > > > of
> > > > > > this size, then DevTools actually gets what is has asked for. I'm
not
> > > sure,
> > > > > > whether is it in fact correct that in Chrome, we return a bitmap
that
> is
> > > > > scaled
> > > > > > up by the device screen density -- surely this can provide a sharper
> > image
> > > > if
> > > > > on
> > > > > > desktop you use a hi-res screen, but technically, this isn't
correct.
> > > > > 
> > > > > What happens on AURA?
> > > > 
> > > > Async readbacks give you back a copy of what would have appeared on the
> > > screen.
> > > > Device scale factor will be applied, the compositor does no rescaling
with
> > the
> > > > texture it returns. That's up the caller.
> > > > 
> > > > Does that answer the question?
> > > 
> > > danakj@: Thanks! Still not. What happens if the size of destination bitmap
> > > passed to cc::CopyOutputRequest::CreateRequest is in CSS pixels instead of
> > > physical pixels?
> > 
> > The units used in CopyFromCompositingSurface() need to match the units used
in
> > GetViewBounds,
> > which is DIP on Android.
> > 
> > When dealing with the layer on Android we are in pixel space. Therefore we
> > convert DIP to pixels in
> > RenderWidgetHostViewAndroid::CopyFromCompositingSurface().
> > 
> > On AURA I think the DPI scaling works a bit different.
> 
> I agree with what you say about units. Let me state what problems we do have
> currently with Screencast:
> 
> 1. For WebView, we must use small destination bitmap (with dimension values in
> DIP pixels), as we do software rasterization, so using a destination bitmap
with
> dimensions in physical pixels on Nexus 5, for example, increases the amount of
> work and slows down screencasting by 9x times, which is not acceptable.
> 
> 2. For non-WebView (Chrome, Content shell, etc) we must scale the resulting
> bitmap back to DIP pixels before sending it to desktop.
> 
> The problem #1 is very important to be solved now, because Screencast is
> currently unusable on WebView. The patch is solving it. I agree, it can
violate
> the contract of CopyFromCompositingSurface, as the resulting bitmap will be in
> DIP pixels, not in physical pixels. But we don't use those bitmaps for
anything
> but WebView Screencast, so it's fine, and will work as a temporary solution
that
> can be merged into M36.
> 
> For solving the problem #2 a more generic solution is needed.
> 
> If you can provide a better working solution at least for the problem #1 right
> now, I'm glad to see it.

I'm still confused :)


Given the same input arguments, does CopyFromCompositingSurface() give you the
same output bitmap size with the synchronous compositor and without?

If so, then that sounds like a bug.

Otherwise what resolution you request is really entirely up to the caller (for
example I believe Chrome on Android usually uses half or quarter res).

Or is that you are saying the implementation for WebView is actually not great
because it rasterizes a whole viewport and that is too slow? But would it be
really slower than a readback?

[no sievers, 2014-05-20 22:05:48]

On 2014/05/20 22:05:21, sievers wrote:
> On 2014/05/20 21:13:00, Mikhail Naganov (Cr) wrote:
> > On 2014/05/20 20:51:48, sievers wrote:
> > > On 2014/05/20 20:46:06, Mikhail Naganov (Cr) wrote:
> > > > On 2014/05/20 20:40:55, danakj wrote:
> > > > > On 2014/05/19 20:07:26, sievers wrote:
> > > > > > On 2014/05/19 20:02:13, Mikhail Naganov (Cr) wrote:
> > > > > > > On 2014/05/19 18:45:08, sievers wrote:
> > > > > > > > Doesn't that break the API? So it would now return you a bitmap
of
> > the
> > > > > wrong
> > > > > > > > size, i.e. different size compared to what the non-synchronous
> > > > compositor
> > > > > > > would
> > > > > > > > return.
> > > > > > > > 
> > > > > > > > If you want a scaled down version why don't you just pass in a
> > > different
> > > > > dst
> > > > > > > > rect? That's what we do for thumbnails in Chrome.
> > > > > > > 
> > > > > > > Pre-downscaling the dst rect will create an implicit dependency
> > between
> > > > the
> > > > > > code
> > > > > > > in RendererOverridesHandler::InnerSwapCompositorFrame (which calls
> > > > > > > CopyFromCompositingSurface) and CopyFromCompositingSurface itself,
> > which
> > > > > > doesn't
> > > > > > > seem good to me.
> > > > > > > 
> > > > > > > I'll let Pavel speak for the API side here. In the meanwhile, I
can
> > > > explain
> > > > > > what
> > > > > > > I have observed here.
> > > > > > > 
> > > > > > > When DevTools sends a request for the bitmap, it provides the size
> in
> > > CSS
> > > > > > pixels
> > > > > > > of its Screencast widget (I have checked this in
> > > > > > > RendererOverridesHandler::InnerSwapCompositorFrame). If we return
a
> > > bitmap
> > > > > of
> > > > > > > this size, then DevTools actually gets what is has asked for. I'm
> not
> > > > sure,
> > > > > > > whether is it in fact correct that in Chrome, we return a bitmap
> that
> > is
> > > > > > scaled
> > > > > > > up by the device screen density -- surely this can provide a
sharper
> > > image
> > > > > if
> > > > > > on
> > > > > > > desktop you use a hi-res screen, but technically, this isn't
> correct.
> > > > > > 
> > > > > > What happens on AURA?
> > > > > 
> > > > > Async readbacks give you back a copy of what would have appeared on
the
> > > > screen.
> > > > > Device scale factor will be applied, the compositor does no rescaling
> with
> > > the
> > > > > texture it returns. That's up the caller.
> > > > > 
> > > > > Does that answer the question?
> > > > 
> > > > danakj@: Thanks! Still not. What happens if the size of destination
bitmap
> > > > passed to cc::CopyOutputRequest::CreateRequest is in CSS pixels instead
of
> > > > physical pixels?
> > > 
> > > The units used in CopyFromCompositingSurface() need to match the units
used
> in
> > > GetViewBounds,
> > > which is DIP on Android.
> > > 
> > > When dealing with the layer on Android we are in pixel space. Therefore we
> > > convert DIP to pixels in
> > > RenderWidgetHostViewAndroid::CopyFromCompositingSurface().
> > > 
> > > On AURA I think the DPI scaling works a bit different.
> > 
> > I agree with what you say about units. Let me state what problems we do have
> > currently with Screencast:
> > 
> > 1. For WebView, we must use small destination bitmap (with dimension values
in
> > DIP pixels), as we do software rasterization, so using a destination bitmap
> with
> > dimensions in physical pixels on Nexus 5, for example, increases the amount
of
> > work and slows down screencasting by 9x times, which is not acceptable.
> > 
> > 2. For non-WebView (Chrome, Content shell, etc) we must scale the resulting
> > bitmap back to DIP pixels before sending it to desktop.
> > 
> > The problem #1 is very important to be solved now, because Screencast is
> > currently unusable on WebView. The patch is solving it. I agree, it can
> violate
> > the contract of CopyFromCompositingSurface, as the resulting bitmap will be
in
> > DIP pixels, not in physical pixels. But we don't use those bitmaps for
> anything
> > but WebView Screencast, so it's fine, and will work as a temporary solution
> that
> > can be merged into M36.
> > 
> > For solving the problem #2 a more generic solution is needed.
> > 
> > If you can provide a better working solution at least for the problem #1
right
> > now, I'm glad to see it.
> 
> I'm still confused :)
> 
> 
> Given the same input arguments, does CopyFromCompositingSurface() give you the
> same output bitmap size with the synchronous compositor and without?
> 
> If so, then that sounds like a bug.
> 

If *not*, then it sounds like a bug.

> Otherwise what resolution you request is really entirely up to the caller (for
> example I believe Chrome on Android usually uses half or quarter res).
> 
> Or is that you are saying the implementation for WebView is actually not great
> because it rasterizes a whole viewport and that is too slow? But would it be
> really slower than a readback?

[mnaganov (inactive), 2014-05-21 07:16:13]

On 2014/05/20 22:05:48, sievers wrote:
> On 2014/05/20 22:05:21, sievers wrote:
> > On 2014/05/20 21:13:00, Mikhail Naganov (Cr) wrote:
> > > On 2014/05/20 20:51:48, sievers wrote:
> > > > On 2014/05/20 20:46:06, Mikhail Naganov (Cr) wrote:
> > > > > On 2014/05/20 20:40:55, danakj wrote:
> > > > > > On 2014/05/19 20:07:26, sievers wrote:
> > > > > > > On 2014/05/19 20:02:13, Mikhail Naganov (Cr) wrote:
> > > > > > > > On 2014/05/19 18:45:08, sievers wrote:
> > > > > > > > > Doesn't that break the API? So it would now return you a
bitmap
> of
> > > the
> > > > > > wrong
> > > > > > > > > size, i.e. different size compared to what the non-synchronous
> > > > > compositor
> > > > > > > > would
> > > > > > > > > return.
> > > > > > > > > 
> > > > > > > > > If you want a scaled down version why don't you just pass in a
> > > > different
> > > > > > dst
> > > > > > > > > rect? That's what we do for thumbnails in Chrome.
> > > > > > > > 
> > > > > > > > Pre-downscaling the dst rect will create an implicit dependency
> > > between
> > > > > the
> > > > > > > code
> > > > > > > > in RendererOverridesHandler::InnerSwapCompositorFrame (which
calls
> > > > > > > > CopyFromCompositingSurface) and CopyFromCompositingSurface
itself,
> > > which
> > > > > > > doesn't
> > > > > > > > seem good to me.
> > > > > > > > 
> > > > > > > > I'll let Pavel speak for the API side here. In the meanwhile, I
> can
> > > > > explain
> > > > > > > what
> > > > > > > > I have observed here.
> > > > > > > > 
> > > > > > > > When DevTools sends a request for the bitmap, it provides the
size
> > in
> > > > CSS
> > > > > > > pixels
> > > > > > > > of its Screencast widget (I have checked this in
> > > > > > > > RendererOverridesHandler::InnerSwapCompositorFrame). If we
return
> a
> > > > bitmap
> > > > > > of
> > > > > > > > this size, then DevTools actually gets what is has asked for.
I'm
> > not
> > > > > sure,
> > > > > > > > whether is it in fact correct that in Chrome, we return a bitmap
> > that
> > > is
> > > > > > > scaled
> > > > > > > > up by the device screen density -- surely this can provide a
> sharper
> > > > image
> > > > > > if
> > > > > > > on
> > > > > > > > desktop you use a hi-res screen, but technically, this isn't
> > correct.
> > > > > > > 
> > > > > > > What happens on AURA?
> > > > > > 
> > > > > > Async readbacks give you back a copy of what would have appeared on
> the
> > > > > screen.
> > > > > > Device scale factor will be applied, the compositor does no
rescaling
> > with
> > > > the
> > > > > > texture it returns. That's up the caller.
> > > > > > 
> > > > > > Does that answer the question?
> > > > > 
> > > > > danakj@: Thanks! Still not. What happens if the size of destination
> bitmap
> > > > > passed to cc::CopyOutputRequest::CreateRequest is in CSS pixels
instead
> of
> > > > > physical pixels?
> > > > 
> > > > The units used in CopyFromCompositingSurface() need to match the units
> used
> > in
> > > > GetViewBounds,
> > > > which is DIP on Android.
> > > > 
> > > > When dealing with the layer on Android we are in pixel space. Therefore
we
> > > > convert DIP to pixels in
> > > > RenderWidgetHostViewAndroid::CopyFromCompositingSurface().
> > > > 
> > > > On AURA I think the DPI scaling works a bit different.
> > > 
> > > I agree with what you say about units. Let me state what problems we do
have
> > > currently with Screencast:
> > > 
> > > 1. For WebView, we must use small destination bitmap (with dimension
values
> in
> > > DIP pixels), as we do software rasterization, so using a destination
bitmap
> > with
> > > dimensions in physical pixels on Nexus 5, for example, increases the
amount
> of
> > > work and slows down screencasting by 9x times, which is not acceptable.
> > > 
> > > 2. For non-WebView (Chrome, Content shell, etc) we must scale the
resulting
> > > bitmap back to DIP pixels before sending it to desktop.
> > > 
> > > The problem #1 is very important to be solved now, because Screencast is
> > > currently unusable on WebView. The patch is solving it. I agree, it can
> > violate
> > > the contract of CopyFromCompositingSurface, as the resulting bitmap will
be
> in
> > > DIP pixels, not in physical pixels. But we don't use those bitmaps for
> > anything
> > > but WebView Screencast, so it's fine, and will work as a temporary
solution
> > that
> > > can be merged into M36.
> > > 
> > > For solving the problem #2 a more generic solution is needed.
> > > 
> > > If you can provide a better working solution at least for the problem #1
> right
> > > now, I'm glad to see it.
> > 
> > I'm still confused :)
> > 
> > 
> > Given the same input arguments, does CopyFromCompositingSurface() give you
the
> > same output bitmap size with the synchronous compositor and without?
> > 
> > If so, then that sounds like a bug.
> > 
> 
> If *not*, then it sounds like a bug.
>

Yes, currently CopyFromCompositingSurface gives the same output bitmap size,
given that the input arguments are the same.

But I'm not super excited about the fact that you pass in size in DIP pixels,
and the output bitmap has size in physical pixels.
 
> > Otherwise what resolution you request is really entirely up to the caller
(for
> > example I believe Chrome on Android usually uses half or quarter res).
> >

So, according to your logic, it's up to the caller of CopyFromCompositingSurface
to pre-downscale the destination bitmap size to compensate the fact that
CopyFromCompositingSurface scales it by the device density?

At least one issue with this, is that the caller -- RendererOverridesHandler
doesn't actually know, whether we use synchronous compositor, or not.
 
> > Or is that you are saying the implementation for WebView is actually not
great
> > because it rasterizes a whole viewport and that is too slow? But would it be
> > really slower than a readback?

We can't use readback for WebView because WebView can be off-screen
intentionally. What I'm saying is that the fact that CopyFromCompositingSurface
inflates the destination bitmap size by the screen density slows down software
rasterization dramatically and makes Screencast for WebView unusable.

[mnaganov (inactive), 2014-05-21 10:44:12]

OK, another take on this. Let's pre-downscale the destination bitmap size at the
site of the caller of CopyFromCompositingSurface -- DevTools'
RendererOverridesHandler.

This solves both problems I have outlined before:

1. Software rendering performed for WebView Screencast now draws into a smaller
bitmap, and is fast.

2. DevTools Screencast now gets the resulting bitmap sized in DIP pixels, not in
physical pixels, essentially what DevTools on desktop asks for.

And CopyFromCompositingSurface is now left intact, so all of its contracts are
preserved.


The only issue I've found with this approach, is that the readback pipeline
specifies SCALER_QUALITY_FAST quality level for the scaler, which results in a
very poor image quality, because it seems to avoid any interpolation. So I'm
changing it to SCALER_QUALITY_GOOD. As
GLHelper::CropScaleReadbackAndCleanTexture is only seems to be used for
readback, I don't think we need to drag the 'quality' parameter all the way from
RenderWidgetHostViewAndroid.

I have tried playing YouTube videos via Screencast on ContentShell running on
Nexus 7 (v1), and the speed looks good.

So, Daniel, Pavel, what do you think about this?

[pfeldman, 2014-05-21 14:09:16]

https://codereview.chromium.org/296493003/diff/60001/content/common/gpu/clien...
File content/common/gpu/client/gl_helper.cc (right):

https://codereview.chromium.org/296493003/diff/60001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.cc:696: GLHelper::SCALER_QUALITY_GOOD);
Wouldn't this affect screencast performance though?

[mnaganov (inactive), 2014-05-21 14:14:54]

On 2014/05/21 14:09:16, pfeldman wrote:
>
https://codereview.chromium.org/296493003/diff/60001/content/common/gpu/clien...
> File content/common/gpu/client/gl_helper.cc (right):
> 
>
https://codereview.chromium.org/296493003/diff/60001/content/common/gpu/clien...
> content/common/gpu/client/gl_helper.cc:696: GLHelper::SCALER_QUALITY_GOOD);
> Wouldn't this affect screencast performance though?

I didn't measure it yet. Didn't notice any regression visually though. Scaling
is done on the GPU side, so it's probably fast. On the other hand, as we now
process a much smaller image, compressing and transferring it (done by CPU)
should go faster.

[no sievers, 2014-05-21 16:34:25]

https://codereview.chromium.org/296493003/diff/60001/content/common/gpu/clien...
File content/common/gpu/client/gl_helper.cc (right):

https://codereview.chromium.org/296493003/diff/60001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.cc:696: GLHelper::SCALER_QUALITY_GOOD);
On 2014/05/21 14:09:17, pfeldman wrote:
> Wouldn't this affect screencast performance though?

Maybe just add this as an argument so you don't risk regressing the perf of
something else?

[mnaganov (inactive), 2014-05-21 17:26:23]

On 2014/05/21 16:34:25, sievers wrote:
>
https://codereview.chromium.org/296493003/diff/60001/content/common/gpu/clien...
> File content/common/gpu/client/gl_helper.cc (right):
> 
>
https://codereview.chromium.org/296493003/diff/60001/content/common/gpu/clien...
> content/common/gpu/client/gl_helper.cc:696: GLHelper::SCALER_QUALITY_GOOD);
> On 2014/05/21 14:09:17, pfeldman wrote:
> > Wouldn't this affect screencast performance though?
> 
> Maybe just add this as an argument so you don't risk regressing the perf of
> something else?

You mean adding 'quality' argument to
RenderWidgetHostViewBase::CopyFromCompositingSurface, so only DevTools handler
sets it to 'GOOD'? Please see the update.

[no sievers, 2014-05-21 18:26:04]

On 2014/05/21 17:26:23, Mikhail Naganov (Cr) wrote:
> On 2014/05/21 16:34:25, sievers wrote:
> >
>
https://codereview.chromium.org/296493003/diff/60001/content/common/gpu/clien...
> > File content/common/gpu/client/gl_helper.cc (right):
> > 
> >
>
https://codereview.chromium.org/296493003/diff/60001/content/common/gpu/clien...
> > content/common/gpu/client/gl_helper.cc:696: GLHelper::SCALER_QUALITY_GOOD);
> > On 2014/05/21 14:09:17, pfeldman wrote:
> > > Wouldn't this affect screencast performance though?
> > 
> > Maybe just add this as an argument so you don't risk regressing the perf of
> > something else?
> 
> You mean adding 'quality' argument to
> RenderWidgetHostViewBase::CopyFromCompositingSurface, so only DevTools handler
> sets it to 'GOOD'? Please see the update.

If you change that API you have to touch a bunch of other files for other
platforms.
(Also I'm not entirely sure GLHelper::ScalerQuality is right, because it's an
implementation detail. On the other hand it's likely that even if we let cc do
scaling it'd still use the helper code for scaling now that we have it.)

Maybe just expose it in gl_helper.h?
In Chrome for Android it might actually also be desirable to scale down in
better quality as long as it does not introduce jank.

[mnaganov (inactive), 2014-05-21 20:36:29]

On 2014/05/21 18:26:04, sievers wrote:
> On 2014/05/21 17:26:23, Mikhail Naganov (Cr) wrote:
> > On 2014/05/21 16:34:25, sievers wrote:
> > >
> >
>
https://codereview.chromium.org/296493003/diff/60001/content/common/gpu/clien...
> > > File content/common/gpu/client/gl_helper.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/296493003/diff/60001/content/common/gpu/clien...
> > > content/common/gpu/client/gl_helper.cc:696:
GLHelper::SCALER_QUALITY_GOOD);
> > > On 2014/05/21 14:09:17, pfeldman wrote:
> > > > Wouldn't this affect screencast performance though?
> > > 
> > > Maybe just add this as an argument so you don't risk regressing the perf
of
> > > something else?
> > 
> > You mean adding 'quality' argument to
> > RenderWidgetHostViewBase::CopyFromCompositingSurface, so only DevTools
handler
> > sets it to 'GOOD'? Please see the update.
> 
> If you change that API you have to touch a bunch of other files for other
> platforms.
> (Also I'm not entirely sure GLHelper::ScalerQuality is right, because it's an
> implementation detail. On the other hand it's likely that even if we let cc do
> scaling it'd still use the helper code for scaling now that we have it.)
> 
> Maybe just expose it in gl_helper.h?
> In Chrome for Android it might actually also be desirable to scale down in
> better quality as long as it does not introduce jank.

OK, please check PS7.

[no sievers, 2014-05-21 20:40:02]

lgtm

[mnaganov (inactive), 2014-05-22 12:04:39]

On 2014/05/21 20:40:02, sievers wrote:
> lgtm

Thanks, Daniel!

I have checked that capturing screenshots for NTP and tab switching, and it
seems fine.

Pavel, can you please look at the final version of
content/browser/devtools/renderer_overrides_handler.cc change?

[pfeldman, 2014-05-22 15:20:22]

lgtm, thanks.

[mnaganov (inactive), 2014-05-22 15:41:08]

The CQ bit was checked by mnaganov@chromium.org

[commit-bot: I haz the power, 2014-05-22 15:42:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mnaganov@chromium.org/296493003/120001

[commit-bot: I haz the power, 2014-05-22 19:32:46]

Change committed as 272273