Implement EGL_ANGLE_image_d3d_texture_2d_share_handle and GL_OES_EGL_image.

[apatrick1, 2011-12-09 23:31:07]

Hi Daniel,

This is not ready for a full review. I was wondering if you could take a look at
it and let me know if you agree with the broad approach.

Thanks,

Al

[John Bauman, 2011-12-09 23:36:02]

I'd recommend testing to make sure that the timing is what you want. It's
possible that the runtime enforces that command buffers are processed in order,
but the underlying mechanism for sharing those textures in the same process is
the exact same mechanism as they use to share them between processes, so there's
no guarantee.

[apatrick1, 2011-12-19 21:49:19]

Daniel,

Any thoughts on the general approach here, before I spend time implementing this
properly?

Thanks,

Al

[dgkoch, 2011-12-21 01:59:50]

Sorry for the delay.  Overall I like this approach much better than the previous
ones.

I haven't gone through the patches in great detail, but definitely approve as
this as a way forward. 

I also agree with John that we'll need to do some testing to make sure the
synchronization is correct.  Even if we have to do fencing manually it shouldn't
be a big deal to add.

http://codereview.appspot.com/5472055/diff/1/src/build_angle.gyp
File src/build_angle.gyp (right):

http://codereview.appspot.com/5472055/diff/1/src/build_angle.gyp#newcode240
src/build_angle.gyp:240: 'libEGL/Image.h',
don't forget changes to stand-alone vcproj files as well.

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

http://codereview.appspot.com/5472055/diff/1/src/libEGL/Display.cpp#newcode1118
src/libEGL/Display.cpp:1118: mExtensionString += "EGL_KHR_image_pixmap ";
I guess these should only be advertised for a d3d9ex device?

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

http://codereview.appspot.com/5472055/diff/1/src/libGLESv2/Texture.cpp#newcod...
src/libGLESv2/Texture.cpp:1652: HRESULT result = device->CreateTexture(width,
height, levels, renderable ? D3DUSAGE_RENDERTARGET : 0, format, pool, &mTexture,
handle);
have to make sure this is a d3d9ex device here.

[apatrick1, 2011-12-21 02:01:50]

Thanks Daniel,

I'll go ahead and get this working then....

Al

[apatrick1, 2012-01-06 02:05:16]

I uploaded another patch.

Thanks.

[dgkoch, 2012-01-10 21:48:18]

This is coming along, although I've spotted a number of issues.

The biggest is that texture redefinition (of existing by EGLImage or by
redefining an EGLImage texture) isn't handled properly as specified in
OES_EGL_image (allocate additional space, duplicate data, etc).  It also seems
like EGL Images will be non-functional after a device reset.

Would also like to see the extension spec.

http://codereview.appspot.com/5472055/diff/8001/src/libEGL/Config.h
File src/libEGL/Config.h (right):

http://codereview.appspot.com/5472055/diff/8001/src/libEGL/Config.h#newcode16
src/libEGL/Config.h:16: #include <EGL/eglext.h>
is this necessary?

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

http://codereview.appspot.com/5472055/diff/8001/src/libEGL/Display.cpp#newcod...
src/libEGL/Display.cpp:717: EGLImageKHR Display::createImage(gl::Context
*context, EGLenum target, EGLClientBuffer buffer, const EGLint *attribList)
I'd rather the attribList be parsed in here instead of passing it into the
Image::create function. This would be more consistent with createWindowSurface
and createOffscreenSurface.

