Add an origin flag for backend (external) textures. Some textures in WebKit have a topdown orienta…

Add an origin flag for backend (external) textures.  Some textures in WebKit have a topdown orientation, and skia needs to be notified of this, so that they are not drawn upside-down.

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

[Stephen White, 2013-01-24 18:48:52]

PTAL.  I've made the orientation a global enum, with the idea that it could
later replace the existing Surface enum.  If you'd prefer it be scoped to the
GrBackendTextureDesc (or some other class), I can do that instead.

[bsalomon, 2013-01-24 18:53:29]

Some thoughts:

Let's go ahead and replace GrSurface::Origin with the new enum. I don't think we
need two. But let's call it GrSurfaceOrigin since it is used by both subclasses
of surface. That ties into my next thought...

A client could wrap a backend texture with kRenderTarget_GrBackendTextureFlag
set and the origin set to kTopLeft. We'd draw texture from it with the origin at
the top-left but we'll still draw into it with the origin at the bottom-left.
That seems wrong. I think we should render to it in the same manner as we
texture from it. We'd have to update the view matrix adjustment code here:
http://code.google.com/p/skia/source/browse/trunk/src/gpu/gl/GrGpuGL_program....
and here:
http://code.google.com/p/skia/source/browse/trunk/src/gpu/gl/GrGpuGL_program....
And we'd also have to account for it when calling read pixels as we already do
when writing pixels to a texture.

Also, to not break existing clients do we need to make the desc field default to
an enum value that means "match the backend's orientation"? Even if we wouldn't
break any known users, it seems like a handy and easy thing to provide.

[Stephen White, 2013-01-28 16:00:49]

On 2013/01/24 18:53:29, bsalomon wrote:
> Some thoughts:
> 
> Let's go ahead and replace GrSurface::Origin with the new enum. I don't think
we
> need two. But let's call it GrSurfaceOrigin since it is used by both
subclasses
> of surface.

Done.

> That ties into my next thought...

> 
> A client could wrap a backend texture with kRenderTarget_GrBackendTextureFlag
> set and the origin set to kTopLeft. We'd draw texture from it with the origin
at
> the top-left but we'll still draw into it with the origin at the bottom-left.
> That seems wrong. I think we should render to it in the same manner as we
> texture from it. We'd have to update the view matrix adjustment code here:
>
http://code.google.com/p/skia/source/browse/trunk/src/gpu/gl/GrGpuGL_program....
> and here:
>
http://code.google.com/p/skia/source/browse/trunk/src/gpu/gl/GrGpuGL_program....
> And we'd also have to account for it when calling read pixels as we already do
> when writing pixels to a texture.

Yes, it seems as if render targets don't really respect the origin, if that's
what you mean.  I'd prefer to add that support in a separate patch, if that's
ok.  I've added the early NULL return for now, so that if someone starts
requesting topleft RT's in GL, it'll fail.

> Also, to not break existing clients do we need to make the desc field default
to
> an enum value that means "match the backend's orientation"? Even if we
wouldn't
> break any known users, it seems like a handy and easy thing to provide.

