Add RGB565 Texture readback support in gl_helper

Add RGB565 Texture readback support in gl_helper

Plan is to add a RGB565 readback and copy conversion for RGBA8 to RGB565 in GLHelper.

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

[sivag, 2013-11-26 11:54:53]

Support RGB565 format for capturing bitmap.

[danakj, 2013-11-26 16:05:56]

I'm not the right reviewer for this.

[no sievers, 2013-11-26 21:45:20]

All the sync readback APIs are actually getting deprecated. If we want to
support this, we'd need to add it to CopyFromCompositingSurface().

Initially I thought you were interested in using native 565 textures for the
image transport with readback in the same format to reduce bandwidth, but the
bug talks about some app-specific needs. Why not just do the conversion in the
app then?

[no sievers, 2013-11-26 21:45:26]

https://codereview.chromium.org/88033002/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/88033002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_android.cc:326:
&draw_rect,
Isn't the point of this to reduce the readback size?
This just seems to add another CPU copy and conversion. And the API for this is
unnecessary here, since the app might just do that itself.

[sivag, 2013-11-27 11:57:50]

On 2013/11/26 21:45:26, sievers wrote:
>
https://codereview.chromium.org/88033002/diff/1/content/browser/renderer_host...
> File content/browser/renderer_host/render_widget_host_view_android.cc (right):
> 
>
https://codereview.chromium.org/88033002/diff/1/content/browser/renderer_host...
> content/browser/renderer_host/render_widget_host_view_android.cc:326:
> &draw_rect,
> Isn't the point of this to reduce the readback size?
> This just seems to add another CPU copy and conversion. And the API for this
is
> unnecessary here, since the app might just do that itself.

I agree this is an app specific requirement, we can do this from the app side as
well.
Say we just support ARGB8888 bitmap format from the native side, But we want to
use a rgb565 format in app for viewport capture.
ex :My dimensions are  1080 * 1729.  From the dalvik side i need to create a
bitmap with argb8888 config  which will take ~ 7469280 bytes of dalvik heap,The
final bitmap i need is of rgb565 config so i need to do a Bitmap	
copy(Bitmap.Config config, boolean isMutable)to get the final bitmap, which will
give one more bitmap of 3734640 bytes. Then we need to recycle the first bitmap
, if not used.
      So instead of doing all this in the app side and increase consumption of
dalvik heap for apps using the capture bitmap more times
i thought its better if we provide the support on the native side format
support.

[sivag, 2013-11-27 12:00:38]

On 2013/11/26 21:45:20, sievers wrote:
> All the sync readback APIs are actually getting deprecated. If we want to
> support this, we'd need to add it to CopyFromCompositingSurface().
> 
> Initially I thought you were interested in using native 565 textures for the
> image transport with readback in the same format to reduce bandwidth, but the
> bug talks about some app-specific needs. Why not just do the conversion in the
> app then?

So the bitmap sync api is now deprecated, can we provide this for our async
implementation CopyFromCompositingSurface.
Does this support format as of now? If not can we add a format option to this?

The simpler approach would be to do a Skia side conversion before sending the
bitmap.

Otherwise to reduce bandwidth, can we implement based on the format neeeded like
for rgb565/argb , create rgb565/argb textures for the
image transport with readback in the same format.

[no sievers, 2013-11-27 19:31:52]

On 2013/11/27 11:57:50, Sikugu wrote:
> On 2013/11/26 21:45:26, sievers wrote:
> >
>
https://codereview.chromium.org/88033002/diff/1/content/browser/renderer_host...
> > File content/browser/renderer_host/render_widget_host_view_android.cc
(right):
> > 
> >
>
https://codereview.chromium.org/88033002/diff/1/content/browser/renderer_host...
> > content/browser/renderer_host/render_widget_host_view_android.cc:326:
> > &draw_rect,
> > Isn't the point of this to reduce the readback size?
> > This just seems to add another CPU copy and conversion. And the API for this
> is
> > unnecessary here, since the app might just do that itself.
> 
> I agree this is an app specific requirement, we can do this from the app side
as
> well.
> Say we just support ARGB8888 bitmap format from the native side, But we want
to
> use a rgb565 format in app for viewport capture.
> ex :My dimensions are  1080 * 1729.  From the dalvik side i need to create a
> bitmap with argb8888 config  which will take ~ 7469280 bytes of dalvik
heap,The
> final bitmap i need is of rgb565 config so i need to do a Bitmap	
> copy(Bitmap.Config config, boolean isMutable)to get the final bitmap, which
will
> give one more bitmap of 3734640 bytes. Then we need to recycle the first
bitmap
> , if not used.
>       So instead of doing all this in the app side and increase consumption of
> dalvik heap for apps using the capture bitmap more times
> i thought its better if we provide the support on the native side format
> support.


With CopyFromCompositingSurface() you get a native skia bitmap.

[no sievers, 2013-11-27 20:04:57]

On 2013/11/27 12:00:38, Sikugu wrote:
> On 2013/11/26 21:45:20, sievers wrote:
> > All the sync readback APIs are actually getting deprecated. If we want to
> > support this, we'd need to add it to CopyFromCompositingSurface().
> > 
> > Initially I thought you were interested in using native 565 textures for the
> > image transport with readback in the same format to reduce bandwidth, but
the
> > bug talks about some app-specific needs. Why not just do the conversion in
the
> > app then?
> 
> So the bitmap sync api is now deprecated, can we provide this for our async
> implementation CopyFromCompositingSurface.
> Does this support format as of now? If not can we add a format option to this?
> 
> The simpler approach would be to do a Skia side conversion before sending the
> bitmap.
> 

But why do you want support for a skia conversion in RWHV? What's the advantage
over just doing this in the embedder layer?


> Otherwise to reduce bandwidth, can we implement based on the format neeeded
like
> for rgb565/argb , create rgb565/argb textures for the
> image transport with readback in the same format.

RGB565 readback sounds interesting to explore. Since the transport texture
format is chosen by the renderer (and see the hardcode to 565 for
IsLowEndDevice() on Android; otherwise it uses RGBA8), I think we'd have to do a
GPU conversion to 565 if the format does not already match. Also for ubercomp
there is no transport texture. CopyFromCompositingSurface() can already handle
multiple ways of getting back the pixels, so we can just use GLHelper to copy
the texture into 565 and then read it back (it already supports doing such a
copy for when scaling is needed).

[sivag, 2013-11-28 13:28:16]

On 2013/11/27 20:04:57, sievers wrote:
> On 2013/11/27 12:00:38, Sikugu wrote:
> > On 2013/11/26 21:45:20, sievers wrote:
> > > All the sync readback APIs are actually getting deprecated. If we want to
> > > support this, we'd need to add it to CopyFromCompositingSurface().
> > > 
> > > Initially I thought you were interested in using native 565 textures for
the
> > > image transport with readback in the same format to reduce bandwidth, but
> the
> > > bug talks about some app-specific needs. Why not just do the conversion in
> the
> > > app then?
> > 
> > So the bitmap sync api is now deprecated, can we provide this for our async
> > implementation CopyFromCompositingSurface.
> > Does this support format as of now? If not can we add a format option to
this?
> > 
> > The simpler approach would be to do a Skia side conversion before sending
the
> > bitmap.
> > 
> 
> But why do you want support for a skia conversion in RWHV? What's the
advantage
> over just doing this in the embedder layer?
> 

#Siva::I was just looking out for possible ways to support the format.
I am not considering this approach any more.



> 
> > Otherwise to reduce bandwidth, can we implement based on the format neeeded
> like
> > for rgb565/argb , create rgb565/argb textures for the
> > image transport with readback in the same format.
> 
> RGB565 readback sounds interesting to explore. Since the transport texture
> format is chosen by the renderer (and see the hardcode to 565 for
> IsLowEndDevice() on Android; otherwise it uses RGBA8), I think we'd have to do
a
> GPU conversion to 565 if the format does not already match. Also for ubercomp
> there is no transport texture. CopyFromCompositingSurface() can already handle
> multiple ways of getting back the pixels, so we can just use GLHelper to copy
> the texture into 565 and then read it back (it already supports doing such a
> copy for when scaling is needed).


#Siva:: I just went through the code, so we are just having the ARGB8888 i.e is
the default render format
for high end devices, for low end  devices its rgb565 .I was just thinking if we
can support the RGB565 texture read back
that would be great. Is some body working on this? If not i would be interested
in taking up this
task .Please let me know.

[no sievers, 2013-12-02 19:01:04]

