Re: [FFmpeg-devel] [PATCH 1/3] avutil/threadmessage: add av_thread_message_flush()

[Clément Bœsch]

On Sun, Dec 06, 2015 at 12:20:28PM +0100, Nicolas George wrote:
[...]
> > +static void free_func_wrap(void *arg, void *msg, int size)
> > +{
> > +void (*free_func)(void *msg) = arg;
> 
> Technically, this is not legal: void* is a data pointer, it could be smaller
> than a function pointer (remember the "medium" memory model for 16-bits
> code). I do not object much, but it is easy to fix: just pass _func as
> argument, or even squarely mq itself.
> 

Changed locally to mq->free_func, thanks, will push soon

[...]

-- 
Clément B.


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/3] avutil/threadmessage: add av_thread_message_flush()

[Clément Bœsch]

On Mon, Dec 07, 2015 at 11:35:34AM +0100, Clément Bœsch wrote:
> On Sun, Dec 06, 2015 at 12:20:28PM +0100, Nicolas George wrote:
> [...]
> > > +static void free_func_wrap(void *arg, void *msg, int size)
> > > +{
> > > +void (*free_func)(void *msg) = arg;
> > 
> > Technically, this is not legal: void* is a data pointer, it could be smaller
> > than a function pointer (remember the "medium" memory model for 16-bits
> > code). I do not object much, but it is easy to fix: just pass _func as
> > argument, or even squarely mq itself.
> > 
> 
> Changed locally to mq->free_func, thanks, will push soon
> 

Pushed, thanks

-- 
Clément B.


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/3] avutil/threadmessage: add av_thread_message_flush()

[Nicolas George]

