Implement support for origin-TopLeft render targets in GL backend.

Implement support for origin-TopLeft render targets. Note that the default behaviour remains the same:  textures default to origin-TopLeft, render targets default to origin-BottomLeft, and backend textures default to origin-BottomLeft. However, the caller can override the default by setting fOrigin in GrTextureDesc, GrBackendTextureDesc or GrBackendRenderTargetDesc.

Committed: https://code.google.com/p/skia/source/detail?r=7594

[Stephen White, 2013-01-28 19:47:46]

PTAL. This should probably get some testing love, but I'm not sure if that
should be a GM or a unit test.

[bsalomon, 2013-01-28 20:05:49]

On 2013/01/28 19:47:46, Stephen White wrote:
> PTAL. This should probably get some testing love, but I'm not sure if that
> should be a GM or a unit test.

We should probably update the ReadPixels and WritePixels unit tests to test
devices backed by both types of RT to make sure pixel operations work correctly.
I'm not sure about whether readPixels will work correctly as it is now.

For rendering maybe we can also do a unit test? It could draw the same thing to
two devices and ensure that the images are the same. It could also be done with
a GM by doing two device-to-device draws where the two src devices are different
orientation (and were rendered with the same thing.)

I supposed we'd have to add GL calls to create the textures to wrap. Maybe we
should do this in tandem with creating textures of either orientation to avoid
that complication?

[Stephen White, 2013-01-30 03:20:21]

On 2013/01/28 20:05:49, bsalomon wrote:
> On 2013/01/28 19:47:46, Stephen White wrote:
> > PTAL. This should probably get some testing love, but I'm not sure if that
> > should be a GM or a unit test.
> 
> We should probably update the ReadPixels and WritePixels unit tests to test
> devices backed by both types of RT to make sure pixel operations work
correctly.
> I'm not sure about whether readPixels will work correctly as it is now.

OK, I've added cases for readPixels (not writePixels yet).

This required quite a bit of code so I'm checkpointing it here for your review.

> For rendering maybe we can also do a unit test? It could draw the same thing
to
> two devices and ensure that the images are the same. It could also be done
with
> a GM by doing two device-to-device draws where the two src devices are
different
> orientation (and were rendered with the same thing.)
>
> I supposed we'd have to add GL calls to create the textures to wrap. Maybe we
> should do this in tandem with creating textures of either orientation to avoid
> that complication?

[bsalomon, 2013-01-30 13:55:54]

This looks mostly right. Comments inline.

https://codereview.appspot.com/7230049/diff/7003/include/gpu/GrTypes.h
File include/gpu/GrTypes.h (right):

https://codereview.appspot.com/7230049/diff/7003/include/gpu/GrTypes.h#newcod...
include/gpu/GrTypes.h:643: GrBackendRenderTargetDesc() { memset(this, 0,
sizeof(*this)); }
I think we should change the constructor to init fOrigin to kDefault.

https://codereview.appspot.com/7230049/diff/7003/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.appspot.com/7230049/diff/7003/src/gpu/GrContext.cpp#newcod...
src/gpu/GrContext.cpp:377: 
I think we must set the desc origin here to either kTopLeft or kBottomLeft
before computing a key. Otherwise we will get keys that encode kDefault which is
really the same as one of the other two values. Therefore we would get redundant
entries.

We may need a helper on GrGpu that tells this function what to default to here.

We should have an assert where we compute the scratch GrResourceKey that the
desc's origin is one of the specific values and not kDefault.

https://codereview.appspot.com/7230049/diff/7003/src/gpu/gl/GrGpuGL.cpp
File src/gpu/gl/GrGpuGL.cpp (right):

https://codereview.appspot.com/7230049/diff/7003/src/gpu/gl/GrGpuGL.cpp#newco...
src/gpu/gl/GrGpuGL.cpp:947: glTexDesc.fOrigin = kDefault_GrSurfaceOrigin ==
desc.fOrigin ?
I think here we really want to default to kTopLeft if srcData is non-NULL.
srcData always points at the top left pixel of the data to upload. So uploading
can be done with one glTexImage call if the texture is top-down.

