Handle Left out paths on Android/Aura for Async ReadBack.

Handle Left out paths on Android/Aura for Async ReadBack.


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

[sivag, 2014-01-24 15:32:17]

WIP.
PTAL...
What all paths we want to handle.

[piman, 2014-01-24 20:27:47]

lgtm

[no sievers, 2014-01-25 00:18:23]

https://codereview.chromium.org/146873002/diff/80002/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/146873002/diff/80002/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:1476: //We have
to draw to a new 565 bitmap and return that.
Do we just need to force it through the texture copy path above and then
DCHECK() here if 565 is requested?

[sivag, 2014-01-25 17:38:46]

https://codereview.chromium.org/146873002/diff/80002/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/146873002/diff/80002/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:1476: //We have
to draw to a new 565 bitmap and return that.
On 2014/01/25 00:18:23, sievers wrote:
> Do we just need to force it through the texture copy path above and then
> DCHECK() here if 565 is requested?

You mean to say, when the src_subrect_in_pixel.size() == dst_size_in_pixel and
the config requested is rgb565 move to the PrepareTextureCopyOutputResult path
and when we call the PrepareBitmapCopyOutputResult explicitly any where DCHECK
and return right.

[sivag, 2014-01-27 15:12:16]

PTAL..

https://codereview.chromium.org/146873002/diff/80002/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/146873002/diff/80002/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:1476: //We have
to draw to a new 565 bitmap and return that.
On 2014/01/25 00:18:23, sievers wrote:
> Do we just need to force it through the texture copy path above and then
> DCHECK() here if 565 is requested?

Done.

https://codereview.chromium.org/146873002/diff/80002/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:1476: //We have
to draw to a new 565 bitmap and return that.
On 2014/01/25 17:38:46, Sikugu wrote:
> On 2014/01/25 00:18:23, sievers wrote:
> > Do we just need to force it through the texture copy path above and then
> > DCHECK() here if 565 is requested?
> 
> You mean to say, when the src_subrect_in_pixel.size() == dst_size_in_pixel and
> the config requested is rgb565 move to the PrepareTextureCopyOutputResult path
> and when we call the PrepareBitmapCopyOutputResult explicitly any where DCHECK
> and return right.
> 
> 
> 
> 

Done.

[no sievers, 2014-01-27 18:41:49]

https://codereview.chromium.org/146873002/diff/160001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/146873002/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1478:
NOTIMPLEMENTED();
Either return here, or just turn this into:
DCHECK(config == SkBitmap::kARGB_8888_Config);

https://codereview.chromium.org/146873002/diff/160001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/146873002/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1936:
callback.Run(false, SkBitmap());
same here

[sivag, 2014-01-28 13:17:00]

PTAL..

https://codereview.chromium.org/146873002/diff/160001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/146873002/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1478:
NOTIMPLEMENTED();
On 2014/01/27 18:41:49, sievers wrote:
> Either return here, or just turn this into:
> DCHECK(config == SkBitmap::kARGB_8888_Config);

Done.

https://codereview.chromium.org/146873002/diff/160001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/146873002/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1936:
callback.Run(false, SkBitmap());
On 2014/01/27 18:41:49, sievers wrote:
> same here

Done.

[sivag, 2014-01-28 13:41:49]

Query regarding  dst_size in copyfromcompositingsurface.

https://codereview.chromium.org/146873002/diff/240001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/146873002/diff/240001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:668: const
gfx::Size& dst_size_in_pixel = ConvertViewSizeToPixel(this, dst_size);
When we pass the const gfx::Size& dst_size to the  copyFromCompositing surface,
the API user expects the destination bitmap to be of the same size if i am not
wrong. 

As per current implementation if i pass (0,0,75,75) as my rect with size 75, 75
the return bitmap for a device with scalefactor 3 would be 0,0,225,225. Are we
missing something here, I think we should have a new api which gives the user
bitmap with the dst_size  and not the dst_size_in_pixels?

Say i am expecting to get the portion of screen with the size specified by user
this doesnt support.

Please share your thoughts?

[no sievers, 2014-01-28 22:35:29]

https://codereview.chromium.org/146873002/diff/240001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/146873002/diff/240001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:668: const
gfx::Size& dst_size_in_pixel = ConvertViewSizeToPixel(this, dst_size);
On 2014/01/28 13:41:50, Sikugu wrote:
> When we pass the const gfx::Size& dst_size to the  copyFromCompositing
surface,
> the API user expects the destination bitmap to be of the same size if i am not
> wrong. 
> 
> As per current implementation if i pass (0,0,75,75) as my rect with size 75,
75
> the return bitmap for a device with scalefactor 3 would be 0,0,225,225. Are we
> missing something here, I think we should have a new api which gives the user
> bitmap with the dst_size  and not the dst_size_in_pixels?
> 
> Say i am expecting to get the portion of screen with the size specified by
user
> this doesnt support.
> 
> Please share your thoughts?

