[vlc-devel] [PATCH 1/2] libvlc: replaced libvlc_log_set with a formatted log callback

Rémi Denis-Courmont remi at remlab.net
Tue May 21 09:30:53 CEST 2019


Hi, 


I refuse to change the existing API which works, follows a well-known convention (indeed libavutil works the same way), and does not add heap overhead and failure modes - which is a MUST for *debug* logs.

Also this creates a reverse abstraction anti-pattern in the stdio handler and existing apps.

Le 20 mai 2019 23:58:40 GMT+03:00, "Jérémy VIGNELLES" <jeremy.vignelles at dev3i.fr> a écrit :
>---
> include/vlc/libvlc.h  | 25 ++++++++++++-------------
> lib/libvlc.sym        |  2 +-
> lib/libvlc_internal.h |  3 ++-
> lib/log.c             | 29 +++++++++++++++++++----------
> 4 files changed, 34 insertions(+), 25 deletions(-)
>
>diff --git a/include/vlc/libvlc.h b/include/vlc/libvlc.h
>index 0ec0045..aac2c7f 100644
>--- a/include/vlc/libvlc.h
>+++ b/include/vlc/libvlc.h
>@@ -410,19 +410,18 @@ LIBVLC_API void libvlc_log_get_object(const
>libvlc_log_t *ctx,
>                const char **name, const char **header, uintptr_t *id);
> 
> /**
>- * Callback prototype for LibVLC log message handler.
>+ * Callback prototype for LibVLC preformatted log message handler.
>  *
>  * \param data data pointer as given to libvlc_log_set()
>  * \param level message level (@ref libvlc_log_level)
>  * \param ctx message context (meta-information about the message)
>- * \param fmt printf() format string (as defined by ISO C11)
>- * \param args variable argument list for the format
>+ * \param message the message, already formatted
>  * \note Log message handlers <b>must</b> be thread-safe.
>- * \warning The message context pointer, the format string parameters
>and the
>- *          variable arguments are only valid until the callback
>returns.
>+ * \warning The message context pointer, and the message string
>parameters
>+ *          are only valid until the callback returns.
>  */
>-typedef void (*libvlc_log_cb)(void *data, int level, const
>libvlc_log_t *ctx,
>-                              const char *fmt, va_list args);
>+typedef void (*libvlc_log_preformatted_cb)(void *data, int level,
>const libvlc_log_t *ctx,
>+                              const char *message);
> 
> /**
>  * Unsets the logging callback.
>@@ -440,11 +439,13 @@ typedef void (*libvlc_log_cb)(void *data, int
>level, const libvlc_log_t *ctx,
> LIBVLC_API void libvlc_log_unset( libvlc_instance_t *p_instance );
> 
> /**
>- * Sets the logging callback for a LibVLC instance.
>+ * Sets the logging callback for a LibVLC instance, that takes a
>preformatted message as input.
>  *
> * This function is thread-safe: it will wait for any pending callbacks
>  * invocation to complete.
>  *
>+ * \param p_instance libvlc instance
>+ * \param min_level the callback will only be called if the message
>level is above this level, 0 means "get all messages" (@ref
>libvlc_log_level)
>  * \param cb callback function pointer
>  * \param data opaque data pointer for the callback function
>  *
>@@ -453,12 +454,10 @@ LIBVLC_API void libvlc_log_unset(
>libvlc_instance_t *p_instance );
>  *
>* \warning A deadlock may occur if this function is called from the
>callback.
>  *
>- * \param p_instance libvlc instance
>- * \version LibVLC 2.1.0 or later
>+ * \version LibVLC 4.0.0 or later
>  */
>-LIBVLC_API void libvlc_log_set( libvlc_instance_t *p_instance,
>-                                libvlc_log_cb cb, void *data );
>-
>+LIBVLC_API void libvlc_log_set_preformatted(libvlc_instance_t
>*p_instance, int min_level,
>+                                            libvlc_log_preformatted_cb
>cb, void* data );
> 
> /**
>  * Sets up logging to a file.
>diff --git a/lib/libvlc.sym b/lib/libvlc.sym
>index 456f9c2..59208c8 100644
>--- a/lib/libvlc.sym
>+++ b/lib/libvlc.sym
>@@ -55,8 +55,8 @@ libvlc_get_fullscreen
> libvlc_get_version
> libvlc_log_get_context
> libvlc_log_get_object
>-libvlc_log_set
> libvlc_log_set_file
>+libvlc_log_set_preformatted
> libvlc_log_unset
> libvlc_media_add_option
> libvlc_media_add_option_flag
>diff --git a/lib/libvlc_internal.h b/lib/libvlc_internal.h
>index 5c1107b..8c39aa2 100644
>--- a/lib/libvlc_internal.h
>+++ b/lib/libvlc_internal.h
>@@ -65,7 +65,8 @@ struct libvlc_instance_t
>     struct libvlc_callback_entry_list_t *p_callback_list;
>     struct
>     {
>-        void (*cb) (void *, int, const libvlc_log_t *, const char *,
>va_list);
>+        void (*cb) (void *, int, const libvlc_log_t *, const char *);
>+        int min_level;
>         void *data;
>     } log;
>     struct
>diff --git a/lib/log.c b/lib/log.c
>index 66e0d99..1e57602 100644
>--- a/lib/log.c
>+++ b/lib/log.c
>@@ -73,7 +73,16 @@ static void libvlc_logf (void *data, int level,
>const vlc_log_t *item,
>         case VLC_MSG_DBG:  level = LIBVLC_DEBUG;   break;
>     }
> 
>-    inst->log.cb (inst->log.data, level, item, fmt, ap);
>+    if(level >= inst->log.min_level)
>+    {
>+        char* msg;
>+        int msgSize = vasprintf(&msg, fmt, ap);
>+        if (unlikely(msgSize < 0))
>+            return;
>+
>+        inst->log.cb(inst->log.data, level, item, msg);
>+        free(msg);
>+    }
> }
> 
> static const struct vlc_logger_operations libvlc_log_ops = {
>@@ -85,28 +94,28 @@ void libvlc_log_unset (libvlc_instance_t *inst)
>     vlc_LogSet (inst->p_libvlc_int, NULL, NULL);
> }
> 
>-void libvlc_log_set (libvlc_instance_t *inst, libvlc_log_cb cb, void
>*data)
>+void libvlc_log_set_preformatted (libvlc_instance_t *p_instance, int
>min_level, libvlc_log_preformatted_cb cb, void* data )
> {
>-    libvlc_log_unset (inst); /* <- Barrier before modifying the
>callback */
>-    inst->log.cb = cb;
>-    inst->log.data = data;
>-    vlc_LogSet(inst->p_libvlc_int, &libvlc_log_ops, inst);
>+    libvlc_log_unset (p_instance); /* <- Barrier before modifying the
>callback */
>+    p_instance->log.cb = cb;
>+    p_instance->log.min_level = min_level;
>+    p_instance->log.data = data;
>+    vlc_LogSet(p_instance->p_libvlc_int, &libvlc_log_ops, p_instance);
> }
> 
> /*** Helpers for logging to files ***/
>static void libvlc_log_file (void *data, int level, const libvlc_log_t
>*log,
>-                             const char *fmt, va_list ap)
>+                             const char *msg)
> {
>     FILE *stream = data;
> 
>     flockfile (stream);
>-    vfprintf (stream, fmt, ap);
>-    fputc ('\n', stream);
>+    fprintf (stream, "%s\n", msg);
>     funlockfile (stream);
>     (void) level; (void) log;
> }
> 
> void libvlc_log_set_file (libvlc_instance_t *inst, FILE *stream)
> {
>-    libvlc_log_set (inst, libvlc_log_file, stream);
>+    libvlc_log_set_preformatted (inst, LIBVLC_DEBUG, libvlc_log_file,
>stream);
> }
>-- 
>2.20.1
>
>_______________________________________________
>vlc-devel mailing list
>To unsubscribe or modify your subscription options:
>https://mailman.videolan.org/listinfo/vlc-devel

-- 
Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20190521/d47be1d9/attachment.html>


More information about the vlc-devel mailing list