Fix the issue that it may get wrong stride in cc::ResourceProvider::MapImage.

Adjusts the call sequences in cc::ResourceProvider::MapImage to map image first and then call GetStride.

Current call sequences may get wrong stride for the on-going SurfaceTexture-based GpuMemoryBuffer for Android.

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

[junj, 2014-02-28 07:51:45]

It is a small fix for the regression introduced in recent refactoring.

reveman, please help to take a look. Thanks.

[reveman, 2014-03-01 23:04:10]

I understand that you want to change this for future SurfaceTexture support but
why does the description say that this is currently an issue? Does this actually
fix an existing issue? Please explain more.

https://codereview.chromium.org/184233002/diff/1/cc/resources/resource_provid...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/184233002/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider.cc:2153: uint8_t* ret = static_cast<uint8_t*>(
nit: s/ret/pixels/

https://codereview.chromium.org/184233002/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider.cc:2156: resource->image_id,
GL_IMAGE_ROWBYTES_CHROMIUM, stride);
It's not obvious that this needs to be called after map. Please add a short
comment here explaining this and a DCHECK to
GLES2Implementation::GetImageParameterivCHROMIUMHelper to guard against
incorrect usage.

[junj, 2014-03-03 10:52:29]

Hi, reveman. Thanks for your comments and suggestions. 

The stride issue is expected to affect the on-going SurfaceTexture-based
GpuMemoryBuffer in Android and it won't affect other platforms like Mac OSX. I
have changed the description. Please take another look. Thanks.

https://codereview.chromium.org/184233002/diff/1/cc/resources/resource_provid...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/184233002/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider.cc:2153: uint8_t* ret = static_cast<uint8_t*>(
On 2014/03/01 23:04:10, reveman wrote:
> nit: s/ret/pixels/

Done.

https://codereview.chromium.org/184233002/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider.cc:2156: resource->image_id,
GL_IMAGE_ROWBYTES_CHROMIUM, stride);
On 2014/03/01 23:04:10, reveman wrote:
> It's not obvious that this needs to be called after map. Please add a short
> comment here explaining this and a DCHECK to
> GLES2Implementation::GetImageParameterivCHROMIUMHelper to guard against
> incorrect usage.

Done.

[reveman, 2014-03-03 16:57:47]

https://codereview.chromium.org/184233002/diff/40001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/184233002/diff/40001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:2155: // MapImage() first and then call
GetStride().
No need to leak details of the implementation into this code. "MapImageCHROMIUM
must be called prior to GetImageParameterivCHROMIUM." is enough.

https://codereview.chromium.org/184233002/diff/40001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/184233002/diff/40001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.cc:3913: // it maps image first
and then calls GetStride().
Same thing here. I would prefer no platform/implementation specific details
here.

Actually, I think this should generate a GL_INVALID_OPERATION instead of a
DCHECK failure as it's not an implementation detail but part of the spec.

[junj, 2014-03-04 11:11:23]

Hi, reveman. Thanks for your comments. Please help to take a look again.

https://codereview.chromium.org/184233002/diff/40001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/184233002/diff/40001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:2155: // MapImage() first and then call
GetStride().
On 2014/03/03 16:57:48, reveman wrote:
> No need to leak details of the implementation into this code.
"MapImageCHROMIUM
> must be called prior to GetImageParameterivCHROMIUM." is enough.

Done.

https://codereview.chromium.org/184233002/diff/40001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/184233002/diff/40001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.cc:3913: // it maps image first
and then calls GetStride().
On 2014/03/03 16:57:48, reveman wrote:
> Same thing here. I would prefer no platform/implementation specific details
> here.
> 
> Actually, I think this should generate a GL_INVALID_OPERATION instead of a
> DCHECK failure as it's not an implementation detail but part of the spec.

Done.

[reveman, 2014-03-04 15:17:31]

lgtm with nit.

+piman for owner review

https://codereview.chromium.org/184233002/diff/60001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/184233002/diff/60001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.cc:3913: "MapImageCHROMIUM should
be called prior to GetImageParameterivCHROMIUM");
nit: "not mapped" is enough. also please use "git cl format" to make sure this
is formatted correctly.

[junj, 2014-03-05 07:50:57]

reveman, thanks for your suggestions.

piman, Could you help to have a look ? Thanks.

https://codereview.chromium.org/184233002/diff/60001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/184233002/diff/60001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.cc:3913: "MapImageCHROMIUM should
be called prior to GetImageParameterivCHROMIUM");
On 2014/03/04 15:17:31, reveman wrote:
> nit: "not mapped" is enough. also please use "git cl format" to make sure this
> is formatted correctly.

Done.

[piman, 2014-03-05 18:52:57]

lgtm

[junj, 2014-03-06 01:29:15]

On 2014/03/05 18:52:57, piman wrote:
> lgtm

reveman and piman, thanks for reviewing the patch. Land it now.

[junj, 2014-03-06 01:29:30]

The CQ bit was checked by jun.a.jiang@chromium.org

[commit-bot: I haz the power, 2014-03-06 01:31:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jun.a.jiang@intel.com/184233002/80001

[commit-bot: I haz the power, 2014-03-06 02:58:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jun.a.jiang@intel.com/184233002/80001

[commit-bot: I haz the power, 2014-03-06 03:20:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jun.a.jiang@intel.com/184233002/80001

[commit-bot: I haz the power, 2014-03-06 06:50:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-06 06:50:31]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[junj, 2014-03-06 08:49:56]

The CQ bit was checked by jun.a.jiang@chromium.org

[commit-bot: I haz the power, 2014-03-06 08:50:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jun.a.jiang@intel.com/184233002/80001

[commit-bot: I haz the power, 2014-03-06 14:00:46]

Change committed as 255330