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.

Created attachment 251701 [details] [review]
debug trace to track where the display comes from

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?

(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.

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.

(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.

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.

(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;
}

(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.

Created attachment 253109 [details] [review]
prepare gst_egl_adaptation_allocate_eglimage to be moved to gstegl lib

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'

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

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

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

Review of attachment 253118 [details] [review]:

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

There is always a query :)

Created attachment 253243 [details] [review]
prepare gst_egl_adaptation_allocate_eglimage to be moved to gstegl lib

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

Created attachment 254060 [details] [review]
prepare  gst_egl_adaptation_allocate_eglimage to be moved

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 )

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

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

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

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

Created attachment 254270 [details] [review]
move to GstEGLImageBufferPool to gstegl lib

add GstEGLImageBufferPoolPrivate

(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.

(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.

(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

Created attachment 254477 [details] [review]
move to GstEGLImageBufferPool to gstegl lib

Created attachment 255700 [details] [review]
move GstEGLImageBufferPool to gstegl lib

Update patch because egl/eglglessink have changed since last time (GstContext changes)

Created attachment 263277 [details] [review]
move GstEGLImageBufferPool to gstegl lib

Update patch because of last changes in eglglessink (iOS port)

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)

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.

(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.

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.

Created attachment 267060 [details] [review]
changes with previous reviewed patch

ok to push ? :)

ping?

Anything blocking this patch Sebastian?

Thank you Julien for all this work btw!

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.

(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. :)

Then I'm going to push that today :)

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)

(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 on attachment 267060 [details] [review]
changes with previous reviewed patch

already squashed in attachment 267059 [details] [review]

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