Well, the existing clients are all defaulting to BottomLeft (at least for
textures, not sure about RT's), so I don't think this patch would break any
existing clients.  If they did suddenly start requesting TopLeft in their RT's,
yes, it'll now fail.

[bsalomon, 2013-01-28 16:02:57]

On 2013/01/28 16:00:49, Stephen White wrote:
> On 2013/01/24 18:53:29, bsalomon wrote:
> > Some thoughts:
> > 
> > Let's go ahead and replace GrSurface::Origin with the new enum. I don't
think
> we
> > need two. But let's call it GrSurfaceOrigin since it is used by both
> subclasses
> > of surface.
> 
> Done.
> 
> > That ties into my next thought...
> 
> > 
> > A client could wrap a backend texture with
kRenderTarget_GrBackendTextureFlag
> > set and the origin set to kTopLeft. We'd draw texture from it with the
origin
> at
> > the top-left but we'll still draw into it with the origin at the
bottom-left.
> > That seems wrong. I think we should render to it in the same manner as we
> > texture from it. We'd have to update the view matrix adjustment code here:
> >
>
http://code.google.com/p/skia/source/browse/trunk/src/gpu/gl/GrGpuGL_program....
> > and here:
> >
>
http://code.google.com/p/skia/source/browse/trunk/src/gpu/gl/GrGpuGL_program....
> > And we'd also have to account for it when calling read pixels as we already
do
> > when writing pixels to a texture.
> 
> Yes, it seems as if render targets don't really respect the origin, if that's
> what you mean.  I'd prefer to add that support in a separate patch, if that's
> ok.  I've added the early NULL return for now, so that if someone starts
> requesting topleft RT's in GL, it'll fail.
> 
> > Also, to not break existing clients do we need to make the desc field
default
> to
> > an enum value that means "match the backend's orientation"? Even if we
> wouldn't
> > break any known users, it seems like a handy and easy thing to provide.
> 
> Well, the existing clients are all defaulting to BottomLeft (at least for
> textures, not sure about RT's), so I don't think this patch would break any
> existing clients.  If they did suddenly start requesting TopLeft in their
RT's,
> yes, it'll now fail.

Ok. Let's just handle it explicitly by returning NULL from
GrGpuGL::onWrapBackendTexture() if an unsupported combo is requested. It seems
preferable to just fail rather than return a texture that will behave
inconsistently.

[Stephen White, 2013-01-28 16:04:05]

On 2013/01/28 16:02:57, bsalomon wrote:
> On 2013/01/28 16:00:49, Stephen White wrote:
> > On 2013/01/24 18:53:29, bsalomon wrote:
> > > Some thoughts:
> > > 
> > > Let's go ahead and replace GrSurface::Origin with the new enum. I don't
> think
> > we
> > > need two. But let's call it GrSurfaceOrigin since it is used by both
> > subclasses
> > > of surface.
> > 
> > Done.
> > 
> > > That ties into my next thought...
> > 
> > > 
> > > A client could wrap a backend texture with
> kRenderTarget_GrBackendTextureFlag
> > > set and the origin set to kTopLeft. We'd draw texture from it with the
> origin
> > at
> > > the top-left but we'll still draw into it with the origin at the
> bottom-left.
> > > That seems wrong. I think we should render to it in the same manner as we
> > > texture from it. We'd have to update the view matrix adjustment code here:
> > >
> >
>
http://code.google.com/p/skia/source/browse/trunk/src/gpu/gl/GrGpuGL_program....
> > > and here:
> > >
> >
>
http://code.google.com/p/skia/source/browse/trunk/src/gpu/gl/GrGpuGL_program....
> > > And we'd also have to account for it when calling read pixels as we
already
> do
> > > when writing pixels to a texture.
> > 
> > Yes, it seems as if render targets don't really respect the origin, if
that's
> > what you mean.  I'd prefer to add that support in a separate patch, if
that's
> > ok.  I've added the early NULL return for now, so that if someone starts
> > requesting topleft RT's in GL, it'll fail.
> > 
> > > Also, to not break existing clients do we need to make the desc field
> default
> > to
> > > an enum value that means "match the backend's orientation"? Even if we
> > wouldn't
> > > break any known users, it seems like a handy and easy thing to provide.
> > 
> > Well, the existing clients are all defaulting to BottomLeft (at least for
> > textures, not sure about RT's), so I don't think this patch would break any
> > existing clients.  If they did suddenly start requesting TopLeft in their
> RT's,
> > yes, it'll now fail.
> 
> Ok. Let's just handle it explicitly by returning NULL from
> GrGpuGL::onWrapBackendTexture() if an unsupported combo is requested. It seems
> preferable to just fail rather than return a texture that will behave
> inconsistently.

Done.

[bsalomon, 2013-01-28 16:09:00]

Mostly LGTM. One real comment and one style thing.

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

https://codereview.appspot.com/7200048/diff/11001/include/gpu/GrTypes.h#newco...
include/gpu/GrTypes.h:435: enum GrSurfaceOrigin {
For enums in the global scope try to make the values have a Sk/Gr in the type
part of the name. E.g. kBottomLeft_GrSurfaceOrigin. 

(Some older Gr enums don't follow this convention. I'm trying to fix them slowly
over time.)

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

https://codereview.appspot.com/7200048/diff/11001/src/gpu/gl/GrGpuGL.cpp#newc...
src/gpu/gl/GrGpuGL.cpp:489: if (kBottomLeft_SurfaceOrigin != desc.fOrigin) {
Don't you need to check if the RT bit is set here? I think this will prevent any
bottom-left texture wrap from succeeding.

[Stephen White, 2013-01-28 16:38:42]

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

https://codereview.appspot.com/7200048/diff/11001/include/gpu/GrTypes.h#newco...
include/gpu/GrTypes.h:435: enum GrSurfaceOrigin {
On 2013/01/28 16:09:00, bsalomon wrote:
> For enums in the global scope try to make the values have a Sk/Gr in the type
> part of the name. E.g. kBottomLeft_GrSurfaceOrigin. 
> 
> (Some older Gr enums don't follow this convention. I'm trying to fix them
slowly
> over time.)

Done.

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

https://codereview.appspot.com/7200048/diff/11001/src/gpu/gl/GrGpuGL.cpp#newc...
src/gpu/gl/GrGpuGL.cpp:489: if (kBottomLeft_SurfaceOrigin != desc.fOrigin) {
On 2013/01/28 16:09:00, bsalomon wrote:
> Don't you need to check if the RT bit is set here? I think this will prevent
any
> bottom-left texture wrap from succeeding. 

Doh!  I thought I was in onWrapBackendRenderTarget().  Fixed.

[bsalomon, 2013-01-28 16:39:57]

On 2013/01/28 16:38:42, Stephen White wrote:
> https://codereview.appspot.com/7200048/diff/11001/include/gpu/GrTypes.h
> File include/gpu/GrTypes.h (right):
> 
>
https://codereview.appspot.com/7200048/diff/11001/include/gpu/GrTypes.h#newco...
> include/gpu/GrTypes.h:435: enum GrSurfaceOrigin {
> On 2013/01/28 16:09:00, bsalomon wrote:
> > For enums in the global scope try to make the values have a Sk/Gr in the
type
> > part of the name. E.g. kBottomLeft_GrSurfaceOrigin. 
> > 
> > (Some older Gr enums don't follow this convention. I'm trying to fix them
> slowly
> > over time.)
> 
> Done.
> 
> https://codereview.appspot.com/7200048/diff/11001/src/gpu/gl/GrGpuGL.cpp
> File src/gpu/gl/GrGpuGL.cpp (right):
> 
>
https://codereview.appspot.com/7200048/diff/11001/src/gpu/gl/GrGpuGL.cpp#newc...
> src/gpu/gl/GrGpuGL.cpp:489: if (kBottomLeft_SurfaceOrigin != desc.fOrigin) {
> On 2013/01/28 16:09:00, bsalomon wrote:
> > Don't you need to check if the RT bit is set here? I think this will prevent
> any
> > bottom-left texture wrap from succeeding. 
> 
> Doh!  I thought I was in onWrapBackendRenderTarget().  Fixed.

LGTM