Maintain minimal error response.

Maintain minimal error response.

1.FORMAT_SUPPORTED is no longer needed as we are falling back
to default RGBA_8888 format.
2.Renamed MEMORY_ALLOCATION_FAILURE to BITMAP_ALLOCATION_FAILURE
used it while returning response.
3.Remove READBACK_NOT_SUPPORTED, instead use READBACK_FAILURE.
4.This also simplifies the read back API by falling back to
returning RGBA bitmaps automatically if the format is not
supported. At the time of read back the preferred config will
be checked for read back support using isReadbackConfigSupported
and if its not supported by hardware then, RGBA_8 will be taken
as preferred format and a bitmap will be returned with that format.
The caller can always check whether they got the requested format
by checking the SkBitmap format type.

BUG=472457
Committed: https://crrev.com/49bc3a8c0fe5faaaa64bdb1d4743601e91691b2f
Cr-Commit-Position: refs/heads/master@{#328945}

[sivag, 2015-04-28 14:40:03]

siva.gunturi@samsung.com changed reviewers:
+ sievers@chromium.org

[sivag, 2015-04-28 14:40:04]

ptal..

[no sievers, 2015-04-28 17:27:14]

sievers@chromium.org changed reviewers:
+ danakj@chromium.org

[no sievers, 2015-04-28 17:42:11]

+Dana to see if she's ok with having AURA fall back to RGBA8 also for
unsupported formats.
I'm not really sure how and where this is used and if this breaks anything.

I definitely like it for Android where 565 is conditionally available, and
trying to figure out ahead of time if it works has caused problems.

Thanks for reducing the error codes. It does look like we have been using them
incorrectly anyhow.

[sivag, 2015-04-29 05:19:08]

@dana, ptal..

[danakj, 2015-04-29 17:47:13]

I think what this is doing is that when using software compositing, if you
request 565 you'd get 8888 instead, rather than falling back in the texture (hw
compositing) path when it's not available.

I'm not so sure when 565 is requested, what would it mean if they got back
something else instead of an error?

[sivag, 2015-04-30 10:55:11]

On 2015/04/29 17:47:13, danakj wrote:
> I think what this is doing is that when using software compositing, if you
> request 565 you'd get 8888 instead, rather than falling back in the texture
(hw
> compositing) path when it's not available.
> 
> I'm not so sure when 565 is requested, what would it mean if they got back
> something else instead of an error?

The caller can check the type of bitmap format he received (if he requested 565
and got RGBA_8888) and act accordingly.
I think its better than returing a failure and again calling with RGBA_8888
format for the snapshot.
WDYT?

[danakj, 2015-04-30 17:01:02]

On Thu, Apr 30, 2015 at 3:55 AM, <siva.gunturi@samsung.com> wrote:

> On 2015/04/29 17:47:13, danakj wrote:
>
>> I think what this is doing is that when using software compositing, if you
>> request 565 you'd get 8888 instead, rather than falling back in the
>> texture
>>
> (hw
>
>> compositing) path when it's not available.
>>
>
>  I'm not so sure when 565 is requested, what would it mean if they got back
>> something else instead of an error?
>>
>
> The caller can check the type of bitmap format he received (if he
> requested 565
> and got RGBA_8888) and act accordingly.
> I think its better than returing a failure and again calling with RGBA_8888
> format for the snapshot.
> WDYT?
>

Who calls this with 565? I can't find a single caller in CodeSearch.


>
> https://codereview.chromium.org/1113573002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[no sievers, 2015-04-30 17:54:33]

On 2015/04/30 17:01:02, danakj wrote:
> On Thu, Apr 30, 2015 at 3:55 AM, <mailto:siva.gunturi@samsung.com> wrote:
> 
> > On 2015/04/29 17:47:13, danakj wrote:
> >
> >> I think what this is doing is that when using software compositing, if you
> >> request 565 you'd get 8888 instead, rather than falling back in the
> >> texture
> >>
> > (hw
> >
> >> compositing) path when it's not available.
> >>
> >
> >  I'm not so sure when 565 is requested, what would it mean if they got back
> >> something else instead of an error?
> >>
> >
> > The caller can check the type of bitmap format he received (if he
> > requested 565
> > and got RGBA_8888) and act accordingly.
> > I think its better than returing a failure and again calling with RGBA_8888
> > format for the snapshot.
> > WDYT?
> >
> 
> Who calls this with 565? I can't find a single caller in CodeSearch.
> 

other embedders might since it's a public API.

[danakj, 2015-04-30 18:23:52]

On 2015/04/30 17:54:33, sievers wrote:
> On 2015/04/30 17:01:02, danakj wrote:
> > On Thu, Apr 30, 2015 at 3:55 AM, <mailto:siva.gunturi@samsung.com> wrote:
> > 
> > > On 2015/04/29 17:47:13, danakj wrote:
> > >
> > >> I think what this is doing is that when using software compositing, if
you
> > >> request 565 you'd get 8888 instead, rather than falling back in the
> > >> texture
> > >>
> > > (hw
> > >
> > >> compositing) path when it's not available.
> > >>
> > >
> > >  I'm not so sure when 565 is requested, what would it mean if they got
back
> > >> something else instead of an error?
> > >>
> > >
> > > The caller can check the type of bitmap format he received (if he
> > > requested 565
> > > and got RGBA_8888) and act accordingly.
> > > I think its better than returing a failure and again calling with
RGBA_8888
> > > format for the snapshot.
> > > WDYT?
> > >
> > 
> > Who calls this with 565? I can't find a single caller in CodeSearch.
> > 
> 
> other embedders might since it's a public API.