On 2013/11/28 13:28:16, Sikugu wrote:
> On 2013/11/27 20:04:57, sievers wrote:
> > On 2013/11/27 12:00:38, Sikugu wrote:
> > > On 2013/11/26 21:45:20, sievers wrote:
> > > > All the sync readback APIs are actually getting deprecated. If we want
to
> > > > support this, we'd need to add it to CopyFromCompositingSurface().
> > > > 
> > > > Initially I thought you were interested in using native 565 textures for
> the
> > > > image transport with readback in the same format to reduce bandwidth,
but
> > the
> > > > bug talks about some app-specific needs. Why not just do the conversion
in
> > the
> > > > app then?
> > > 
> > > So the bitmap sync api is now deprecated, can we provide this for our
async
> > > implementation CopyFromCompositingSurface.
> > > Does this support format as of now? If not can we add a format option to
> this?
> > > 
> > > The simpler approach would be to do a Skia side conversion before sending
> the
> > > bitmap.
> > > 
> > 
> > But why do you want support for a skia conversion in RWHV? What's the
> advantage
> > over just doing this in the embedder layer?
> > 
> 
> #Siva::I was just looking out for possible ways to support the format.
> I am not considering this approach any more.
> 
> 
> 
> > 
> > > Otherwise to reduce bandwidth, can we implement based on the format
neeeded
> > like
> > > for rgb565/argb , create rgb565/argb textures for the
> > > image transport with readback in the same format.
> > 
> > RGB565 readback sounds interesting to explore. Since the transport texture
> > format is chosen by the renderer (and see the hardcode to 565 for
> > IsLowEndDevice() on Android; otherwise it uses RGBA8), I think we'd have to
do
> a
> > GPU conversion to 565 if the format does not already match. Also for
ubercomp
> > there is no transport texture. CopyFromCompositingSurface() can already
handle
> > multiple ways of getting back the pixels, so we can just use GLHelper to
copy
> > the texture into 565 and then read it back (it already supports doing such a
> > copy for when scaling is needed).
> 
> 
> #Siva:: I just went through the code, so we are just having the ARGB8888 i.e
is
> the default render format
> for high end devices, for low end  devices its rgb565 .I was just thinking if
we
> can support the RGB565 texture read back
> that would be great. Is some body working on this? If not i would be
interested
> in taking up this
> task .Please let me know.

I don't think anybody is working on that yet. There is a YUV readback path in
GLHelper that also tries to minimize the readback size, but I think  otherwise
it currently assumes RGBA.

Note that the more interesting readback path is where the CopyOutputRequest on
the layer goes into a temporary texture (because of ubercomp), so you'd want to
teach GLHelper to be able to read back 565. We could then extend this to
introduce an extra copy from RGBA8 to 565 where necessary (or teach the
compositor to draw into a texture using 565).

[sivag, 2013-12-04 19:57:30]

On 2013/12/02 19:01:04, sievers wrote:
> On 2013/11/28 13:28:16, Sikugu wrote:
> > On 2013/11/27 20:04:57, sievers wrote:
> > > On 2013/11/27 12:00:38, Sikugu wrote:
> > > > On 2013/11/26 21:45:20, sievers wrote:
> > > > > All the sync readback APIs are actually getting deprecated. If we want
> to
> > > > > support this, we'd need to add it to CopyFromCompositingSurface().
> > > > > 
> > > > > Initially I thought you were interested in using native 565 textures
for
> > the
> > > > > image transport with readback in the same format to reduce bandwidth,
> but
> > > the
> > > > > bug talks about some app-specific needs. Why not just do the
conversion
> in
> > > the
> > > > > app then?
> > > > 
> > > > So the bitmap sync api is now deprecated, can we provide this for our
> async
> > > > implementation CopyFromCompositingSurface.
> > > > Does this support format as of now? If not can we add a format option to
> > this?
> > > > 
> > > > The simpler approach would be to do a Skia side conversion before
sending
> > the
> > > > bitmap.
> > > > 
> > > 
> > > But why do you want support for a skia conversion in RWHV? What's the
> > advantage
> > > over just doing this in the embedder layer?
> > > 
> > 
> > #Siva::I was just looking out for possible ways to support the format.
> > I am not considering this approach any more.
> > 
> > 
> > 
> > > 
> > > > Otherwise to reduce bandwidth, can we implement based on the format
> neeeded
> > > like
> > > > for rgb565/argb , create rgb565/argb textures for the
> > > > image transport with readback in the same format.
> > > 
> > > RGB565 readback sounds interesting to explore. Since the transport texture
> > > format is chosen by the renderer (and see the hardcode to 565 for
> > > IsLowEndDevice() on Android; otherwise it uses RGBA8), I think we'd have
to
> do
> > a
> > > GPU conversion to 565 if the format does not already match. Also for
> ubercomp
> > > there is no transport texture. CopyFromCompositingSurface() can already
> handle
> > > multiple ways of getting back the pixels, so we can just use GLHelper to
> copy
> > > the texture into 565 and then read it back (it already supports doing such
a
> > > copy for when scaling is needed).
> > 
> > 
> > #Siva:: I just went through the code, so we are just having the ARGB8888 i.e
> is
> > the default render format
> > for high end devices, for low end  devices its rgb565 .I was just thinking
if
> we
> > can support the RGB565 texture read back
> > that would be great. Is some body working on this? If not i would be
> interested
> > in taking up this
> > task .Please let me know.
> 
> I don't think anybody is working on that yet. There is a YUV readback path in
> GLHelper that also tries to minimize the readback size, but I think  otherwise
> it currently assumes RGBA.
> 
> Note that the more interesting readback path is where the CopyOutputRequest on
> the layer goes into a temporary texture (because of ubercomp), so you'd want
to 
> teach GLHelper to be able to read back 565. We could then extend this to
> introduce an extra copy from RGBA8 to 565 where necessary (or teach the
> compositor to draw into a texture using 565).


@SIVA ::: So we have 2 things here right
1.Draw directly to rgb565 texture and read back.
2.copy from rgba8m to rgb565 texture and read back.

I will start this implementation and will update you in the same bug/review

[no sievers, 2013-12-05 19:17:44]

On 2013/12/04 19:57:30, Sikugu wrote:
> On 2013/12/02 19:01:04, sievers wrote:
> > On 2013/11/28 13:28:16, Sikugu wrote:
> > > On 2013/11/27 20:04:57, sievers wrote:
> > > > On 2013/11/27 12:00:38, Sikugu wrote:
> > > > > On 2013/11/26 21:45:20, sievers wrote:
> > > > > > All the sync readback APIs are actually getting deprecated. If we
want
> > to
> > > > > > support this, we'd need to add it to CopyFromCompositingSurface().
> > > > > > 
> > > > > > Initially I thought you were interested in using native 565 textures
> for
> > > the
> > > > > > image transport with readback in the same format to reduce
bandwidth,
> > but
> > > > the
> > > > > > bug talks about some app-specific needs. Why not just do the
> conversion
> > in
> > > > the
> > > > > > app then?
> > > > > 
> > > > > So the bitmap sync api is now deprecated, can we provide this for our
> > async
> > > > > implementation CopyFromCompositingSurface.
> > > > > Does this support format as of now? If not can we add a format option
to
> > > this?
> > > > > 
> > > > > The simpler approach would be to do a Skia side conversion before
> sending
> > > the
> > > > > bitmap.
> > > > > 
> > > > 
> > > > But why do you want support for a skia conversion in RWHV? What's the
> > > advantage
> > > > over just doing this in the embedder layer?
> > > > 
> > > 
> > > #Siva::I was just looking out for possible ways to support the format.
> > > I am not considering this approach any more.
> > > 
> > > 
> > > 
> > > > 
> > > > > Otherwise to reduce bandwidth, can we implement based on the format
> > neeeded
> > > > like
> > > > > for rgb565/argb , create rgb565/argb textures for the
> > > > > image transport with readback in the same format.
> > > > 
> > > > RGB565 readback sounds interesting to explore. Since the transport
texture
> > > > format is chosen by the renderer (and see the hardcode to 565 for
> > > > IsLowEndDevice() on Android; otherwise it uses RGBA8), I think we'd have
> to
> > do
> > > a
> > > > GPU conversion to 565 if the format does not already match. Also for
> > ubercomp
> > > > there is no transport texture. CopyFromCompositingSurface() can already
> > handle
> > > > multiple ways of getting back the pixels, so we can just use GLHelper to
> > copy
> > > > the texture into 565 and then read it back (it already supports doing
such
> a
> > > > copy for when scaling is needed).
> > > 
> > > 
> > > #Siva:: I just went through the code, so we are just having the ARGB8888
i.e
> > is
> > > the default render format
> > > for high end devices, for low end  devices its rgb565 .I was just thinking
> if
> > we
> > > can support the RGB565 texture read back
> > > that would be great. Is some body working on this? If not i would be
> > interested
> > > in taking up this
> > > task .Please let me know.
> > 
> > I don't think anybody is working on that yet. There is a YUV readback path
in
> > GLHelper that also tries to minimize the readback size, but I think 
otherwise
> > it currently assumes RGBA.
> > 
> > Note that the more interesting readback path is where the CopyOutputRequest
on
> > the layer goes into a temporary texture (because of ubercomp), so you'd want
> to 
> > teach GLHelper to be able to read back 565. We could then extend this to
> > introduce an extra copy from RGBA8 to 565 where necessary (or teach the
> > compositor to draw into a texture using 565).
> 
> 
> @SIVA ::: So we have 2 things here right
> 1.Draw directly to rgb565 texture and read back.
> 2.copy from rgba8m to rgb565 texture and read back.
> 