Perhaps kDefault should not be defined to be "match the backend's convention"
but rather "make the best guess at optimality."

For internally created SkGpuDevice backing RTs it might be best to choose
kTopLeft to make readbacks faster...

I can imagine having two special special values: kDefault and
kBackendConvention. kDefault would mean "let GrContext decide." kDefault would
be the default value for textures we create (via GrTextureDesc) and
kBackendConvention would be the default value for wrapped objects
(GrBackendTextureDesc and GrBackendRenderTargetDesc). *ducks*

In that model kDefault makes no promises about the orientation. Therefore when
searching for a scratch we could try searching for either orientation before
creating a texture.

I'm just thinking this through here... not saying we need this all now.

https://codereview.appspot.com/7230049/diff/7003/src/gpu/gl/GrGpuGL.h
File src/gpu/gl/GrGpuGL.h (right):

https://codereview.appspot.com/7230049/diff/7003/src/gpu/gl/GrGpuGL.h#newcode347
src/gpu/gl/GrGpuGL.h:347: GrSurfaceOrigin         fLastOrigin;
Can we move this into fHWPathMatrixState?

https://codereview.appspot.com/7230049/diff/7003/src/gpu/gl/GrGpuGL_program.cpp
File src/gpu/gl/GrGpuGL_program.cpp (right):

https://codereview.appspot.com/7230049/diff/7003/src/gpu/gl/GrGpuGL_program.c...
src/gpu/gl/GrGpuGL_program.cpp:154: fLastOrigin = rt->origin();
I don't think we should set this here.. we need to separately track the state of
the fixed function / nv_path matrix from each program's uniform matrix.

[Stephen White, 2013-02-04 16:05:26]

This patch now passes all GM's and unit tests, albeit by restoring the old
behaviour via kDefault_GrSurfaceOrigin:  textures default TopLeft, RenderTargets
default BottomLeft.  

TopLeft RenderTargets still have some unresolved issues (e.g.,
fragmentPosition() requires some shader builder work, and GpuCopyBitmapTest
fails).  However, we should be able to move forward with this patch and resolve
those issues in followups.

[bsalomon, 2013-02-04 16:12:31]

https://codereview.appspot.com/7230049/diff/21001/src/gpu/gl/GrGpuGL.h
File src/gpu/gl/GrGpuGL.h (right):

https://codereview.appspot.com/7230049/diff/21001/src/gpu/gl/GrGpuGL.h#newcod...
src/gpu/gl/GrGpuGL.h:340: GrSurfaceOrigin         fLastOrigin;
Let's move this to the fHWPathMatrixState struct.

[Stephen White, 2013-02-04 16:22:15]

On 2013/02/04 16:12:31, bsalomon wrote:
> https://codereview.appspot.com/7230049/diff/21001/src/gpu/gl/GrGpuGL.h
> File src/gpu/gl/GrGpuGL.h (right):
> 
>
https://codereview.appspot.com/7230049/diff/21001/src/gpu/gl/GrGpuGL.h#newcod...
> src/gpu/gl/GrGpuGL.h:340: GrSurfaceOrigin         fLastOrigin;
> Let's move this to the fHWPathMatrixState struct.

Done.

[bsalomon, 2013-02-04 16:23:04]

lgtm

[Stephen White, 2013-02-04 16:44:07]

Updated past your ANGLE change; please take a look to make sure I didn't mess
something up.

[bsalomon, 2013-02-04 16:45:13]

On 2013/02/04 16:44:07, Stephen White wrote:
> Updated past your ANGLE change; please take a look to make sure I didn't mess
> something up.

lgtm

[Stephen White, 2013-02-05 16:06:53]

PTAL.  I messed up onWrapBackendTexture(), since I didn't notice that it forces
all textures to BottomLeft, not just RenderTargets.  Should be fixed now.

[bsalomon, 2013-02-05 16:12:06]

LGTM modulo silly style comments. But let's hold off on re-landing until the
DEPS roll sticks.

https://codereview.appspot.com/7230049/diff/46001/src/gpu/gl/GrGpuGL.cpp
File src/gpu/gl/GrGpuGL.cpp (right):

