KhronosGroup / OpenGL-Registry

OpenGL, OpenGL ES, and OpenGL ES-SC API and Extension Registry
678 stars 274 forks source link

EGL_EXT_image_dma_buf_import / EXT_EGL_image_storage interactions not relevant anymore #536

Open bl4ckb0ne opened 2 years ago

bl4ckb0ne commented 2 years ago

295 is limiting the interaction with EGL_EXT_image_dma_buf_import and EXT_EGL_image_storage based on the fact that the internal storage format can't be specified, and another concern about YUV buffers.

I am fixing the mesa impl of EXT_EGL_image_storage and I don't think the statement is true anymore. DRM fourcc formats 1 has YUV formats, and users are able to detect a YUV format during import and specify EGL_YUV_COLOR_SPACE_HINT_EXT. For the internal format, mesa 2 seems to have no issues about that.

I don't know the state of the other linux driver (nvidia), but I'd like to know if this limitation could be lifted from the spec.

stonesthrow commented 2 years ago

@cubanismo for comment about NVidia

@bl4ckb0ne what is your proposal to change?

bl4ckb0ne commented 2 years ago

Simply remove the paragraph about DMABUF limitations

If the EGL image was created using EGL_EXT_image_dma_buf_import, then the following applies:

  • must be GL_TEXTURE_2D or GL_TEXTURE_EXTERNAL_OES. Otherwise, the error INVALID_OPERATION is generated.
  • if is GL_TEXTURE_2D, then the resultant texture must have a sized internal format which is colorspace and size compatible with the dma-buf. If the GL is unable to determine such a format, the error INVALID_OPERATION is generated.
  • if is GL_TEXTURE_EXTERNAL_OES, the internal format of the texture is implementation defined.
stonesthrow commented 2 years ago

Understood. remove format ambiguity and expect format from FourCC. I hope we get some feedback here, and then proceed with a MR.

cubanismo commented 2 years ago

I don't understand what is in conflict with the paragraph you're suggesting be removed. Which part of that paragraph is causing issues, and can you clarify what the issues are?

bl4ckb0ne commented 2 years ago

I don't find the interaction with EGL_EXT_image_dma_buf_import relevant anymore, why limit to only 2D textures if other textures can be exported as DMABUFs

It goes in pair with https://github.com/KhronosGroup/EGL-Registry/pull/165 for what I am trying to achieve, sending a whole cubemap through DMABUF.

cubanismo commented 2 years ago

I see. I thought the concern was something about formats based on the description.

If you'd like to use other texture types, how do you determine whether the implementation supports them with a given image, or at all? I would think a new extension string is needed at minimum to differentiate any support from no support (I.e., old drivers at least), but additionally our format modifiers would likely be different for different target types, so you'd likely need to extend the format modifier query mechanism as well to handle this more generally on NVIDIA hardware.

bl4ckb0ne commented 2 years ago

The texture availablility would be defined by EGL_KHR_gl_image (and EGL_MESA_gl_texture_cubemap_image eventually), on both side (exporting and importing the DMABUF). I would not be against splitting EXT_EGL_image_storage in 2, one to use a regular EGLImage as storage, and something like EXT_EGL_image_dmabuf_storage to use DMABUF images as stroage, that would make it more clear.

Regarding format modifiers, do you mean different kind of textures like GL_TEXTURE_2D_ARRAY, GL_TEXTURE_3D or GL_TEXTURE_CUBE_MAP would require specific modifier or modifiers would have to adapt to how those textures are stored internally?

cubanismo commented 2 years ago

The texture availablility would be defined by EGL_KHR_gl_image (and EGL_MESA_gl_texture_cubemap_image eventually), on both side (exporting and importing the DMABUF).

I'm not clear what the issue is then. Wouldn't EGL_MESA_gl_texture_cubemap_image just define an interaction with EXT_EGL_image_storage that adds cube maps to the allowed targets for dma-bufs if that is its intent?

Regarding format modifiers, do you mean different kind of textures like GL_TEXTURE_2D_ARRAY, GL_TEXTURE_3D or GL_TEXTURE_CUBE_MAP would require specific modifier or modifiers would have to adapt to how those textures are stored internally?

Yes. There are reserved/currently unused fields in NV_DRM_FORMAT_MODIFIER_BLOCK_LINEAR_2D() to handle these layouts in anticipation of someone adding such usage, but the macro and associated Mesa (And NV) driver code would have to be modified to enable their use. Additional changes would be needed to support the multisample texture targets. I don't know whether or not other HW has similar issues. That's all fine, my point was just that existing drivers won't just automatically support all targets, so the interaction text can't just be deleted from an existing extension. A new extension is needed to modify it and allow apps to be aware the additional support is present, and to query specific layouts for the new usage.

bl4ckb0ne commented 2 years ago

Wouldn't EGL_MESA_gl_texture_cubemap_image just define an interaction with EXT_EGL_image_storage that adds cube maps to the allowed targets for dma-bufs if that is its intent?

You're right sorry, I got confused there. EGL_MESA_gl_texture_cubemap_image would define interaction with EXT_EGL_image_storage but would not impact dma-buf images directly.

So the new extension will modify this paragraph from EXT_EGL_image_storage about how dmabuf images are loaded into textures target, and return GL_INVALID_OPERATION if the driver does not support loading the requested texture.

cubanismo commented 2 years ago

I don't think returning GL_INVALID_OPERATION is sufficient. I don't think the Mesa extension is functionally complete without a way to query the correct format modifiers for such targets, as applications have no way to figure out why such an operation is invalid, nor how to get the right format modifiers (Returning the new modifiers from existing queries would be wrong, since those are intended to work as regular 2D or external textures).

bl4ckb0ne commented 2 years ago

I was thinking about something that would mirror the eglQueryDmaBufModifiersEXT function from EXT_image_dma_buf_import_modifiers 1

GLboolean glGetTextureDmaBufModifiersEXT(
                            GLenum target,
                            GLint internalFormat,
                            GLsizei max_modifiers,
                            GLuint64 *modifiers,
                            GLsizei *num_modifiers)
cubanismo commented 2 years ago

Yes, I believe something along those lines would be sufficient. It's a little non-orthogonal that one's in OGL and one's in EGL, but I'm not clear what drove the original extension's EGL-focused design, so I can't comment on whether one is preferable over the other. The only advantage of having it in EGL I can think of is that you can query it prior to context creation, which makes it easier to query it prior to surface creation, which would be useful in some extensions we're working on to create EGL surfaces out of EGLImages, but I'm not clear we're ever going to have non-GL_TEXTURE_2D-based EGLSurface color images.