cc: TextureUploader - Reset GL_UNPACK_ALIGNMENT to 4 before uploading images.

cc: TextureUploader - Reset GL_UNPACK_ALIGNMENT after Gpu Raster.

TextureUploader assumes GL_UNPACK_ALIGNMENT is 4 for performance reasons
however Ganesh may modify GL_UNPACK_ALIGNMENT.

Explicitly reset GL_UNPACK_ALIGNMENT to 4 after Gpu Raster.

BUG=442966
Committed: https://crrev.com/188a15c6786087d9be2b0fd92ef05d73b3e8fb27
Cr-Commit-Position: refs/heads/master@{#309653}

[vmiura, 2014-12-19 19:54:24]

vmiura@chromium.org changed reviewers:
+ danakj@chromium.org, reveman@chromium.org

[vmiura, 2014-12-19 19:54:24]

danakj@chromium.org: Please review all changes.
reveman@chromium.org: Please review all changes.

[danakj, 2014-12-19 20:00:46]

https://codereview.chromium.org/817673004/diff/1/cc/resources/texture_uploade...
File cc/resources/texture_uploader.cc (right):

https://codereview.chromium.org/817673004/diff/1/cc/resources/texture_uploade...
cc/resources/texture_uploader.cc:254: gl_->PixelStorei(GL_UNPACK_ALIGNMENT, 4);
Should we do this when we take control of the context back from ganesh instead
of doing it on every upload? Like we do in GLRenderer:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/output/gl_rende...

[vmiura, 2014-12-19 21:28:16]

https://codereview.chromium.org/817673004/diff/1/cc/resources/texture_uploade...
File cc/resources/texture_uploader.cc (right):

https://codereview.chromium.org/817673004/diff/1/cc/resources/texture_uploade...
cc/resources/texture_uploader.cc:254: gl_->PixelStorei(GL_UNPACK_ALIGNMENT, 4);
On 2014/12/19 20:00:46, danakj wrote:
> Should we do this when we take control of the context back from ganesh instead
> of doing it on every upload? Like we do in GLRenderer:
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/output/gl_rende...

If there's a good spot we could do that.

Discussing a bit with Vangelis, we were thinking that as the PixelStorei() call
is not costly compared to actually transferring a texture, then we could put it
here and not be concerned about other code that may inadvertently modify the
state.

[danakj, 2014-12-19 21:31:28]

On 2014/12/19 21:28:16, vmiura wrote:
>
https://codereview.chromium.org/817673004/diff/1/cc/resources/texture_uploade...
> File cc/resources/texture_uploader.cc (right):
> 
>
https://codereview.chromium.org/817673004/diff/1/cc/resources/texture_uploade...
> cc/resources/texture_uploader.cc:254: gl_->PixelStorei(GL_UNPACK_ALIGNMENT,
4);
> On 2014/12/19 20:00:46, danakj wrote:
> > Should we do this when we take control of the context back from ganesh
instead
> > of doing it on every upload? Like we do in GLRenderer:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/output/gl_rende...
> 
> If there's a good spot we could do that.
> 
> Discussing a bit with Vangelis, we were thinking that as the PixelStorei()
call
> is not costly compared to actually transferring a texture, then we could put
it
> here and not be concerned about other code that may inadvertently modify the
> state.

While that's true, from the comments this seems quite intentional, and avoiding
it when we can would be nice.

I found this:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/scope...

Would that be a good spot?

[vmiura, 2014-12-19 22:28:31]

> While that's true, from the comments this seems quite intentional, and
avoiding
> it when we can would be nice.
> 
> I found this:
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/scope...
> 
> Would that be a good spot?

Would this work?

[danakj, 2014-12-19 22:29:32]

LGTM if that fixes it. Do we need to do this in GLRenderer too?

[danakj, 2014-12-19 22:32:30]

On 2014/12/19 22:29:32, danakj wrote:
> LGTM if that fixes it. Do we need to do this in GLRenderer too?

Maybe GLRenderer's Scoped thing should use ScopedGpuRaster. The flush there is
currently done manually is the difference.

[vmiura, 2014-12-19 23:04:15]

On 2014/12/19 22:32:30, danakj wrote:
> On 2014/12/19 22:29:32, danakj wrote:
> > LGTM if that fixes it. Do we need to do this in GLRenderer too?
> 
> Maybe GLRenderer's Scoped thing should use ScopedGpuRaster. The flush there is
> currently done manually is the difference.

I'll look into using ScopedGpuRaster.

I wonder shouldn't GLRenderer also call GrContext::Flush()?  Ganesh might defer
something until Flush().

[danakj, 2014-12-19 23:07:31]

On Fri, Dec 19, 2014 at 3:04 PM, <vmiura@chromium.org> wrote:

> On 2014/12/19 22:32:30, danakj wrote:
>
>> On 2014/12/19 22:29:32, danakj wrote:
>> > LGTM if that fixes it. Do we need to do this in GLRenderer too?
>>
>
>  Maybe GLRenderer's Scoped thing should use ScopedGpuRaster. The flush
>> there is
>> currently done manually is the difference.
>>
>
> I'll look into using ScopedGpuRaster.
>
> I wonder shouldn't GLRenderer also call GrContext::Flush()?  Ganesh might
> defer
> something until Flush().
>

It calls canvas->flush() which flushes the GrContext. The comment says
this, you can take a look at the blame/history too, I'm the last one who
touched that I think.


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

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

[vmiura, 2014-12-19 23:44:39]

Added for ScopedUseGrContext as well.  PTAL.

[danakj, 2014-12-20 07:11:39]

Looks like some cc_ and compositor_unittest failures to resolve.
On Dec 19, 2014 3:44 PM, <vmiura@chromium.org> wrote:

> Added for ScopedUseGrContext as well.  PTAL.
>
> https://codereview.chromium.org/817673004/
>

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

[vmiura, 2014-12-22 21:08:04]

> Looks like some cc_ and compositor_unittest failures to resolve.

Yeah, sorry inheritance was the wrong way to go about it.

Fixed now.

[danakj, 2014-12-23 17:24:36]

LGTM, but

https://codereview.chromium.org/817673004/diff/60001/cc/resources/scoped_gpu_...
File cc/resources/scoped_gpu_raster.cc (right):

https://codereview.chromium.org/817673004/diff/60001/cc/resources/scoped_gpu_...
cc/resources/scoped_gpu_raster.cc:42: // Restore default GL unpack alignment. 
TextureUploader expects this.
Is it possible to unit test this?

[vmiura, 2014-12-23 23:58:14]

https://codereview.chromium.org/817673004/diff/60001/cc/resources/scoped_gpu_...
File cc/resources/scoped_gpu_raster.cc (right):

https://codereview.chromium.org/817673004/diff/60001/cc/resources/scoped_gpu_...
cc/resources/scoped_gpu_raster.cc:42: // Restore default GL unpack alignment. 
TextureUploader expects this.
On 2014/12/23 17:24:36, danakj wrote:
> Is it possible to unit test this?

Done.

[danakj, 2014-12-24 00:52:16]

Cool, LGTM

https://codereview.chromium.org/817673004/diff/80001/cc/cc_tests.gyp
File cc/cc_tests.gyp (right):

https://codereview.chromium.org/817673004/diff/80001/cc/cc_tests.gyp#newcode88
cc/cc_tests.gyp:88: 'resources/scoped_gpu_raster_unittest.cc',
add to BUILD.gn too?

[vmiura, 2014-12-25 22:10:05]

The CQ bit was checked by vmiura@chromium.org

[commit-bot: I haz the power, 2014-12-25 22:11:08]

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

[commit-bot: I haz the power, 2014-12-25 23:11:43]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2014-12-25 23:12:36]

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