Closed GoogleCodeExporter closed 9 years ago
- Do you have specifications for the two new EGL extensions? I'd really like these to be documented (even if we just host them on the project page here -- but at the same point, I don't see a good reason not to put them in the EGL registry either).
- Do you have an enum value for EGL_D3D_TEXTURE_SHARE_HANDLE_ANGLE? I don't see that defined anywhere. We'll have to query the EGL registry maintainer to allocate a value in the EGL namespace for this token. We should probably just request a block for ANGLE. Would you like me to do this?
Patch comments (line #'s based on the patch in the viewer)
- line 75: Surface constructor - should probably initialize mWindowSubclassed = false;
- libEGL.cpp looks like you implemented eglCreatePixmapSurface not eglCreatePbufferSurface. Was this a merging problem?
- libEGL.cpp the EGL_TEXTURE_TARGET case statement has duplicated code in it.
- re: eglQuerySurfacePointerANGLE - I guess you didn't want to just shoehorn a pointer into the EGLint parameter from eglQuerySurface?
Overall looks pretty good - presuming it works :-)
Original comment by dan...@transgaming.com
on 7 Dec 2010 at 4:27
Oh wow, massive bad merge. that's what I get for trying to get the patch up
before falling asleep :-) will fix and write up the extension specs.
For the enum value, would you mind requesting a block for ANGLE? I was using a
temporary value from the vendor block I placed in a new eglangle.h, but I'm not
sure if that's the right place for it to live (whatever the actual value is).
I didn't want to modify the upstream egl.h/eglext.h headers.
Original comment by vladim...@gmail.com
on 7 Dec 2010 at 7:24
EGL enums have been requested (Bug 7139) hopefully it doesn't take too long.
I believe the proper place to include the new defines is in
include/EGL/eglext.h (see include/GLES2/gl2ext.h for the GLES2 ANGLE
extensions). Eventually these will get propagated back into the upstream
headers.
Original comment by dan...@transgaming.com
on 7 Dec 2010 at 7:45
Ok, here's an updated patch. Also fixed some of the config attribs (max
pbuffer size). Also attached are the two extension specs.
Original comment by vladim...@gmail.com
on 7 Dec 2010 at 10:33
Attachments:
Additional patch -- make BindTexImage/ReleaseTexImage just return FALSE,
instead of triggering UNIMPLEMENTED(). This allows common code paths to
attempt to bind and get a failure signal.
Original comment by vladim...@gmail.com
on 14 Dec 2010 at 8:54
Attachments:
ANGLE_query_surface_pointer.txt
- looks good overall.
- slight grammar issue in the Overview: "This extension adds allows querying"
- Dependencies says it's written against the EGL 1.4 spec, but edit in Chapter 3 is listed against the 1.2 spec. Edit should be made against the 1.4 spec instead.
- line 72-3: "queried via eglQuerySurfacePointerANGLE is not of type 'pointer'," <-- add "of type"
- should probably include text which says something to the effect that "eglQuerySurfacePointerANGLE behaves identically to eglQuerySurface, except that …." Either that, or you need do add language describing error behaviour and what the other parameters are for.
EGL_ANGLE_surface_d3d_share_handle.txt
- I wonder if we should clarify that this is a d3d9ex share handle… I could imagine that we might want to expose a dxgi1.1/d3d10.1 surface sharing at some point in the future. Would we use the same query token? How would the application know if it needed to use D3D9Ex or DXGI1.1/D3D10.1?
- the enum value you selected is in our reserved range, so consider it allocated for this now.
- again, the edits should be done against the EGL 1.4 spec.
angle-pbuffers.patch (using the patch line numbers)
- line 20 - should #define EGL_ANGLE_surface_d3d_share_handle to 1 here.
- line 81-83: are EGL_MAX_PBUFFER_{WIDTH,HEIGHT,PIXELS} actually supposed to be
used for matching in ConfigSet::getConfigs? EGL 1.4 spec p 25 says "If
EGL_MAX_PBUFFER_WIDTH, EGL_MAX_PBUFFER_HEIGHT, EGL_MAX_- PBUFFER_PIXELS, or
EGL_NATIVE_VISUAL_ID are specified in attrib list, then they are ignored
(however, if present, these attributes must still be followed by an attribute
value in attrib list)." I suspect we should not be setting "match" in those
cases, but just skipping over those entries (not 100% certain about this
though).
- otherwise it's looking pretty good to me.
Original comment by dan...@transgaming.com
on 21 Dec 2010 at 8:27
egl-bind-tex-image-fix.patch: looks good too btw.
Original comment by dan...@transgaming.com
on 21 Dec 2010 at 10:01
> - I wonder if we should clarify that this is a d3d9ex share handle… I
could imagine that we might want to expose a dxgi1.1/d3d10.1 surface sharing at
some point in the future. Would we use the same query token? How would the
application know if it needed to use D3D9Ex or DXGI1.1/D3D10.1?
It's actually the same share token whether it's used with D3D9Ex or DXGI/D3D10.
(In Firefox we actually take the token and call d3d10 OpenSharedResource on it
to get a d3d10 2d texture for use with our d3d10 compositor.) I'll make that
clear, though.
I'll update with the other comments -- I thought I was using EGL 1.4, but I may
have an older PDF downloaded (whoops!)
Original comment by vladim...@gmail.com
on 21 Dec 2010 at 10:38
> It's actually the same share token whether it's used with D3D9Ex or
DXGI/D3D10. (In Firefox we actually take the token and call d3d10
OpenSharedResource on it to get a d3d10 2d texture for use with our d3d10
compositor.) I'll make that clear, though.
Ah, that's good to know. What you have looks good then.
Original comment by dan...@transgaming.com
on 21 Dec 2010 at 10:56
Updated the texture specs. I was using the correct EGL doc, I just copy-pasted
the header from an older extension and didn't update.
Fixed the typos, and I added a paragraph explaining expected usage of the share
handle from the D3D side. (In particular, on the d3d9 side, "level" must be 1,
otherwise I have no idea what happens if there's a mismatch.)
I also changed the token to EGL_D3D_TEXTURE_2D_SHARE_HANDLE_ANGLE, including
the "2D" to make clear what type of object handle is being retreived. I also
renamed the extension to the (rather long)
EGL_ANGLE_surface_d3d_texture_2d_share_handle as well, but I think that's more
accurate should we add other share tokens in the future. I can change
either/both of these changes back though, if you think they're too wordy.
I'll update the patch shortly.
Original comment by vladim...@gmail.com
on 22 Dec 2010 at 1:38
Attachments:
Would implementing one of the existing EGL extensions:
EGL_KHR_image
GL_OES_EGL_image
(instead of defining new ones) work for the purposes of sharing images between
processes?
Original comment by vange...@google.com
on 12 Jan 2011 at 12:50
Going the EGL_image route would work, and I considered it, but it's
significantly more complex. We'd have to implement EGL_KHR_image_base, and
we'd still have to define an EGL_KHR_image_d3d_share_handle (similar to
EGL_KHR_image_pixmap; assuming we want a d3d share handle passed in). Then,
we'd have to implement EGL_KHR_gl_texture_2D_image (and/or renderbuffer_image).
The patch here doesn't help if you want to do rendering with ANGLE across
processes (or even across devices) -- to do that, you'd have to implement the
full EGL image mechanism, as well as probably OES_EGL_image on the GLESv2 side.
The mechanism here only helps with getting access to an ANGLE offscreen
surface render target that can be used as a texture for D3D rendering.
Original comment by vladim...@gmail.com
on 12 Jan 2011 at 1:21
Why not just implement WGL_NV_DX_interop? , ok we are talking about EGL and not
WGL but anyway that's windows specific and also should allow using common path
in firefox for using angle or ogl drivers..
Original comment by rtf...@gmail.com
on 13 Jan 2011 at 10:49
WGL_NV_DX_interop is also much more extensive than is necessary, and would
require defining a new extension that works with EGL.
Original comment by vladim...@gmail.com
on 13 Jan 2011 at 10:57
NV_DX_interop seems to be for using DX objects in GL, not so much for getting
DX objects from GL. I could see this potentially being more useful for getting
DXVA decoded video frames into ANGLE (although I could also see using EGL_image
for that purpose).
I tend to agree with Vlad here. EGL_image and/or NV_DX_interop type of
functionality might be good eventual goals, but for the shorter-term it doesn't
seem like an exactly match and is significantly more work than necessary for
just exposing the share handle.
Original comment by dan...@transgaming.com
on 14 Jan 2011 at 1:11
Updated patch for latest ANGLE. Was pretty straightforward.
Original comment by vladim...@gmail.com
on 31 Jan 2011 at 7:37
Attachments:
- for the extension string, I would prefer it be built up in the Display class
and retrieved in eglQueryString instead of hardcoding it here (similar to how
it is done with Context->initExtensionString/getExtensionString in libGLESv2).
This will allow us to dynamically turn extensions on and off easier.
- in particular, there doesn't seem to be much point in advertising
EGL_ANGLE_surface_d3d_texture_2d_share_handle if we're not on a d3d9ex device.
- This doesn't actually implement EGL_ANGLE_surface_d3d_texture_2d_share_handle
- it uses the token names and strings from an earlier draft where it was called
EGL_ANGLE_surface_d3d_share_handle (I do prefer the more explicit 2D naming).
Other than that, it and the extensions look good.
Original comment by dan...@transgaming.com
on 1 Feb 2011 at 8:28
Updated for the above (whoops on the wrong extension name!)
Original comment by vladim...@gmail.com
on 4 Feb 2011 at 1:56
Attachments:
This version looks good. Can you commit it? Once in, I'll submit the
extensions to the EGL registry (unless you'd rather do that yourself).
(Note that I just approved a patch from Al to rename mD3d9ex to mD3d9Ex etc, so
depending on the order they go in, you might need to update this patch
slightly).
Original comment by dan...@transgaming.com
on 9 Feb 2011 at 2:05
Checked in as rev 558 (with D3d9Ex change -- you capitalized the E, but not the
second D?! :-). If you want to submit the extensions, go for it; I'm not sure
what the process is. Otherwise, drop me an email with the process and I can do
it.
Original comment by vladim...@gmail.com
on 11 Feb 2011 at 12:56
I'll submit the extensions.
Original comment by dan...@transgaming.com
on 11 Feb 2011 at 2:22
Original issue reported on code.google.com by
vladim...@gmail.com
on 7 Dec 2010 at 5:51Attachments: