After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 706054 - move GstEGLImageBufferPool and allocator from eglglessink to gstegl lib
move GstEGLImageBufferPool and allocator from eglglessink to gstegl lib
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other All
: Normal enhancement
: 1.3.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-08-15 08:48 UTC by Julien Isorce
Modified: 2014-02-25 17:32 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
refactor GstEGLImageBufferPool (74.68 KB, patch)
2013-08-15 08:48 UTC, Julien Isorce
needs-work Details | Review
buffer pool does not need to know the display when created (3.57 KB, patch)
2013-08-15 08:50 UTC, Julien Isorce
reviewed Details | Review
debug trace to track where the display comes from (1.43 KB, patch)
2013-08-15 08:51 UTC, Julien Isorce
reviewed Details | Review
replace GstEglGlesRenderContext by GstEGLContext (21.21 KB, patch)
2013-08-23 17:52 UTC, Julien Isorce
needs-work Details | Review
prepare gst_egl_adaptation_allocate_eglimage to be moved to gstegl lib (12.07 KB, patch)
2013-08-26 10:36 UTC, Julien Isorce
needs-work Details | Review
add GstEGLImageBufferPoolSendBlockingAllocate callback (7.39 KB, patch)
2013-08-26 11:15 UTC, Julien Isorce
needs-work Details | Review
GstEGLImageBufferPool does not need to maintain a ref on the display (3.09 KB, patch)
2013-08-26 11:28 UTC, Julien Isorce
committed Details | Review
change pool->sink->last_buffer to be pool->last_buffer (5.61 KB, patch)
2013-08-26 11:55 UTC, Julien Isorce
committed Details | Review
prepare gst_egl_adaptation_allocate_eglimage to be moved to gstegl lib (11.31 KB, patch)
2013-08-27 13:10 UTC, Julien Isorce
accepted-commit_now Details | Review
add GstEGLImageBufferPoolSendBlockingAllocate callback (7.38 KB, patch)
2013-08-27 13:13 UTC, Julien Isorce
committed Details | Review
prepare gst_egl_adaptation_allocate_eglimage to be moved (11.31 KB, patch)
2013-09-04 11:59 UTC, Julien Isorce
committed Details | Review
move to GstEGLImageBufferPool to gstegl lib (18.67 KB, patch)
2013-09-04 12:15 UTC, Julien Isorce
needs-work Details | Review
move to GstEGLImageBufferPool to gstegl lib (19.28 KB, patch)
2013-09-06 16:04 UTC, Julien Isorce
none Details | Review
move to GstEGLImageBufferPool to gstegl lib (20.11 KB, patch)
2013-09-09 12:47 UTC, Julien Isorce
none Details | Review
move GstEGLImageBufferPool to gstegl lib (19.79 KB, patch)
2013-09-25 15:09 UTC, Julien Isorce
none Details | Review
move GstEGLImageBufferPool to gstegl lib (19.74 KB, patch)
2013-12-02 10:18 UTC, Julien Isorce
reviewed Details | Review
eglglessink: move GstEGLImageBufferPool to gstegl lib (20.40 KB, patch)
2014-01-23 16:07 UTC, Julien Isorce
committed Details | Review
changes with previous reviewed patch (4.83 KB, patch)
2014-01-23 16:08 UTC, Julien Isorce
reviewed Details | Review

Description Julien Isorce 2013-08-15 08:48:58 UTC
Created attachment 251699 [details] [review]
refactor GstEGLImageBufferPool

So that we can avoid use it in webkitVideoSink to avoid code duplication
Comment 1 Julien Isorce 2013-08-15 08:50:20 UTC
Created attachment 251700 [details] [review]
buffer pool does not need to know the display when created

I moved it to be a param of the function that call eglCreateImageKHR. I had to do that because in webkit I do not know the display when the bufferpool is created.
Comment 2 Julien Isorce 2013-08-15 08:51:53 UTC
Created attachment 251701 [details] [review]
debug trace to track where the display comes from
Comment 3 Sebastian Dröge (slomo) 2013-08-20 09:50:45 UTC
Review of attachment 251699 [details] [review]:

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