http://codereview.appspot.com/5472055/diff/8001/src/libEGL/Display.cpp#newcod...
src/libEGL/Display.cpp:717: EGLImageKHR Display::createImage(gl::Context
*context, EGLenum target, EGLClientBuffer buffer, const EGLint *attribList)
May want to make a note in this function that context == EGL_NO_CONTEXT is
possible (even though we aren't using context at all right now).

http://codereview.appspot.com/5472055/diff/8001/src/libEGL/Display.cpp#newcod...
src/libEGL/Display.cpp:762: // Restore any surfaces that may have been lost
We probably need a way to restore images that may have been released above, or
some way of invalidating the images.  Right now, I expect we'll have a bunch of
useless images after a reset.

http://codereview.appspot.com/5472055/diff/8001/src/libEGL/Image.cpp
File src/libEGL/Image.cpp (right):

http://codereview.appspot.com/5472055/diff/8001/src/libEGL/Image.cpp#newcode2
src/libEGL/Image.cpp:2: // Copyright (c) 2002-2011 The ANGLE Project Authors.
All rights reserved.
use (c) 2012

http://codereview.appspot.com/5472055/diff/8001/src/libEGL/Image.cpp#newcode30
src/libEGL/Image.cpp:30: if (!handle)
Seems like maybe we should validate this at the caller side as well?

http://codereview.appspot.com/5472055/diff/8001/src/libEGL/Image.cpp#newcode45
src/libEGL/Image.cpp:45: switch (attribute[0])
Ugh. Would be nice if we didn't need to add all these attributes, but I guess
there is no way around it in d3d9ex.  With D3D10/11 we could use
OpenSharedResource and then figure out what type it was and use GetDesc to get
the information.

As I mentioned in Display.cpp, I'd prefer if the attribute parsing was done
there, and then just passed in a validated values.

http://codereview.appspot.com/5472055/diff/8001/src/libEGL/Image.cpp#newcode63
src/libEGL/Image.cpp:63: return error(EGL_BAD_ATTRIBUTE,
static_cast<Image*>(NULL));
I believe this should be EGL_BAD_PARAMETER (based on the errors in the spec)

http://codereview.appspot.com/5472055/diff/8001/src/libEGL/Image.h
File src/libEGL/Image.h (right):

http://codereview.appspot.com/5472055/diff/8001/src/libEGL/Image.h#newcode2
src/libEGL/Image.h:2: // Copyright (c) 2002-2011 The ANGLE Project Authors. All
rights reserved.
Use (c) 2012

http://codereview.appspot.com/5472055/diff/8001/src/libEGL/libEGL.cpp
File src/libEGL/libEGL.cpp (right):

http://codereview.appspot.com/5472055/diff/8001/src/libEGL/libEGL.cpp#newcode89
src/libEGL/libEGL.cpp:89: return error(EGL_BAD_DISPLAY, false);
should be EGL_BAD_PARAMETER

http://codereview.appspot.com/5472055/diff/8001/src/libEGL/libEGL.cpp#newcode...
src/libEGL/libEGL.cpp:1213: return EGL_FALSE;
This should return EGL_NO_IMAGE_KHR

http://codereview.appspot.com/5472055/diff/8001/src/libGLESv2/Renderbuffer.cpp
File src/libGLESv2/Renderbuffer.cpp (right):

http://codereview.appspot.com/5472055/diff/8001/src/libGLESv2/Renderbuffer.cp...
src/libGLESv2/Renderbuffer.cpp:181: void Renderbuffer::image(egl::Image* image)
This function needs to set INVALID_OPERATION on any early returns.

http://codereview.appspot.com/5472055/diff/8001/src/libGLESv2/Renderbuffer.cp...
src/libGLESv2/Renderbuffer.cpp:181: void Renderbuffer::image(egl::Image* image)
can we validate the image on the display here?

http://codereview.appspot.com/5472055/diff/8001/src/libGLESv2/Renderbuffer.cp...
src/libGLESv2/Renderbuffer.cpp:183: IDirect3DTexture9* texture =
image->getTexture();
need to verify texture isn't NULL

http://codereview.appspot.com/5472055/diff/8001/src/libGLESv2/Renderbuffer.cp...
src/libGLESv2/Renderbuffer.cpp:185: IDirect3DSurface9* surface;
= NULL

http://codereview.appspot.com/5472055/diff/8001/src/libGLESv2/Renderbuffer.cp...
src/libGLESv2/Renderbuffer.cpp:187: ASSERT(SUCCEEDED(hr));
need to verify surface isn't NULL

http://codereview.appspot.com/5472055/diff/8001/src/libGLESv2/Renderbuffer.cp...
src/libGLESv2/Renderbuffer.cpp:193: ASSERT(SUCCEEDED(hr));
desc seems to be unused.

http://codereview.appspot.com/5472055/diff/8001/src/libGLESv2/Renderbuffer.h
File src/libGLESv2/Renderbuffer.h (right):

http://codereview.appspot.com/5472055/diff/8001/src/libGLESv2/Renderbuffer.h#...
src/libGLESv2/Renderbuffer.h:151: void image(egl::Image* image);
Would prefer a name that makes it clear this is taking an EGL image (like the
texture one).

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

http://codereview.appspot.com/5472055/diff/8001/src/libGLESv2/Texture.cpp#new...
src/libGLESv2/Texture.cpp:2031: void Texture2D::image(egl::Image* image)
any of the early (failure) return paths from this function should generate
INVALID_OPERATION

http://codereview.appspot.com/5472055/diff/8001/src/libGLESv2/Texture.cpp#new...
src/libGLESv2/Texture.cpp:2031: void Texture2D::image(egl::Image* image)
We should probably have the Texture2D keep a pointer to the EGLImage, just so
that we know it's an EGLImage target texture.  This will be necessary so that we
know to handle cleanup/copy of EGL image textures when they are redefined.

http://codereview.appspot.com/5472055/diff/8001/src/libGLESv2/Texture.cpp#new...
src/libGLESv2/Texture.cpp:2031: void Texture2D::image(egl::Image* image)
can we validate the image on the display here?

http://codereview.appspot.com/5472055/diff/8001/src/libGLESv2/Texture.cpp#new...
src/libGLESv2/Texture.cpp:2055: return;
This leaks the texture reference

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

http://codereview.appspot.com/5472055/diff/8001/src/libGLESv2/Texture.h#newco...
src/libGLESv2/Texture.h:297: void image(egl::Image* image);
This is potentially confusing with setImage, and bindTexImage.  Perhaps calling
it bindEGLImage (or something similar) would be better.

http://codereview.appspot.com/5472055/diff/8001/src/libGLESv2/libGLESv2.cpp
File src/libGLESv2/libGLESv2.cpp (right):

http://codereview.appspot.com/5472055/diff/8001/src/libGLESv2/libGLESv2.cpp#n...
src/libGLESv2/libGLESv2.cpp:6156: gl::Texture2D *texture =
context->getTexture2D();
INVALID_OPERATION if there is no texture, or if the texture is immutable.

http://codereview.appspot.com/5472055/diff/8001/src/libGLESv2/libGLESv2.cpp#n...
src/libGLESv2/libGLESv2.cpp:6158: return
texture->image(static_cast<egl::Image*>(image));
feels a bit strange to return the result from a void function from a void
function..

http://codereview.appspot.com/5472055/diff/8001/src/libGLESv2/libGLESv2.cpp#n...
src/libGLESv2/libGLESv2.cpp:6189: return
renderbuffer->image(static_cast<egl::Image*>(image));
ditto with the returns

[apatrick1, 2012-01-10 23:38:34]

Thanks. I addressed some but not all of the issues. You should probably hold off
on reviewing this again until it's done.

I had a thought though. I think, for my purposes, I only need 
GL_OES_EGL_image_external. It is much more limited - only one level allowed, no
redefining with TexImage2D, no renderbuffers, etc.

It might be a bit more work than hooking up TEXTURE_EXTERNAL_OES but I'm leaning
towards not painting us into a corner by supporting the full generality of
TEXTURE_2D.

I'll take a shot at it soon but thought I would run the idea by you first.

http://codereview.appspot.com/5472055/diff/8001/src/libEGL/Config.h
File src/libEGL/Config.h (right):

http://codereview.appspot.com/5472055/diff/8001/src/libEGL/Config.h#newcode16
src/libEGL/Config.h:16: #include <EGL/eglext.h>
On 2012/01/10 21:48:18, dgkoch wrote:
> is this necessary?

It's a bit wierd. I needed to pull in the EGL extension related constants and
typedefs like EGLImageKHR. Putting it in a more sensible place, Display.h as I
recall, causes it to be included by libGLESv2 thing and it blew up the
preprocessor, I forget why exactly.

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

http://codereview.appspot.com/5472055/diff/8001/src/libEGL/Display.cpp#newcod...
src/libEGL/Display.cpp:762: // Restore any surfaces that may have been lost
I could have ImageD3DTexture hold on to the share handle and attempt to bind to
it again?

http://codereview.appspot.com/5472055/diff/8001/src/libEGL/Image.cpp
File src/libEGL/Image.cpp (right):

http://codereview.appspot.com/5472055/diff/8001/src/libEGL/Image.cpp#newcode45
src/libEGL/Image.cpp:45: switch (attribute[0])
Yeah I agonized over that and couldn't figure out a way to do it if the buffer
is a share handle. I messed around with DXGI as well. It doesn't seem to talk
D3D9Ex unfortunately. Moved attrib parsing though.

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

http://codereview.appspot.com/5472055/diff/8001/src/libGLESv2/Texture.cpp#new...
src/libGLESv2/Texture.cpp:2031: void Texture2D::image(egl::Image* image)
I was thinking that because D3D textures are (largely) immutable, the EGLImage
cannot really be redefined. Texture2D holds a reference count on the EGLImage's
D3D texture, so the texture is orphaned. Or do you mean something else? Added
the mImage anyway, it's probably useful.

http://codereview.appspot.com/5472055/diff/8001/src/libGLESv2/libGLESv2.cpp
File src/libGLESv2/libGLESv2.cpp (right):

http://codereview.appspot.com/5472055/diff/8001/src/libGLESv2/libGLESv2.cpp#n...
src/libGLESv2/libGLESv2.cpp:6158: return
texture->image(static_cast<egl::Image*>(image));
typo.

[dgkoch, 2012-01-11 14:00:37]

http://codereview.appspot.com/5472055/diff/8001/src/libEGL/Config.h
File src/libEGL/Config.h (right):

http://codereview.appspot.com/5472055/diff/8001/src/libEGL/Config.h#newcode16
src/libEGL/Config.h:16: #include <EGL/eglext.h>
strange!

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

http://codereview.appspot.com/5472055/diff/8001/src/libEGL/Display.cpp#newcod...
src/libEGL/Display.cpp:762: // Restore any surfaces that may have been lost
On 2012/01/10 23:38:35, apatrick1 wrote:
> I could have ImageD3DTexture hold on to the share handle and attempt to bind
to
> it again?
Yeah that sounds reasonable.  Not sure what would happen if the original texture
was deleted, I guess it would fail?  Presumably you'll delete the associated
image before you delete the original texture though.

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

http://codereview.appspot.com/5472055/diff/8001/src/libGLESv2/Texture.cpp#new...
src/libGLESv2/Texture.cpp:2031: void Texture2D::image(egl::Image* image)
On 2012/01/10 23:38:35, apatrick1 wrote:
> I was thinking that because D3D textures are (largely) immutable, the EGLImage
> cannot really be redefined. Texture2D holds a reference count on the
EGLImage's
> D3D texture, so the texture is orphaned. Or do you mean something else? Added
> the mImage anyway, it's probably useful.

Yes the d3d9 textures are immutable, the issue is that the Texture2D is not.  
As described in OES_EGL_image extension, it's possible to call *TexImage to
redefine the GL texture and that mandates duplicating the EGLImage data into the
new GL allocation.  Really messy stuff actually!

[dgkoch, 2012-01-11 14:07:28]

On 2012/01/10 23:38:34, apatrick1 wrote:
> Thanks. I addressed some but not all of the issues. You should probably hold
off
> on reviewing this again until it's done.
Holding off further reviews for the moment.
 
> I had a thought though. I think, for my purposes, I only need 
> GL_OES_EGL_image_external. It is much more limited - only one level allowed,
no
> redefining with TexImage2D, no renderbuffers, etc.
> 
> It might be a bit more work than hooking up TEXTURE_EXTERNAL_OES but I'm
leaning
> towards not painting us into a corner by supporting the full generality of
> TEXTURE_2D.
> 
> I'll take a shot at it soon but thought I would run the idea by you first.

I definitely like that idea. The interactions with full EGL_Image support and
texture redefinition is rather daunting. While EGL_image_external may seem like
more work to hook up the additional binding point and samplers, I suspect you'll
find it much more straight-forward to implement.

[apatrick1, 2012-01-14 00:54:48]

Daniel,

I implemented GL_EGL_image_external and I think it's workable. I split the EGL
side of things into EGL_ANGLE_image_d3d_surface and
EGL_ANGLE_image_d3d_texture_2d_share_handle, though I think I want to get rid of
the latter.

EGL_ANGLE_image_d3d_surface has a much cleaner interface. eglCreateImageKHR just
takes a IDirect3DSurface9 pointer and figures out the dimensions, format, etc
from that, so no ugly attribute list necessary. It is also possible for ANGLE to
take care of the synchronization internally so long as the application calls
BindTexture appropriately to make its side effects visible to ANGLE.

This was made possible because GL_EGL_image_external I think (by omission) it
does not allow external textures to be attached to FBOs and glCopyTexImage2D
doesn't work.

I allowed all formats that the display reports can be used as textures. I'm a
bit nervous about that but I think ANGLE has no way to get at the bits so maybe
its alright. I also supported any format that can be converted to A8R8G8B8 or
X8R8G8B8 during the copy.

EGL_ANGLE_image_d3d_texture_2d_share_handle works as before except only with
EGL_image_external. What do you think about ditching it? Maybe it could just
live in rietveld until somebody needs it.

An advantage it has is that it avoids a redundant copy in some cases. I have to
do the copy anyway for my use case.

Another advantage it has is, should we ever want to implement GL_EGL_image,
changes made by ANGLE will be visible externally. That is not the case for
EGL_ANGLE_image_d3d_surface.

I added TextureExternal and TextureStorageExternal to GL. The latter represents
an EGLImage sibling. The format implements the reguler Texture interface.

This patch is dependent on http://codereview.appspot.com/5530081/.

I'm sure I missed some related to the new texture target. Also, the use of
referenc counting on EGLImage might be wrong. What happens when an EGLImage is
destroyed is, it decrements the reference count and removes it from the displays
image set so that it is invalid. The object lives on until the last external
texture is destroyed though. That seemed right according to my interpretation of
the spec.

I have done only rudimentary testing at this point. PTAL.

Thanks,

Al

[dgkoch, 2012-01-16 17:22:01]

http://codereview.appspot.com/5472055/diff/21001/include/EGL/eglext.h
File include/EGL/eglext.h (right):

http://codereview.appspot.com/5472055/diff/21001/include/EGL/eglext.h#newcode355
include/EGL/eglext.h:355: #define EGL_D3D_SURFACE_ANGLE	0x3206
Once this is finalized, you'll have to talk to me to get the official enum
value(s) allocated.

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

http://codereview.appspot.com/5472055/diff/21001/src/libEGL/Display.cpp#newco...
src/libEGL/Display.cpp:744: case EGL_IMAGE_WIDTH_ANGLE:
you really only want most of these attribs to only be valid for
EGL_D3D_TEXTURE_2D_SHARE_HANDLE_ANGLE

http://codereview.appspot.com/5472055/diff/21001/src/libEGL/Display.cpp#newco...
src/libEGL/Display.cpp:824: (*image)->reset();
these images still won't recover..

http://codereview.appspot.com/5472055/diff/21001/src/libEGL/Image.cpp
File src/libEGL/Image.cpp (right):

http://codereview.appspot.com/5472055/diff/21001/src/libEGL/Image.cpp#newcode99
src/libEGL/Image.cpp:99: Image *ImageD3DSurface::create(Display *display,
IDirect3DSurface9 *sourceSurface, const ImageAttribs &attribs)
is attribs really needed?

http://codereview.appspot.com/5472055/diff/21001/src/libEGL/Image.cpp#newcode105
src/libEGL/Image.cpp:105: return error(EGL_BAD_ACCESS,
static_cast<Image*>(NULL));
BAD_ACCESS doesn't seem like the most appropriate error.

http://codereview.appspot.com/5472055/diff/21001/src/libEGL/Image.cpp#newcode113
src/libEGL/Image.cpp:113: if (display->canTextureFromFormat(desc.Format))
canTextureFromFormat / canConvertFormat: are reporting what *our* device can do.
 This seems irrelevant since as far as I can tell the conversion (if any) would
happen on the source (external) device.

http://codereview.appspot.com/5472055/diff/21001/src/libEGL/Image.cpp#newcode139
src/libEGL/Image.cpp:139: return error(EGL_BAD_PARAMETER,
static_cast<Image*>(NULL));
leaking the sourceDevice here. Can be fixed by moving the release from below up
to above the FAILED check

http://codereview.appspot.com/5472055/diff/21001/src/libEGL/Image.cpp#newcode145
src/libEGL/Image.cpp:145: sourceDeviceEx->CreateQuery(D3DQUERYTYPE_EVENT,
&sourceQuery);
This could fail...

http://codereview.appspot.com/5472055/diff/21001/src/libEGL/Image.cpp#newcode168
src/libEGL/Image.cpp:168: result = sourceTexture->GetSurfaceLevel(0,
&targetSurface);
shouldn't this be targetTexture?  or should targetSurface be called something
else?  This is all a bit convoluted/confusing...

http://codereview.appspot.com/5472055/diff/21001/src/libEGL/Image.cpp#newcode175
src/libEGL/Image.cpp:175: if (attribs.preserve)
I don't this is the correct use of the preserve attribute.  It indicates if the
contents in the *original* client buffer are preserved.  I don't think we'll
ever muck with those

http://codereview.appspot.com/5472055/diff/21001/src/libEGL/Image.cpp#newcode234
src/libEGL/Image.cpp:234: D3DXLoadSurfaceFromSurface(mTargetSurface, NULL, NULL,
mSourceSurface, NULL, NULL, D3DX_FILTER_NONE, 0);
wouldn't StretchRect be better / more relevant to the canConvert/TextureFormat
checks?

http://codereview.appspot.com/5472055/diff/21001/src/libEGL/Image.cpp#newcode270
src/libEGL/Image.cpp:270: mSourceTexture = sourceTexture;
do you need mSourceTexture?

http://codereview.appspot.com/5472055/diff/21001/src/libEGL/Image.h
File src/libEGL/Image.h (right):

http://codereview.appspot.com/5472055/diff/21001/src/libEGL/Image.h#newcode46
src/libEGL/Image.h:46: virtual void release();
can you make Image a RefCountObject for the reference tracking?

http://codereview.appspot.com/5472055/diff/21001/src/libEGL/Image.h#newcode48
src/libEGL/Image.h:48: virtual void reset() = 0;
reset seems like a poor name since it only really releases stuff.

http://codereview.appspot.com/5472055/diff/21001/src/libGLESv2/Context.cpp
File src/libGLESv2/Context.cpp (right):

http://codereview.appspot.com/5472055/diff/21001/src/libGLESv2/Context.cpp#ne...
src/libGLESv2/Context.cpp:1050: if (oldTexture != newTexture)
if "bound" forces a refresh from the external image, don't you actually want a
rebind of the same texture to do something?

http://codereview.appspot.com/5472055/diff/21001/src/libGLESv2/Context.cpp#ne...
src/libGLESv2/Context.cpp:3445: mExtensionString += "GL_OES_EGL_image_external
";
presumably this should only be on d3d9ex devices?

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

http://codereview.appspot.com/5472055/diff/21001/src/libGLESv2/Texture.cpp#ne...
src/libGLESv2/Texture.cpp:2518: mTexStorage = new TextureStorageExternal(image);
texture is leaked.

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

http://codereview.appspot.com/5472055/diff/21001/src/libGLESv2/Texture.h#newc...
src/libGLESv2/Texture.h:350: egl::Image *mEGLImage;
if the egl::Image is a RefCountObject you could use a BindingPointer here.

http://codereview.appspot.com/5472055/diff/21001/src/libGLESv2/utilities.cpp
File src/libGLESv2/utilities.cpp (right):

http://codereview.appspot.com/5472055/diff/21001/src/libGLESv2/utilities.cpp#...
src/libGLESv2/utilities.cpp:334: bool IsInternalTextureTarget(GLenum target)
it might be nice to do this change as a separate patch first.

[dgkoch, 2012-01-16 17:26:27]

On 2012/01/14 00:54:48, apatrick1 wrote:
> Daniel,
> 
> I implemented GL_EGL_image_external and I think it's workable. I split the EGL
> side of things into EGL_ANGLE_image_d3d_surface and
> EGL_ANGLE_image_d3d_texture_2d_share_handle, though I think I want to get rid
of
> the latter.
> 
> EGL_ANGLE_image_d3d_surface has a much cleaner interface. eglCreateImageKHR
just
> takes a IDirect3DSurface9 pointer and figures out the dimensions, format, etc
> from that, so no ugly attribute list necessary. It is also possible for ANGLE
to
> take care of the synchronization internally so long as the application calls
> BindTexture appropriately to make its side effects visible to ANGLE.
> 
> This was made possible because GL_EGL_image_external I think (by omission) it
> does not allow external textures to be attached to FBOs and glCopyTexImage2D
> doesn't work.
No, this is by design. The EXT_image_external spec says:
  "There is no support for most of the functions that manipulate
    other texture targets (e.g. you cannot use gl*Tex*Image*() functions with
    TEXTURE_EXTERNAL_OES)"

> 
> I allowed all formats that the display reports can be used as textures. I'm a
> bit nervous about that but I think ANGLE has no way to get at the bits so
maybe
> its alright. I also supported any format that can be converted to A8R8G8B8 or
> X8R8G8B8 during the copy.
> 
> EGL_ANGLE_image_d3d_texture_2d_share_handle works as before except only with
> EGL_image_external. What do you think about ditching it? Maybe it could just
> live in rietveld until somebody needs it.
I'm fine with ditching it (see below).

> An advantage it has is that it avoids a redundant copy in some cases. I have
to
> do the copy anyway for my use case.
Would you have to do the copy if you were using the other api?

> Another advantage it has is, should we ever want to implement GL_EGL_image,
> changes made by ANGLE will be visible externally. That is not the case for
> EGL_ANGLE_image_d3d_surface.
It would also supports cross-process images a bit nicer.  Do you need
cross-process at all?
With a pointer we need to be in the same process space.

> 
> I added TextureExternal and TextureStorageExternal to GL. The latter
represents
> an EGLImage sibling. The format implements the reguler Texture interface.
> 
> This patch is dependent on http://codereview.appspot.com/5530081/.
> 
> I'm sure I missed some related to the new texture target. Also, the use of
> referenc counting on EGLImage might be wrong. What happens when an EGLImage is
> destroyed is, it decrements the reference count and removes it from the
displays
> image set so that it is invalid. The object lives on until the last external
> texture is destroyed though. That seemed right according to my interpretation
of
> the spec.
> 
> I have done only rudimentary testing at this point. PTAL.

I looked mostly at the EGL/Image side and not so much at the GL/Texture side
> 
> Thanks,
> 
> Al

[apatrick1, 2012-01-23 21:35:48]

On 2012/01/16 17:26:27, dgkoch wrote:
> > I allowed all formats that the display reports can be used as textures. I'm
a
> > bit nervous about that but I think ANGLE has no way to get at the bits so
> maybe
> > its alright. I also supported any format that can be converted to A8R8G8B8
or
> > X8R8G8B8 during the copy.
> > 
> > EGL_ANGLE_image_d3d_texture_2d_share_handle works as before except only with
> > EGL_image_external. What do you think about ditching it? Maybe it could just
> > live in rietveld until somebody needs it.
> I'm fine with ditching it (see below).
> 
I will do that. I'll upload EGL_ANGLE_image_d3d_texture_2d_share_handle so it
can be restored in the future if it's useful. Feel free not to not review it.

> > An advantage it has is that it avoids a redundant copy in some cases. I have
> to
> > do the copy anyway for my use case.
> Would you have to do the copy if you were using the other api?
> 
No. In general the D3D surfaces produced by DXVA are not from textures. That is
my use-case for now. I suppose if the surfaces did turn out to sometimes be
textures I could optimize for that using
EGL_ANGLE_image_d3d_texture_2d_share_handle but I have no evidence that that
ever happens.

> > Another advantage it has is, should we ever want to implement GL_EGL_image,
> > changes made by ANGLE will be visible externally. That is not the case for
> > EGL_ANGLE_image_d3d_surface.
> It would also supports cross-process images a bit nicer.  Do you need
> cross-process at all?
> With a pointer we need to be in the same process space.
> 
I don't need that either, although it might be useful in the future. An
alternative would be for the caller to create a D3D device and create a texture
from the handle and then pass that texture into EGL_ANGLE_image_d3d_surface.

This is ready for another round of review I think.

I think I can also write the spec now, unless you see anything that will have to
fundamentally change?

Al

[apatrick1, 2012-01-25 00:08:02]

Sorry, forgot to upload replies.

http://codereview.appspot.com/5472055/diff/21001/include/EGL/eglext.h
File include/EGL/eglext.h (right):

http://codereview.appspot.com/5472055/diff/21001/include/EGL/eglext.h#newcode355
include/EGL/eglext.h:355: #define EGL_D3D_SURFACE_ANGLE	0x3206
On 2012/01/16 17:22:01, dgkoch wrote:
> Once this is finalized, you'll have to talk to me to get the official enum
> value(s) allocated.

Will do.

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

http://codereview.appspot.com/5472055/diff/21001/src/libEGL/Display.cpp#newco...
src/libEGL/Display.cpp:744: case EGL_IMAGE_WIDTH_ANGLE:
On 2012/01/16 17:22:01, dgkoch wrote:
> you really only want most of these attribs to only be valid for
> EGL_D3D_TEXTURE_2D_SHARE_HANDLE_ANGLE

Done.

http://codereview.appspot.com/5472055/diff/21001/src/libEGL/Display.cpp#newco...
src/libEGL/Display.cpp:824: (*image)->reset();
On 2012/01/16 17:22:01, dgkoch wrote:
> these images still won't recover..

I put in a best effort attempt to restore these images. It assumes that the
external device has not been lost. I think in practice, the application should
destroy all EGLImages associated with its lost devices before expecting ANGLE to
be able to recover. I can put something along those lines in the spec.

http://codereview.appspot.com/5472055/diff/21001/src/libEGL/Image.cpp
File src/libEGL/Image.cpp (right):

http://codereview.appspot.com/5472055/diff/21001/src/libEGL/Image.cpp#newcode99
src/libEGL/Image.cpp:99: Image *ImageD3DSurface::create(Display *display,
IDirect3DSurface9 *sourceSurface, const ImageAttribs &attribs)
On 2012/01/16 17:22:01, dgkoch wrote:
> is attribs really needed?

Not anymore.

http://codereview.appspot.com/5472055/diff/21001/src/libEGL/Image.cpp#newcode105
src/libEGL/Image.cpp:105: return error(EGL_BAD_ACCESS,
static_cast<Image*>(NULL));
On 2012/01/16 17:22:01, dgkoch wrote:
> BAD_ACCESS doesn't seem like the most appropriate error.

Done.

http://codereview.appspot.com/5472055/diff/21001/src/libEGL/Image.cpp#newcode113
src/libEGL/Image.cpp:113: if (display->canTextureFromFormat(desc.Format))
On 2012/01/16 17:22:01, dgkoch wrote:
> canTextureFromFormat / canConvertFormat: are reporting what *our* device can
do.
>  This seems irrelevant since as far as I can tell the conversion (if any)
would
> happen on the source (external) device.

I think we want to use the internal device to determine whether ANGLE can use a
particular format for texturing and the external device to determine whether the
conversion is possible.

http://codereview.appspot.com/5472055/diff/21001/src/libEGL/Image.cpp#newcode139
src/libEGL/Image.cpp:139: return error(EGL_BAD_PARAMETER,
static_cast<Image*>(NULL));
On 2012/01/16 17:22:01, dgkoch wrote:
> leaking the sourceDevice here. Can be fixed by moving the release from below
up
> to above the FAILED check

I simplified this by using a smart pointer for D3D objects.

http://codereview.appspot.com/5472055/diff/21001/src/libEGL/Image.cpp#newcode145
src/libEGL/Image.cpp:145: sourceDeviceEx->CreateQuery(D3DQUERYTYPE_EVENT,
&sourceQuery);
On 2012/01/16 17:22:01, dgkoch wrote:
> This could fail...

Added a comment noting this.

http://codereview.appspot.com/5472055/diff/21001/src/libEGL/Image.cpp#newcode168
src/libEGL/Image.cpp:168: result = sourceTexture->GetSurfaceLevel(0,
&targetSurface);
On 2012/01/16 17:22:01, dgkoch wrote:
> shouldn't this be targetTexture?  or should targetSurface be called something
> else?  This is all a bit convoluted/confusing...

Two things. It's not a good distinction. I renamed source to external and target
to internal. Second, this is a bug. The only reason it worked was because
D3DXLoadSurfaceFromSurface is smart enough to recognize that two surfaces belong
to different devices and somehow make it work, maybe a readback.

http://codereview.appspot.com/5472055/diff/21001/src/libEGL/Image.cpp#newcode175
src/libEGL/Image.cpp:175: if (attribs.preserve)
On 2012/01/16 17:22:01, dgkoch wrote:
> I don't this is the correct use of the preserve attribute.  It indicates if
the
> contents in the *original* client buffer are preserved.  I don't think we'll
> ever muck with those

It was an optimization to remove a redundant and potentially expensive surface
copy in the event the caller does not expect the surface to be preserved. I
think the right thing to do though is to defer the first copy to
EGLImageTargetTexture2DOES.

http://codereview.appspot.com/5472055/diff/21001/src/libEGL/Image.cpp#newcode234
src/libEGL/Image.cpp:234: D3DXLoadSurfaceFromSurface(mTargetSurface, NULL, NULL,
mSourceSurface, NULL, NULL, D3DX_FILTER_NONE, 0);
On 2012/01/16 17:22:01, dgkoch wrote:
> wouldn't StretchRect be better / more relevant to the canConvert/TextureFormat
> checks?

D3DXLoadSurfaceFromSurface does a StretchRect behind the scenes. I confirmed it
calls it. My idea was that it can also handle external surfaces that would not
work with StretchRect, like those in the system memory pool.

http://codereview.appspot.com/5472055/diff/21001/src/libEGL/Image.cpp#newcode270
src/libEGL/Image.cpp:270: mSourceTexture = sourceTexture;
On 2012/01/16 17:22:01, dgkoch wrote:
> do you need mSourceTexture?

No. Done.

http://codereview.appspot.com/5472055/diff/21001/src/libEGL/Image.h
File src/libEGL/Image.h (right):

http://codereview.appspot.com/5472055/diff/21001/src/libEGL/Image.h#newcode46
src/libEGL/Image.h:46: virtual void release();
On 2012/01/16 17:22:01, dgkoch wrote:
> can you make Image a RefCountObject for the reference tracking?

Done.

http://codereview.appspot.com/5472055/diff/21001/src/libEGL/Image.h#newcode48
src/libEGL/Image.h:48: virtual void reset() = 0;
On 2012/01/16 17:22:01, dgkoch wrote:
> reset seems like a poor name since it only really releases stuff.

Renamed to releaseResources so as not to conflict with
RefCountObject::release().

http://codereview.appspot.com/5472055/diff/21001/src/libGLESv2/Context.cpp
File src/libGLESv2/Context.cpp (right):

http://codereview.appspot.com/5472055/diff/21001/src/libGLESv2/Context.cpp#ne...
src/libGLESv2/Context.cpp:1050: if (oldTexture != newTexture)
On 2012/01/16 17:22:01, dgkoch wrote:
> if "bound" forces a refresh from the external image, don't you actually want a
> rebind of the same texture to do something?

It was my interpretation of:

        "Binding (or re-binding if
        already bound) an external texture by calling BindTexture after all
        modifications are complete guarantees that sampling done in future
        draw calls will return values corresponding to the values in the
        buffer at or after the time that BindTexture is called."

