Add support for eglBindTexImage and eglReleaseTexImage

With this patch we can bind pbuffers as textures. Once we add support for using a sharing handle to create a pbuffer we can use this to allow us to use it as a texture as well.

BUG=129
TEST=modified Simple_Texture2D works

[apatrick1, 2011-03-24 20:15:24]

Good stuff! A few thoughts below. I'll add Daniel as a reviewer as well.

http://codereview.appspot.com/4291066/diff/1/src/libEGL/Display.cpp
File src/libEGL/Display.cpp (right):

http://codereview.appspot.com/4291066/diff/1/src/libEGL/Display.cpp#newcode415
src/libEGL/Display.cpp:415: Surface *Display::createOffscreenSurface(int width,
int height, EGLConfig config, const EGLint *attrib_list)
ANGLE style is different from Chromium. I don't think there's a doc; you just
have to look around the code. In this case "attrib_list" should be "attribList".

http://codereview.appspot.com/4291066/diff/1/src/libEGL/Display.h
File src/libEGL/Display.h (right):

http://codereview.appspot.com/4291066/diff/1/src/libEGL/Display.h#newcode46
src/libEGL/Display.h:46: egl::Surface *createOffscreenSurface(int width, int
height, EGLConfig config, const EGLint *attrib_list);
Same here.

http://codereview.appspot.com/4291066/diff/1/src/libEGL/Surface.cpp
File src/libEGL/Surface.cpp (right):

http://codereview.appspot.com/4291066/diff/1/src/libEGL/Surface.cpp#newcode44
src/libEGL/Surface.cpp:44: Surface::Surface(Display *display, const Config
*config, EGLint width, EGLint height, const EGLint *attrib_list)
Same here.

http://codereview.appspot.com/4291066/diff/1/src/libEGL/Surface.cpp#newcode65
src/libEGL/Surface.cpp:65: while (*attrib_list != EGL_NONE)
I think we should iterate over the attribute array in one place. Also, I think
doing it in a constructor makes it hard to return failure if any attributes are
invalid.

http://codereview.appspot.com/4291066/diff/1/src/libEGL/Surface.cpp#newcode163
src/libEGL/Surface.cpp:163: presentParameters.BackBufferFormat =
mConfig->mRenderTargetFormat;
Maybe move this line to a final else clause of the if construct below if
mTextureFormat is EGL_NO_TEXTURE. Also, are the semantics definitely to take the
format from the config in that case?