::: 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

@@ +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?

@@ +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?
Comment 4 Julien Isorce 2013-08-23 16:09:05 UTC
(In reply to comment #3)
> Review of attachment 251699 [details] [review]:
> 
> 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.
Comment 5 Julien Isorce 2013-08-23 17:52:38 UTC
Created attachment 252934 [details] [review]
replace GstEglGlesRenderContext by  GstEGLContext

Ok so let's do it step by step, please find attached the first one :)

If it's ok I'll start the next step.
Comment 6 Sebastian Dröge (slomo) 2013-08-26 08:07:05 UTC
(In reply to comment #4)
> > ::: 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 ?

That would be done by the callback that is passed to the pool, no?

> > @@ +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

Because they're internal helper functions and not really pretty for general usage.

> > @@ +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)
> [...]
> 
> ?

I guess so

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

That's not really the goal of that library, we don't want to create GObject-like bindings for EGL. It should just contain whatever is necessary to allow EGLImage usage between different elements in GStreamer pipelines. These elements can use EGL and GL/GLES themselves.
Comment 7 Sebastian Dröge (slomo) 2013-08-26 08:10:41 UTC
Review of attachment 252934 [details] [review]:

::: gst-libs/gst/egl/egl.h
@@ +71,3 @@
+    GstEGLContext * eglcontext);
+gboolean gst_context_get_egl_context (GstContext * context,
+    GstEGLContext ** eglcontext);

I don't think an EGLContext should be distributed via GstContext. Why would you want to do that? Every element should probably have its own

@@ +92,3 @@
+
+EGLConfig gst_egl_display_get_config (GstEGLDisplay * display);
+void gst_egl_display_set_config (GstEGLDisplay * display, EGLConfig config);

As said above, no GObject-like EGL bindings. People can just call EGL API themselves.
Comment 8 Julien Isorce 2013-08-26 09:46:34 UTC
(In reply to comment #7)
> Review of attachment 252934 [details] [review]:
> 
> ::: gst-libs/gst/egl/egl.h
> @@ +71,3 @@
> +    GstEGLContext * eglcontext);
> +gboolean gst_context_get_egl_context (GstContext * context,
> +    GstEGLContext ** eglcontext);
> 
> I don't think an EGLContext should be distributed via GstContext. Why would you
> want to do that? Every element should probably have its own

Here the reason was to "mimic" what is done for GstGLDisplay. So I'll remove it then. 
I do not know yet how GstContext works exactly. This is how gst-omx retrieve the display right ? Wellout of the purpose of this bug/ticket, even if "Every element should probably have its own", would it be a way to share glcontexts together ?

> 
> @@ +92,3 @@
> +
> +EGLConfig gst_egl_display_get_config (GstEGLDisplay * display);
> +void gst_egl_display_set_config (GstEGLDisplay * display, EGLConfig config);
> 
> As said above, no GObject-like EGL bindings. People can just call EGL API
> themselves.

Ok I did it that because I removed GstEglGlesRenderContext. And I replaced it with GstEGLContext. Then naturally config went do display. 

Well I'll submit an other solution that does not need to delete GstEglGlesRenderContext, so that does not need to have GstEGLContext. But I would need this:

