[Bug 706054] move GstEGLImageBufferPool and allocator from eglglessink to gstegl lib

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=706054
  GStreamer | gst-plugins-bad | git

--- Comment #4 from Julien Isorce <julien.isorce at gmail.com> 2013-08-23 16:09:05 UTC ---
(In reply to comment #3)
> Review of attachment 251699 [details]:
> 
> Just looked at the header API for now. Could you do the refactoring in-place
> first so it's easier to see the diff? And then move files around in a second
> step?
> 
> ::: gst-libs/gst/egl/Makefile.am
> @@ +20,3 @@
>      $(GST_LIBS) \
> +    $(EGL_LIBS) \
> +    $(EGLGLES_LIBS)
> 
> This now makes the egl library depend on GL or GLES, that's not how it was
> intended

Yes sure but how to deal with glGenTextures/glBindTexture/glTexImage2D calls
before the eglCreateImageKHR call then ?

> 
> ::: gst-libs/gst/egl/egl.h
> @@ +37,3 @@
> +typedef struct _GstEGLContext GstEGLContext;
> +
> +GST_DEBUG_CATEGORY_EXTERN (egl_debug);
> 
> Why is it in the header? I don't think it should
> 
> @@ +42,3 @@
> +
> +gboolean gst_gl_got_error (const char *wtf);
> +gboolean gst_egl_got_error (const char *wtf);
> 
> These should not be public

why ? if we want to use it in eglglessink as well

> 
> @@ +90,3 @@
> +EGLint gst_egl_display_get_major (GstEGLDisplay * display);
> +EGLint gst_egl_display_get_minor (GstEGLDisplay * display);
> +void gst_egl_display_set_version (GstEGLDisplay * display, EGLint major,
> EGLint minor);
> 
> Shouldn't the display get the version from itself, instead of having the user
> supply it?

ok so I should move the call to eglInitialise to gstegl lib:

So it's like replacing gst_egl_display_set_version by:

gboolean gst_egl_display_initialize (GstEGLDisplay * display)
{
...
  res = eglInitialize (display->display, &egl_major, &egl_minor)
...
  if (res) display->egl_major = egl_major;
...
  return res;
}

?

Actually every call to egl should go progressively to gstegl lib.

> 
> @@ +112,3 @@
> +/* EGLImageBufferPool */
> +
> +typedef GstBuffer* (*GstEGLImageBufferPoolSendBlockingAllocate) (GstBufferPool
> *pool, GstObject * object);
> 
> This is a callback that is supposed to block and defer the allocation of the
> EGLImage in a different thread?
> 
> @@ +114,3 @@
> +typedef GstBuffer* (*GstEGLImageBufferPoolSendBlockingAllocate) (GstBufferPool
> *pool, GstObject * object);
> +
> +typedef struct _GstEGLImageBufferPool GstEGLImageBufferPool;
> 
> For making g-i and gtk-doc happy you should have all the GObject boilerplate
> stuff here and also the structs public
> 
> @@ +121,3 @@
> +
> +GstBufferPool *gst_egl_image_buffer_pool_new (GstEGLDisplay * display,
> +    GstObject * blocking_allocate_data,
> 
> Why a GstObject? Why not just a gpointer and a GDestroyNotify?

You right I'll change that too.

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.