Use GLHelper for glReadPixels and format detection

Use GLHelper for glReadPixels and format detection

GLHelper has functionality for testing for appropriate GL readback
format support, caching this information where appropriate. This change
switches RendererGpuVideoAcceleratorFactories over to using that code
for supported format identification and glReadPixels in the desired
format. The code continues to do a software swizzle if necessary.

TEST=WebGL conformance tests
BUG=None

Committed: https://crrev.com/729d1b4ab86eeb1e095534449e442edee16d524e
Cr-Commit-Position: refs/heads/master@{#293358}

[robert.bradford, 2014-08-08 14:51:33]

piman@ please could you take a look, this change was inspired by your feedback
in https://codereview.chromium.org/316553002

[piman, 2014-08-08 21:02:07]

https://codereview.chromium.org/456823002/diff/1/content/renderer/media/rende...
File content/renderer/media/renderer_gpu_video_accelerator_factories.cc (right):

https://codereview.chromium.org/456823002/diff/1/content/renderer/media/rende...
content/renderer/media/renderer_gpu_video_accelerator_factories.cc:66:
context_provider_ = NULL;
nit: it would be good to reset the gl_helper_ here.

https://codereview.chromium.org/456823002/diff/1/content/renderer/media/rende...
content/renderer/media/renderer_gpu_video_accelerator_factories.cc:214:
pixels_ptr[i] = (b << 0) | (g << 8) | (r << 16) | (a << 24);
nit: why not keep the SK_*_SHIFT here? That way it works for any layout.

[robert.bradford, 2014-08-08 21:32:31]

https://codereview.chromium.org/456823002/diff/1/content/renderer/media/rende...
File content/renderer/media/renderer_gpu_video_accelerator_factories.cc (right):

https://codereview.chromium.org/456823002/diff/1/content/renderer/media/rende...
content/renderer/media/renderer_gpu_video_accelerator_factories.cc:66:
context_provider_ = NULL;
On 2014/08/08 21:02:06, piman (slow to review) wrote:
> nit: it would be good to reset the gl_helper_ here.

Acknowledged.

https://codereview.chromium.org/456823002/diff/1/content/renderer/media/rende...
content/renderer/media/renderer_gpu_video_accelerator_factories.cc:214:
pixels_ptr[i] = (b << 0) | (g << 8) | (r << 16) | (a << 24);
On 2014/08/08 21:02:06, piman (slow to review) wrote:
> nit: why not keep the SK_*_SHIFT here? That way it works for any layout.

Those macros, AFAICT, represent the native representation which may not map to
the format of the bitmap. We know we need to just swap B & R so I think this is
safer.

[piman, 2014-08-08 21:57:12]

https://codereview.chromium.org/456823002/diff/1/content/renderer/media/rende...
File content/renderer/media/renderer_gpu_video_accelerator_factories.cc (right):

https://codereview.chromium.org/456823002/diff/1/content/renderer/media/rende...
content/renderer/media/renderer_gpu_video_accelerator_factories.cc:214:
pixels_ptr[i] = (b << 0) | (g << 8) | (r << 16) | (a << 24);
On 2014/08/08 21:32:31, robert.bradford wrote:
> On 2014/08/08 21:02:06, piman (slow to review) wrote:
> > nit: why not keep the SK_*_SHIFT here? That way it works for any layout.
> 
> Those macros, AFAICT, represent the native representation which may not map to
> the format of the bitmap. We know we need to just swap B & R so I think this
is
> safer.

How do we know that?

We can check that the format is kN32_SkColorType and fail if it isn't. Then
whatever platform, it will be correct to unpack the colors from RGBA (the format
that GL gives us) and repack them with the SK_*_SHIFTs into what Skia expects.

[robert.bradford, 2014-08-11 10:43:58]

> We can check that the format is kN32_SkColorType and fail if it isn't. Then
> whatever platform, it will be correct to unpack the colors from RGBA (the
format
> that GL gives us) and repack them with the SK_*_SHIFTs into what Skia expects.

Splendid idea and integrated into the latest version of my patch.

[oetuaho-nv, 2014-08-11 14:56:41]

I like the basic idea of the patch, but there's still a few issues.

https://codereview.chromium.org/456823002/diff/20001/content/renderer/media/r...
File content/renderer/media/renderer_gpu_video_accelerator_factories.cc (left):

https://codereview.chromium.org/456823002/diff/20001/content/renderer/media/r...
content/renderer/media/renderer_gpu_video_accelerator_factories.cc:195:
GL_TEXTURE_2D, texture_id, tmp_texture, 0, GL_RGBA, GL_UNSIGNED_BYTE);
I don't think that the code can work without the copyTextureCHROMIUM part, which
you have removed from here. The texture might be external
(GL_TEXTURE_EXTERNAL_OES) in which case it is not possible to attach it directly
to a framebuffer.

https://codereview.chromium.org/456823002/diff/20001/content/renderer/media/r...
File content/renderer/media/renderer_gpu_video_accelerator_factories.h (right):

https://codereview.chromium.org/456823002/diff/20001/content/renderer/media/r...
content/renderer/media/renderer_gpu_video_accelerator_factories.h:16: #include
"content/common/gpu/client/gl_helper.h"
It seems it would be enough to declare GLHelper here and have the #include only
in the .cc file.

[robert.bradford, 2014-09-04 10:45:39]

On 2014/08/11 14:56:41, oetuaho-nv wrote:
> I like the basic idea of the patch, but there's still a few issues.

Hi, I integrated both your items of feedback. I did look at possibly passing
through the texture target from the callers of this function (to do the
copyTextureCHROMIUM conditionally) but from the comments it was obvious that the
expectation is that this is going to be a GL_TEXTURE_EXTERNAL_OES.

[piman, 2014-09-04 17:41:59]

lgtm

[robert.bradford, 2014-09-04 18:29:36]

The CQ bit was checked by robert.bradford@intel.com

[commit-bot: I haz the power, 2014-09-04 18:31:37]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/robert.bradford@intel.com/456823002/40001

[commit-bot: I haz the power, 2014-09-04 22:24:32]

Committed patchset #3 (id:40001) as c00d8cf2a0fa4489b75147dffe9bebe006f0af40

[commit-bot: I haz the power, 2014-09-10 03:33:21]

Patchset 3 (id:??) landed as
https://crrev.com/729d1b4ab86eeb1e095534449e442edee16d524e
Cr-Commit-Position: refs/heads/master@{#293358}