GNOME Bugzilla – Bug 768602
EGL_DMA_Buf: Wrong attribute list type for EGL 1.5
Last modified: 2016-08-16 05:35:28 UTC
Below attribute list type is wrong for EGL 1.5. It is right for EGL version below 1.5. GstEGLImage * gst_egl_image_from_dmabuf (GstGLContext * context, gint dmabuf, GstVideoInfo * in_info, gint plane, gsize offset) { ... EGLint attribs[13]; ... } For EGL 1.5 spec, the attribute list type should be EGLAttrib. EGLAttrib is same as intptr_t. intptr_t will be 64bit type on 64bit system. So create EGL image from DMA Buf FD will fail and then fall back to raw data upload. Below is the state in GEL 1.5 spec: "EGLAttrib is an integral type defined to be equivalent to the ISO C intptr_t type. It is used in the commands eglCreateImage, eglCreateSync, eglCreatePlatformWindowSurface, eglCreatePlatformPixmapSurface, eglGetPlatformDisplay, and eglGetSyncAttrib, and will be used for all similar commands in the future which take attribute lists or return attribute values, since such commands might at some point need to represent handle and pointer values in attribute lists as well as other integral types"
Do you want to provide a patch? It shouldn't be an #ifdef though, it should be a runtime check for the EGL version and then two code paths.
How come there is a difference between (EGLInt *) and (intptr_t) ? What error do you get ?
On 64bit platform, EGLInt is 32 bit value. intptr_t is 64 bit value point. So the attribute value will be wrong.
as EGLAttrib only defined in EGL 1.5, so i should use #ifdef.
Yes but you additionally need the runtime check too... so that using EGL 1.4 or below still works when having compiled with EGL 1.5.
Created attachment 331389 [details] [review] Patch to fix the issue.
Created attachment 331391 [details] [review] Replace old patch
Review of attachment 331389 [details] [review]: ::: gst-libs/gst/gl/egl/gsteglimage.c @@ +261,3 @@ GST_VIDEO_INFO_COMP_HEIGHT (in_info, plane)); +#ifdef EGL_VERSION_1_5 It must be runtime check, since you coul dhave EGL 1.5 but run using older context.
Review of attachment 331391 [details] [review]: .
Review of attachment 331391 [details] [review]: sorry.
Review of attachment 331391 [details] [review]: ::: gst-libs/gst/gl/egl/gsteglimage.c @@ +277,3 @@ + attribs_1_5[atti] = EGL_NONE; + for (int i = 0; i < atti; i++) + GST_LOG ("attr %i: %08X", i, attribs_1_5[i]); I think the string format is not correct here. Should probably be G_GUINTPTR_FORMAT. @@ +304,3 @@ +#ifdef EGL_VERSION_1_5 + if (GST_GL_CHECK_GL_VERSION (context->majorVersion, context->minorVersion, 1, 5)) { I would just duplicate the assertions rather then doing the run-time check twice.
Review of attachment 331391 [details] [review]: I was trying to think of a way to deduplicate the attribute declarations across both EGL versions and the only thing that I could see that made sense was allocating some memory to hold the attributes and performing pointer arithmetic to set the parameters. Kind of like what GArray does. This is more a suggestion though if you feel like implementing that. ::: gst-libs/gst/gl/egl/gsteglimage.c @@ +279,3 @@ + GST_LOG ("attr %i: %08X", i, attribs_1_5[i]); + } else { +#endif This should be: } else #endif { to avoid the #ifdef at the end of the block @@ +308,3 @@ + EGL_LINUX_DMA_BUF_EXT, NULL, attribs_1_5); + } else { +#endif } else #endif { ::: gst-libs/gst/gl/gstglcontext.h @@ +74,3 @@ + gint majorVersion; + gint minorVersion; No camelCase please. Should be major/minor_version
Created attachment 331456 [details] [review] Updated patch.
Review of attachment 331456 [details] [review]: This won't compile with EGL_VERSION_1_5 defined ::: gst-libs/gst/gl/egl/gsteglimage.c @@ +262,3 @@ +#ifdef EGL_VERSION_1_5 + if (GST_GL_CHECK_GL_VERSION (context->majorVersion, context->minorVersion, 1, 5)) { erm, aren't these egl_major and egl_minor now? Also, don't you want to access the ctx_egl instance, not the parent class context instance? ::: gst-libs/gst/gl/gstglcontext.h @@ +74,3 @@ + gint egl_major; + gint egl_minor; Erm, adding egl versions to the parent class is a bad idea. This should go in GstGLContextEGL.
Created attachment 331465 [details] [review] For EGL 1.5 spec, the attribute list type should be EGLAttrib Update for typo.
Can help to review?
Review of attachment 331465 [details] [review]: This fails to compile here: gsteglimage.c: In function ‘gst_egl_image_from_dmabuf’: gsteglimage.c:285:38: error: passing argument 5 of ‘ctx_egl->eglCreateImage’ from incompatible pointer type [-Werror=incompatible-pointer-types] EGL_LINUX_DMA_BUF_EXT, NULL, attribs_1_5); ^~~~~~~~~~~ gsteglimage.c:285:38: note: expected ‘const EGLint * {aka const int *}’ but argument is of type ‘EGLAttrib * {aka long int *}’ Unfortunately, it seems that we will need two different function declarations for eglCreateImage to overcome the EGLInt vs EGLAttrib issue.
I only verified the patch on GStreamer 1.8.1. I will update the patch later.
(In reply to kevin from comment #18) > I only verified the patch on GStreamer 1.8.1. I will update the patch later. GStreamer version has nothing to do with that error. It's the incompatible definitions of eglCreateImage between EGL 1.5 and the EGL extension for < 1.5 now that causes that error.
Created attachment 333313 [details] [review] For EGL 1.5 spec, the attribute list type should be EGLAttrib. Update for build fail.
commit 4e04f28478fee0b85b677b9f636c50d6887a6165 Author: Song Bing <bing.song@nxp.com> Date: Wed Jul 13 17:15:44 2016 +0800 gl/egl/dmabuf: Wrong attribute list type for EGL 1.5 For EGL 1.5 spec, the attribute list type should be EGLAttrib. https://bugzilla.gnome.org/show_bug.cgi?id=768602