EGLContext gst_egl_adaptation_context_get_eglcontext (GstEglAdaptationContext * ctx)
{
  return ctx->eglglesctx->eglcontext;
}
Comment 9 Julien Isorce 2013-08-26 10:07:38 UTC
(In reply to comment #6)
> (In reply to comment #4)
> > > ::: 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 ?
> 
> That would be done by the callback that is passed to the pool, no?

No, there is 2 things. The callback I added in the API is to ask allocation in another thread.
For example in eglglessink it would be a function that just call gst_eglglessink_queue_object with structure name ""eglglessink-allocate-eglimage". Actually you cannot see it well in the patch because of the move of the function. Sorry about that I'll do it step by step.

The second thing is just gst_egl_adaptation_allocate_eglimage that I renamed to

GstBuffer *
gst_egl_image_allocator_alloc_eglimage (GstAllocator * allocator,
    GstEGLDisplay * display, GstEGLContext * context, GstVideoFormat format,
    gint width, gint height)

and this is the function that I really need to factorize to avoid big copy/past in webkitVideoSink (and also GstEGLBufferPool).
But this function depends on both egl and gl so ? Well I'll try to find a solution

> 
> > > @@ +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
> 
> Because they're internal helper functions and not really pretty for general
> usage.
> 
> > > @@ +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)
> > [...]
> > 
> > ?
> 
> I guess so
> 
> > Actually every call to egl should go progressively to gstegl lib.
> 
> That's not really the goal of that library, we don't want to create
> GObject-like bindings for EGL. It should just contain whatever is necessary to
> allow EGLImage usage between different elements in GStreamer pipelines. These
> elements can use EGL and GL/GLES themselves.
Comment 10 Julien Isorce 2013-08-26 10:36:34 UTC
Created attachment 253109 [details] [review]
prepare gst_egl_adaptation_allocate_eglimage to be moved to gstegl lib
Comment 11 Julien Isorce 2013-08-26 11:15:32 UTC
Created attachment 253118 [details] [review]
add GstEGLImageBufferPoolSendBlockingAllocate callback

The goal here is to prepare GstEGLBufferPool to be moved into gstegl lib. So it has to not depend on 'gst_eglglessink_queue_object'
Comment 12 Julien Isorce 2013-08-26 11:28:02 UTC
Created attachment 253119 [details] [review]
GstEGLImageBufferPool does not need to maintain a ref on  the display

It does not need it anymore and also it may not know about the display when we create the pool
Comment 13 Julien Isorce 2013-08-26 11:55:16 UTC
Created attachment 253122 [details] [review]
change pool->sink->last_buffer to be  pool->last_buffer

So that GstEGLImageBufferPool does not depend on GstEglGlesSink
The goal is still to move it into gstegl lib.

Well here I think the result is the same but I'm not 100% sure.

Reading
"
/* XXX: Don't return the memory we just rendered, glEGLImageTargetTexture2DOES()
 * keeps the EGLImage unmappable until the next one is uploaded
 */
"
it seems that GstEGLImageBufferPool directly depends on this last_buffer
Comment 14 Sebastian Dröge (slomo) 2013-08-27 08:50:11 UTC
Review of attachment 253109 [details] [review]:

::: ext/eglgles/gstegladaptation_egl.c
@@ +839,3 @@
 mem_error:
   {
+    GST_ERROR_OBJECT (GST_CAT_DEFAULT, "Failed to create EGLImage");

This will crash, make it GST_ERROR("Failed ...") instead.

::: ext/eglgles/gsteglglessink.c
@@ +539,3 @@
+            (eglglessink->pool)->allocator, eglglessink->egl_context->display,
+            gst_egl_adaptation_context_get_egl_context
+            (eglglessink->egl_context), format, width, height);

This is only valid for EGL anyway, having this code here for EAGL will break compilation (and already does before your changes). So no need to add the new function
Comment 15 Sebastian Dröge (slomo) 2013-08-27 08:55:28 UTC
Review of attachment 253118 [details] [review]:

::: ext/eglgles/gsteglglessink.c
@@ +298,3 @@
+
+  if (query)
+    gst_query_unref (query);

There is always a query :)
Comment 16 Julien Isorce 2013-08-27 13:10:57 UTC
Created attachment 253243 [details] [review]
prepare gst_egl_adaptation_allocate_eglimage to be moved to gstegl lib
Comment 17 Julien Isorce 2013-08-27 13:13:43 UTC
Created attachment 253244 [details] [review]
add GstEGLImageBufferPoolSendBlockingAllocate callback

