[vlc-devel] [vlc-commits] vlc_opengl: refactor vlc_gl_t creation
Thomas Guillem
thomas at gllm.fr
Mon Dec 19 19:19:48 CET 2016
On Mon, Dec 19, 2016, at 19:00, Rémi Denis-Courmont wrote:
> Le maanantaina 19. joulukuuta 2016, 17.50.38 EET Thomas Guillem a écrit :
> > On Mon, Dec 19, 2016, at 17:02, Rémi Denis-Courmont wrote:
> > > On December 19, 2016 5:41:26 PM GMT+02:00, Thomas Guillem
> > >
> > > <thomas at gllm.fr> wrote:
> > > >On Mon, Dec 19, 2016, at 15:59, Rémi Denis-Courmont wrote:
> > > >> On December 19, 2016 4:04:07 PM GMT+02:00, git at videolan.org wrote:
> > > >> >vlc | branch: master | Filip Roséen <filip at atch.se> | Mon Dec 19
> > > >> >03:07:43 2016 +0100| [79b2187631f2786f46a8225b538f3ec7ae7d1c9e] |
> > > >> >committer: Thomas Guillem
> > > >> >
> > > >> >vlc_opengl: refactor vlc_gl_t creation
> > > >> >
> > > >> >Given that there are places in the codebase that just require a
> > > >> >created object of type vlc_gl_t ("gl"), these changes split the
> > > >
> > > >object
> > > >
> > > >> >creation into two functions:
> > > >> > - vlc_gl_Create: a simple wrapper around vlc_custom_create
> > > >> > - vlc_gl_ModuleCreate: creates a vlc_gl_t with an associated
> > > >> >
> > > >> > module/surface.
> > > >> >
> > > >> >refs #17795
> > > >> >
> > > >> >Signed-off-by: Thomas Guillem <thomas at gllm.fr>
> > > >>
> > > >>http://git.videolan.org/gitweb.cgi/vlc.git/?a=commit;h=79b2187631f2786f4
> > > >>6a8225b538f3ec7ae7d1c9e> >>
> > > >> >---
> > > >> >
> > > >> > include/vlc_opengl.h | 22 ++++++++++++++-
> > > >> > modules/video_output/opengl/display.c | 2 +-
> > > >> > src/libvlccore.sym | 1 +
> > > >> >
> > > >> >src/video_output/opengl.c | 51
> > > >> >++++++++++++++++-------------------
> > > >> >
> > > >> > 4 files changed, 46 insertions(+), 30 deletions(-)
> > > >> >
> > > >> >diff --git a/include/vlc_opengl.h b/include/vlc_opengl.h
> > > >> >index 53d73d0..f7704bc 100644
> > > >> >--- a/include/vlc_opengl.h
> > > >> >+++ b/include/vlc_opengl.h
> > > >> >@@ -58,7 +58,27 @@ enum {
> > > >> >
> > > >> > VLC_OPENGL_ES2,
> > > >> >
> > > >> > };
> > > >> >
> > > >> >-VLC_API vlc_gl_t *vlc_gl_Create(struct vout_window_t *, unsigned,
> > > >> >const char *) VLC_USED;
> > > >> >+/**
> > > >> >+ * Create a VLC OpenGL object
> > > >> >+ *
> > > >> >+ * @note The allocated resource, if any, shall be released
> > > >> >+ * with a call to \ref vlc_gl_Destroy.
> > > >> >+ * @return a created object on success, NULL on failure.
> > > >> >+ **/
> > > >> >+VLC_API vlc_gl_t *vlc_gl_Create(vlc_object_t* parent);
> > > >> >+
> > > >> >+/**
> > > >> >+ * Creates a VLC OpenGL object with associated surface and module
> > > >> >+ *
> > > >> >+ * @note In most cases, you should vlc_gl_MakeCurrent() afterward.
> > > >> >+ *
> > > >> >+ * @param wnd window to use as OpenGL surface
> > > >> >+ * @param flags OpenGL context type
> > > >> >+ * @param name module name (or NULL for auto)
> > > >> >+ * @return a new context, or NULL on failure
> > > >> >+ */
> > > >> >+VLC_API vlc_gl_t *vlc_gl_ModuleCreate(struct vout_window_t *wnd,
> > > >> >unsigned flags,
> > > >> >+ const char *name) VLC_USED;
> > > >> >
> > > >> > VLC_API void vlc_gl_Destroy(vlc_gl_t *);
> > > >> >
> > > >> > static inline int vlc_gl_MakeCurrent(vlc_gl_t *gl)
> > > >> >
> > > >> >diff --git a/modules/video_output/opengl/display.c
> > > >> >b/modules/video_output/opengl/display.c
> > > >> >index eefb5cf..68418b3 100644
> > > >> >--- a/modules/video_output/opengl/display.c
> > > >> >+++ b/modules/video_output/opengl/display.c
> > > >> >@@ -107,7 +107,7 @@ static int Open (vlc_object_t *obj)
> > > >> >
> > > >> > goto error;
> > > >> >
> > > >> > }
> > > >> >
> > > >> >- sys->gl = vlc_gl_Create (surface, API, "$" MODULE_VARNAME);
> > > >> >+ sys->gl = vlc_gl_ModuleCreate (surface, API, "$"
> > > >
> > > >MODULE_VARNAME);
> > > >
> > > >> > if (sys->gl == NULL)
> > > >> >
> > > >> > goto error;
> > > >> >
> > > >> >diff --git a/src/libvlccore.sym b/src/libvlccore.sym
> > > >> >index 7b9657a..b65f538 100644
> > > >> >--- a/src/libvlccore.sym
> > > >> >+++ b/src/libvlccore.sym
> > > >> >@@ -691,6 +691,7 @@ vlc_fifo_GetCount
> > > >> >
> > > >> > vlc_fifo_GetBytes
> > > >> > vlc_gl_Create
> > > >> > vlc_gl_Destroy
> > > >> >
> > > >> >+vlc_gl_ModuleCreate
> > > >> >
> > > >> > vlc_gl_surface_Create
> > > >> > vlc_gl_surface_CheckSize
> > > >> > vlc_gl_surface_Destroy
> > > >> >
> > > >> >diff --git a/src/video_output/opengl.c b/src/video_output/opengl.c
> > > >> >index ac16c3f..6459cbf 100644
> > > >> >--- a/src/video_output/opengl.c
> > > >> >+++ b/src/video_output/opengl.c
> > > >> >@@ -29,47 +29,43 @@
> > > >> >
> > > >> > #include <vlc_opengl.h>
> > > >> > #include "libvlc.h"
> > > >> > #include <vlc_modules.h>
> > > >> >
> > > >> >+#include <vlc_vout_window.h>
> > > >> >
> > > >> >-#undef vlc_gl_Create
> > > >> >-/**
> > > >> >- * Creates an OpenGL context (and its underlying surface).
> > > >> >- *
> > > >> >- * @note In most cases, you should vlc_gl_MakeCurrent() afterward.
> > > >> >- *
> > > >> >- * @param wnd window to use as OpenGL surface
> > > >> >- * @param flags OpenGL context type
> > > >> >- * @param name module name (or NULL for auto)
> > > >> >- * @return a new context, or NULL on failure
> > > >> >- */
> > > >> >-vlc_gl_t *vlc_gl_Create(struct vout_window_t *wnd, unsigned flags,
> > > >> >- const char *name)
> > > >> >+vlc_gl_t *vlc_gl_Create(vlc_object_t* parent)
> > > >> >
> > > >> > {
> > > >> >
> > > >> >- vlc_object_t *parent = (vlc_object_t *)wnd;
> > > >> >- vlc_gl_t *gl;
> > > >> >- const char *type;
> > > >> >+ return vlc_custom_create(parent, sizeof(vlc_gl_t), "gl");
> > > >> >+}
> > > >> >
> > > >> >- switch (flags /*& VLC_OPENGL_API_MASK*/)
> > > >> >+vlc_gl_t *vlc_gl_ModuleCreate(struct vout_window_t *wnd, unsigned
> > > >> >flags,
> > > >> >+ const char *module_name)
> > > >> >+{
> > > >> >+ const char *module_type;
> > > >> >+
> > > >> >+ switch (flags)
> > > >> >
> > > >> > {
> > > >> >
> > > >> > case VLC_OPENGL:
> > > >> >- type = "opengl";
> > > >> >+ module_type = "opengl";
> > > >> >
> > > >> > break;
> > > >> >
> > > >> > case VLC_OPENGL_ES:
> > > >> >- type = "opengl es";
> > > >> >+ module_type = "opengl es";
> > > >> >
> > > >> > break;
> > > >> >
> > > >> > case VLC_OPENGL_ES2:
> > > >> >- type = "opengl es2";
> > > >> >+ module_type = "opengl es2";
> > > >> >
> > > >> > break;
> > > >> >
> > > >> >+
> > > >> >
> > > >> > default:
> > > >> > return NULL;
> > > >> >
> > > >> > }
> > > >> >
> > > >> >- gl = vlc_custom_create(parent, sizeof (*gl), "gl");
> > > >> >- if (unlikely(gl == NULL))
> > > >> >+ vlc_gl_t *gl = vlc_gl_Create(VLC_OBJECT(wnd));
> > > >> >+
> > > >> >+ if( unlikely( !gl ) )
> > > >> >
> > > >> > return NULL;
> > > >> >
> > > >> > gl->surface = wnd;
> > > >> >
> > > >> >- gl->module = module_need(gl, type, name, true);
> > > >> >- if (gl->module == NULL)
> > > >> >+ gl->module = module_need(gl, module_type, module_name, true);
> > > >> >+
> > > >> >+ if (gl->module == NULL )
> > > >> >
> > > >> > {
> > > >> >
> > > >> > vlc_object_release(gl);
> > > >> > return NULL;
> > > >> >
> > > >> >@@ -80,12 +76,11 @@ vlc_gl_t *vlc_gl_Create(struct vout_window_t
> > > >
> > > >*wnd,
> > > >
> > > >> >unsigned flags,
> > > >> >
> > > >> > void vlc_gl_Destroy(vlc_gl_t *gl)
> > > >> > {
> > > >> >
> > > >> >- module_unneed(gl, gl->module);
> > > >> >+ if(gl->module)
> > > >> >+ module_unneed(gl, gl->module);
> > > >> >
> > > >> > vlc_object_release(gl);
> > > >> >
> > > >> > }
> > > >> >
> > > >> >-#include <vlc_vout_window.h>
> > > >> >-
> > > >> >
> > > >> > typedef struct vlc_gl_surface
> > > >> > {
> > > >> >
> > > >> > int width;
> > > >> >
> > > >> >@@ -130,7 +125,7 @@ vlc_gl_t *vlc_gl_surface_Create(vlc_object_t
> > > >
> > > >*obj,
> > > >
> > > >> > *wp = surface;
> > > >> >
> > > >> > /* TODO: support ES? */
> > > >> >
> > > >> >- vlc_gl_t *gl = vlc_gl_Create(surface, VLC_OPENGL, NULL);
> > > >> >+ vlc_gl_t *gl = vlc_gl_ModuleCreate(surface, VLC_OPENGL, NULL);
> > > >> >
> > > >> > if (gl == NULL) {
> > > >> >
> > > >> > vout_window_Delete(surface);
> > > >> > return NULL;
> > > >> >
> > > >> >_______________________________________________
> > > >> >vlc-commits mailing list
> > > >> >vlc-commits at videolan.org
> > > >> >https://mailman.videolan.org/listinfo/vlc-commits
> > > >>
> > > >> This API makes no sense whatsoever, please revert.
> > > >
> > > >Why does it make no sense ? Do you have an other solution in mind ?
> > > >
> > > >_______________________________________________
> > > >vlc-devel mailing list
> > > >To unsubscribe or modify your subscription options:
> > > >https://mailman.videolan.org/listinfo/vlc-devel
> > >
> > > Making an API less logical and more complex when you can achieve the same
> > > with the existing API does not make sense.
> >
> > I don't see how to do it differently.
>
> Then maybe you need to sleep
No I'm fine thank you, and I just had a coffee.
> or think about it for a few seconds more,
> because
> it is utterly obvious that you can use vlc_object_create() and
> vlc_object_release() instead of breaking the fine GL API.
We thought about this solution, but we preferred the one we pushed. I
don't really care about the solution we use, both are fine to me. I'll
revert and implement the one you want.
PS: we are different people, what is obvious for someone is not
necessarily obvious for the other.
>
> > Some video outputs don't have a valid window
>
> First, that is completely irrelevant.
>
> Second, that is not an excuse for messing up a fine API this way.
>
> And third, I don´t even care and neither should you. The video window
> abstraction was added 7.5 years ago or at least two orders of magnitude a
> reasonable time for transition.
>
> Also the WGL video output should be removed since it just duplicates the
> functionality of the more generic WGL provider. But then again, it
> supports
> windows anyway.
>
> > and can't use the legacy
> > vlc_gl_Create() function (unless we do an ugly cast).
>
> Yes; they can´t. And they don´t need to. And they shouldn´t.
>
> --
> Rémi Denis-Courmont
> https://www.remlab.net/
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
More information about the vlc-devel
mailing list