https://codereview.appspot.com/7230049/diff/46001/src/gpu/gl/GrGpuGL.cpp#newc...
src/gpu/gl/GrGpuGL.cpp:150: static GrSurfaceOrigin
resolve_origin(GrSurfaceOrigin origin, bool renderTarget) {
namespace { rather than static.

The other helpers in this file are defined just above where they are (first)
used. I think most of Skia code is like this.

[reed1, 2013-02-05 16:14:37]

lets run this locally in a chrome build, to try out DRT before committing.

[Stephen White, 2013-02-05 16:16:24]

On 2013/02/05 16:14:37, reed1 wrote:
> lets run this locally in a chrome build, to try out DRT before committing.

I ran all the css3/filters tests, which pass.  Will do a complete run before
landing.

[Stephen White, 2013-02-05 16:22:27]

https://codereview.appspot.com/7230049/diff/46001/src/gpu/gl/GrGpuGL.cpp
File src/gpu/gl/GrGpuGL.cpp (right):

https://codereview.appspot.com/7230049/diff/46001/src/gpu/gl/GrGpuGL.cpp#newc...
src/gpu/gl/GrGpuGL.cpp:150: static GrSurfaceOrigin
resolve_origin(GrSurfaceOrigin origin, bool renderTarget) {
On 2013/02/05 16:12:06, bsalomon wrote:
> namespace { rather than static.

Done.

> The other helpers in this file are defined just above where they are (first)
> used. I think most of Skia code is like this.

Actually I was trying to follow the function above it (fbo_test), which is
static, rather than namespace {}.  :)

[bsalomon, 2013-02-05 16:29:24]

On 2013/02/05 16:22:27, Stephen White wrote:
> https://codereview.appspot.com/7230049/diff/46001/src/gpu/gl/GrGpuGL.cpp
> File src/gpu/gl/GrGpuGL.cpp (right):
> 
>
https://codereview.appspot.com/7230049/diff/46001/src/gpu/gl/GrGpuGL.cpp#newc...
> src/gpu/gl/GrGpuGL.cpp:150: static GrSurfaceOrigin
> resolve_origin(GrSurfaceOrigin origin, bool renderTarget) {
> On 2013/02/05 16:12:06, bsalomon wrote:
> > namespace { rather than static.
> 
> Done.
> 
> > The other helpers in this file are defined just above where they are (first)
> > used. I think most of Skia code is like this.
> 
> Actually I was trying to follow the function above it (fbo_test), which is
> static, rather than namespace {}.  :)

Oh haha.. I guess that one was never updated. It's not even called anymore and
should probably be removed. It was part of a workaround for a driver bug that
hopefully was fixed long ago.

Can we add a comment to the decl of kDefault that it is deprecated and will be
removed? (No need to reupload)

[Stephen White, 2013-02-05 18:09:18]

On 2013/02/05 16:29:24, bsalomon wrote:
> On 2013/02/05 16:22:27, Stephen White wrote:
> > https://codereview.appspot.com/7230049/diff/46001/src/gpu/gl/GrGpuGL.cpp
> > File src/gpu/gl/GrGpuGL.cpp (right):
> > 
> >
>
https://codereview.appspot.com/7230049/diff/46001/src/gpu/gl/GrGpuGL.cpp#newc...
> > src/gpu/gl/GrGpuGL.cpp:150: static GrSurfaceOrigin
> > resolve_origin(GrSurfaceOrigin origin, bool renderTarget) {
> > On 2013/02/05 16:12:06, bsalomon wrote:
> > > namespace { rather than static.
> > 
> > Done.
> > 
> > > The other helpers in this file are defined just above where they are
(first)
> > > used. I think most of Skia code is like this.
> > 
> > Actually I was trying to follow the function above it (fbo_test), which is
> > static, rather than namespace {}.  :)
> 
> Oh haha.. I guess that one was never updated. It's not even called anymore and
> should probably be removed. It was part of a workaround for a driver bug that
> hopefully was fixed long ago.
> 
> Can we add a comment to the decl of kDefault that it is deprecated and will be
> removed? (No need to reupload)

Will do.  Thanks for the review.