Could you review 253122 too ? About 253119 I think you already said ok on IRC (1 week ago I think) Thx
Comment 18 Sebastian Dröge (slomo) 2013-08-30 11:38:17 UTC
Comment on attachment 253244 [details] [review]
add GstEGLImageBufferPoolSendBlockingAllocate callback

Please go ahead and push them all. Just get rid of the C99 comments and replace them by normal /* */ comments
Comment 19 Julien Isorce 2013-09-04 11:59:58 UTC
Created attachment 254060 [details] [review]
prepare  gst_egl_adaptation_allocate_eglimage to be moved
Comment 20 Julien Isorce 2013-09-04 12:15:07 UTC
Created attachment 254062 [details] [review]
move to GstEGLImageBufferPool to gstegl lib

I'm about to push the first 4 patches. In the meantime find attached the move of GstEGLImageBufferPool into gstegl lib

Then next step is to move gstegladaptation_egl.c::gst_egl_image_allocator_alloc_eglimage function. But this function depends on both egl (eglCreateEGLImage) and gl (glGenTextures/glTexImage2D).

So should I create a lib "eglgl" and move gst_egl_image_allocator_alloc_eglimage into gst-libs/gst/eglgl ? Then it's just to a matter of time when it would be merged into gst-plugins-gl (and put back into -bad)

( The other idea would to move it into gst-libs/gst/gl (but I guess It would conflict when installing gst-plugins-gl) and just move eglCreateImageKHR into a new egl.h::gst_egl_create_image function )
Comment 21 Julien Isorce 2013-09-04 12:44:11 UTC
commit 3a8487529490bd9e435137d8c905f5b1b9462ae8
Author: Julien Isorce <julien.isorce@collabora.co.uk>
Date:   Wed Sep 4 10:58:24 2013 +0100

    eglglessink: change pool->sink->last_buffer to pool->last_buffer
    
    So that GstEGLImageBufferPool does not depend on GstEglGlesSink
    The goal is still to move it into gstegl lib

commit fdaa26e1c869f811fdf258a0ab6f286a3e6b3689
Author: Julien Isorce <julien.isorce@collabora.co.uk>
Date:   Wed Sep 4 10:56:12 2013 +0100

    eglglessink: buffer pool does not need to maintain a ref on the display
    
    Because it does not use it and also it may not know it when
    we create the pool

commit d16583d771da5562cb57e0263dbb908a70a6e02d
Author: Julien Isorce <julien.isorce@collabora.co.uk>
Date:   Wed Sep 4 10:52:51 2013 +0100

    eglglessink: add GstEGLImageBufferPoolSendBlockingAllocate callback
    
    The goal here is to prepare GstEGLBufferPool to be moved into
    gstegl lib. So it has to not depend on 'gst_eglglessink_queue_object'

commit c0ca9bc422e06eab8818f285f70ccc96656fb56e
Author: Julien Isorce <julien.isorce@collabora.co.uk>
Date:   Wed Sep 4 10:48:34 2013 +0100

    eglglessink: prepare gst_egl_adaptation_allocate_eglimage to be moved
    
    into gstegl lib or splited between gstegl lib and gstgl lib
    because it both depends on egl and gl
    
    So it has to not depend on GstEglAdaptationContext
Comment 22 Sebastian Dröge (slomo) 2013-09-04 15:11:03 UTC
Review of attachment 254062 [details] [review]:

::: gst-libs/gst/egl/egl.c
@@ +393,3 @@
+  GstEGLImageBufferPoolSendBlockingAllocate send_blocking_allocate_func;
+  gpointer send_blocking_allocate_data;
+  GDestroyNotify send_blocking_allocate_destroy;

and most of this has to be moved into a private struct

::: gst-libs/gst/egl/egl.h
@@ +72,3 @@
 