Le quartidi 14 frimaire, an CCXXIV, Clement Boesch a écrit :
> From: Clément Bœsch 
> 
> ---
>  libavutil/threadmessage.c | 31 +++
>  libavutil/threadmessage.h | 16 
>  2 files changed, 47 insertions(+)
> 
> diff --git a/libavutil/threadmessage.c b/libavutil/threadmessage.c
> index b7fcbe2..a5f1507 100644
> --- a/libavutil/threadmessage.c
> +++ b/libavutil/threadmessage.c
> @@ -40,6 +40,7 @@ struct AVThreadMessageQueue {
>  int err_send;
>  int err_recv;
>  unsigned elsize;
> +void (*free_func)(void *msg);
>  #else
>  int dummy;
>  #endif
> @@ -81,10 +82,17 @@ int av_thread_message_queue_alloc(AVThreadMessageQueue 
> **mq,
>  #endif /* HAVE_THREADS */
>  }
>  
> +void av_thread_message_queue_set_free_func(AVThreadMessageQueue *mq,
> +   void (*free_func)(void *msg))
> +{
> +mq->free_func = free_func;
> +}
> +
>  void av_thread_message_queue_free(AVThreadMessageQueue **mq)
>  {
>  #if HAVE_THREADS
>  if (*mq) {
> +av_thread_message_flush(*mq);
>  av_fifo_freep(&(*mq)->fifo);
>  pthread_cond_destroy(&(*mq)->cond);
>  pthread_mutex_destroy(&(*mq)->lock);
> @@ -182,3 +190,26 @@ void 
> av_thread_message_queue_set_err_recv(AVThreadMessageQueue *mq,
>  pthread_mutex_unlock(>lock);
>  #endif /* HAVE_THREADS */
>  }
> +

> +static void free_func_wrap(void *arg, void *msg, int size)
> +{
> +void (*free_func)(void *msg) = arg;

Technically, this is not legal: void* is a data pointer, it could be smaller
than a function pointer (remember the "medium" memory model for 16-bits
code). I do not object much, but it is easy to fix: just pass _func as
argument, or even squarely mq itself.

> +free_func(msg);
> +}
> +
> +void av_thread_message_flush(AVThreadMessageQueue *mq)
> +{
> +#if HAVE_THREADS
> +int used, off;
> +void *free_func = mq->free_func;
> +
> +pthread_mutex_lock(>lock);
> +used = av_fifo_size(mq->fifo);
> +if (free_func)
> +for (off = 0; off < used; off += mq->elsize)
> +av_fifo_generic_peek_at(mq->fifo, free_func, off, mq->elsize, 
> free_func_wrap);
> +av_fifo_drain(mq->fifo, used);
> +pthread_cond_broadcast(>cond);
> +pthread_mutex_unlock(>lock);
> +#endif /* HAVE_THREADS */
> +}
> diff --git a/libavutil/threadmessage.h b/libavutil/threadmessage.h
> index a8481d8..e256cae 100644
> --- a/libavutil/threadmessage.h
> +++ b/libavutil/threadmessage.h
> @@ -88,4 +88,20 @@ void 
> av_thread_message_queue_set_err_send(AVThreadMessageQueue *mq,
>  void av_thread_message_queue_set_err_recv(AVThreadMessageQueue *mq,
>int err);
>  
> +/**
> + * Set the optional free message callback function which will be called if an
> + * operation is removing messages from the queue.
> + */
> +void av_thread_message_queue_set_free_func(AVThreadMessageQueue *mq,
> +   void (*free_func)(void *msg));
> +
> +/**
> + * Flush the message queue
> + *
> + * This function is mostly equivalent to reading and free-ing every message
> + * except that it will be done in a single operation (no lock/unlock between
> + * reads).
> + */
> +void av_thread_message_flush(AVThreadMessageQueue *mq);
> +
>  #endif /* AVUTIL_THREADMESSAGE_H */

Rest looks good to me, thanks for humouring me.

Regards,

-- 
  Nicolas George


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 1/3] avutil/threadmessage: add av_thread_message_flush()

[Clément Bœsch]

From: Clément Bœsch 

---
 libavutil/threadmessage.c | 31 +++
 libavutil/threadmessage.h | 16 
 2 files changed, 47 insertions(+)

diff --git a/libavutil/threadmessage.c b/libavutil/threadmessage.c
index b7fcbe2..a5f1507 100644
--- a/libavutil/threadmessage.c
+++ b/libavutil/threadmessage.c
@@ -40,6 +40,7 @@ struct AVThreadMessageQueue {
 int err_send;
 int err_recv;
 unsigned elsize;
+void (*free_func)(void *msg);
 #else
 int dummy;
 #endif
@@ -81,10 +82,17 @@ int av_thread_message_queue_alloc(AVThreadMessageQueue **mq,
 #endif /* HAVE_THREADS */
 }
 
+void av_thread_message_queue_set_free_func(AVThreadMessageQueue *mq,
+   void (*free_func)(void *msg))
+{
+mq->free_func = free_func;
+}
+
 void av_thread_message_queue_free(AVThreadMessageQueue **mq)
 {
 #if HAVE_THREADS
 if (*mq) {
+av_thread_message_flush(*mq);
 av_fifo_freep(&(*mq)->fifo);
 pthread_cond_destroy(&(*mq)->cond);
 pthread_mutex_destroy(&(*mq)->lock);
@@ -182,3 +190,26 @@ void 
av_thread_message_queue_set_err_recv(AVThreadMessageQueue *mq,
 pthread_mutex_unlock(>lock);
 #endif /* HAVE_THREADS */
 }
+
+static void free_func_wrap(void *arg, void *msg, int size)
+{
+void (*free_func)(void *msg) = arg;
+free_func(msg);
+}
+
+void av_thread_message_flush(AVThreadMessageQueue *mq)
+{
+#if HAVE_THREADS
+int used, off;
+void *free_func = mq->free_func;
+
+pthread_mutex_lock(>lock);
+used = av_fifo_size(mq->fifo);
+if (free_func)
+for (off = 0; off < used; off += mq->elsize)
+av_fifo_generic_peek_at(mq->fifo, free_func, off, mq->elsize, 
free_func_wrap);
+av_fifo_drain(mq->fifo, used);
+pthread_cond_broadcast(>cond);
+pthread_mutex_unlock(>lock);
+#endif /* HAVE_THREADS */
+}
diff --git a/libavutil/threadmessage.h b/libavutil/threadmessage.h
index a8481d8..e256cae 100644
--- a/libavutil/threadmessage.h
+++ b/libavutil/threadmessage.h
@@ -88,4 +88,20 @@ void 
av_thread_message_queue_set_err_send(AVThreadMessageQueue *mq,
 void av_thread_message_queue_set_err_recv(AVThreadMessageQueue *mq,
   int err);
 
+/**
+ * Set the optional free message callback function which will be called if an
+ * operation is removing messages from the queue.
+ */
+void av_thread_message_queue_set_free_func(AVThreadMessageQueue *mq,
+   void (*free_func)(void *msg));
+
+/**
+ * Flush the message queue
+ *
+ * This function is mostly equivalent to reading and free-ing every message
+ * except that it will be done in a single operation (no lock/unlock between
+ * reads).
+ */
+void av_thread_message_flush(AVThreadMessageQueue *mq);
+
 #endif /* AVUTIL_THREADMESSAGE_H */
-- 
2.6.3

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 1/3] avutil/threadmessage: add av_thread_message_flush()

[Clément Bœsch]

From: Clément Bœsch 

---
 libavutil/threadmessage.c | 37 ++---
 libavutil/threadmessage.h | 21 ++---
 2 files changed, 52 insertions(+), 6 deletions(-)

diff --git a/libavutil/threadmessage.c b/libavutil/threadmessage.c
index b7fcbe2..87ce8dc 100644
--- a/libavutil/threadmessage.c
+++ b/libavutil/threadmessage.c
@@ -40,14 +40,16 @@ struct AVThreadMessageQueue {
 int err_send;
 int err_recv;
 unsigned elsize;
+void (*free_func)(void *msg);
 #else
 int dummy;
 #endif
 };
 
-int av_thread_message_queue_alloc(AVThreadMessageQueue **mq,
-  unsigned nelem,
-  unsigned elsize)
+int av_thread_message_queue_alloc2(AVThreadMessageQueue **mq,
+   unsigned nelem,
+   unsigned elsize,
+   void (*free_func)(void *msg))
 {
 #if HAVE_THREADS
 AVThreadMessageQueue *rmq;
@@ -73,6 +75,7 @@ int av_thread_message_queue_alloc(AVThreadMessageQueue **mq,
 return AVERROR(ret);
 }
 rmq->elsize = elsize;
+rmq->free_func = free_func;
 *mq = rmq;
 return 0;
 #else
@@ -81,10 +84,18 @@ int av_thread_message_queue_alloc(AVThreadMessageQueue **mq,
 #endif /* HAVE_THREADS */
 }
 
+int av_thread_message_queue_alloc(AVThreadMessageQueue **mq,
+  unsigned nelem,
+  unsigned elsize)
+{
+return av_thread_message_queue_alloc2(mq, nelem, elsize, NULL);
+}
+
 void av_thread_message_queue_free(AVThreadMessageQueue **mq)
 {
 #if HAVE_THREADS
 if (*mq) {
+av_thread_message_flush(*mq);
 av_fifo_freep(&(*mq)->fifo);
 pthread_cond_destroy(&(*mq)->cond);
 pthread_mutex_destroy(&(*mq)->lock);
@@ -182,3 +193,23 @@ void 
av_thread_message_queue_set_err_recv(AVThreadMessageQueue *mq,
 pthread_mutex_unlock(>lock);
 #endif /* HAVE_THREADS */
 }
+
+void av_thread_message_flush(AVThreadMessageQueue *mq)
+{
+#if HAVE_THREADS
+int used, off;
+
+pthread_mutex_lock(>lock);
+used = av_fifo_size(mq->fifo);
+if (mq->free_func) {
+for (off = 0; off < used; off += mq->elsize) {
+void *msg;
+av_fifo_generic_peek_at(mq->fifo, , off, mq->elsize, NULL);
+mq->free_func(msg);
+}
+}
+av_fifo_drain(mq->fifo, used);
+pthread_cond_broadcast(>cond);
+pthread_mutex_unlock(>lock);
+#endif /* HAVE_THREADS */
+}
diff --git a/libavutil/threadmessage.h b/libavutil/threadmessage.h
index a8481d8..f9004a8 100644
--- a/libavutil/threadmessage.h
+++ b/libavutil/threadmessage.h
@@ -33,17 +33,27 @@ typedef enum AVThreadMessageFlags {
 } AVThreadMessageFlags;
 
 /**
+ * @deprecated use av_thread_message_queue_alloc2 instead
+ */
+attribute_deprecated
+int av_thread_message_queue_alloc(AVThreadMessageQueue **mq,
+  unsigned nelem,
+  unsigned elsize);
+
+/**
  * Allocate a new message queue.
  *
  * @param mq  pointer to the message queue
  * @param nelem   maximum number of elements in the queue
  * @param elsize  size of each element in the queue
+ * @param free_func free message callback function, can be NULL
  * @return  >=0 for success; <0 for error, in particular AVERROR(ENOSYS) if
  *  lavu was built without thread support
  */
-int av_thread_message_queue_alloc(AVThreadMessageQueue **mq,
-  unsigned nelem,
-  unsigned elsize);
+int av_thread_message_queue_alloc2(AVThreadMessageQueue **mq,
+   unsigned nelem,
+   unsigned elsize,
+   void (*free_func)(void *msg));
 
 /**
  * Free a message queue.
@@ -88,4 +98,9 @@ void 
av_thread_message_queue_set_err_send(AVThreadMessageQueue *mq,
 void av_thread_message_queue_set_err_recv(AVThreadMessageQueue *mq,
   int err);
 
+/**
+ * Flush the message queue
+ */
+void av_thread_message_flush(AVThreadMessageQueue *mq);
+
 #endif /* AVUTIL_THREADMESSAGE_H */
-- 
2.6.2

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/3] avutil/threadmessage: add av_thread_message_flush()

[Clément Bœsch]

On Wed, Dec 02, 2015 at 05:09:23PM +0100, Nicolas George wrote:
> Le duodi 12 frimaire, an CCXXIV, Clement Boesch a écrit :
> > What would be the difference? both av_fifo_generic_peek and
> > av_fifo_generic_peek_at require a copy of the element in a destination
> > buffer, unless I'm missing something.
> 
> As I see it, av_fifo_generic_peek() does NOT require a copy if a callback is
> supplied.
> 

Ah, indeed. av_fifo_generic_peek_at seems to have the same, I didn't
realize. Code is much simpler that way, thanks.

-- 
Clément B.


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/3] avutil/threadmessage: add av_thread_message_flush()

[Nicolas George]

Le duodi 12 frimaire, an CCXXIV, Clement Boesch a écrit :
> From: Clément Bœsch 
> 
> ---
>  libavutil/threadmessage.c | 32 
>  libavutil/threadmessage.h | 12 
>  2 files changed, 44 insertions(+)
> 
> diff --git a/libavutil/threadmessage.c b/libavutil/threadmessage.c
> index b7fcbe2..66b5fc6 100644
> --- a/libavutil/threadmessage.c
> +++ b/libavutil/threadmessage.c
> @@ -40,6 +40,8 @@ struct AVThreadMessageQueue {
>  int err_send;
>  int err_recv;
>  unsigned elsize;
> +void (*free_func)(void *msg);
> +void *tmp_msg;
>  #else
>  int dummy;
>  #endif
> @@ -81,10 +83,21 @@ int av_thread_message_queue_alloc(AVThreadMessageQueue 
> **mq,
>  #endif /* HAVE_THREADS */
>  }
>  
> +int av_thread_message_queue_set_free_func(AVThreadMessageQueue *mq,
> +  void (*free_func)(void *msg))
> +{
> +mq->free_func = free_func;
> +av_freep(>tmp_msg);
> +mq->tmp_msg = av_mallocz(mq->elsize);
> +return mq->tmp_msg ? 0 : AVERROR(ENOMEM);
> +}
> +
>  void av_thread_message_queue_free(AVThreadMessageQueue **mq)
>  {
>  #if HAVE_THREADS
>  if (*mq) {
> +av_thread_message_flush(*mq);
> +av_freep(&(*mq)->tmp_msg);
>  av_fifo_freep(&(*mq)->fifo);
>  pthread_cond_destroy(&(*mq)->cond);
>  pthread_mutex_destroy(&(*mq)->lock);
> @@ -182,3 +195,22 @@ void 
> av_thread_message_queue_set_err_recv(AVThreadMessageQueue *mq,
>  pthread_mutex_unlock(>lock);
>  #endif /* HAVE_THREADS */
>  }
> +
> +void av_thread_message_flush(AVThreadMessageQueue *mq)
> +{
> +#if HAVE_THREADS
> +int used, off;
> +
> +pthread_mutex_lock(>lock);
> +used = av_fifo_size(mq->fifo);
> +if (mq->free_func) {
> +for (off = 0; off < used; off += mq->elsize) {

> +av_fifo_generic_peek_at(mq->fifo, mq->tmp_msg, off, mq->elsize, 
> NULL);
> +mq->free_func(mq->tmp_msg);

Could this use av_fifo_generic_peek() to avoid the ugly extra allocation?

> +}
> +}
> +av_fifo_drain(mq->fifo, used);
> +pthread_cond_broadcast(>cond);
> +pthread_mutex_unlock(>lock);
> +#endif /* HAVE_THREADS */
> +}
> diff --git a/libavutil/threadmessage.h b/libavutil/threadmessage.h
> index a8481d8..c59cb06 100644
> --- a/libavutil/threadmessage.h
> +++ b/libavutil/threadmessage.h
> @@ -88,4 +88,16 @@ void 
> av_thread_message_queue_set_err_send(AVThreadMessageQueue *mq,
>  void av_thread_message_queue_set_err_recv(AVThreadMessageQueue *mq,
>int err);
>  
> +/**
> + * Set the optional free message callback function which will be called if an
> + * operation is removing messages from the queue.
> + */
> +int av_thread_message_queue_set_free_func(AVThreadMessageQueue *mq,
> +  void (*free_func)(void *msg));
> +
> +/**

> + * Flush the message queue

I will not block for that, but I am still not satisfied with the
explanations, and therefore I find this documentation insufficient. Maybe
wait for other advice?

> + */
> +void av_thread_message_flush(AVThreadMessageQueue *mq);
> +
>  #endif /* AVUTIL_THREADMESSAGE_H */

Regards,

-- 
  Nicolas George


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/3] avutil/threadmessage: add av_thread_message_flush()

[Nicolas George]

Le duodi 12 frimaire, an CCXXIV, Clement Boesch a écrit :
> What would be the difference? both av_fifo_generic_peek and
> av_fifo_generic_peek_at require a copy of the element in a destination
> buffer, unless I'm missing something.

As I see it, av_fifo_generic_peek() does NOT require a copy if a callback is
supplied.

Regards,

-- 
  Nicolas George


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/3] avutil/threadmessage: add av_thread_message_flush()

[Clément Bœsch]

On Wed, Dec 02, 2015 at 04:59:15PM +0100, Nicolas George wrote:
[...]
> > +av_fifo_generic_peek_at(mq->fifo, mq->tmp_msg, off, 
> > mq->elsize, NULL);
> > +mq->free_func(mq->tmp_msg);
> 
> Could this use av_fifo_generic_peek() to avoid the ugly extra allocation?
> 

What would be the difference? both av_fifo_generic_peek and
av_fifo_generic_peek_at require a copy of the element in a destination
buffer, unless I'm missing something.

> > +}
> > +}
> > +av_fifo_drain(mq->fifo, used);
> > +pthread_cond_broadcast(>cond);
> > +pthread_mutex_unlock(>lock);
> > +#endif /* HAVE_THREADS */
> > +}
> > diff --git a/libavutil/threadmessage.h b/libavutil/threadmessage.h
> > index a8481d8..c59cb06 100644
> > --- a/libavutil/threadmessage.h
> > +++ b/libavutil/threadmessage.h
> > @@ -88,4 +88,16 @@ void 
> > av_thread_message_queue_set_err_send(AVThreadMessageQueue *mq,
> >  void av_thread_message_queue_set_err_recv(AVThreadMessageQueue *mq,
> >int err);
> >  
> > +/**
> > + * Set the optional free message callback function which will be called if 
> > an
> > + * operation is removing messages from the queue.
> > + */
> > +int av_thread_message_queue_set_free_func(AVThreadMessageQueue *mq,
> > +  void (*free_func)(void *msg));
> > +
> > +/**
> 
> > + * Flush the message queue
> 
> I will not block for that, but I am still not satisfied with the
> explanations, and therefore I find this documentation insufficient. Maybe
> wait for other advice?
> 

Sure, no problem.

-- 
Clément B.


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel