Message ID | 20201118165247.4130-19-jamrial@gmail.com |
---|---|
State | New |
Headers | show |
Series | AVPacketList public API | expand |
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 |
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".
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.
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. [...]
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". >
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 --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
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(-)