+/* EGLImage buffer pool */
+typedef struct _GstEGLImageBufferPool GstEGLImageBufferPool;

This struct and the class struct must be public to make g-i and gtk-doc happy

@@ +75,3 @@
+typedef GstBuffer *(*GstEGLImageBufferPoolSendBlockingAllocate) (GstBufferPool *
+    pool, gpointer data);
+#define GST_EGL_IMAGE_BUFFER_POOL(p) ((GstEGLImageBufferPool*)(p))

G_TYPE_CHECK_INSTANCE_CAST() or whatever the GLib macro for this was :)
Comment 23 Julien Isorce 2013-09-05 15:19:43 UTC
> ::: gst-libs/gst/egl/egl.h
> @@ +72,3 @@
> 
> +/* EGLImage buffer pool */
> +typedef struct _GstEGLImageBufferPool GstEGLImageBufferPool;
> 
> This struct and the class struct must be public to make g-i and gtk-doc happy

Ok but why this is not the case for GstEGLDisplay ? :)
Comment 24 Julien Isorce 2013-09-05 16:03:49 UTC
Also should I put:
GstAllocator *allocator;
GstAllocationParams params;
GstVideoInfo info;
gboolean add_metavideo;
gboolean want_eglimage;
GstBuffer *last_buffer;

into GstEGLImageBufferPoolPrivate too ?



And what do you think about "gst_egl_image_allocator_alloc_eglimage" in comment #20 ? thx
Comment 25 Julien Isorce 2013-09-06 16:04:35 UTC
Created attachment 254270 [details] [review]
move to GstEGLImageBufferPool to gstegl lib

add GstEGLImageBufferPoolPrivate
Comment 26 Sebastian Dröge (slomo) 2013-09-09 11:04:47 UTC
(In reply to comment #23)
> > ::: gst-libs/gst/egl/egl.h
> > @@ +72,3 @@
> > 
> > +/* EGLImage buffer pool */
> > +typedef struct _GstEGLImageBufferPool GstEGLImageBufferPool;
> > 
> > This struct and the class struct must be public to make g-i and gtk-doc happy
> 
> Ok but why this is not the case for GstEGLDisplay ? :)