But I think you're right.

http://codereview.appspot.com/5472055/diff/21001/src/libGLESv2/Context.cpp#ne...
src/libGLESv2/Context.cpp:3445: mExtensionString += "GL_OES_EGL_image_external
";
On 2012/01/16 17:22:01, dgkoch wrote:
> presumably this should only be on d3d9ex devices?

Done.

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

http://codereview.appspot.com/5472055/diff/21001/src/libGLESv2/Texture.cpp#ne...
src/libGLESv2/Texture.cpp:2506: texture->Release();
Actually I think the error is here. Image::getTexture does not AddRef.

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

http://codereview.appspot.com/5472055/diff/21001/src/libGLESv2/Texture.h#newc...
src/libGLESv2/Texture.h:350: egl::Image *mEGLImage;
On 2012/01/16 17:22:01, dgkoch wrote:
> if the egl::Image is a RefCountObject you could use a BindingPointer here.

Done.

http://codereview.appspot.com/5472055/diff/21001/src/libGLESv2/utilities.cpp
File src/libGLESv2/utilities.cpp (right):

http://codereview.appspot.com/5472055/diff/21001/src/libGLESv2/utilities.cpp#...
src/libGLESv2/utilities.cpp:334: bool IsInternalTextureTarget(GLenum target)
On 2012/01/16 17:22:01, dgkoch wrote:
> it might be nice to do this change as a separate patch first.

Already landed.