[vlc-devel] [vlc-commits] vlc_opengl: refactor vlc_gl_t creation
Rémi Denis-Courmont
remi at remlab.net
Mon Dec 19 19:00:32 CET 2016
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 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.
> 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/
More information about the vlc-devel
mailing list