Because that's a boxed, opaque type.
Comment 27 Sebastian Dröge (slomo) 2013-09-09 11:05:28 UTC
(In reply to comment #24)
> Also should I put:
> GstAllocator *allocator;
> GstAllocationParams params;
> GstVideoInfo info;
> gboolean add_metavideo;
> gboolean want_eglimage;
> GstBuffer *last_buffer;
> 
> into GstEGLImageBufferPoolPrivate too ?

Yes, everything that should not be accessed from the outside. Also add padding to the public structs.
Comment 28 Sebastian Dröge (slomo) 2013-09-09 11:34:36 UTC
(In reply to comment #20)
> Created an attachment (id=254062) [details] [review]
> move to GstEGLImageBufferPool to gstegl lib
> 
> I'm about to push the first 4 patches. In the meantime find attached the move
> of GstEGLImageBufferPool into gstegl lib
> 
> Then next step is to move
> gstegladaptation_egl.c::gst_egl_image_allocator_alloc_eglimage function. But
> this function depends on both egl (eglCreateEGLImage) and gl
> (glGenTextures/glTexImage2D).
> 
> So should I create a lib "eglgl" and move
> gst_egl_image_allocator_alloc_eglimage into gst-libs/gst/eglgl ? Then it's just
> to a matter of time when it would be merged into gst-plugins-gl (and put back
> into -bad)
> 
> ( The other idea would to move it into gst-libs/gst/gl (but I guess It would
> conflict when installing gst-plugins-gl) and just move eglCreateImageKHR into a
> new egl.h::gst_egl_create_image function )

Maybe it should be in gst-plugins-gl instead? It's not really an EGL helper function but a GL one
Comment 29 Julien Isorce 2013-09-09 12:47:12 UTC
Created attachment 254477 [details] [review]
move to GstEGLImageBufferPool to gstegl lib
Comment 30 Julien Isorce 2013-09-25 15:09:55 UTC
Created attachment 255700 [details] [review]
move GstEGLImageBufferPool to gstegl lib

Update patch because egl/eglglessink have changed since last time (GstContext changes)
Comment 31 Julien Isorce 2013-12-02 10:18:56 UTC
Created attachment 263277 [details] [review]
move GstEGLImageBufferPool to gstegl lib

Update patch because of last changes in eglglessink (iOS port)
Comment 32 Julien Isorce 2014-01-21 08:45:13 UTC
is it ok to push ? (could also be useful to avoid duplicating the GstEGLImageBufferPool here http://cgit.freedesktop.org/gstreamer/gst-omx/tree/examples/egl/testegl.c)
Comment 33 Sebastian Dröge (slomo) 2014-01-21 09:26:59 UTC
Review of attachment 263277 [details] [review]:

I did not have time to review it completely yet.

::: gst-libs/gst/egl/egl.h
@@ +106,3 @@
+    gpointer blocking_allocate_data, GDestroyNotify destroy_func);
+void gst_egl_image_buffer_pool_replace_last_buffer (GstEGLImageBufferPool * pool, GstBuffer * buffer);
+void gst_egl_image_buffer_pool_get_video_infos (GstEGLImageBufferPool * pool, GstVideoFormat * format, gint * width, gint * height);

Why not return a GstVideoInfo here?

@@ +109,3 @@
+
+/* TODO: will be removed after moving gst_egl_image_allocator_alloc_eglimage */
+GstAllocator *gst_egl_image_buffer_pool_get_allocator (GstEGLImageBufferPool * pool);

Do we want to move it, and where and how? It's involving lots of GL calls and with that we get lots of problems :)

Maybe for all this we should just get gst-plugins-gl into shape.
Comment 34 Julien Isorce 2014-01-22 08:56:29 UTC
(In reply to comment #33)
> Review of attachment 263277 [details] [review]:
> 
> I did not have time to review it completely yet.

Thx!

> 
> ::: gst-libs/gst/egl/egl.h
> @@ +106,3 @@
> +    gpointer blocking_allocate_data, GDestroyNotify destroy_func);
> +void gst_egl_image_buffer_pool_replace_last_buffer (GstEGLImageBufferPool *
> pool, GstBuffer * buffer);
> +void gst_egl_image_buffer_pool_get_video_infos (GstEGLImageBufferPool * pool,
> GstVideoFormat * format, gint * width, gint * height);
> 
> Why not return a GstVideoInfo here?

Indeed :)

> 
> @@ +109,3 @@
> +
> +/* TODO: will be removed after moving gst_egl_image_allocator_alloc_eglimage
> */
> +GstAllocator *gst_egl_image_buffer_pool_get_allocator (GstEGLImageBufferPool *
> pool);
> 
> Do we want to move it, and where and how? It's involving lots of GL calls and
> with that we get lots of problems :)
> 
> Maybe for all this we should just get gst-plugins-gl into shape.

About "gst_egl_image_allocator_alloc_eglimage" here is the function: http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/ext/eglgles/gstegladaptation_egl.c#n427

So yup I should at least change the comment. Because I will not move it to gstegl lib for sure. After pushing attached patch 263277 the next step will be to split gst_egl_image_allocator_alloc_eglimage to separate eglCreateImageKHR calls and GL calls.

Also because we can create an eglimage from other EGLClientBuffer than gltextures (ex: 

EGLint eglImageAttributes[] = {EGL_WIDTH, ImageWidth, EGL_HEIGHT, ImageHeight, EGL_MATCH_FORMAT_KHR,  EGL_FORMAT_RGB_565_KHR, EGL_IMAGE_PRESERVED_KHR, EGL_TRUE, EGL_NONE};

On RPI:
eglCreateImageKHR(display, EGL_NO_CONTEXT, EGL_IMAGE_BRCM_MULTIMEDIA, buf, eglImageAttributes);

Or on Android:

EGLImageKHR image = eglCreateImageKHR(display, EGL_NO_CONTEXT, EGL_NATIVE_BUFFER_ANDROID, cbuf, eglImageAttributes);

)