Basically yes. Let's focus on the ubercomp path though, so 1. would be a cc
feature in the CopyOutputRequest, while 2. would be a change in GLHelper.
I'd tackle them separately, and since 1. is only an optimization (because you
could always simply do 2.) you might want to start with 2.

[sivag, 2013-12-05 20:48:25]

I am starting with the 2nd task adding support in glhelper.

[David Trainor- moved to gerrit, 2013-12-13 09:52:44]

On 2013/12/05 20:48:25, Sikugu wrote:
> I am starting with the 2nd task adding support in glhelper.

Doing the copy on the CPU seems pretty expensive.  It should (I think?) be
pretty easy to teach GLHelper to read back into RGB565 by passing the expected
format of the resulting pixels into the readback method in the GL Helper and
using that format when creating the texture we read back from.

[sivag, 2013-12-17 10:51:57]

(After enabling Uber-Compositing path):
----------------------
I tried two approaches to accomplish this,
Both approaches instead of ARGB8888, RGB565 config skia bitmap will be passed to
gl_helper.

Method-1:
---------
1. In CropScaleReadbackAndCleanTexture, We get the Scaled destination Texture
using (ScaleTexture()) RGBA format which is bound as color attachment to the
FBO.
2.Here i created a new texture ,and applied the format as
GL_RGB,GL_UNSIGNED_SHORT_5_6_5. Binded this texture.
3.copied the fbo contents using copyTexSubImage2D ,
4.add the new texture as color attachment to fbo,
5.Read back using ReadBackAsync function.delete the textures.
6. I have changed the row bytes and row stride values, and the copy out values
in the ReadBack done to width*2

Method-2 :
----------
1. Changed ScaleTexture Code, send a GL_RGB , with GL_UNSIGNED_SHORT_5_6_5 pixel
format texture as destination texture directly for copy and scaling.
2. This will make sure , the src texture(RGBA) will paint to the destination
texture (RGB).
3.Bind the destination texture as color attachment to FBO.
4.Read this using ReadBackAsync.
5.I have changed the row bytes and row stride values, and the copy out values in
the ReadBack done to width*2.

Finally i am able to dump the image to a file, using a PNG encoder in , the
content_view_core call back.
But the final output, looks like a twice replicated image version for both the
approaches ,Please check the attachment in the bug.

In the above patch Method-1 supporting code is enabled, though the patch
consists of Method-2 code as well, which is tested and gives the same output as
Method-1, but it is not enabled. 

I am trying to solve this issue.
Any  pointers ,regarding this will be helpful.

[hubbe1, 2013-12-17 19:03:59]

If the image is replicated twice, the size of the result must be twice as
big as expected?
Can you attach the output image, and perhaps also an image that shows what
it *should* look like?

     /Hubbe



On Tue, Dec 17, 2013 at 2:51 AM, <siva.gunturi@samsung.com> wrote:

> (After enabling Uber-Compositing path):
> ----------------------
> I tried two approaches to accomplish this,
> Both approaches instead of ARGB8888, RGB565 config skia bitmap will be
> passed to
> gl_helper.
>
> Method-1:
> ---------
> 1. In CropScaleReadbackAndCleanTexture, We get the Scaled destination
> Texture
> using (ScaleTexture()) RGBA format which is bound as color attachment to
> the
> FBO.
> 2.Here i created a new texture ,and applied the format as
> GL_RGB,GL_UNSIGNED_SHORT_5_6_5. Binded this texture.
> 3.copied the fbo contents using copyTexSubImage2D ,
> 4.add the new texture as color attachment to fbo,
> 5.Read back using ReadBackAsync function.delete the textures.
> 6. I have changed the row bytes and row stride values, and the copy out
> values
> in the ReadBack done to width*2
>
> Method-2 :
> ----------
> 1. Changed ScaleTexture Code, send a GL_RGB , with GL_UNSIGNED_SHORT_5_6_5
> pixel
> format texture as destination texture directly for copy and scaling.
> 2. This will make sure , the src texture(RGBA) will paint to the
> destination
> texture (RGB).
> 3.Bind the destination texture as color attachment to FBO.
> 4.Read this using ReadBackAsync.
> 5.I have changed the row bytes and row stride values, and the copy out
> values in
> the ReadBack done to width*2.
>
> Finally i am able to dump the image to a file, using a PNG encoder in , the
> content_view_core call back.
> But the final output, looks like a twice replicated image version for both
> the
> approaches ,Please check the attachment in the bug.
>
> In the above patch Method-1 supporting code is enabled, though the patch
> consists of Method-2 code as well, which is tested and gives the same
> output as
> Method-1, but it is not enabled.
>
> I am trying to solve this issue.
> Any  pointers ,regarding this will be helpful.
>
> https://codereview.chromium.org/88033002/
>

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

[sivag, 2013-12-17 19:34:01]

On 2013/12/17 19:03:59, hubbe1 wrote:
> If the image is replicated twice, the size of the result must be twice as
> big as expected?
> Can you attach the output image, and perhaps also an image that shows what
> it *should* look like?
> 
>      /Hubbe
> 
> 

Hi Hubbe ,

I have attached the image in the bug 
Please check below
http://code.google.com/p/chromium/issues/detail?id=323150

-Siva.

[hubbe1, 2013-12-17 21:13:41]

