diff mbox series

[FFmpeg-devel,RFC,18/18] avcodec/avpacket: make the AVPacketList API thread safe

Message ID 20201118165247.4130-19-jamrial@gmail.com
State New
Headers show
Series AVPacketList public API | expand

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

James Almer Nov. 18, 2020, 4:52 p.m. UTC 
Signed-off-by: James Almer <jamrial@gmail.com>
---
I don't know if this is necessary, so i'm sending it as an RFC.

 libavcodec/avpacket.c        | 34 +++++++++++++++++++++++++++++++---
 libavcodec/packet_internal.h |  2 ++
 2 files changed, 33 insertions(+), 3 deletions(-)

Comments

Marton Balint Nov. 18, 2020, 8:08 p.m. UTC | #1 
On Wed, 18 Nov 2020, James Almer wrote:

> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> I don't know if this is necessary, so i'm sending it as an RFC.

I don't think this API should provide locking by default, maybe 
as an option. But considering that it was not often needed so far, and the 
API can be wrapped into locked versions (like decklink does) relatively 
easily, I think it is fine as is.

Regards,
Marton

>
> libavcodec/avpacket.c        | 34 +++++++++++++++++++++++++++++++---
> libavcodec/packet_internal.h |  2 ++
> 2 files changed, 33 insertions(+), 3 deletions(-)
>
> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> index 0db47c1d62..a8e934913e 100644
> --- a/libavcodec/avpacket.c
> +++ b/libavcodec/avpacket.c
> @@ -807,6 +807,8 @@ AVPacketList *av_packet_list_alloc(void)
>     if (!pktl)
>         return NULL;
> 
> +    ff_mutex_init(&pktl->mutex, NULL);
> +
>     return (AVPacketList *)pktl;
> }
> 
> @@ -840,6 +842,7 @@ int av_packet_list_put(AVPacketList *list, AVPacket *pkt,
>         av_packet_move_ref(&pktle->pkt, pkt);
>     }
> 
> +    ff_mutex_lock(&pktl->mutex);
>     if (pktl->head)
>         pktl->tail->next = pktle;
>     else
> @@ -847,6 +850,7 @@ int av_packet_list_put(AVPacketList *list, AVPacket *pkt,
>
>     /* Add the packet in the buffered packet list. */
>     pktl->tail = pktle;
> +    ff_mutex_unlock(&pktl->mutex);
>
>     return 0;
> }
> @@ -861,8 +865,11 @@ int av_packet_list_get(AVPacketList *list, AVPacket *pkt,
>     AVPacketList *pktl = list;
> #endif
> 
> -    if (!pktl->head)
> +    ff_mutex_lock(&pktl->mutex);
> +    if (!pktl->head) {
> +        ff_mutex_unlock(&pktl->mutex);
>         return AVERROR(EAGAIN);
> +    }
>
>     pktle      = pktl->head;
>     if (pkt)
> @@ -873,6 +880,7 @@ int av_packet_list_get(AVPacketList *list, AVPacket *pkt,
>
>     if (!pktle->next)
>         pktl->tail = NULL;
> +    ff_mutex_unlock(&pktl->mutex);
>
>     av_freep(&pktle);
> 
> @@ -891,14 +899,20 @@ int av_packet_list_peek(AVPacketList *list, AVPacket *pkt,
>     AVPacket tmp = { 0 };
>     int ret;
> 
> -    if (!pktl->head)
> +    ff_mutex_lock(&pktl->mutex);
> +    if (!pktl->head) {
> +        ff_mutex_unlock(&pktl->mutex);
>         return AVERROR(EAGAIN);
> +    }
> 
> -    if (!pkt)
> +    if (!pkt) {
> +       ff_mutex_unlock(&pktl->mutex);
>        return 0;
> +    }
>
>     pktle = pktl->head;
>     ret = av_packet_ref(&tmp, &pktle->pkt);
> +    ff_mutex_unlock(&pktl->mutex);
>     if (ret < 0)
>         return ret;
> 
> @@ -915,6 +929,7 @@ void av_packet_list_flush(AVPacketList *list)
>     AVPacketList *pktl = list;
> #endif
> 
> +    ff_mutex_lock(&pktl->mutex);
>     while (pktl->head) {
>         PacketListEntry *pktle = pktl->head;
>         pktl->head = pktle->next;
> @@ -923,16 +938,29 @@ void av_packet_list_flush(AVPacketList *list)
>     }
>
>     pktl->tail = NULL;
> +    ff_mutex_unlock(&pktl->mutex);
> }
> 
> void av_packet_list_free(AVPacketList **plist)
> {
>     AVPacketList *list = *plist;
> +#if FF_API_PACKET_LIST
> +    struct PacketList *pktl;
> +#else
> +    AVPacketList *pktl;
> +#endif
>
>     if (!list)
>         return;
> 
> +#if FF_API_PACKET_LIST
> +    pktl = (struct PacketList *)list;
> +#else
> +    pktl = list;
> +#endif
> +
>     av_packet_list_flush(list);
> +    ff_mutex_destroy(&pktl->mutex);
>     av_freep(plist);
> }
> 
> diff --git a/libavcodec/packet_internal.h b/libavcodec/packet_internal.h
> index fd9637bc44..4cb3fb4bbd 100644
> --- a/libavcodec/packet_internal.h
> +++ b/libavcodec/packet_internal.h
> @@ -21,6 +21,7 @@
> 
> #include <stdint.h>
> 
> +#include "libavutil/thread.h"
> #include "packet.h"
> 
> typedef struct PacketListEntry {
> @@ -36,6 +37,7 @@ struct AVPacketList {
> #endif
>     PacketListEntry *head;
>     PacketListEntry *tail;
> +    AVMutex mutex;
> };
> 
> #if LIBAVCODEC_VERSION_MAJOR < 59
> -- 
> 2.29.2
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
James Almer Nov. 19, 2020, 1:57 a.m. UTC | #2 
On 11/18/2020 5:08 PM, Marton Balint wrote:
> 
> 
> On Wed, 18 Nov 2020, James Almer wrote:
> 
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>> I don't know if this is necessary, so i'm sending it as an RFC.
> 
> I don't think this API should provide locking by default, maybe as an 
> option. But considering that it was not often needed so far, and the API 
> can be wrapped into locked versions (like decklink does) relatively 
> easily, I think it is fine as is.

My worry was adding a linked list public API that's not thread safe.

The other example we have is AVBufferPool, which does locking 
unconditionally by itself before adding or removing elements from the 
list, so it's valid to assume this one should maybe do the same.
Michael Niedermayer Nov. 19, 2020, 4:18 p.m. UTC | #3 
On Wed, Nov 18, 2020 at 01:52:47PM -0300, James Almer wrote:
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> I don't know if this is necessary, so i'm sending it as an RFC.
> 
>  libavcodec/avpacket.c        | 34 +++++++++++++++++++++++++++++++---
>  libavcodec/packet_internal.h |  2 ++
>  2 files changed, 33 insertions(+), 3 deletions(-)

This seems to break build on mingw

In file included from src/libavutil/thread.h:146:0,
                 from src/libavcodec/packet_internal.h:24,
                 from src/libavformat/internal.h:29,
                 from src/libavformat/options_table.h:28,
                 from src/doc/print_options.c:43:
src/compat/w32pthreads.h:39:10: fatal error: windows.h: No such file or directory
 #include <windows.h>
          ^~~~~~~~~~~
compilation terminated.


[...]
James Almer Nov. 19, 2020, 4:36 p.m. UTC | #4 
On 11/19/2020 1:18 PM, Michael Niedermayer wrote:
> On Wed, Nov 18, 2020 at 01:52:47PM -0300, James Almer wrote:
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>> I don't know if this is necessary, so i'm sending it as an RFC.
>>
>>   libavcodec/avpacket.c        | 34 +++++++++++++++++++++++++++++++---
>>   libavcodec/packet_internal.h |  2 ++
>>   2 files changed, 33 insertions(+), 3 deletions(-)
> 
> This seems to break build on mingw
> 
> In file included from src/libavutil/thread.h:146:0,
>                   from src/libavcodec/packet_internal.h:24,
>                   from src/libavformat/internal.h:29,
>                   from src/libavformat/options_table.h:28,
>                   from src/doc/print_options.c:43:
> src/compat/w32pthreads.h:39:10: fatal error: windows.h: No such file or directory
>   #include <windows.h>

This makes no sense. How can that header be missing?

Either you're using a very old mingw build, or a broken one, because i 
tested this on mingw myself.

>            ^~~~~~~~~~~
> compilation terminated.
> 
> 
> [...]
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Marton Balint Nov. 20, 2020, 8:50 p.m. UTC | #5 
On Wed, 18 Nov 2020, James Almer wrote:

> On 11/18/2020 5:08 PM, Marton Balint wrote:
>> 
>> 
>> On Wed, 18 Nov 2020, James Almer wrote:
>> 
>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>> ---
>>> I don't know if this is necessary, so i'm sending it as an RFC.
>> 
>> I don't think this API should provide locking by default, maybe as an 
>> option. But considering that it was not often needed so far, and the API 
>> can be wrapped into locked versions (like decklink does) relatively 
>> easily, I think it is fine as is.
>
> My worry was adding a linked list public API that's not thread safe.
>
> The other example we have is AVBufferPool, which does locking 
> unconditionally by itself before adding or removing elements from the 
> list, so it's valid to assume this one should maybe do the same.

AVBufferPool has to be thread safe because the AVBufferRef free function 
has to be thread safe as well.

Maybe a better example is av_fifo_generic_read/write which is not thread 
safe... Do you know if the overhead of making this thread safe is 
significant/measureable? Because obviously that is my primary concern.

Thanks,
Marton
diff mbox series

Patch

diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
index 0db47c1d62..a8e934913e 100644
--- a/libavcodec/avpacket.c
+++ b/libavcodec/avpacket.c
@@ -807,6 +807,8 @@  AVPacketList *av_packet_list_alloc(void)
     if (!pktl)
         return NULL;
 
+    ff_mutex_init(&pktl->mutex, NULL);
+
     return (AVPacketList *)pktl;
 }
 
@@ -840,6 +842,7 @@  int av_packet_list_put(AVPacketList *list, AVPacket *pkt,
         av_packet_move_ref(&pktle->pkt, pkt);
     }
 
+    ff_mutex_lock(&pktl->mutex);
     if (pktl->head)
         pktl->tail->next = pktle;
     else
@@ -847,6 +850,7 @@  int av_packet_list_put(AVPacketList *list, AVPacket *pkt,
 
     /* Add the packet in the buffered packet list. */
     pktl->tail = pktle;
+    ff_mutex_unlock(&pktl->mutex);
 
     return 0;
 }
@@ -861,8 +865,11 @@  int av_packet_list_get(AVPacketList *list, AVPacket *pkt,
     AVPacketList *pktl = list;
 #endif
 
-    if (!pktl->head)
+    ff_mutex_lock(&pktl->mutex);
+    if (!pktl->head) {
+        ff_mutex_unlock(&pktl->mutex);
         return AVERROR(EAGAIN);
+    }
 
     pktle      = pktl->head;
     if (pkt)
@@ -873,6 +880,7 @@  int av_packet_list_get(AVPacketList *list, AVPacket *pkt,
 
     if (!pktle->next)
         pktl->tail = NULL;
+    ff_mutex_unlock(&pktl->mutex);
 
     av_freep(&pktle);
 
@@ -891,14 +899,20 @@  int av_packet_list_peek(AVPacketList *list, AVPacket *pkt,
     AVPacket tmp = { 0 };
     int ret;
 
-    if (!pktl->head)
+    ff_mutex_lock(&pktl->mutex);
+    if (!pktl->head) {
+        ff_mutex_unlock(&pktl->mutex);
         return AVERROR(EAGAIN);
+    }
 
-    if (!pkt)
+    if (!pkt) {
+       ff_mutex_unlock(&pktl->mutex);
        return 0;
+    }
 
     pktle = pktl->head;
     ret = av_packet_ref(&tmp, &pktle->pkt);
+    ff_mutex_unlock(&pktl->mutex);
     if (ret < 0)
         return ret;
 
@@ -915,6 +929,7 @@  void av_packet_list_flush(AVPacketList *list)
     AVPacketList *pktl = list;
 #endif
 
+    ff_mutex_lock(&pktl->mutex);
     while (pktl->head) {
         PacketListEntry *pktle = pktl->head;
         pktl->head = pktle->next;
@@ -923,16 +938,29 @@  void av_packet_list_flush(AVPacketList *list)
     }
 
     pktl->tail = NULL;
+    ff_mutex_unlock(&pktl->mutex);
 }
 
 void av_packet_list_free(AVPacketList **plist)
 {
     AVPacketList *list = *plist;
+#if FF_API_PACKET_LIST
+    struct PacketList *pktl;
+#else
+    AVPacketList *pktl;
+#endif
 
     if (!list)
         return;
 
+#if FF_API_PACKET_LIST
+    pktl = (struct PacketList *)list;
+#else
+    pktl = list;
+#endif
+
     av_packet_list_flush(list);
+    ff_mutex_destroy(&pktl->mutex);
     av_freep(plist);
 }
 
diff --git a/libavcodec/packet_internal.h b/libavcodec/packet_internal.h
index fd9637bc44..4cb3fb4bbd 100644
--- a/libavcodec/packet_internal.h
+++ b/libavcodec/packet_internal.h
@@ -21,6 +21,7 @@ 
 
 #include <stdint.h>
 
+#include "libavutil/thread.h"
 #include "packet.h"
 
 typedef struct PacketListEntry {
@@ -36,6 +37,7 @@  struct AVPacketList {
 #endif
     PacketListEntry *head;
     PacketListEntry *tail;
+    AVMutex mutex;
 };
 
 #if LIBAVCODEC_VERSION_MAJOR < 59