http://codereview.appspot.com/4291066/diff/1/src/libEGL/Surface.cpp#newcode178
src/libEGL/Surface.cpp:178: } else if (mTextureFormat == EGL_TEXTURE_RGBA) {
nit: newline after } and before {

http://codereview.appspot.com/4291066/diff/1/src/libEGL/Surface.cpp#newcode424
src/libEGL/Surface.cpp:424: void Surface::setBoundTexture(void *texture)
Can void* be gl::Texture* ?

http://codereview.appspot.com/4291066/diff/1/src/libEGL/libEGL.cpp
File src/libEGL/libEGL.cpp (right):

http://codereview.appspot.com/4291066/diff/1/src/libEGL/libEGL.cpp#newcode419
src/libEGL/libEGL.cpp:419: case EGL_TEXTURE_TARGET:
We should probably still check EGL_TEXTURE_FORMAT and EGL_TEXTURE_TARGET have
valid values somewhere.

http://codereview.appspot.com/4291066/diff/1/src/libEGL/libEGL.cpp#newcode797
src/libEGL/libEGL.cpp:797: 
I think the check that the config supports EGL_BIND_TO_TEXTURE_RGBA or
EGL_BIND_TO_TEXTURE_RGB should be in here. Also, I think we can add those to all
the configs. Also, EGL_PBUFFER_BIT appears to be missing from the configs. Could
you add that as well?

http://codereview.appspot.com/4291066/diff/1/src/libEGL/libEGL.cpp#newcode800
src/libEGL/libEGL.cpp:800: return success(EGL_TRUE);
Looks like eglBindTexImage should do an implicit glFlush or glFinish depending
on whether the display and surface are current.

http://codereview.appspot.com/4291066/diff/1/src/libGLESv2/Texture.cpp
File src/libGLESv2/Texture.cpp (right):

http://codereview.appspot.com/4291066/diff/1/src/libGLESv2/Texture.cpp#newcod...
src/libGLESv2/Texture.cpp:1313: GLenum format = surface->getTextureFormat() ==
EGL_TEXTURE_RGBA ? GL_RGBA : GL_RGB;
Should the format not also be based on the size of the config's color channels?

http://codereview.appspot.com/4291066/diff/1/src/libGLESv2/Texture.cpp#newcod...
src/libGLESv2/Texture.cpp:1320: if (mTexture)
I think redefineTexture ensures that this is null here. How about just an
assertion that it is null?

http://codereview.appspot.com/4291066/diff/1/src/libGLESv2/libGLESv2.cpp
File src/libGLESv2/libGLESv2.cpp (right):

http://codereview.appspot.com/4291066/diff/1/src/libGLESv2/libGLESv2.cpp#newc...
src/libGLESv2/libGLESv2.cpp:4603: if (textureObject) {
nit: drop brace to next line.

http://codereview.appspot.com/4291066/diff/1/src/libGLESv2/libGLESv2.cpp#newc...
src/libGLESv2/libGLESv2.cpp:4614: void __stdcall glReleaseTexImage(void
*texture)
Can this be gl::Texture* ? I don't think the dependency from libEGL is a
problem.

http://codereview.appspot.com/4291066/diff/1/src/libGLESv2/libGLESv2.cpp#newc...
src/libGLESv2/libGLESv2.cpp:4623: if (textureObject) {
nit: drop brace to next line.

[John Bauman, 2011-03-25 00:02:52]

Thanks for looking at this.

http://codereview.appspot.com/4291066/diff/1/src/libEGL/Display.cpp
File src/libEGL/Display.cpp (right):

http://codereview.appspot.com/4291066/diff/1/src/libEGL/Display.cpp#newcode415
src/libEGL/Display.cpp:415: Surface *Display::createOffscreenSurface(int width,
int height, EGLConfig config, const EGLint *attrib_list)
On 2011/03/24 20:15:24, apatrick1 wrote:
> ANGLE style is different from Chromium. I don't think there's a doc; you just
> have to look around the code. In this case "attrib_list" should be
"attribList".
Oh, it doesn't seem to be the most consistent style - I based this off of
eglChooseConfig.

http://codereview.appspot.com/4291066/diff/1/src/libEGL/Surface.cpp
File src/libEGL/Surface.cpp (right):

http://codereview.appspot.com/4291066/diff/1/src/libEGL/Surface.cpp#newcode65
src/libEGL/Surface.cpp:65: while (*attrib_list != EGL_NONE)
On 2011/03/24 20:15:24, apatrick1 wrote:
> I think we should iterate over the attribute array in one place. Also, I think
> doing it in a constructor makes it hard to return failure if any attributes
are
> invalid.
Yeah, I was trying to avoid bloating up the argument list too much, but I guess
two extra arguments isn't too bad.

http://codereview.appspot.com/4291066/diff/1/src/libEGL/Surface.cpp#newcode163
src/libEGL/Surface.cpp:163: presentParameters.BackBufferFormat =
mConfig->mRenderTargetFormat;
On 2011/03/24 20:15:24, apatrick1 wrote:
> Maybe move this line to a final else clause of the if construct below if
> mTextureFormat is EGL_NO_TEXTURE. Also, are the semantics definitely to take
the
> format from the config in that case?

I'll do that. The spec does say that the color buffers should match the config,
although how the texture format input and the config interact isn't
well-specified.

http://codereview.appspot.com/4291066/diff/1/src/libEGL/Surface.cpp#newcode424
src/libEGL/Surface.cpp:424: void Surface::setBoundTexture(void *texture)
On 2011/03/24 20:15:24, apatrick1 wrote:
> Can void* be gl::Texture* ?
Sure. I didn't know how much we wanted to depend on libGLESv2 types.

http://codereview.appspot.com/4291066/diff/1/src/libEGL/libEGL.cpp
File src/libEGL/libEGL.cpp (right):

http://codereview.appspot.com/4291066/diff/1/src/libEGL/libEGL.cpp#newcode797
src/libEGL/libEGL.cpp:797: 
On 2011/03/24 20:15:24, apatrick1 wrote:
> I think the check that the config supports EGL_BIND_TO_TEXTURE_RGBA or
> EGL_BIND_TO_TEXTURE_RGB should be in here. Also, I think we can add those to
all
> the configs. Also, EGL_PBUFFER_BIT appears to be missing from the configs.
Could
> you add that as well?

Good point, sure.

http://codereview.appspot.com/4291066/diff/1/src/libEGL/libEGL.cpp#newcode800
src/libEGL/libEGL.cpp:800: return success(EGL_TRUE);
On 2011/03/24 20:15:24, apatrick1 wrote:
> Looks like eglBindTexImage should do an implicit glFlush or glFinish depending
> on whether the display and surface are current.
I can try that, though it doesn't look those would do anything useful in this GL
implementation.

http://codereview.appspot.com/4291066/diff/1/src/libGLESv2/Texture.cpp
File src/libGLESv2/Texture.cpp (right):

http://codereview.appspot.com/4291066/diff/1/src/libGLESv2/Texture.cpp#newcod...
src/libGLESv2/Texture.cpp:1313: GLenum format = surface->getTextureFormat() ==
EGL_TEXTURE_RGBA ? GL_RGBA : GL_RGB;
On 2011/03/24 20:15:24, apatrick1 wrote:
> Should the format not also be based on the size of the config's color
channels?

Looking at the spec again, it does seem like that's the case.

[John Bauman, 2011-03-25 02:39:37]

Fixed a bunch of style issues, added some extra checks, and added some new egl
configs which can be bound as textures but must be created as pbuffers, so we
can prevent some incorrect behavior.

[apatrick1, 2011-03-25 18:40:05]

Looks good to me but you surpass my EGL-fu so I will hand over sign-off to
Daniel.

http://codereview.appspot.com/4291066/diff/8003/src/libEGL/Config.cpp
File src/libEGL/Config.cpp (right):

http://codereview.appspot.com/4291066/diff/8003/src/libEGL/Config.cpp#newcode174
src/libEGL/Config.cpp:174: mSurfaceType = asTexture ? EGL_PBUFFER_BIT :
(EGL_PBUFFER_BIT | EGL_WINDOW_BIT | EGL_SWAP_BEHAVIOR_PRESERVED_BIT);
I leave this one to Daniel :)

http://codereview.appspot.com/4291066/diff/8003/src/libEGL/Config.cpp#newcode261
src/libEGL/Config.cpp:261: SORT(mBindToTextureRGBA);
Look at table 3.4 in the EGL 1.4 spec for the sorting order.
http://www.khronos.org/registry/egl/specs/eglspec.1.4.20101006.pdf

http://codereview.appspot.com/4291066/diff/8003/src/libEGL/Surface.cpp
File src/libEGL/Surface.cpp (right):

http://codereview.appspot.com/4291066/diff/8003/src/libEGL/Surface.cpp#newcod...
src/libEGL/Surface.cpp:155: 
Ah, that makes more sense.

[John Bauman, 2011-03-25 22:21:31]

http://codereview.appspot.com/4291066/diff/8003/src/libEGL/Config.cpp
File src/libEGL/Config.cpp (right):

http://codereview.appspot.com/4291066/diff/8003/src/libEGL/Config.cpp#newcode261
src/libEGL/Config.cpp:261: SORT(mBindToTextureRGBA);
On 2011/03/25 18:40:05, apatrick1 wrote:
> Look at table 3.4 in the EGL 1.4 spec for the sorting order.
> http://www.khronos.org/registry/egl/specs/eglspec.1.4.20101006.pdf

Thanks. The spec doesn't require that these are sorted, but our set
implementation does, and sorting them can't hurt.

[dgkoch, 2011-03-29 04:33:26]

Note that there have been changes to Texture.h/.cpp since this patch was
generated, and thus it may have to be updated.

http://codereview.appspot.com/4291066/diff/8003/src/libEGL/Config.cpp
File src/libEGL/Config.cpp (right):

http://codereview.appspot.com/4291066/diff/8003/src/libEGL/Config.cpp#newcode174
src/libEGL/Config.cpp:174: mSurfaceType = asTexture ? EGL_PBUFFER_BIT :
(EGL_PBUFFER_BIT | EGL_WINDOW_BIT | EGL_SWAP_BEHAVIOR_PRESERVED_BIT);
On 2011/03/25 18:40:05, apatrick1 wrote:
> I leave this one to Daniel :)

Is there a specific reason for this change?  This will prevent existing configs
from getting used, and only allowing the new "texture-able" ones from being
matched here, but is that really necessary?  (I'm not saying it may not be a
good idea -- I'm just asking if there was a specific reason)

http://codereview.appspot.com/4291066/diff/8003/src/libEGL/Surface.h
File src/libEGL/Surface.h (right):

http://codereview.appspot.com/4291066/diff/8003/src/libEGL/Surface.h#newcode61
src/libEGL/Surface.h:61: gl::Texture *getBoundTexture() const;
should probably be "virtual" to match setBoundTexture

http://codereview.appspot.com/4291066/diff/8003/src/libEGL/libEGL.cpp
File src/libEGL/libEGL.cpp (right):

http://codereview.appspot.com/4291066/diff/8003/src/libEGL/libEGL.cpp#newcode474
src/libEGL/libEGL.cpp:474: display->getConfigAttrib(config,
EGL_BIND_TO_TEXTURE_RGBA, &bindToTextureRGBAValue);
we should also check that the config supports PBUFFERS (else EGL_BAD_MATCH)

http://codereview.appspot.com/4291066/diff/8003/src/libEGL/libEGL.cpp#newcode811
src/libEGL/libEGL.cpp:811: EGLBoolean __stdcall eglBindTexImage(EGLDisplay dpy,
EGLSurface surface, EGLint buffer)
The EGL 1.4 spec (p39) for eglBindTexImage mentions implicit glFlush or glFinish
depending on if dpy and surface are the current display and/or surface for the
current thread.  Have you considered this at all?

http://codereview.appspot.com/4291066/diff/8003/src/libEGL/libEGL.cpp#newcode827
src/libEGL/libEGL.cpp:827: return error(EGL_BAD_ACCESS, EGL_FALSE);
this should be EGL_BAD_PARAMETER

http://codereview.appspot.com/4291066/diff/8003/src/libEGL/libEGL.cpp#newcode837
src/libEGL/libEGL.cpp:837: return error(EGL_BAD_MATCH, EGL_FALSE);
I think this should be EGL_BAD_SURFACE, or perhaps split into two checks, one
validating EGL_TEXTURE_FORMAT (else BAD_MATCH), and one validating the surface
is a pbuffer that supports texture binding (else BAD_SURFACE).

http://codereview.appspot.com/4291066/diff/8003/src/libEGL/libEGL.cpp#newcode868
src/libEGL/libEGL.cpp:868: return error(EGL_BAD_ACCESS, EGL_FALSE);
this should be EGL_BAD_PARAMETER

http://codereview.appspot.com/4291066/diff/8003/src/libEGL/libEGL.cpp#newcode871
src/libEGL/libEGL.cpp:871: gl::Texture *texture = surf->getBoundTexture();
there are also a few error conditions relating to the surface and surface
attributes which need to be validated.

http://codereview.appspot.com/4291066/diff/8003/src/libGLESv2/Texture.cpp
File src/libGLESv2/Texture.cpp (right):

http://codereview.appspot.com/4291066/diff/8003/src/libGLESv2/Texture.cpp#new...
src/libGLESv2/Texture.cpp:55: mSurface = NULL;
this should be in Texture2D::Texture2D

http://codereview.appspot.com/4291066/diff/8003/src/libGLESv2/Texture.cpp#new...
src/libGLESv2/Texture.cpp:1300: mSurface = NULL;
move this up into the if block just above

http://codereview.appspot.com/4291066/diff/8003/src/libGLESv2/Texture.cpp#new...
src/libGLESv2/Texture.cpp:1324: format = GL_RGBA;
the default case here should likely be UNIMPLEMENTED and (probably) silently
fail.  I don't think we should try to guess what the matching format should be.

http://codereview.appspot.com/4291066/diff/8003/src/libGLESv2/Texture.cpp#new...
src/libGLESv2/Texture.cpp:1328: redefineTexture(0, format, surface->getWidth(),
surface->getHeight(), GL_UNSIGNED_BYTE, true);
Note that unless we redefine all the textures levels (at least up to
log2(std::max(width, height)) this will only work with non-mipmapped filters
(other wise it'll fail the completeness test).

http://codereview.appspot.com/4291066/diff/8003/src/libGLESv2/Texture.h
File src/libGLESv2/Texture.h (right):

http://codereview.appspot.com/4291066/diff/8003/src/libGLESv2/Texture.h#newco...
src/libGLESv2/Texture.h:81: virtual void releaseTexImage() {}
these should only be defined for 2D textures

http://codereview.appspot.com/4291066/diff/8003/src/libGLESv2/Texture.h#newco...
src/libGLESv2/Texture.h:141: egl::Surface *mSurface;
This should be in Texture2D

http://codereview.appspot.com/4291066/diff/8003/src/libGLESv2/libGLESv2.cpp
File src/libGLESv2/libGLESv2.cpp (right):

http://codereview.appspot.com/4291066/diff/8003/src/libGLESv2/libGLESv2.cpp#n...
src/libGLESv2/libGLESv2.cpp:4589: void __stdcall glBindTexImage(egl::Surface
*surface)
put this down at the bottom with the other "internal" functions instead of
inline with the GL core functions

http://codereview.appspot.com/4291066/diff/8003/src/libGLESv2/libGLESv2.cpp#n...
src/libGLESv2/libGLESv2.cpp:4600: gl::Texture *textureObject =
context->getTexture2D();
this should be a gl::Texture2D

[John Bauman, 2011-03-29 05:09:30]

http://codereview.appspot.com/4291066/diff/8003/src/libEGL/Config.cpp
File src/libEGL/Config.cpp (right):

http://codereview.appspot.com/4291066/diff/8003/src/libEGL/Config.cpp#newcode174
src/libEGL/Config.cpp:174: mSurfaceType = asTexture ? EGL_PBUFFER_BIT :
(EGL_PBUFFER_BIT | EGL_WINDOW_BIT | EGL_SWAP_BEHAVIOR_PRESERVED_BIT);
On 2011/03/29 04:33:26, dgkoch wrote:
> On 2011/03/25 18:40:05, apatrick1 wrote:
> > I leave this one to Daniel :)
> 
> Is there a specific reason for this change?  This will prevent existing
configs
> from getting used, and only allowing the new "texture-able" ones from being
> matched here, but is that really necessary?  (I'm not saying it may not be a
> good idea -- I'm just asking if there was a specific reason)

I wanted to make it absolutely clear that only pbuffers would be supported for
BindTexImage. Looking at the spec some more, it seems like just having
BindTexImage give an EGL_BAD_SURFACE error from window surfaces should also
work, but I wanted to be certain. Which way do you prefer?

http://codereview.appspot.com/4291066/diff/8003/src/libEGL/libEGL.cpp
File src/libEGL/libEGL.cpp (right):

http://codereview.appspot.com/4291066/diff/8003/src/libEGL/libEGL.cpp#newcode811
src/libEGL/libEGL.cpp:811: EGLBoolean __stdcall eglBindTexImage(EGLDisplay dpy,
EGLSurface surface, EGLint buffer)
On 2011/03/29 04:33:26, dgkoch wrote:
> The EGL 1.4 spec (p39) for eglBindTexImage mentions implicit glFlush or
glFinish
> depending on if dpy and surface are the current display and/or surface for the
> current thread.  Have you considered this at all?

The only purpose of the finish would seem to be making sure that all the
previous rendering commands are actually reflected in the surface, which seems
to already be the case in this implementation, so going to the extra expense to
do that would be unnecessary. A similar thing goes for the flush. Do we care
enough about actually flushing or finishing the commands to bother with them?

[dgkoch, 2011-03-29 12:48:51]

http://codereview.appspot.com/4291066/diff/8003/src/libEGL/Config.cpp
File src/libEGL/Config.cpp (right):

http://codereview.appspot.com/4291066/diff/8003/src/libEGL/Config.cpp#newcode174
src/libEGL/Config.cpp:174: mSurfaceType = asTexture ? EGL_PBUFFER_BIT :
(EGL_PBUFFER_BIT | EGL_WINDOW_BIT | EGL_SWAP_BEHAVIOR_PRESERVED_BIT);
On 2011/03/29 05:09:30, John Bauman wrote:

> I wanted to make it absolutely clear that only pbuffers would be supported for
> BindTexImage. Looking at the spec some more, it seems like just having
> BindTexImage give an EGL_BAD_SURFACE error from window surfaces should also
> work, but I wanted to be certain. Which way do you prefer?

It seems pretty clear from the spec that eglBindTexImage requires a pbuffer
surface that supports texture binding.  There is a specific error called out if
this is not the case (EGL_BAD_SURFACE).  I'd prefer to leave this validation to
the eglBindTexImage call (where it really has to be done anyways -- I don't
think there is any way to prevent an application from stuffing whatever surface
it wants into that call).

If we go that way, is there even any need to specifically add the two configs
which set "asTexture = true"? It seems like maybe we could just always enable
feature that for the 2 formats which we are supporting it for?

http://codereview.appspot.com/4291066/diff/8003/src/libEGL/libEGL.cpp
File src/libEGL/libEGL.cpp (right):

http://codereview.appspot.com/4291066/diff/8003/src/libEGL/libEGL.cpp#newcode811
src/libEGL/libEGL.cpp:811: EGLBoolean __stdcall eglBindTexImage(EGLDisplay dpy,
EGLSurface surface, EGLint buffer)
On 2011/03/29 05:09:30, John Bauman wrote:
> On 2011/03/29 04:33:26, dgkoch wrote:
> > The EGL 1.4 spec (p39) for eglBindTexImage mentions implicit glFlush or
> glFinish
> > depending on if dpy and surface are the current display and/or surface for
the
> > current thread.  Have you considered this at all?
> 
> The only purpose of the finish would seem to be making sure that all the
> previous rendering commands are actually reflected in the surface, which seems
> to already be the case in this implementation, so going to the extra expense
to
> do that would be unnecessary. A similar thing goes for the flush. Do we care
> enough about actually flushing or finishing the commands to bother with them?

I don't know, do you? :-)  I suspect this might start to matter once you start
going multi-process.

[John Bauman, 2011-03-29 13:06:51]

http://codereview.appspot.com/4291066/diff/8003/src/libEGL/Config.cpp
File src/libEGL/Config.cpp (right):

http://codereview.appspot.com/4291066/diff/8003/src/libEGL/Config.cpp#newcode174
src/libEGL/Config.cpp:174: mSurfaceType = asTexture ? EGL_PBUFFER_BIT :
(EGL_PBUFFER_BIT | EGL_WINDOW_BIT | EGL_SWAP_BEHAVIOR_PRESERVED_BIT);
On 2011/03/29 12:48:51, dgkoch wrote:
> On 2011/03/29 05:09:30, John Bauman wrote:
> 
> > I wanted to make it absolutely clear that only pbuffers would be supported
for
> > BindTexImage. Looking at the spec some more, it seems like just having
> > BindTexImage give an EGL_BAD_SURFACE error from window surfaces should also
> > work, but I wanted to be certain. Which way do you prefer?
> 
> It seems pretty clear from the spec that eglBindTexImage requires a pbuffer
> surface that supports texture binding.  There is a specific error called out
if
> this is not the case (EGL_BAD_SURFACE).  I'd prefer to leave this validation
to
> the eglBindTexImage call (where it really has to be done anyways -- I don't
> think there is any way to prevent an application from stuffing whatever
surface
> it wants into that call).
> 
> If we go that way, is there even any need to specifically add the two configs
> which set "asTexture = true"? It seems like maybe we could just always enable
> feature that for the 2 formats which we are supporting it for?

Sure, that makes sense, and that definitely removes the need for these extra
configs.

http://codereview.appspot.com/4291066/diff/8003/src/libEGL/libEGL.cpp
File src/libEGL/libEGL.cpp (right):

http://codereview.appspot.com/4291066/diff/8003/src/libEGL/libEGL.cpp#newcode811
src/libEGL/libEGL.cpp:811: EGLBoolean __stdcall eglBindTexImage(EGLDisplay dpy,
EGLSurface surface, EGLint buffer)
On 2011/03/29 12:48:51, dgkoch wrote:
> On 2011/03/29 05:09:30, John Bauman wrote:
> > On 2011/03/29 04:33:26, dgkoch wrote:
> > > The EGL 1.4 spec (p39) for eglBindTexImage mentions implicit glFlush or
> > glFinish
> > > depending on if dpy and surface are the current display and/or surface for
> the
> > > current thread.  Have you considered this at all?
> > 
> > The only purpose of the finish would seem to be making sure that all the
> > previous rendering commands are actually reflected in the surface, which
seems
> > to already be the case in this implementation, so going to the extra expense
> to
> > do that would be unnecessary. A similar thing goes for the flush. Do we care
> > enough about actually flushing or finishing the commands to bother with
them?
> 
> I don't know, do you? :-)  I suspect this might start to matter once you start
> going multi-process.
I don't really care about this. Once the application starts going multi-process
(and possibly multi-api) the egl library doesn't really know what process the
object came from and how it's synchronized, so that makes it hard to call finish
in a context that's in another process. The application will need to handle that
itself.

[John Bauman, 2011-03-29 15:10:31]

http://codereview.appspot.com/4291066/diff/8003/src/libGLESv2/Texture.cpp
File src/libGLESv2/Texture.cpp (right):

http://codereview.appspot.com/4291066/diff/8003/src/libGLESv2/Texture.cpp#new...
src/libGLESv2/Texture.cpp:1328: redefineTexture(0, format, surface->getWidth(),
surface->getHeight(), GL_UNSIGNED_BYTE, true);
On 2011/03/29 04:33:26, dgkoch wrote:
> Note that unless we redefine all the textures levels (at least up to
> log2(std::max(width, height)) this will only work with non-mipmapped filters
> (other wise it'll fail the completeness test).

We don't support EGL_MIPMAP_TEXTURE (yet...), so that's probably the correct
result.

[John Bauman, 2011-03-29 15:50:20]

Get rid of the unnecessary new configs, move all of the Texture code into
Texture2D, and fix some of the error checking.

[dgkoch, 2011-03-29 18:35:32]

Hi John,

Almost there…  a couple more egl errors to check, and that's all I could spot.  

I'd also like if Nicolas could look over this for the texture redefinition side
of things, since he just rewrote that and should be most familiar with it.

Daniel

http://codereview.appspot.com/4291066/diff/26001/src/libEGL/libEGL.cpp
File src/libEGL/libEGL.cpp (right):

http://codereview.appspot.com/4291066/diff/26001/src/libEGL/libEGL.cpp#newcod...
src/libEGL/libEGL.cpp:459: if (width == 0 || height == 0)
should also test for width and height < 0 (else BAD_PARAMETER)

http://codereview.appspot.com/4291066/diff/26001/src/libEGL/libEGL.cpp#newcod...
src/libEGL/libEGL.cpp:469: 
should probably add in the validation check mentioned in the spec for the size
if NPOT textures are not fully supported (ie if D3DPTEXTURECAPS_POW2 and/or
D3DPTEXTURECAPS_NONPOW2CONDITIONAL are set in the d3d9 caps).   We don't
currently have a function for this on the Display, but we should add one --
similar to the other Display::get*Support() functions -- check with Al, he may
have written one for the preliminary OES_texture_npot extension that he had
written.

[nicolas, 2011-03-30 15:47:53]

This generally looks good to me. The redefineTexture() behavior isn't entirely
correct for all situations, but that's actually because it wasn't correct before
(it shouldn't set all the image array dirty flags to true). I will fix that
myself later and make sure I don't break this new functionality.

Great job!

[dgkoch, 2011-03-30 16:13:25]

LGTM.

Don't forget to update src/common/version.h when you land this.

[John Bauman, 2011-03-30 18:52:37]

Thanks for the reviews. Now to get commit access (or get someone else to commit
for me)....

[dgkoch, 2011-03-30 19:46:19]

On 2011/03/30 18:52:37, John Bauman wrote:
> Thanks for the reviews. Now to get commit access (or get someone else to
commit
> for me)....

achievement unlocked!

[apatrick, 2011-03-30 20:39:07]

Excellent work jbauman. You landed running. Very impressive.


On Wed, Mar 30, 2011 at 12:46 PM, <daniel@transgaming.com> wrote:

> On 2011/03/30 18:52:37, John Bauman wrote:
>
>> Thanks for the reviews. Now to get commit access (or get someone else
>>
> to commit
>
>> for me)....
>>
>
> achievement unlocked!
>
>
> http://codereview.appspot.com/4291066/
>

[apatrick, 2011-03-30 20:39:59]

I have a change to build_angle.gyp that will automatically generate the
subversion revision number. I will land it soon.

[dgkoch, 2011-03-30 20:45:25]

On 2011-03-30, at 4:38 PM, Alastair Patrick wrote:

> I have a change to build_angle.gyp that will automatically generate the
subversion revision number. I will land it soon.
> 


Nice!  looking forward to having that in.

Daniel