The API is in view bounds and it should match what Aura does, so that unit tests
and dev tools work independent of the platform.

Can't the caller just do the necessary conversion?

[sivag, 2014-01-29 11:22:02]

On 2014/01/28 22:35:29, sievers wrote:
>
https://codereview.chromium.org/146873002/diff/240001/content/browser/rendere...
> File content/browser/renderer_host/render_widget_host_view_android.cc (right):
> 
>
https://codereview.chromium.org/146873002/diff/240001/content/browser/rendere...
> content/browser/renderer_host/render_widget_host_view_android.cc:668: const
> gfx::Size& dst_size_in_pixel = ConvertViewSizeToPixel(this, dst_size);
> On 2014/01/28 13:41:50, Sikugu wrote:
> > When we pass the const gfx::Size& dst_size to the  copyFromCompositing
> surface,
> > the API user expects the destination bitmap to be of the same size if i am
not
> > wrong. 
> > 
> > As per current implementation if i pass (0,0,75,75) as my rect with size 75,
> 75
> > the return bitmap for a device with scalefactor 3 would be 0,0,225,225. Are
we
> > missing something here, I think we should have a new api which gives the
user
> > bitmap with the dst_size  and not the dst_size_in_pixels?
> > 
> > Say i am expecting to get the portion of screen with the size specified by
> user
> > this doesnt support.
> > 
> > Please share your thoughts?
> 
> The API is in view bounds and it should match what Aura does, so that unit
tests
> and dev tools work independent of the platform.
> 
> Can't the caller just do the necessary conversion?

Thanks for the information.
I will go through this and get back to you if i have any queries.
Regarding this CL any further changes needed..?

[sivag, 2014-01-29 11:24:24]

Regarding this CL any further changes needed..?

@Siva::I will try this part.

[sivag, 2014-01-29 11:25:44]

Sorry I was replying to your other question.

Can't the caller just do the necessary conversion?

@Siva:: I will try this part.

[sivag, 2014-02-04 16:02:19]

PTAL..
Tested and added the Contentview side transformation
support.

[sivag, 2014-02-05 13:19:26]

Powei has exposed the transformation api, till contentview so i reverted my
changes and added the Config to the api.Thanks.

PTAL..

[vivekg, 2014-02-05 13:48:30]

https://codereview.chromium.org/146873002/diff/340001/content/public/browser/...
File content/public/browser/android/content_view_core.h (right):

https://codereview.chromium.org/146873002/diff/340001/content/public/browser/...
content/public/browser/android/content_view_core.h:61: bool is565Config,
I think having an enumeration instead of a boolean would make this public API
future proof. Thoughts?

[sivag, 2014-02-06 16:46:35]

Hi Sievers,Piman,

Please take a look at the, code for left out paths based on 
config support.

Without this code the flow would be incomplete.
Please let me know your views.

Thanks,
Siva

https://codereview.chromium.org/146873002/diff/340001/content/public/browser/...
File content/public/browser/android/content_view_core.h (right):

https://codereview.chromium.org/146873002/diff/340001/content/public/browser/...
content/public/browser/android/content_view_core.h:61: bool is565Config,
On 2014/02/05 13:48:31, vivekg_ wrote:
> I think having an enumeration instead of a boolean would make this public API
> future proof. Thoughts?
Yes we can do this, but as the other supporting patch for this got reverted
twice it seems, we can change this once that patch gets landed.

[powei, 2014-02-06 20:16:19]

+cc

I'm not sure if exposing this field to content_view_core is necessary for now,
but I'll defer to sievers@.  If you would like leave it exposed to
content_view_core, then please give dtrainor@, sievers@, or me a heads up, as it
will require changes on the android side.

As you might have noticed, the patch that this depends on
(https://codereview.chromium.org/143803004/) is in bit of a flux.  We're turning
on delegated rendering, and it's been difficult to make it stick.  Please split
this into two separate patches (aura and android) if you are time pressed.

[sivag, 2014-02-07 07:43:34]

The CQ bit was checked by siva.gunturi@samsung.com

[sivag, 2014-02-07 07:43:34]

The CQ bit was unchecked by siva.gunturi@samsung.com

[piman, 2014-02-07 23:26:30]

lgtm

[sivag, 2014-02-07 23:38:54]

The CQ bit was checked by siva.gunturi@samsung.com

[sivag, 2014-02-07 23:39:21]

The CQ bit was unchecked by siva.gunturi@samsung.com

[sivag, 2014-02-07 23:39:31]

The CQ bit was checked by siva.gunturi@samsung.com

[commit-bot: I haz the power, 2014-02-07 23:41:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/siva.gunturi@samsung.com/146873002/570001

[commit-bot: I haz the power, 2014-02-08 02:26:34]

Change committed as 249900