I see. I guess I'm okay with people asking 565 when using software compositing
and getting back 8888 instead. Making a copy request and missing it would be
annoying cuz you have to do another compositor draw to try again.

[sivag, 2015-05-06 16:30:12]

@Sievers, Dana 
Can we go ahead with this change then?

[no sievers, 2015-05-06 23:08:04]

Can we also remove PreferredReadbackFormat() everywhere then?
It's always 'kN32_SkColorType'.

https://codereview.chromium.org/1113573002/diff/60001/content/browser/composi...
File content/browser/compositor/delegated_frame_host.cc (right):

https://codereview.chromium.org/1113573002/diff/60001/content/browser/composi...
content/browser/compositor/delegated_frame_host.cc:586: //
GLHelper::IsReadbackConfigSupported before we processs the result.
What keeps us from doing this here? And falling back to RGBA if not supported?
Glancing over the bug the problem seemed to arise if we did this earlier before
the compositor is initialized, but here we should be ok, right?

I'm just wondering... this can be done in a separate patch, esp. if it might
still cause some problem we'd want to isolate.

https://codereview.chromium.org/1113573002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/1113573002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:265: const
SkColorType color_type) {
Should we rename this to |preferred_color_type| then here and everywhere?

[sivag, 2015-05-07 14:14:20]

@sievers, ptal..

https://codereview.chromium.org/1113573002/diff/60001/content/browser/composi...
File content/browser/compositor/delegated_frame_host.cc (right):

https://codereview.chromium.org/1113573002/diff/60001/content/browser/composi...
content/browser/compositor/delegated_frame_host.cc:586: //
GLHelper::IsReadbackConfigSupported before we processs the result.
On 2015/05/06 23:08:03, sievers wrote:
> What keeps us from doing this here? And falling back to RGBA if not supported?
> Glancing over the bug the problem seemed to arise if we did this earlier
before
> the compositor is initialized, but here we should be ok, right?
> 
> I'm just wondering... this can be done in a separate patch, esp. if it might
> still cause some problem we'd want to isolate.

Actually this fails in the browser tests, where some snapshot is requested
before the compositor gets initialized, may be we can check this separately.

ptal at the below issue raised for this
http://code.google.com/p/chromium/issues/detail?id=415131

https://codereview.chromium.org/1113573002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/1113573002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:265: const
SkColorType color_type) {
On 2015/05/06 23:08:03, sievers wrote:
> Should we rename this to |preferred_color_type| then here and everywhere?

Done.

[no sievers, 2015-05-07 18:11:53]

lgtm. Can you update the comment that this also simplifies the readback API by
falling back to returning RGBA bitmaps automatically if the format is not
supported, which in practice only affects RGB565 though? And say that it
simplifies things because the support for 565 has to be dynamically queried.

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

https://codereview.chromium.org/1113573002/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:437:
SkColorType color_type,
nit: change it here too and in content_view_core_impl.h/.cc

[no sievers, 2015-05-07 18:13:05]

On 2015/05/07 18:11:53, sievers wrote:
> lgtm. Can you update the comment that this also simplifies the readback API by
> falling back to returning RGBA bitmaps automatically if the format is not
> supported, which in practice only affects RGB565 though? And say that it
> simplifies things because the support for 565 has to be dynamically queried.

Also I think you can probably reference the other bugs here, where we had
problems querying GLHelper::IsReadbackConfigSupported() too early.

[sivag, 2015-05-08 10:12:46]

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

https://codereview.chromium.org/1113573002/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:437:
SkColorType color_type,
On 2015/05/07 18:11:53, sievers wrote:
> nit: change it here too and in content_view_core_impl.h/.cc

Done.

[sivag, 2015-05-08 10:13:38]

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

[sivag, 2015-05-08 10:13:39]

The patchset sent to the CQ was uploaded after l-g-t-m from sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1113573002/#ps100001
(title: "Addressed review comments.")

[commit-bot: I haz the power, 2015-05-08 10:13:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1113573002/100001

[sivag, 2015-05-08 10:36:25]

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

[sivag, 2015-05-08 10:36:26]

The patchset sent to the CQ was uploaded after l-g-t-m from sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1113573002/#ps120001
(title: "Fixed build issue.")

[commit-bot: I haz the power, 2015-05-08 10:36:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1113573002/120001

[commit-bot: I haz the power, 2015-05-08 11:50:05]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-05-08 11:50:58]

Patchset 7 (id:??) landed as
https://crrev.com/49bc3a8c0fe5faaaa64bdb1d4743601e91691b2f
Cr-Commit-Position: refs/heads/master@{#328945}