Have you checked that the device actually suports 565 render targets?
(OpenGL ES doesn't guarantee it)
(Btw, what device is this? It seems to have insanely high resolution)

From looking at the picture, it kind of looks like some part of the
pipeilne still treats everything as 4 bytes per pixel, so when it gets data
with 2 bytes per pixel, it looks compressed horizontally. Not sure why it
doesn't end up with a bunch of garbage at the bottom though.



On Tue, Dec 17, 2013 at 11:34 AM, <siva.gunturi@samsung.com> wrote:

> On 2013/12/17 19:03:59, hubbe1 wrote:
>
>> If the image is replicated twice, the size of the result must be twice as
>> big as expected?
>> Can you attach the output image, and perhaps also an image that shows what
>> it *should* look like?
>>
>
>        /Hubbe
>>
>
>
>
> Hi Hubbe ,
>
> I have attached the image in the bug
> Please check below
> http://code.google.com/p/chromium/issues/detail?id=323150
>
> -Siva.
>
> https://codereview.chromium.org/88033002/
>

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

[no sievers, 2013-12-17 21:17:27]

Also, you have to verify that the readback is supported in that format
(regardless of whether that format is supported for the texture) by querying
glGetInteger() with IMPLEMENTATION_COLOR_READ_FORMAT with the read framebuffer
bound.

[sivag, 2013-12-18 17:42:26]

On 2013/12/17 21:13:41, hubbe1 wrote:
> Have you checked that the device actually suports 565 render targets?
> (OpenGL ES doesn't guarantee it)
> (Btw, what device is this? It seems to have insanely high resolution)

> From looking at the picture, it kind of looks like some part of the
> pipeilne still treats everything as 4 bytes per pixel, so when it gets data
> with 2 bytes per pixel, it looks compressed horizontally. Not sure why it
> doesn't end up with a bunch of garbage at the bottom though.
> 
> 

The image is taken on samsung Galaxy Grand device.
I will look into it the device support for 565 render targets ,
So we may have to support the format readback based on this logic.
Regarding the replicated image, it looks like its considering  4 bytes per
pixel.
I will investigate further on this, will let you know my findings.

[sivag, 2013-12-18 17:44:16]

On 2013/12/17 21:17:27, sievers wrote:
> Also, you have to verify that the readback is supported in that format
> (regardless of whether that format is supported for the texture) by querying
> glGetInteger() with IMPLEMENTATION_COLOR_READ_FORMAT with the read framebuffer
> bound.

The initial patch which i submitted is not having these checks.
Will take care of checking these conditions.

[sivag, 2013-12-19 12:39:53]

The patch is up and working.
The problem i faced earlier was due to writing the image in the wrong format i.e
RGB565->RGBA. Now its solved.
I tested the code for both ARGB8888 and RGB565 bitmaps.
Added the error check before reading from fbo as suggested by sievers.
Tried using geterror and getTexParameteriv api's
but not able access them in gl_helper, for more error checking.
Please review the code and share your comments.

Thanks,
Siva

[sivag, 2013-12-23 06:47:21]

On 2013/12/17 21:17:27, sievers wrote:
> Also, you have to verify that the readback is supported in that format
> (regardless of whether that format is supported for the texture) by querying
> glGetInteger() with IMPLEMENTATION_COLOR_READ_FORMAT with the read framebuffer
> bound.

PTAL..

[hubbe, 2013-12-26 21:41:48]

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

https://codereview.chromium.org/88033002/diff/60001/content/common/gpu/client...
content/common/gpu/client/gl_helper.cc:391: int readback_factor,
readback_factor is redundant and not well named.
Just do something like:
int bytes_per_pixel = readback_config_rgb565 ? 2 : 4;
inside the function instead.

https://codereview.chromium.org/88033002/diff/60001/content/common/gpu/client...
content/common/gpu/client/gl_helper.cc:426: //Using copy to texture
Nit: Space after //

https://codereview.chromium.org/88033002/diff/60001/content/common/gpu/client...
content/common/gpu/client/gl_helper.cc:476: int readback_factor = 0;
readback_factor -> bytes_per_pixel

https://codereview.chromium.org/88033002/diff/60001/content/common/gpu/client...
content/common/gpu/client/gl_helper.cc:484: dst_rgb_565_texture =
CopyTo565Texture(dst_size);
It would (probably) be faster to have the scaler write into a 565 texture
directly instead of scaling and then copying.

https://codereview.chromium.org/88033002/diff/60001/content/common/gpu/client...
content/common/gpu/client/gl_helper.cc:492: //Check format of read after binding
with fbo.
Space after //

https://codereview.chromium.org/88033002/diff/60001/content/common/gpu/client...
content/common/gpu/client/gl_helper.cc:496: if((ext_format != GL_RGB) ||
(ext_type != GL_UNSIGNED_SHORT_5_6_5)){
This is the wrong place to detect this.
This detection should happen long before this function is ever called.
Also, we need to make sure that we can render into/copy to 565 textures before
calling CopyTo565Texure, right?

I suggest that we add a function like: CanUseRgb565Readback() that does the
testing, so that the caller can check if 565 readback is supporting before
decinging to use the "readback_config_rgb565" parameter.

PS: There is an alternate way to do this that is *always* supported on GLES2:
You create a shader that outputs RGBA pixels, but in fact puts one 565 pixel
into the "RG" components and another 565 pixel into the "BA" components. Then
you read out RGBA as usual, but what you get is actually 565. Because RGBA
rendering and readback is guaranteed by GLES2, this should work everywhere.
However, it will fail if the precision of the shaders is less than 8 bits per
channel.

This is essentially how the YUV readback works.

https://codereview.chromium.org/88033002/diff/60001/content/common/gpu/client...
content/common/gpu/client/gl_helper.cc:497: LOG(ERROR)<<"Readbackformat rgb565 
not supported";
NIT: space around <<

[sivag, 2013-12-27 17:15:14]

The patch now takes care of
1.scaler write into a 565 texture directly
2.565 format check at the right place.
and other review comments.

PTAL..

[sivag, 2014-01-06 04:58:56]

On 2013/12/26 21:41:48, hubbe wrote:
>
https://codereview.chromium.org/88033002/diff/60001/content/common/gpu/client...
> File content/common/gpu/client/gl_helper.cc (right):
> 
>
https://codereview.chromium.org/88033002/diff/60001/content/common/gpu/client...
> content/common/gpu/client/gl_helper.cc:391: int readback_factor,
> readback_factor is redundant and not well named.
> Just do something like:
> int bytes_per_pixel = readback_config_rgb565 ? 2 : 4;
> inside the function instead.
> 
>
https://codereview.chromium.org/88033002/diff/60001/content/common/gpu/client...
> content/common/gpu/client/gl_helper.cc:426: //Using copy to texture
> Nit: Space after //
> 
>
https://codereview.chromium.org/88033002/diff/60001/content/common/gpu/client...
> content/common/gpu/client/gl_helper.cc:476: int readback_factor = 0;
> readback_factor -> bytes_per_pixel
> 
>
https://codereview.chromium.org/88033002/diff/60001/content/common/gpu/client...
> content/common/gpu/client/gl_helper.cc:484: dst_rgb_565_texture =
> CopyTo565Texture(dst_size);
> It would (probably) be faster to have the scaler write into a 565 texture
> directly instead of scaling and then copying.
> 
>
https://codereview.chromium.org/88033002/diff/60001/content/common/gpu/client...
> content/common/gpu/client/gl_helper.cc:492: //Check format of read after
binding
> with fbo.
> Space after //
> 
>
https://codereview.chromium.org/88033002/diff/60001/content/common/gpu/client...
> content/common/gpu/client/gl_helper.cc:496: if((ext_format != GL_RGB) ||
> (ext_type != GL_UNSIGNED_SHORT_5_6_5)){
> This is the wrong place to detect this.
> This detection should happen long before this function is ever called.
> Also, we need to make sure that we can render into/copy to 565 textures before
> calling CopyTo565Texure, right?
> 
> I suggest that we add a function like: CanUseRgb565Readback() that does the
> testing, so that the caller can check if 565 readback is supporting before
> decinging to use the "readback_config_rgb565" parameter.
> 
> PS: There is an alternate way to do this that is *always* supported on GLES2:
> You create a shader that outputs RGBA pixels, but in fact puts one 565 pixel
> into the "RG" components and another 565 pixel into the "BA" components. Then
> you read out RGBA as usual, but what you get is actually 565. Because RGBA
> rendering and readback is guaranteed by GLES2, this should work everywhere.
> However, it will fail if the precision of the shaders is less than 8 bits per
> channel.
> 
> This is essentially how the YUV readback works.
> 
>
https://codereview.chromium.org/88033002/diff/60001/content/common/gpu/client...
> content/common/gpu/client/gl_helper.cc:497: LOG(ERROR)<<"Readbackformat rgb565

> not supported";
> NIT: space around <<

Hi,
Please take a look at this modified patch and provide your comments.

[no sievers, 2014-01-07 16:26:01]

Can you also add a test in gl_helper_unittests.cc?

https://codereview.chromium.org/88033002/diff/100001/content/browser/android/...
File content/browser/android/content_view_core_impl.h (right):

https://codereview.chromium.org/88033002/diff/100001/content/browser/android/...
content/browser/android/content_view_core_impl.h:313: bool
readback_config_rgb565);
Adding this here is unnecessary since ContentViewCoreImpl does not make the API
more public than RWHV. It'd have to be virtual and exposed in
content/public/browser/android/content_view_core.h also.

Maybe we can sort out how this should be exposed to the embedder in a separate
patch?

https://codereview.chromium.org/88033002/diff/100001/content/browser/android/...
content/browser/android/content_view_core_impl.h:314: bool
IsReadBackConfig565();
Same for this. It should be in the same place and exposed to the embedder as the
above.

Maybe the requested format should even be added as an argument to
CopyFromCompositingSurface().

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

https://codereview.chromium.org/88033002/diff/100001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:599:
if(readback_config_rgb565 && !gl_helper->CanUseRgb565Readback()){
Can we check CanUseRgb565Readback() only once? It's not necessarily cheap
because it allocates memory and implies a synchronous GL call. Maybe a static
bool is good enough here.

https://codereview.chromium.org/88033002/diff/100001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:1341:
bitmap->setConfig(SkBitmap::kRGB_565_Config,
nit: no need for if-else, can do 'readback_config_rgb565 ? : ... : ...'

https://codereview.chromium.org/88033002/diff/100001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:1347:
dst_size_in_pixel.width(), dst_size_in_pixel.height(),
nit: indent here and above

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

https://codereview.chromium.org/88033002/diff/100001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.cc:310: bool  readback_config_rgb565,
nit: two spaces

https://codereview.chromium.org/88033002/diff/100001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.cc:314: void ReadbackDone(Request
*request,int bytes_per_pixel);
nit: space after comma missing

https://codereview.chromium.org/88033002/diff/100001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.cc:357: bool  readback_config_rgb565,
nit: two spaces

https://codereview.chromium.org/88033002/diff/100001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.cc:370: if(readback_config_rgb565){
nit: space missing

https://codereview.chromium.org/88033002/diff/100001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.cc:380: }else{
nit: spaces missing

https://codereview.chromium.org/88033002/diff/100001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.cc:402: const base::Callback<void(bool)>&
callback) {
Can you add a check for GL_IMPLEMENTATION_COLOR_READ_FORMAT like you do below
here and fail this with a logged error if 565 is not supported?

https://codereview.chromium.org/88033002/diff/100001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.cc:420: if(readback_config_rgb565){
nit: space missing in between ')' and '{'

https://codereview.chromium.org/88033002/diff/100001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.cc:422: GL_RGB, GL_UNSIGNED_SHORT_5_6_5,
NULL);
nit: indent here and in 425.

https://codereview.chromium.org/88033002/diff/100001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.cc:794: const int test_size = 100;
Can you use something power of two?

https://codereview.chromium.org/88033002/diff/100001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.cc:797: context_->texImage2D(GL_TEXTURE_2D,
nit: indent

https://codereview.chromium.org/88033002/diff/100001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.cc:805: NULL);
The texture is incomplete because the default min_filter is
NEAREST_MIPMAP_LINEAR but you didn't define mipmaps.

I'd just set MIN_/MAG_FILTER to linear (and also set WRAP_S/_T to CLAMP_TO_EDGE)
here.

[sivag, 2014-01-09 14:40:27]

PTAL ...My comments below.


On 2014/01/07 16:26:01, sievers wrote:
> Can you also add a test in gl_helper_unittests.cc?
> 
@Siva : I am preparing this test, will add this.

>
https://codereview.chromium.org/88033002/diff/100001/content/browser/android/...
> File content/browser/android/content_view_core_impl.h (right):
> 
>
https://codereview.chromium.org/88033002/diff/100001/content/browser/android/...
> content/browser/android/content_view_core_impl.h:313: bool
> readback_config_rgb565);
> Adding this here is unnecessary since ContentViewCoreImpl does not make the
API
> more public than RWHV. It'd have to be virtual and exposed in
> content/public/browser/android/content_view_core.h also.

@Siva: Removed.
> 
> Maybe we can sort out how this should be exposed to the embedder in a separate
> patch?
> 
>
https://codereview.chromium.org/88033002/diff/100001/content/browser/android/...
> content/browser/android/content_view_core_impl.h:314: bool
> IsReadBackConfig565();
> Same for this. It should be in the same place and exposed to the embedder as
the
> above.

@Siva: Removed.

> 
> Maybe the requested format should even be added as an argument to
> CopyFromCompositingSurface().

@Siva: Argument added to CopyFromCompositingSurface.
Necessary changes done.
> 
>
https://codereview.chromium.org/88033002/diff/100001/content/browser/renderer...
> File content/browser/renderer_host/render_widget_host_view_android.cc (right):
> 
>
https://codereview.chromium.org/88033002/diff/100001/content/browser/renderer...
> content/browser/renderer_host/render_widget_host_view_android.cc:599:
> if(readback_config_rgb565 && !gl_helper->CanUseRgb565Readback()){
> Can we check CanUseRgb565Readback() only once? It's not necessarily cheap
> because it allocates memory and implies a synchronous GL call. Maybe a static
> bool is good enough here.

@Siva:Done

> 
>
https://codereview.chromium.org/88033002/diff/100001/content/browser/renderer...
> content/browser/renderer_host/render_widget_host_view_android.cc:1341:
> bitmap->setConfig(SkBitmap::kRGB_565_Config,
> nit: no need for if-else, can do 'readback_config_rgb565 ? : ... : ...'

@Siva:Done

> 
>
https://codereview.chromium.org/88033002/diff/100001/content/browser/renderer...
> content/browser/renderer_host/render_widget_host_view_android.cc:1347:
> dst_size_in_pixel.width(), dst_size_in_pixel.height(),
> nit: indent here and above

@Siva:Done

> 
>
https://codereview.chromium.org/88033002/diff/100001/content/common/gpu/clien...
> File content/common/gpu/client/gl_helper.cc (right):
> 
>
https://codereview.chromium.org/88033002/diff/100001/content/common/gpu/clien...
> content/common/gpu/client/gl_helper.cc:310: bool  readback_config_rgb565,
> nit: two spaces

@Siva:Done

> 
>
https://codereview.chromium.org/88033002/diff/100001/content/common/gpu/clien...
> content/common/gpu/client/gl_helper.cc:314: void ReadbackDone(Request
> *request,int bytes_per_pixel);
> nit: space after comma missing
> 
@Siva:Done

>
https://codereview.chromium.org/88033002/diff/100001/content/common/gpu/clien...
> content/common/gpu/client/gl_helper.cc:357: bool  readback_config_rgb565,
> nit: two spaces
> 
@Siva:Done

>
https://codereview.chromium.org/88033002/diff/100001/content/common/gpu/clien...
> content/common/gpu/client/gl_helper.cc:370: if(readback_config_rgb565){
> nit: space missing
> 
@Siva:Done

>
https://codereview.chromium.org/88033002/diff/100001/content/common/gpu/clien...
> content/common/gpu/client/gl_helper.cc:380: }else{
> nit: spaces missing
> 
@Siva:Done

>
https://codereview.chromium.org/88033002/diff/100001/content/common/gpu/clien...
> content/common/gpu/client/gl_helper.cc:402: const base::Callback<void(bool)>&
> callback) {
> Can you add a check for GL_IMPLEMENTATION_COLOR_READ_FORMAT like you do below
> here and fail this with a logged error if 565 is not supported?
> 
@Siva:Is this needed as we are already doing the check and failing it
in the begining of copyfromcomposiitingsurface( in
renderer_widget_host_view_android.cc

>
https://codereview.chromium.org/88033002/diff/100001/content/common/gpu/clien...
> content/common/gpu/client/gl_helper.cc:420: if(readback_config_rgb565){
> nit: space missing in between ')' and '{'

@Siva:Done
> 
>
https://codereview.chromium.org/88033002/diff/100001/content/common/gpu/clien...
> content/common/gpu/client/gl_helper.cc:422: GL_RGB, GL_UNSIGNED_SHORT_5_6_5,
> NULL);
> nit: indent here and in 425.

@Siva:Done

> 
>
https://codereview.chromium.org/88033002/diff/100001/content/common/gpu/clien...
> content/common/gpu/client/gl_helper.cc:794: const int test_size = 100;
> Can you use something power of two?

@Siva:Done
> 
>
https://codereview.chromium.org/88033002/diff/100001/content/common/gpu/clien...
> content/common/gpu/client/gl_helper.cc:797:
context_->texImage2D(GL_TEXTURE_2D,
> nit: indent

@Siva:Done
> 
>
https://codereview.chromium.org/88033002/diff/100001/content/common/gpu/clien...
> content/common/gpu/client/gl_helper.cc:805: NULL);
> The texture is incomplete because the default min_filter is
> NEAREST_MIPMAP_LINEAR but you didn't define mipmaps.
> 
> I'd just set MIN_/MAG_FILTER to linear (and also set WRAP_S/_T to
CLAMP_TO_EDGE)
> here.

@Siva:Done

[sivag, 2014-01-09 14:51:49]

PTAL..
I am preparing this test, will add that.

https://codereview.chromium.org/88033002/diff/100001/content/browser/android/...
File content/browser/android/content_view_core_impl.h (right):

https://codereview.chromium.org/88033002/diff/100001/content/browser/android/...
content/browser/android/content_view_core_impl.h:313: bool
readback_config_rgb565);
On 2014/01/07 16:26:02, sievers wrote:
> Adding this here is unnecessary since ContentViewCoreImpl does not make the
API
> more public than RWHV. It'd have to be virtual and exposed in
> content/public/browser/android/content_view_core.h also.
> 
> Maybe we can sort out how this should be exposed to the embedder in a separate
> patch?
Removed.

https://codereview.chromium.org/88033002/diff/100001/content/browser/android/...
content/browser/android/content_view_core_impl.h:314: bool
IsReadBackConfig565();
On 2014/01/07 16:26:02, sievers wrote:
> Same for this. It should be in the same place and exposed to the embedder as
the
> above.
> 
> Maybe the requested format should even be added as an argument to
> CopyFromCompositingSurface().

Removed.Argument added to CopyFromCompositingSurface.

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

https://codereview.chromium.org/88033002/diff/100001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:599:
if(readback_config_rgb565 && !gl_helper->CanUseRgb565Readback()){
On 2014/01/07 16:26:02, sievers wrote:
> Can we check CanUseRgb565Readback() only once? It's not necessarily cheap
> because it allocates memory and implies a synchronous GL call. Maybe a static
> bool is good enough here.

Done.

https://codereview.chromium.org/88033002/diff/100001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:1341:
bitmap->setConfig(SkBitmap::kRGB_565_Config,
On 2014/01/07 16:26:02, sievers wrote:
> nit: no need for if-else, can do 'readback_config_rgb565 ? : ... : ...'
Done.

https://codereview.chromium.org/88033002/diff/100001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:1347:
dst_size_in_pixel.width(), dst_size_in_pixel.height(),
On 2014/01/07 16:26:02, sievers wrote:
> nit: indent here and above
Done.

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

https://codereview.chromium.org/88033002/diff/100001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.cc:310: bool  readback_config_rgb565,
On 2014/01/07 16:26:02, sievers wrote:
> nit: two spaces

DOne.

https://codereview.chromium.org/88033002/diff/100001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.cc:314: void ReadbackDone(Request
*request,int bytes_per_pixel);
On 2014/01/07 16:26:02, sievers wrote:
> nit: space after comma missing
Done.

https://codereview.chromium.org/88033002/diff/100001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.cc:357: bool  readback_config_rgb565,
On 2014/01/07 16:26:02, sievers wrote:
> nit: two spaces
Done

https://codereview.chromium.org/88033002/diff/100001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.cc:370: if(readback_config_rgb565){
On 2014/01/07 16:26:02, sievers wrote:
> nit: space missing

Done.

https://codereview.chromium.org/88033002/diff/100001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.cc:380: }else{
On 2014/01/07 16:26:02, sievers wrote:
> nit: spaces missing

Done.

https://codereview.chromium.org/88033002/diff/100001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.cc:402: const base::Callback<void(bool)>&
callback) {
On 2014/01/07 16:26:02, sievers wrote:
> Can you add a check for GL_IMPLEMENTATION_COLOR_READ_FORMAT like you do below
> here and fail this with a logged error if 565 is not supported?

Is this needed as we are already doing the check and failing it
in the begining of copyfromcomposiitingsurface( in
renderer_widget_host_view_anddroid.cc

https://codereview.chromium.org/88033002/diff/100001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.cc:420: if(readback_config_rgb565){
On 2014/01/07 16:26:02, sievers wrote:
> nit: space missing in between ')' and '{'

Done.

https://codereview.chromium.org/88033002/diff/100001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.cc:422: GL_RGB, GL_UNSIGNED_SHORT_5_6_5,
NULL);
On 2014/01/07 16:26:02, sievers wrote:
> nit: indent here and in 425. 

Done.

https://codereview.chromium.org/88033002/diff/100001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.cc:794: const int test_size = 100;
On 2014/01/07 16:26:02, sievers wrote:
> Can you use something power of two? 

Done.

https://codereview.chromium.org/88033002/diff/100001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.cc:797: context_->texImage2D(GL_TEXTURE_2D,
On 2014/01/07 16:26:02, sievers wrote:
> nit: indent
Done.

https://codereview.chromium.org/88033002/diff/100001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.cc:805: NULL);
On 2014/01/07 16:26:02, sievers wrote:
> The texture is incomplete because the default min_filter is
> NEAREST_MIPMAP_LINEAR but you didn't define mipmaps.
> 
> I'd just set MIN_/MAG_FILTER to linear (and also set WRAP_S/_T to
CLAMP_TO_EDGE)
> here.
Done.

[no sievers, 2014-01-09 14:58:52]

On 2014/01/09 14:51:49, Sikugu wrote:
> PTAL..
> I am preparing this test, will add that.
> 

Thanks! Otherwise looks good.

+piman for the CopyFromCompositingSurface() argument change.
Maybe this should be a format enum?


https://codereview.chromium.org/88033002/diff/100001/content/common/gpu/clien...
> content/common/gpu/client/gl_helper.cc:402: const base::Callback<void(bool)>&
> callback) {
> On 2014/01/07 16:26:02, sievers wrote:
> > Can you add a check for GL_IMPLEMENTATION_COLOR_READ_FORMAT like you do
below
> > here and fail this with a logged error if 565 is not supported?
> 
> Is this needed as we are already doing the check and failing it
> in the begining of copyfromcomposiitingsurface( in
> renderer_widget_host_view_anddroid.cc
> 

Sounds fine. Actually the spec sounds like the answer to
GL_IMPLEMENTATION_COLOR_READ_FORMAT (for the second non-RGBA8 format) should
only vary depending on the bound framebuffer's format, so the global test should
be good.

https://codereview.chromium.org/88033002/diff/310001/content/browser/android/...
File content/browser/android/content_view_core_impl.cc (left):

https://codereview.chromium.org/88033002/diff/310001/content/browser/android/...
content/browser/android/content_view_core_impl.cc:1298: 
nit: whitespace change

https://codereview.chromium.org/88033002/diff/310001/content/browser/android/...
File content/browser/android/content_view_core_impl.h (left):

https://codereview.chromium.org/88033002/diff/310001/content/browser/android/...
content/browser/android/content_view_core_impl.h:320: 
nit: whitespace change

[piman, 2014-01-09 18:12:14]

Can we add a test for the 565 path?

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

https://codereview.chromium.org/88033002/diff/310001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:618: static
bool check_rgb565_support = gl_helper->CanUseRgb565Readback();
Why static?
Maybe you want to cache this in the GLHelper.

https://codereview.chromium.org/88033002/diff/310001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:619:
if(readback_config_rgb565 && !check_rgb565_support){
nit: space before ( and after )

https://codereview.chromium.org/88033002/diff/310001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/88033002/diff/310001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:1813:
PrepareTextureCopyOutputResult(dst_size_in_pixel, false,
Well, it sounds like readback_config_rgb565 should be passed through.

https://codereview.chromium.org/88033002/diff/310001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_gtk.cc (right):

https://codereview.chromium.org/88033002/diff/310001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_gtk.cc:1051: bool
readback_config_rgb565) {
Fail if readback_config_rgb565 is true?

https://codereview.chromium.org/88033002/diff/310001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_mac.h (right):

https://codereview.chromium.org/88033002/diff/310001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_mac.h:279: bool
readback_config_rgb565) OVERRIDE;
There should be a corresponding change in the .mm

https://codereview.chromium.org/88033002/diff/310001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

https://codereview.chromium.org/88033002/diff/310001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_win.cc:864: bool
readback_config_rgb565) {
Fail if readback_config_rgb565 is true?

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

https://codereview.chromium.org/88033002/diff/310001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.cc:760: }
nit: add empty line

https://codereview.chromium.org/88033002/diff/310001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.cc:762: const int test_size = 64;
nit: kTestSize

https://codereview.chromium.org/88033002/diff/310001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.cc:791: return ((ext_format == GL_RGB) &&
(ext_type == GL_UNSIGNED_SHORT_5_6_5));
nit: no need for outer ()

https://codereview.chromium.org/88033002/diff/310001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.cc:792: }
nit: add empty line

[sivag, 2014-01-10 12:20:00]

PTAL...

The unit test needs the async readback to be tested,
When i checked there is no test for the async read for ARGB8888 as well, so this
will cover both the formats.
This is a WIP will update on this.

Do we need to support the enum for all ports? Please advice.

https://codereview.chromium.org/88033002/diff/310001/content/browser/android/...
File content/browser/android/content_view_core_impl.cc (left):

https://codereview.chromium.org/88033002/diff/310001/content/browser/android/...
content/browser/android/content_view_core_impl.cc:1298: 
On 2014/01/09 14:58:53, sievers wrote:
> nit: whitespace change

Done.

https://codereview.chromium.org/88033002/diff/310001/content/browser/android/...
File content/browser/android/content_view_core_impl.h (left):

https://codereview.chromium.org/88033002/diff/310001/content/browser/android/...
content/browser/android/content_view_core_impl.h:320: 
On 2014/01/09 14:58:53, sievers wrote:
> nit: whitespace change

Done.

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

https://codereview.chromium.org/88033002/diff/310001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:618: static
bool check_rgb565_support = gl_helper->CanUseRgb565Readback();
On 2014/01/09 18:12:15, piman wrote:
> Why static?
> Maybe you want to cache this in the GLHelper.

Done.
Cached the 565 support status using member variables.

https://codereview.chromium.org/88033002/diff/310001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:619:
if(readback_config_rgb565 && !check_rgb565_support){
On 2014/01/09 18:12:15, piman wrote:
> nit: space before ( and after )
Done

https://codereview.chromium.org/88033002/diff/310001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/88033002/diff/310001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:1813:
PrepareTextureCopyOutputResult(dst_size_in_pixel, false,
On 2014/01/09 18:12:15, piman wrote:
> Well, it sounds like readback_config_rgb565 should be passed through.

Do you mean add this support to CopyFromCompositingSurfaceHasResult?

https://codereview.chromium.org/88033002/diff/310001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_gtk.cc (right):

https://codereview.chromium.org/88033002/diff/310001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_gtk.cc:1051: bool
readback_config_rgb565) {
On 2014/01/09 18:12:15, piman wrote:
> Fail if readback_config_rgb565 is true?
Done.

https://codereview.chromium.org/88033002/diff/310001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_mac.h (right):

https://codereview.chromium.org/88033002/diff/310001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_mac.h:279: bool
readback_config_rgb565) OVERRIDE;
On 2014/01/09 18:12:15, piman wrote:
> There should be a corresponding change in the .mm
Done.

https://codereview.chromium.org/88033002/diff/310001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

https://codereview.chromium.org/88033002/diff/310001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_win.cc:864: bool
readback_config_rgb565) {
On 2014/01/09 18:12:15, piman wrote:
> Fail if readback_config_rgb565 is true?

Done.

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

https://codereview.chromium.org/88033002/diff/310001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.cc:760: }
On 2014/01/09 18:12:15, piman wrote:
> nit: add empty line

Done.

https://codereview.chromium.org/88033002/diff/310001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.cc:762: const int test_size = 64;
On 2014/01/09 18:12:15, piman wrote:
> nit: kTestSize

Done.

https://codereview.chromium.org/88033002/diff/310001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.cc:791: return ((ext_format == GL_RGB) &&
(ext_type == GL_UNSIGNED_SHORT_5_6_5));
On 2014/01/09 18:12:15, piman wrote:
> nit: no need for outer ()

Done.

[piman, 2014-01-10 22:38:32]

There's no guarantee that 565 readback is supported by drivers, so all callers
should handle failure and retry with 8888. Therefore, it's probably ok to have
Gtk/Win/Mac to fail 565. In any case, Gtk/Win will go away soon, being replaced
by Aura. On Mac there is work in progress to make it more like Aura too (if not
full fledge), and at that point we can pick up the logic using GLHelper.

https://codereview.chromium.org/88033002/diff/310001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/88033002/diff/310001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:1813:
PrepareTextureCopyOutputResult(dst_size_in_pixel, false,
On 2014/01/10 12:20:01, Sikugu wrote:
> On 2014/01/09 18:12:15, piman wrote:
> > Well, it sounds like readback_config_rgb565 should be passed through.
> 
> Do you mean add this support to CopyFromCompositingSurfaceHasResult?

Right... in CopyFromCompositingSurface the parameter is passed, but currently
it's dropped on the floor and when we get here, we hardcode to RGBA8, so the
caller gets a RGBA8 result when it asked for RGB565.

CopyFromCompositingSurface needs to pass readback_config_rgb565 to here (with
the Bind), so that we can give it to PrepareTextureCopyOutputResult

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

https://codereview.chromium.org/88033002/diff/450001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.cc:798: if((ext_format == GL_RGB) &&
(ext_type == GL_UNSIGNED_SHORT_5_6_5)){
nit: space before ( and after )

https://codereview.chromium.org/88033002/diff/450001/content/common/gpu/clien...
File content/common/gpu/client/gl_helper_unittests.cc (right):

https://codereview.chromium.org/88033002/diff/450001/content/common/gpu/clien...
content/common/gpu/client/gl_helper_unittests.cc:891: DCHECK((bitmap_config ==
SkBitmap::kRGB_565_Config)||
nit: space before ||

https://codereview.chromium.org/88033002/diff/450001/content/common/gpu/clien...
content/common/gpu/client/gl_helper_unittests.cc:892: (bitmap_config ==
SkBitmap::kARGB_8888_Config));
nit: indent (1 more space)

https://codereview.chromium.org/88033002/diff/450001/content/common/gpu/clien...
content/common/gpu/client/gl_helper_unittests.cc:896: if(rgb565_format &&
!helper_->CanUseRgb565Readback())
nit: space between if and (

https://codereview.chromium.org/88033002/diff/450001/content/common/gpu/clien...
content/common/gpu/client/gl_helper_unittests.cc:897: {
nit: on previous line

[sivag, 2014-01-15 15:24:13]

PTAL....

https://codereview.chromium.org/88033002/diff/310001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/88033002/diff/310001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:1813:
PrepareTextureCopyOutputResult(dst_size_in_pixel, false,
On 2014/01/09 18:12:15, piman wrote:
> Well, it sounds like readback_config_rgb565 should be passed through.

Done.

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

https://codereview.chromium.org/88033002/diff/450001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.cc:798: if((ext_format == GL_RGB) &&
(ext_type == GL_UNSIGNED_SHORT_5_6_5)){
On 2014/01/10 22:38:33, piman wrote:
> nit: space before ( and after )

Done.

https://codereview.chromium.org/88033002/diff/450001/content/common/gpu/clien...
File content/common/gpu/client/gl_helper_unittests.cc (right):

https://codereview.chromium.org/88033002/diff/450001/content/common/gpu/clien...
content/common/gpu/client/gl_helper_unittests.cc:891: DCHECK((bitmap_config ==
SkBitmap::kRGB_565_Config)||
On 2014/01/10 22:38:33, piman wrote:
> nit: space before ||

Done.

https://codereview.chromium.org/88033002/diff/450001/content/common/gpu/clien...
content/common/gpu/client/gl_helper_unittests.cc:892: (bitmap_config ==
SkBitmap::kARGB_8888_Config));
On 2014/01/10 22:38:33, piman wrote:
> nit: indent (1 more space)

Done.

https://codereview.chromium.org/88033002/diff/450001/content/common/gpu/clien...
content/common/gpu/client/gl_helper_unittests.cc:896: if(rgb565_format &&
!helper_->CanUseRgb565Readback())
On 2014/01/10 22:38:33, piman wrote:
> nit: space between if and (

Done.

https://codereview.chromium.org/88033002/diff/450001/content/common/gpu/clien...
content/common/gpu/client/gl_helper_unittests.cc:897: {
On 2014/01/10 22:38:33, piman wrote:
> nit: on previous line

Done.

[piman, 2014-01-15 21:32:52]

Last few things in the test.

https://codereview.chromium.org/88033002/diff/690001/content/common/gpu/clien...
File content/common/gpu/client/gl_helper_unittests.cc (right):

https://codereview.chromium.org/88033002/diff/690001/content/common/gpu/clien...
content/common/gpu/client/gl_helper_unittests.cc:848: return std::abs(c1 - c2)
<= 40;
nit: 40 is arbitrary. It also seems high...
The right value probably depends on the config (565 has less resolution, so
needs more wiggle room than 8888). Mind passing it through from ColorClose, and
adjust as needed?

Given the test, for 8888 we should really have an exact result. For 565 we
should be within the precision difference, i.e. 3 bits, i.e. 7.

https://codereview.chromium.org/88033002/diff/690001/content/common/gpu/clien...
content/common/gpu/client/gl_helper_unittests.cc:859: SkColorGetB(color2));
SkColorGetA ?

https://codereview.chromium.org/88033002/diff/690001/content/common/gpu/clien...
content/common/gpu/client/gl_helper_unittests.cc:861: return  red && blue &&
green;
nit: You have 2 spaces before 'red'. Remove one.

https://codereview.chromium.org/88033002/diff/690001/content/common/gpu/clien...
content/common/gpu/client/gl_helper_unittests.cc:902: true :
nit: no need for ? true : false. (bitmap_config == SkBitmap::kRGB_565_Config) is
already a boolean.

https://codereview.chromium.org/88033002/diff/690001/content/common/gpu/clien...
content/common/gpu/client/gl_helper_unittests.cc:934: output_pixels.eraseARGB(0,
0, 255, 0);
Can you add an explanation for why this doesn't match the color set in
input_pixels?

[sivag, 2014-01-16 11:50:58]

PTAL...

https://codereview.chromium.org/88033002/diff/690001/content/common/gpu/clien...
File content/common/gpu/client/gl_helper_unittests.cc (right):

https://codereview.chromium.org/88033002/diff/690001/content/common/gpu/clien...
content/common/gpu/client/gl_helper_unittests.cc:848: return std::abs(c1 - c2)
<= 40;
On 2014/01/15 21:32:53, piman wrote:
> nit: 40 is arbitrary. It also seems high...
> The right value probably depends on the config (565 has less resolution, so
> needs more wiggle room than 8888). Mind passing it through from ColorClose,
and
> adjust as needed?
> 
> Given the test, for 8888 we should really have an exact result. For 565 we
> should be within the precision difference, i.e. 3 bits, i.e. 7.

Done.

https://codereview.chromium.org/88033002/diff/690001/content/common/gpu/clien...
content/common/gpu/client/gl_helper_unittests.cc:859: SkColorGetB(color2));
On 2014/01/15 21:32:53, piman wrote:
> SkColorGetA ?

Done.

https://codereview.chromium.org/88033002/diff/690001/content/common/gpu/clien...
content/common/gpu/client/gl_helper_unittests.cc:861: return  red && blue &&
green;
On 2014/01/15 21:32:53, piman wrote:
> nit: You have 2 spaces before 'red'. Remove one.

Done.

https://codereview.chromium.org/88033002/diff/690001/content/common/gpu/clien...
content/common/gpu/client/gl_helper_unittests.cc:902: true :
On 2014/01/15 21:32:53, piman wrote:
> nit: no need for ? true : false. (bitmap_config == SkBitmap::kRGB_565_Config)
is
> already a boolean.

Done.

https://codereview.chromium.org/88033002/diff/690001/content/common/gpu/clien...
content/common/gpu/client/gl_helper_unittests.cc:934: output_pixels.eraseARGB(0,
0, 255, 0);
On 2014/01/15 21:32:53, piman wrote:
> Can you add an explanation for why this doesn't match the color set in
> input_pixels?

Done.

[piman, 2014-01-16 19:31:59]

lgtm

[commit-bot: I haz the power, 2014-01-17 02:13:49]

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

[commit-bot: I haz the power, 2014-01-17 02:14:01]

Failed to apply patch for content/common/gpu/client/gl_helper_unittests.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: content/common/gpu/client/gl_helper_unittests.cc
  |diff --git a/content/common/gpu/client/gl_helper_unittests.cc
b/content/common/gpu/client/gl_helper_unittests.cc
  |index
5abeabd260096ffc88cf4bdd0bf1b1ce1ec672d1..a42f46a936a0813f8061e6b3ecb92761e7ffc2e2
100644
  |--- a/content/common/gpu/client/gl_helper_unittests.cc
  |+++ b/content/common/gpu/client/gl_helper_unittests.cc
  --------------------------
  No file to patch.  Skipping patch.
  3 out of 3 hunks ignored

Patch:       content/common/gpu/client/gl_helper_unittests.cc
Index: content/common/gpu/client/gl_helper_unittests.cc
diff --git a/content/common/gpu/client/gl_helper_unittests.cc
b/content/common/gpu/client/gl_helper_unittests.cc
index
5abeabd260096ffc88cf4bdd0bf1b1ce1ec672d1..a42f46a936a0813f8061e6b3ecb92761e7ffc2e2
100644
--- a/content/common/gpu/client/gl_helper_unittests.cc
+++ b/content/common/gpu/client/gl_helper_unittests.cc
@@ -662,7 +662,8 @@ class GLHelperTest : public testing::Test {
     helper_->ReadbackTextureSync(
         dst_texture,
         gfx::Rect(0, 0, scaled_xsize, scaled_ysize),
-        static_cast<unsigned char*>(output_pixels.getPixels()));
+        static_cast<unsigned char*>(output_pixels.getPixels()),
+        SkBitmap::kARGB_8888_Config);
     if (flip) {
       // Flip the pixels back.
       FlipSKBitmap(&output_pixels);
@@ -841,6 +842,127 @@ class GLHelperTest : public testing::Test {
     }
   }
 
+  bool ColorComponentsClose(SkColor component1,
+                            SkColor component2,
+                            SkBitmap::Config config) {
+    int c1 = static_cast<int>(component1);
+    int c2 = static_cast<int>(component2);
+    bool result = false;
+    switch (config) {
+      case SkBitmap::kARGB_8888_Config:
+        result = (std::abs(c1 - c2) == 0);
+        break;
+      case SkBitmap::kRGB_565_Config:
+        result = (std::abs(c1 - c2) <= 7);
+        break;
+      default:
+        break;
+    }
+    return result;
+  }
+
+  bool ColorsClose(SkColor color1, SkColor color2, SkBitmap::Config config) {
+    bool red = ColorComponentsClose(SkColorGetR(color1),
+                                    SkColorGetR(color2), config);
+    bool green = ColorComponentsClose(SkColorGetG(color1),
+                                        SkColorGetG(color2), config);
+    bool blue = ColorComponentsClose(SkColorGetB(color1),
+                                     SkColorGetB(color2), config);
+    bool alpha = ColorComponentsClose(SkColorGetA(color1),
+                                      SkColorGetA(color2), config);
+    if (config == SkBitmap::kRGB_565_Config) {
+      return red && blue && green;
+    }
+    return red && blue && green && alpha;
+  }
+
+  bool IsEqual(const SkBitmap& bmp1, const SkBitmap& bmp2) {
+    if (bmp1.isNull() && bmp2.isNull())
+      return true;
+    if (bmp1.width() != bmp2.width() ||
+        bmp1.height() != bmp2.height()) {
+        LOG(ERROR) << "Bitmap geometry check failure";
+        return false;
+    }
+    if (bmp1.getConfig() != bmp2.getConfig())
+      return false;
+
+    SkAutoLockPixels lock1(bmp1);
+    SkAutoLockPixels lock2(bmp2);
+    if (!bmp1.getPixels() || !bmp2.getPixels()) {
+      LOG(ERROR) << "Empty Bitmap!";
+      return false;
+    }
+    for (int y = 0; y < bmp1.height(); ++y) {
+      for (int x = 0; x < bmp1.width(); ++x) {
+        if (!ColorsClose(bmp1.getColor(x,y),
+                         bmp2.getColor(x,y),
+                         bmp1.getConfig())) {
+          LOG(ERROR) << "Bitmap color comparision failure";
+          return false;
+        }
+      }
+    }
+    return true;
+  }
+
+  // Test basic format readback.
+  bool TestTextureFormatReadback(const gfx::Size& src_size,
+                         SkBitmap::Config bitmap_config) {
+    DCHECK((bitmap_config == SkBitmap::kRGB_565_Config) ||
+           (bitmap_config == SkBitmap::kARGB_8888_Config));
+    bool rgb565_format = (bitmap_config == SkBitmap::kRGB_565_Config);
+    if (rgb565_format && !helper_->CanUseRgb565Readback()) {
+      LOG(ERROR) << "RGB565 Format Not supported on this platform";
+      return false;
+    }
+    WebGLId src_texture = context_->createTexture();
+    SkBitmap input_pixels;
+    input_pixels.setConfig(bitmap_config, src_size.width(),
+                           src_size.height());
+    input_pixels.allocPixels();
+    SkAutoLockPixels lock1(input_pixels);
+    // Erase the input bitmap with red color.
+    input_pixels.eraseColor(SK_ColorRED);
+    context_->bindTexture(GL_TEXTURE_2D, src_texture);
+    GLenum format = (bitmap_config == SkBitmap::kRGB_565_Config) ?
+                    GL_RGB : GL_RGBA;
+    GLenum type = (bitmap_config == SkBitmap::kRGB_565_Config) ?
+                  GL_UNSIGNED_SHORT_5_6_5 : GL_UNSIGNED_BYTE;
+    context_->texImage2D(GL_TEXTURE_2D,
+                         0,
+                         format,
+                         src_size.width(),
+                         src_size.height(),
+                         0,
+                         format,
+                         type,
+                         input_pixels.getPixels());
+    SkBitmap output_pixels;
+    output_pixels.setConfig(bitmap_config, src_size.width(),
+                           src_size.height());
+    output_pixels.allocPixels();
+    SkAutoLockPixels lock2(output_pixels);
+    // Initialize the output bitmap with Green color.
+    // When the readback is over output bitmap should have the red color.
+    output_pixels.eraseColor(SK_ColorGREEN);
+    uint8* pixels = static_cast<uint8*>(output_pixels.getPixels());
+    helper_->ReadbackTextureSync(src_texture,
+                                 gfx::Rect(src_size),
+                                 pixels,
+                                 bitmap_config);
+    bool result = IsEqual(input_pixels, output_pixels);
+    if (!result) {
+      LOG(ERROR) << "Bitmap comparision failure";
+      return false;
+    }
+    context_->deleteTexture(src_texture);
+    if (HasFailure()) {
+      return false;
+    }
+    return true;
+  }
+
   // YUV readback test. Create a test pattern, convert to YUV
   // with reference implementation and compare to what gl_helper
   // returns.
@@ -1216,6 +1338,20 @@ class GLHelperTest : public testing::Test {
   std::deque<GLHelperScaling::ScaleOp> x_ops_, y_ops_;
 };
 
+TEST_F(GLHelperTest, RGBAReadBackTest) {
+  const int kTestSize = 64;
+  bool result = TestTextureFormatReadback(gfx::Size(kTestSize,kTestSize),
+                                          SkBitmap::kARGB_8888_Config);
+  EXPECT_EQ(result, true);
+}
+
+TEST_F(GLHelperTest, RGB565ReadBackTest) {
+  const int kTestSize = 64;
+  bool result = TestTextureFormatReadback(gfx::Size(kTestSize,kTestSize),
+                                          SkBitmap::kRGB_565_Config);
+  EXPECT_EQ(result, true);
+}
+
 TEST_F(GLHelperTest, YUVReadbackOptTest) {
   // This test uses the cb_command tracing events to detect how many
   // scaling passes are actually performed by the YUV readback pipeline.

[commit-bot: I haz the power, 2014-01-17 04:28:27]

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

[commit-bot: I haz the power, 2014-01-17 06:16:24]

Change committed as 245431

[Mads Ager (chromium), 2014-01-17 10:00:27]

A revert of this CL has been created in
https://codereview.chromium.org/137783022/ by ager@chromium.org.

The reason for reverting is: content_gl_tests failing on multiple gpu bots on
the chromium.webkit buildbot.

Example:
http://chromegw.corp.google.com/i/chromium.webkit/builders/GPU%20Mac10.7/buil....

[sivag, 2014-01-17 11:06:11]

On 2014/01/17 10:00:27, Mads Ager (chromium) wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/137783022/ by mailto:ager@chromium.org.
> 
> The reason for reverting is: content_gl_tests failing on multiple gpu bots on
> the chromium.webkit buildbot.
> 
> Example:
>
http://chromegw.corp.google.com/i/chromium.webkit/builders/GPU%2520Mac10.7/bu....

The reason why this failed is, if some hardware doesnt support rgb565 format we
are detecting it prior to running the test
here i am failing it intentionally, i should have skipped the test itself if
there is no support.

Will upload with that change.
Sorry for the trouble.

[sivag, 2014-01-17 11:07:24]

The reason why this failed is, if some hardware doesnt support rgb565 format we
are detecting it prior to running the test
here i am failing it intentionally, i should have skipped the test itself if
there is no support.

Will upload with that change in a new issue, as this is closed.
Sorry for the trouble.