And the remaining GL calls will stay in eglglessink for now.

So this way we would be able to use GstEGLImageBufferPool in:

Current code that already use it:
- eglglessink
- gst-omx/testegl.c

Future code:
- gst-plugins-gl: we could start using the GstEGLImageBufferPool even if we keep its code in -bad/gstegl lib for now.
- omxvideodec: create this buffer pool using EGL_NO_CONTEXT (see above) and put glEGLImageTargetTexture2DOES call into a gst_video_gl_texture_upload_meta_upload, so that webkit could use it without having to use  the GstEGLImageBufferPool directly from the video sink.
Comment 35 Julien Isorce 2014-01-23 16:07:33 UTC
Created attachment 267059 [details] [review]
eglglessink: move GstEGLImageBufferPool to gstegl lib

Use gst_egl_image_allocator_obtain to retrieve the allocator instead of introducing a new API.
Use gst_buffer_pool_config_get_params to retrieve video format, width and height instead of introducing a new API.
Comment 36 Julien Isorce 2014-01-23 16:08:33 UTC
Created attachment 267060 [details] [review]
changes with previous reviewed patch
Comment 37 Julien Isorce 2014-01-29 18:50:37 UTC
ok to push ? :)
Comment 38 Julien Isorce 2014-02-07 12:20:15 UTC
ping?
Comment 39 Philippe Normand 2014-02-25 08:22:33 UTC
Anything blocking this patch Sebastian?

Thank you Julien for all this work btw!
Comment 40 Sebastian Dröge (slomo) 2014-02-25 08:48:00 UTC
Yes, time is blocking :) I guess it's fine to get this into -bad, we can still change the API if it looks suboptimal.

Philippe, are you planning to use this in WebKit? If you do, be aware that the -bad libraries can change API between different versions.
Comment 41 Philippe Normand 2014-02-25 08:52:28 UTC
(In reply to comment #40)
> Yes, time is blocking :) I guess it's fine to get this into -bad, we can still
> change the API if it looks suboptimal.
> 
> Philippe, are you planning to use this in WebKit? If you do, be aware that the
> -bad libraries can change API between different versions.

Yes and yes, I'm aware of that. :)
Comment 42 Julien Isorce 2014-02-25 10:08:06 UTC
Then I'm going to push that today :)
Comment 43 Sebastian Dröge (slomo) 2014-02-25 10:11:30 UTC
Comment on attachment 267060 [details] [review]
changes with previous reviewed patch

Please squash this into the other patch (also use unified diffs in the future, diff -u)
Comment 44 Julien Isorce 2014-02-25 10:23:56 UTC
(In reply to comment #43)
> (From update of attachment 267060 [details] [review])
> Please squash this into the other patch 
It's already in. I made this diff just to highlight changes after your review from comment #33 and updating the patch in #35 :)

> (also use unified diffs in the future, diff -u)
ok np
Comment 45 Julien Isorce 2014-02-25 10:26:23 UTC
Comment on attachment 267060 [details] [review]
changes with previous reviewed patch

already squashed in attachment 267059 [details] [review]
Comment 46 Julien Isorce 2014-02-25 17:31:22 UTC
Comment on attachment 267059 [details] [review]
eglglessink: move GstEGLImageBufferPool to gstegl lib

commit 966fb81db067b997d7650f998b808b25e2213b96
Author: Julien Isorce <julien.isorce@collabora.co.uk>
Date:   Mon Dec 2 10:01:12 2013 +0000

    eglglessink: move GstEGLImageBufferPool to gstegl lib
    
    https://bugzilla.gnome.org/show_bug.cgi?id=706054