| Message ID | 20201204153729.12783-1-jamrial@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/decode: port last_pkt_props to AVFifoBuffer | 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 |
James Almer: > Signed-off-by: James Almer <jamrial@gmail.com> > --- > The idea of making avpriv_packet_list_* public was not liked, and it was > suggested to just use AVFifoBuffer instead. > > After this, the avpriv_packet_list_* functions can be made local to > libavformat again. > > libavcodec/decode.c | 41 +++++++++++++++++++++++++++++------------ > libavcodec/internal.h | 4 ++-- > libavcodec/utils.c | 11 ++++++----- > 3 files changed, 37 insertions(+), 19 deletions(-) > > diff --git a/libavcodec/decode.c b/libavcodec/decode.c > index 5a1849f944..0550637029 100644 > --- a/libavcodec/decode.c > +++ b/libavcodec/decode.c > @@ -145,22 +145,40 @@ fail2: > > #define IS_EMPTY(pkt) (!(pkt)->data) > > +static int copy_packet_props(void *_src, void *_dst, int size) > +{ > + AVPacket *src = _src, *dst = _dst; > + int ret; > + > + av_init_packet(dst); > + ret = av_packet_copy_props(dst, src); > + if (ret < 0) > + return ret;> + > + dst->size = src->size; // HACK: Needed for ff_decode_frame_props(). > + dst->data = (void*)1; // HACK: Needed for IS_EMPTY(). > + > + return sizeof(*dst); > +} This is not how a write function for a fifo should look like: A fifo may need to store the beginning of a packet at the end of the fifo and the end of a packet at the beginning of a fifo; you can check for this by checking whether size is < sizeof(AVPacket). (The current implementation seems to actually only allocate multiples of a certain unit if all av_fifo_grow()/av_fifo_realloc2()/av_fifo_alloc() calls use only multiples of this unit, but it doesn't seem to be documented. Should probably be done as this simplifies using fifo arrays.) And apart from that: The current implementation of av_fifo_generic_write() does not forward errors from the write function; and changing this require be an API break. > + > static int extract_packet_props(AVCodecInternal *avci, AVPacket *pkt) > { > int ret = 0; > > - ret = avpriv_packet_list_put(&avci->pkt_props, &avci->pkt_props_tail, pkt, > - av_packet_copy_props, 0); > + if (av_fifo_space(avci->pkt_props) < sizeof(*pkt)) { > + ret = av_fifo_grow(avci->pkt_props, sizeof(*pkt)); > + if (ret < 0) > + return ret; > + } > + > + ret = av_fifo_generic_write(avci->pkt_props, pkt, sizeof(*pkt), copy_packet_props); > if (ret < 0) > return ret; > - avci->pkt_props_tail->pkt.size = pkt->size; // HACK: Needed for ff_decode_frame_props(). > - avci->pkt_props_tail->pkt.data = (void*)1; // HACK: Needed for IS_EMPTY(). > > + av_assert0(ret == sizeof(*pkt)); > if (IS_EMPTY(avci->last_pkt_props)) { > - ret = avpriv_packet_list_get(&avci->pkt_props, > - &avci->pkt_props_tail, > - avci->last_pkt_props); > - av_assert0(ret != AVERROR(EAGAIN)); > + ret = av_fifo_generic_read(avci->pkt_props, avci->last_pkt_props, > + sizeof(*avci->last_pkt_props), NULL); > } > return ret; > } > @@ -1721,10 +1739,9 @@ int ff_decode_frame_props(AVCodecContext *avctx, AVFrame *frame) > { AV_PKT_DATA_S12M_TIMECODE, AV_FRAME_DATA_S12M_TIMECODE }, > }; > > - if (IS_EMPTY(pkt)) > - avpriv_packet_list_get(&avctx->internal->pkt_props, > - &avctx->internal->pkt_props_tail, > - pkt); > + if (IS_EMPTY(pkt) && av_fifo_size(avctx->internal->pkt_props) >= sizeof(*pkt)) > + av_fifo_generic_read(avctx->internal->pkt_props, > + pkt, sizeof(*pkt), NULL); > > if (pkt) { > frame->pts = pkt->pts; > diff --git a/libavcodec/internal.h b/libavcodec/internal.h > index 17defb9b50..8a51bca2df 100644 > --- a/libavcodec/internal.h > +++ b/libavcodec/internal.h > @@ -28,6 +28,7 @@ > > #include "libavutil/buffer.h" > #include "libavutil/channel_layout.h" > +#include "libavutil/fifo.h" > #include "libavutil/mathematics.h" > #include "libavutil/pixfmt.h" > #include "avcodec.h" > @@ -145,8 +146,7 @@ typedef struct AVCodecInternal { > * for decoding. > */ > AVPacket *last_pkt_props; > - AVPacketList *pkt_props; > - AVPacketList *pkt_props_tail; > + AVFifoBuffer *pkt_props; > > /** > * temporary buffer used for encoders to store their bitstream > diff --git a/libavcodec/utils.c b/libavcodec/utils.c > index 45295dd3ce..c81cc972dc 100644 > --- a/libavcodec/utils.c > +++ b/libavcodec/utils.c > @@ -593,10 +593,12 @@ int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *code > avci->es.in_frame = av_frame_alloc(); > avci->ds.in_pkt = av_packet_alloc(); > avci->last_pkt_props = av_packet_alloc(); > + avci->pkt_props = av_fifo_alloc(sizeof(AVPacket)); > if (!avci->compat_decode_frame || !avci->compat_encode_packet || > !avci->buffer_frame || !avci->buffer_pkt || > !avci->es.in_frame || !avci->ds.in_pkt || > - !avci->to_free || !avci->last_pkt_props) { > + !avci->to_free || !avci->last_pkt_props || > + !avci->pkt_props) { > ret = AVERROR(ENOMEM); > goto free_and_end; > } > @@ -1076,6 +1078,7 @@ FF_ENABLE_DEPRECATION_WARNINGS > av_packet_free(&avci->compat_encode_packet); > av_packet_free(&avci->buffer_pkt); > av_packet_free(&avci->last_pkt_props); > + av_fifo_freep(&avci->pkt_props); > > av_packet_free(&avci->ds.in_pkt); > av_frame_free(&avci->es.in_frame); > @@ -1116,8 +1119,7 @@ void avcodec_flush_buffers(AVCodecContext *avctx) > av_packet_unref(avci->buffer_pkt); > > av_packet_unref(avci->last_pkt_props); > - avpriv_packet_list_free(&avci->pkt_props, > - &avci->pkt_props_tail); > + av_fifo_reset(avci->pkt_props); > The packets in the fifo may contain side-data (because av_packet_copy_props() copies it) and if so, it leaks here. > av_frame_unref(avci->es.in_frame); > av_packet_unref(avci->ds.in_pkt); > @@ -1180,8 +1182,7 @@ av_cold int avcodec_close(AVCodecContext *avctx) > av_packet_free(&avctx->internal->compat_encode_packet); > av_packet_free(&avctx->internal->buffer_pkt); > av_packet_free(&avctx->internal->last_pkt_props); > - avpriv_packet_list_free(&avctx->internal->pkt_props, > - &avctx->internal->pkt_props_tail); > + av_fifo_freep(&avctx->internal->pkt_props); > > av_packet_free(&avctx->internal->ds.in_pkt); > av_frame_free(&avctx->internal->es.in_frame); >
On 12/4/2020 2:08 PM, Andreas Rheinhardt wrote: > James Almer: >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> The idea of making avpriv_packet_list_* public was not liked, and it was >> suggested to just use AVFifoBuffer instead. >> >> After this, the avpriv_packet_list_* functions can be made local to >> libavformat again. >> >> libavcodec/decode.c | 41 +++++++++++++++++++++++++++++------------ >> libavcodec/internal.h | 4 ++-- >> libavcodec/utils.c | 11 ++++++----- >> 3 files changed, 37 insertions(+), 19 deletions(-) >> >> diff --git a/libavcodec/decode.c b/libavcodec/decode.c >> index 5a1849f944..0550637029 100644 >> --- a/libavcodec/decode.c >> +++ b/libavcodec/decode.c >> @@ -145,22 +145,40 @@ fail2: >> >> #define IS_EMPTY(pkt) (!(pkt)->data) >> >> +static int copy_packet_props(void *_src, void *_dst, int size) >> +{ >> + AVPacket *src = _src, *dst = _dst; >> + int ret; >> + >> + av_init_packet(dst); >> + ret = av_packet_copy_props(dst, src); >> + if (ret < 0) >> + return ret;> + >> + dst->size = src->size; // HACK: Needed for ff_decode_frame_props(). >> + dst->data = (void*)1; // HACK: Needed for IS_EMPTY(). >> + >> + return sizeof(*dst); >> +} > > This is not how a write function for a fifo should look like: A fifo may > need to store the beginning of a packet at the end of the fifo and the > end of a packet at the beginning of a fifo; you can check for this by > checking whether size is < sizeof(AVPacket). I'm already ensuring there's always sizeof(AVPacket) bytes left before calling av_fifo_generic_write(). > (The current implementation seems to actually only allocate multiples of > a certain unit if all av_fifo_grow()/av_fifo_realloc2()/av_fifo_alloc() > calls use only multiples of this unit, but it doesn't seem to be > documented. Should probably be done as this simplifies using fifo arrays.) > > And apart from that: The current implementation of > av_fifo_generic_write() does not forward errors from the write function; > and changing this require be an API break. Accepting a function that can return < 0 but must not be an error sounds like an awful oversight when defining this API... I guess i can just do the av_packet_copy_props() call in extract_packet_props() and not use a custom function at all. > >> + >> static int extract_packet_props(AVCodecInternal *avci, AVPacket *pkt) >> { >> int ret = 0; >> >> - ret = avpriv_packet_list_put(&avci->pkt_props, &avci->pkt_props_tail, pkt, >> - av_packet_copy_props, 0); >> + if (av_fifo_space(avci->pkt_props) < sizeof(*pkt)) { >> + ret = av_fifo_grow(avci->pkt_props, sizeof(*pkt)); >> + if (ret < 0) >> + return ret; >> + } >> + >> + ret = av_fifo_generic_write(avci->pkt_props, pkt, sizeof(*pkt), copy_packet_props); >> if (ret < 0) >> return ret; >> - avci->pkt_props_tail->pkt.size = pkt->size; // HACK: Needed for ff_decode_frame_props(). >> - avci->pkt_props_tail->pkt.data = (void*)1; // HACK: Needed for IS_EMPTY(). >> >> + av_assert0(ret == sizeof(*pkt)); >> if (IS_EMPTY(avci->last_pkt_props)) { >> - ret = avpriv_packet_list_get(&avci->pkt_props, >> - &avci->pkt_props_tail, >> - avci->last_pkt_props); >> - av_assert0(ret != AVERROR(EAGAIN)); >> + ret = av_fifo_generic_read(avci->pkt_props, avci->last_pkt_props, >> + sizeof(*avci->last_pkt_props), NULL); >> } >> return ret; >> } >> @@ -1721,10 +1739,9 @@ int ff_decode_frame_props(AVCodecContext *avctx, AVFrame *frame) >> { AV_PKT_DATA_S12M_TIMECODE, AV_FRAME_DATA_S12M_TIMECODE }, >> }; >> >> - if (IS_EMPTY(pkt)) >> - avpriv_packet_list_get(&avctx->internal->pkt_props, >> - &avctx->internal->pkt_props_tail, >> - pkt); >> + if (IS_EMPTY(pkt) && av_fifo_size(avctx->internal->pkt_props) >= sizeof(*pkt)) >> + av_fifo_generic_read(avctx->internal->pkt_props, >> + pkt, sizeof(*pkt), NULL); >> >> if (pkt) { >> frame->pts = pkt->pts; >> diff --git a/libavcodec/internal.h b/libavcodec/internal.h >> index 17defb9b50..8a51bca2df 100644 >> --- a/libavcodec/internal.h >> +++ b/libavcodec/internal.h >> @@ -28,6 +28,7 @@ >> >> #include "libavutil/buffer.h" >> #include "libavutil/channel_layout.h" >> +#include "libavutil/fifo.h" >> #include "libavutil/mathematics.h" >> #include "libavutil/pixfmt.h" >> #include "avcodec.h" >> @@ -145,8 +146,7 @@ typedef struct AVCodecInternal { >> * for decoding. >> */ >> AVPacket *last_pkt_props; >> - AVPacketList *pkt_props; >> - AVPacketList *pkt_props_tail; >> + AVFifoBuffer *pkt_props; >> >> /** >> * temporary buffer used for encoders to store their bitstream >> diff --git a/libavcodec/utils.c b/libavcodec/utils.c >> index 45295dd3ce..c81cc972dc 100644 >> --- a/libavcodec/utils.c >> +++ b/libavcodec/utils.c >> @@ -593,10 +593,12 @@ int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *code >> avci->es.in_frame = av_frame_alloc(); >> avci->ds.in_pkt = av_packet_alloc(); >> avci->last_pkt_props = av_packet_alloc(); >> + avci->pkt_props = av_fifo_alloc(sizeof(AVPacket)); >> if (!avci->compat_decode_frame || !avci->compat_encode_packet || >> !avci->buffer_frame || !avci->buffer_pkt || >> !avci->es.in_frame || !avci->ds.in_pkt || >> - !avci->to_free || !avci->last_pkt_props) { >> + !avci->to_free || !avci->last_pkt_props || >> + !avci->pkt_props) { >> ret = AVERROR(ENOMEM); >> goto free_and_end; >> } >> @@ -1076,6 +1078,7 @@ FF_ENABLE_DEPRECATION_WARNINGS >> av_packet_free(&avci->compat_encode_packet); >> av_packet_free(&avci->buffer_pkt); >> av_packet_free(&avci->last_pkt_props); >> + av_fifo_freep(&avci->pkt_props); >> >> av_packet_free(&avci->ds.in_pkt); >> av_frame_free(&avci->es.in_frame); >> @@ -1116,8 +1119,7 @@ void avcodec_flush_buffers(AVCodecContext *avctx) >> av_packet_unref(avci->buffer_pkt); >> >> av_packet_unref(avci->last_pkt_props); >> - avpriv_packet_list_free(&avci->pkt_props, >> - &avci->pkt_props_tail); >> + av_fifo_reset(avci->pkt_props); >> > > The packets in the fifo may contain side-data (because > av_packet_copy_props() copies it) and if so, it leaks here. Right. Will drain it, then. > >> av_frame_unref(avci->es.in_frame); >> av_packet_unref(avci->ds.in_pkt); >> @@ -1180,8 +1182,7 @@ av_cold int avcodec_close(AVCodecContext *avctx) >> av_packet_free(&avctx->internal->compat_encode_packet); >> av_packet_free(&avctx->internal->buffer_pkt); >> av_packet_free(&avctx->internal->last_pkt_props); >> - avpriv_packet_list_free(&avctx->internal->pkt_props, >> - &avctx->internal->pkt_props_tail); >> + av_fifo_freep(&avctx->internal->pkt_props); >> >> av_packet_free(&avctx->internal->ds.in_pkt); >> av_frame_free(&avctx->internal->es.in_frame); >> > > _______________________________________________ > 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: > On 12/4/2020 2:08 PM, Andreas Rheinhardt wrote: >> James Almer: >>> Signed-off-by: James Almer <jamrial@gmail.com> >>> --- >>> The idea of making avpriv_packet_list_* public was not liked, and it was >>> suggested to just use AVFifoBuffer instead. >>> >>> After this, the avpriv_packet_list_* functions can be made local to >>> libavformat again. >>> >>> libavcodec/decode.c | 41 +++++++++++++++++++++++++++++------------ >>> libavcodec/internal.h | 4 ++-- >>> libavcodec/utils.c | 11 ++++++----- >>> 3 files changed, 37 insertions(+), 19 deletions(-) >>> >>> diff --git a/libavcodec/decode.c b/libavcodec/decode.c >>> index 5a1849f944..0550637029 100644 >>> --- a/libavcodec/decode.c >>> +++ b/libavcodec/decode.c >>> @@ -145,22 +145,40 @@ fail2: >>> #define IS_EMPTY(pkt) (!(pkt)->data) >>> +static int copy_packet_props(void *_src, void *_dst, int size) >>> +{ >>> + AVPacket *src = _src, *dst = _dst; >>> + int ret; >>> + >>> + av_init_packet(dst); >>> + ret = av_packet_copy_props(dst, src); >>> + if (ret < 0) >>> + return ret;> + >>> + dst->size = src->size; // HACK: Needed for ff_decode_frame_props(). >>> + dst->data = (void*)1; // HACK: Needed for IS_EMPTY(). >>> + >>> + return sizeof(*dst); >>> +} >> >> This is not how a write function for a fifo should look like: A fifo may >> need to store the beginning of a packet at the end of the fifo and the >> end of a packet at the beginning of a fifo; you can check for this by >> checking whether size is < sizeof(AVPacket). > > I'm already ensuring there's always sizeof(AVPacket) bytes left before > calling av_fifo_generic_write(). > And? This just means one can write sizeof(AVPacket) bytes to the fifo, not that there are sizeof(AVPacket) contiguous bytes available at the current write pointer. The free space might be partially at the end and partially at the beginning of the fifo buffer. >> (The current implementation seems to actually only allocate multiples of >> a certain unit if all av_fifo_grow()/av_fifo_realloc2()/av_fifo_alloc() >> calls use only multiples of this unit, but it doesn't seem to be >> documented. Should probably be done as this simplifies using fifo >> arrays.) >> >> And apart from that: The current implementation of >> av_fifo_generic_write() does not forward errors from the write function; >> and changing this require be an API break. > > Accepting a function that can return < 0 but must not be an error sounds > like an awful oversight when defining this API... > > I guess i can just do the av_packet_copy_props() call in > extract_packet_props() and not use a custom function at all. > >> >>> + >>> static int extract_packet_props(AVCodecInternal *avci, AVPacket *pkt) >>> { >>> int ret = 0; >>> - ret = avpriv_packet_list_put(&avci->pkt_props, >>> &avci->pkt_props_tail, pkt, >>> - av_packet_copy_props, 0); >>> + if (av_fifo_space(avci->pkt_props) < sizeof(*pkt)) { >>> + ret = av_fifo_grow(avci->pkt_props, sizeof(*pkt)); >>> + if (ret < 0) >>> + return ret; >>> + } >>> + >>> + ret = av_fifo_generic_write(avci->pkt_props, pkt, sizeof(*pkt), >>> copy_packet_props); >>> if (ret < 0) >>> return ret; >>> - avci->pkt_props_tail->pkt.size = pkt->size; // HACK: Needed for >>> ff_decode_frame_props(). >>> - avci->pkt_props_tail->pkt.data = (void*)1; // HACK: Needed for >>> IS_EMPTY(). >>> + av_assert0(ret == sizeof(*pkt)); >>> if (IS_EMPTY(avci->last_pkt_props)) { >>> - ret = avpriv_packet_list_get(&avci->pkt_props, >>> - &avci->pkt_props_tail, >>> - avci->last_pkt_props); >>> - av_assert0(ret != AVERROR(EAGAIN)); >>> + ret = av_fifo_generic_read(avci->pkt_props, >>> avci->last_pkt_props, >>> + sizeof(*avci->last_pkt_props), >>> NULL); >>> } >>> return ret; >>> } >>> @@ -1721,10 +1739,9 @@ int ff_decode_frame_props(AVCodecContext >>> *avctx, AVFrame *frame) >>> { AV_PKT_DATA_S12M_TIMECODE, >>> AV_FRAME_DATA_S12M_TIMECODE }, >>> }; >>> - if (IS_EMPTY(pkt)) >>> - avpriv_packet_list_get(&avctx->internal->pkt_props, >>> - &avctx->internal->pkt_props_tail, >>> - pkt); >>> + if (IS_EMPTY(pkt) && av_fifo_size(avctx->internal->pkt_props) >= >>> sizeof(*pkt)) >>> + av_fifo_generic_read(avctx->internal->pkt_props, >>> + pkt, sizeof(*pkt), NULL); >>> if (pkt) { >>> frame->pts = pkt->pts; >>> diff --git a/libavcodec/internal.h b/libavcodec/internal.h >>> index 17defb9b50..8a51bca2df 100644 >>> --- a/libavcodec/internal.h >>> +++ b/libavcodec/internal.h >>> @@ -28,6 +28,7 @@ >>> #include "libavutil/buffer.h" >>> #include "libavutil/channel_layout.h" >>> +#include "libavutil/fifo.h" >>> #include "libavutil/mathematics.h" >>> #include "libavutil/pixfmt.h" >>> #include "avcodec.h" >>> @@ -145,8 +146,7 @@ typedef struct AVCodecInternal { >>> * for decoding. >>> */ >>> AVPacket *last_pkt_props; >>> - AVPacketList *pkt_props; >>> - AVPacketList *pkt_props_tail; >>> + AVFifoBuffer *pkt_props; >>> /** >>> * temporary buffer used for encoders to store their bitstream >>> diff --git a/libavcodec/utils.c b/libavcodec/utils.c >>> index 45295dd3ce..c81cc972dc 100644 >>> --- a/libavcodec/utils.c >>> +++ b/libavcodec/utils.c >>> @@ -593,10 +593,12 @@ int attribute_align_arg >>> avcodec_open2(AVCodecContext *avctx, const AVCodec *code >>> avci->es.in_frame = av_frame_alloc(); >>> avci->ds.in_pkt = av_packet_alloc(); >>> avci->last_pkt_props = av_packet_alloc(); >>> + avci->pkt_props = av_fifo_alloc(sizeof(AVPacket)); >>> if (!avci->compat_decode_frame || !avci->compat_encode_packet || >>> !avci->buffer_frame || !avci->buffer_pkt || >>> !avci->es.in_frame || !avci->ds.in_pkt || >>> - !avci->to_free || !avci->last_pkt_props) { >>> + !avci->to_free || !avci->last_pkt_props || >>> + !avci->pkt_props) { >>> ret = AVERROR(ENOMEM); >>> goto free_and_end; >>> } >>> @@ -1076,6 +1078,7 @@ FF_ENABLE_DEPRECATION_WARNINGS >>> av_packet_free(&avci->compat_encode_packet); >>> av_packet_free(&avci->buffer_pkt); >>> av_packet_free(&avci->last_pkt_props); >>> + av_fifo_freep(&avci->pkt_props); >>> av_packet_free(&avci->ds.in_pkt); >>> av_frame_free(&avci->es.in_frame); >>> @@ -1116,8 +1119,7 @@ void avcodec_flush_buffers(AVCodecContext *avctx) >>> av_packet_unref(avci->buffer_pkt); >>> av_packet_unref(avci->last_pkt_props); >>> - avpriv_packet_list_free(&avci->pkt_props, >>> - &avci->pkt_props_tail); >>> + av_fifo_reset(avci->pkt_props); >>> >> >> The packets in the fifo may contain side-data (because >> av_packet_copy_props() copies it) and if so, it leaks here. > > Right. Will drain it, then. > >> >>> av_frame_unref(avci->es.in_frame); >>> av_packet_unref(avci->ds.in_pkt); >>> @@ -1180,8 +1182,7 @@ av_cold int avcodec_close(AVCodecContext *avctx) >>> av_packet_free(&avctx->internal->compat_encode_packet); >>> av_packet_free(&avctx->internal->buffer_pkt); >>> av_packet_free(&avctx->internal->last_pkt_props); >>> - avpriv_packet_list_free(&avctx->internal->pkt_props, >>> - &avctx->internal->pkt_props_tail); >>> + av_fifo_freep(&avctx->internal->pkt_props); >>> av_packet_free(&avctx->internal->ds.in_pkt); >>> av_frame_free(&avctx->internal->es.in_frame); >>> >>
On 12/4/2020 4:08 PM, Andreas Rheinhardt wrote: > James Almer: >> On 12/4/2020 2:08 PM, Andreas Rheinhardt wrote: >>> James Almer: >>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>> --- >>>> The idea of making avpriv_packet_list_* public was not liked, and it was >>>> suggested to just use AVFifoBuffer instead. >>>> >>>> After this, the avpriv_packet_list_* functions can be made local to >>>> libavformat again. >>>> >>>> libavcodec/decode.c | 41 +++++++++++++++++++++++++++++------------ >>>> libavcodec/internal.h | 4 ++-- >>>> libavcodec/utils.c | 11 ++++++----- >>>> 3 files changed, 37 insertions(+), 19 deletions(-) >>>> >>>> diff --git a/libavcodec/decode.c b/libavcodec/decode.c >>>> index 5a1849f944..0550637029 100644 >>>> --- a/libavcodec/decode.c >>>> +++ b/libavcodec/decode.c >>>> @@ -145,22 +145,40 @@ fail2: >>>> #define IS_EMPTY(pkt) (!(pkt)->data) >>>> +static int copy_packet_props(void *_src, void *_dst, int size) >>>> +{ >>>> + AVPacket *src = _src, *dst = _dst; >>>> + int ret; >>>> + >>>> + av_init_packet(dst); >>>> + ret = av_packet_copy_props(dst, src); >>>> + if (ret < 0) >>>> + return ret;> + >>>> + dst->size = src->size; // HACK: Needed for ff_decode_frame_props(). >>>> + dst->data = (void*)1; // HACK: Needed for IS_EMPTY(). >>>> + >>>> + return sizeof(*dst); >>>> +} >>> >>> This is not how a write function for a fifo should look like: A fifo may >>> need to store the beginning of a packet at the end of the fifo and the >>> end of a packet at the beginning of a fifo; you can check for this by >>> checking whether size is < sizeof(AVPacket). >> >> I'm already ensuring there's always sizeof(AVPacket) bytes left before >> calling av_fifo_generic_write(). >> > > And? This just means one can write sizeof(AVPacket) bytes to the fifo, > not that there are sizeof(AVPacket) contiguous bytes available at the > current write pointer. The free space might be partially at the end and > partially at the beginning of the fifo buffer. It wraps around? This is not obvious from reading the doxy. In any case, by always incrementing the FIFO by sizeof(AVPacket) bytes it shouldn't be an issue since it will always be a multiple of it. But as i said, i'll just do it all outside of av_fifo_generic_write() anyway since i can't propagate errors. > >>> (The current implementation seems to actually only allocate multiples of >>> a certain unit if all av_fifo_grow()/av_fifo_realloc2()/av_fifo_alloc() >>> calls use only multiples of this unit, but it doesn't seem to be >>> documented. Should probably be done as this simplifies using fifo >>> arrays.) Return values for av_fifo_generic_read are also undocumented. Currently, it's always 0 no matter what. >>> >>> And apart from that: The current implementation of >>> av_fifo_generic_write() does not forward errors from the write function; >>> and changing this require be an API break. >> >> Accepting a function that can return < 0 but must not be an error sounds >> like an awful oversight when defining this API... >> >> I guess i can just do the av_packet_copy_props() call in >> extract_packet_props() and not use a custom function at all. >> >>> >>>> + >>>> static int extract_packet_props(AVCodecInternal *avci, AVPacket *pkt) >>>> { >>>> int ret = 0; >>>> - ret = avpriv_packet_list_put(&avci->pkt_props, >>>> &avci->pkt_props_tail, pkt, >>>> - av_packet_copy_props, 0); >>>> + if (av_fifo_space(avci->pkt_props) < sizeof(*pkt)) { >>>> + ret = av_fifo_grow(avci->pkt_props, sizeof(*pkt)); >>>> + if (ret < 0) >>>> + return ret; >>>> + } >>>> + >>>> + ret = av_fifo_generic_write(avci->pkt_props, pkt, sizeof(*pkt), >>>> copy_packet_props); >>>> if (ret < 0) >>>> return ret; >>>> - avci->pkt_props_tail->pkt.size = pkt->size; // HACK: Needed for >>>> ff_decode_frame_props(). >>>> - avci->pkt_props_tail->pkt.data = (void*)1; // HACK: Needed for >>>> IS_EMPTY(). >>>> + av_assert0(ret == sizeof(*pkt)); >>>> if (IS_EMPTY(avci->last_pkt_props)) { >>>> - ret = avpriv_packet_list_get(&avci->pkt_props, >>>> - &avci->pkt_props_tail, >>>> - avci->last_pkt_props); >>>> - av_assert0(ret != AVERROR(EAGAIN)); >>>> + ret = av_fifo_generic_read(avci->pkt_props, >>>> avci->last_pkt_props, >>>> + sizeof(*avci->last_pkt_props), >>>> NULL); >>>> } >>>> return ret; >>>> } >>>> @@ -1721,10 +1739,9 @@ int ff_decode_frame_props(AVCodecContext >>>> *avctx, AVFrame *frame) >>>> { AV_PKT_DATA_S12M_TIMECODE, >>>> AV_FRAME_DATA_S12M_TIMECODE }, >>>> }; >>>> - if (IS_EMPTY(pkt)) >>>> - avpriv_packet_list_get(&avctx->internal->pkt_props, >>>> - &avctx->internal->pkt_props_tail, >>>> - pkt); >>>> + if (IS_EMPTY(pkt) && av_fifo_size(avctx->internal->pkt_props) >= >>>> sizeof(*pkt)) >>>> + av_fifo_generic_read(avctx->internal->pkt_props, >>>> + pkt, sizeof(*pkt), NULL); >>>> if (pkt) { >>>> frame->pts = pkt->pts; >>>> diff --git a/libavcodec/internal.h b/libavcodec/internal.h >>>> index 17defb9b50..8a51bca2df 100644 >>>> --- a/libavcodec/internal.h >>>> +++ b/libavcodec/internal.h >>>> @@ -28,6 +28,7 @@ >>>> #include "libavutil/buffer.h" >>>> #include "libavutil/channel_layout.h" >>>> +#include "libavutil/fifo.h" >>>> #include "libavutil/mathematics.h" >>>> #include "libavutil/pixfmt.h" >>>> #include "avcodec.h" >>>> @@ -145,8 +146,7 @@ typedef struct AVCodecInternal { >>>> * for decoding. >>>> */ >>>> AVPacket *last_pkt_props; >>>> - AVPacketList *pkt_props; >>>> - AVPacketList *pkt_props_tail; >>>> + AVFifoBuffer *pkt_props; >>>> /** >>>> * temporary buffer used for encoders to store their bitstream >>>> diff --git a/libavcodec/utils.c b/libavcodec/utils.c >>>> index 45295dd3ce..c81cc972dc 100644 >>>> --- a/libavcodec/utils.c >>>> +++ b/libavcodec/utils.c >>>> @@ -593,10 +593,12 @@ int attribute_align_arg >>>> avcodec_open2(AVCodecContext *avctx, const AVCodec *code >>>> avci->es.in_frame = av_frame_alloc(); >>>> avci->ds.in_pkt = av_packet_alloc(); >>>> avci->last_pkt_props = av_packet_alloc(); >>>> + avci->pkt_props = av_fifo_alloc(sizeof(AVPacket)); >>>> if (!avci->compat_decode_frame || !avci->compat_encode_packet || >>>> !avci->buffer_frame || !avci->buffer_pkt || >>>> !avci->es.in_frame || !avci->ds.in_pkt || >>>> - !avci->to_free || !avci->last_pkt_props) { >>>> + !avci->to_free || !avci->last_pkt_props || >>>> + !avci->pkt_props) { >>>> ret = AVERROR(ENOMEM); >>>> goto free_and_end; >>>> } >>>> @@ -1076,6 +1078,7 @@ FF_ENABLE_DEPRECATION_WARNINGS >>>> av_packet_free(&avci->compat_encode_packet); >>>> av_packet_free(&avci->buffer_pkt); >>>> av_packet_free(&avci->last_pkt_props); >>>> + av_fifo_freep(&avci->pkt_props); >>>> av_packet_free(&avci->ds.in_pkt); >>>> av_frame_free(&avci->es.in_frame); >>>> @@ -1116,8 +1119,7 @@ void avcodec_flush_buffers(AVCodecContext *avctx) >>>> av_packet_unref(avci->buffer_pkt); >>>> av_packet_unref(avci->last_pkt_props); >>>> - avpriv_packet_list_free(&avci->pkt_props, >>>> - &avci->pkt_props_tail); >>>> + av_fifo_reset(avci->pkt_props); >>>> >>> >>> The packets in the fifo may contain side-data (because >>> av_packet_copy_props() copies it) and if so, it leaks here. >> >> Right. Will drain it, then. >> >>> >>>> av_frame_unref(avci->es.in_frame); >>>> av_packet_unref(avci->ds.in_pkt); >>>> @@ -1180,8 +1182,7 @@ av_cold int avcodec_close(AVCodecContext *avctx) >>>> av_packet_free(&avctx->internal->compat_encode_packet); >>>> av_packet_free(&avctx->internal->buffer_pkt); >>>> av_packet_free(&avctx->internal->last_pkt_props); >>>> - avpriv_packet_list_free(&avctx->internal->pkt_props, >>>> - &avctx->internal->pkt_props_tail); >>>> + av_fifo_freep(&avctx->internal->pkt_props); >>>> av_packet_free(&avctx->internal->ds.in_pkt); >>>> av_frame_free(&avctx->internal->es.in_frame); >>>> >>> > _______________________________________________ > 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: > On 12/4/2020 4:08 PM, Andreas Rheinhardt wrote: >> James Almer: >>> On 12/4/2020 2:08 PM, Andreas Rheinhardt wrote: >>>> James Almer: >>>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>>> --- >>>>> The idea of making avpriv_packet_list_* public was not liked, and >>>>> it was >>>>> suggested to just use AVFifoBuffer instead. >>>>> >>>>> After this, the avpriv_packet_list_* functions can be made local to >>>>> libavformat again. >>>>> >>>>> libavcodec/decode.c | 41 >>>>> +++++++++++++++++++++++++++++------------ >>>>> libavcodec/internal.h | 4 ++-- >>>>> libavcodec/utils.c | 11 ++++++----- >>>>> 3 files changed, 37 insertions(+), 19 deletions(-) >>>>> >>>>> diff --git a/libavcodec/decode.c b/libavcodec/decode.c >>>>> index 5a1849f944..0550637029 100644 >>>>> --- a/libavcodec/decode.c >>>>> +++ b/libavcodec/decode.c >>>>> @@ -145,22 +145,40 @@ fail2: >>>>> #define IS_EMPTY(pkt) (!(pkt)->data) >>>>> +static int copy_packet_props(void *_src, void *_dst, int size) >>>>> +{ >>>>> + AVPacket *src = _src, *dst = _dst; >>>>> + int ret; >>>>> + >>>>> + av_init_packet(dst); >>>>> + ret = av_packet_copy_props(dst, src); >>>>> + if (ret < 0) >>>>> + return ret;> + >>>>> + dst->size = src->size; // HACK: Needed for >>>>> ff_decode_frame_props(). >>>>> + dst->data = (void*)1; // HACK: Needed for IS_EMPTY(). >>>>> + >>>>> + return sizeof(*dst); >>>>> +} >>>> >>>> This is not how a write function for a fifo should look like: A fifo >>>> may >>>> need to store the beginning of a packet at the end of the fifo and the >>>> end of a packet at the beginning of a fifo; you can check for this by >>>> checking whether size is < sizeof(AVPacket). >>> >>> I'm already ensuring there's always sizeof(AVPacket) bytes left before >>> calling av_fifo_generic_write(). >>> >> >> And? This just means one can write sizeof(AVPacket) bytes to the fifo, >> not that there are sizeof(AVPacket) contiguous bytes available at the >> current write pointer. The free space might be partially at the end and >> partially at the beginning of the fifo buffer. > > It wraps around? This is not obvious from reading the doxy. > > In any case, by always incrementing the FIFO by sizeof(AVPacket) bytes > it shouldn't be an issue since it will always be a multiple of it. But > as i said, i'll just do it all outside of av_fifo_generic_write() anyway > since i can't propagate errors. > James, this is a fifo: It uses a circular buffer. Of course it wraps around. And this is documented: "a very simple circular buffer FIFO implementation". And as I said: >> >>>> (The current implementation seems to actually only allocate >>>> multiples of >>>> a certain unit if all av_fifo_grow()/av_fifo_realloc2()/av_fifo_alloc() >>>> calls use only multiples of this unit, but it doesn't seem to be >>>> documented. Should probably be done as this simplifies using fifo >>>> arrays.) > So, yes, it really seems to work nicely when using it to store arrays; yet this is undocumented. - Andreas
On 12/4/2020 5:18 PM, Andreas Rheinhardt wrote: > James Almer: >> On 12/4/2020 4:08 PM, Andreas Rheinhardt wrote: >>> James Almer: >>>> On 12/4/2020 2:08 PM, Andreas Rheinhardt wrote: >>>>> James Almer: >>>>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>>>> --- >>>>>> The idea of making avpriv_packet_list_* public was not liked, and >>>>>> it was >>>>>> suggested to just use AVFifoBuffer instead. >>>>>> >>>>>> After this, the avpriv_packet_list_* functions can be made local to >>>>>> libavformat again. >>>>>> >>>>>> libavcodec/decode.c | 41 >>>>>> +++++++++++++++++++++++++++++------------ >>>>>> libavcodec/internal.h | 4 ++-- >>>>>> libavcodec/utils.c | 11 ++++++----- >>>>>> 3 files changed, 37 insertions(+), 19 deletions(-) >>>>>> >>>>>> diff --git a/libavcodec/decode.c b/libavcodec/decode.c >>>>>> index 5a1849f944..0550637029 100644 >>>>>> --- a/libavcodec/decode.c >>>>>> +++ b/libavcodec/decode.c >>>>>> @@ -145,22 +145,40 @@ fail2: >>>>>> #define IS_EMPTY(pkt) (!(pkt)->data) >>>>>> +static int copy_packet_props(void *_src, void *_dst, int size) >>>>>> +{ >>>>>> + AVPacket *src = _src, *dst = _dst; >>>>>> + int ret; >>>>>> + >>>>>> + av_init_packet(dst); >>>>>> + ret = av_packet_copy_props(dst, src); >>>>>> + if (ret < 0) >>>>>> + return ret;> + >>>>>> + dst->size = src->size; // HACK: Needed for >>>>>> ff_decode_frame_props(). >>>>>> + dst->data = (void*)1; // HACK: Needed for IS_EMPTY(). >>>>>> + >>>>>> + return sizeof(*dst); >>>>>> +} >>>>> >>>>> This is not how a write function for a fifo should look like: A fifo >>>>> may >>>>> need to store the beginning of a packet at the end of the fifo and the >>>>> end of a packet at the beginning of a fifo; you can check for this by >>>>> checking whether size is < sizeof(AVPacket). >>>> >>>> I'm already ensuring there's always sizeof(AVPacket) bytes left before >>>> calling av_fifo_generic_write(). >>>> >>> >>> And? This just means one can write sizeof(AVPacket) bytes to the fifo, >>> not that there are sizeof(AVPacket) contiguous bytes available at the >>> current write pointer. The free space might be partially at the end and >>> partially at the beginning of the fifo buffer. >> >> It wraps around? This is not obvious from reading the doxy. >> >> In any case, by always incrementing the FIFO by sizeof(AVPacket) bytes >> it shouldn't be an issue since it will always be a multiple of it. But >> as i said, i'll just do it all outside of av_fifo_generic_write() anyway >> since i can't propagate errors. >> > > James, this is a fifo: It uses a circular buffer. Of course it wraps > around. And this is documented: "a very simple circular buffer FIFO > implementation". Right, was thinking about it and handling it like a linked list FIFO where you always add complete elements at the end than as a byte array with read/write index pointers. > > And as I said: > >>> >>>>> (The current implementation seems to actually only allocate >>>>> multiples of >>>>> a certain unit if all av_fifo_grow()/av_fifo_realloc2()/av_fifo_alloc() >>>>> calls use only multiples of this unit, but it doesn't seem to be >>>>> documented. Should probably be done as this simplifies using fifo >>>>> arrays.) >> > > So, yes, it really seems to work nicely when using it to store arrays; > yet this is undocumented. > > - Andreas > _______________________________________________ > 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". >
diff --git a/libavcodec/decode.c b/libavcodec/decode.c index 5a1849f944..0550637029 100644 --- a/libavcodec/decode.c +++ b/libavcodec/decode.c @@ -145,22 +145,40 @@ fail2: #define IS_EMPTY(pkt) (!(pkt)->data) +static int copy_packet_props(void *_src, void *_dst, int size) +{ + AVPacket *src = _src, *dst = _dst; + int ret; + + av_init_packet(dst); + ret = av_packet_copy_props(dst, src); + if (ret < 0) + return ret; + + dst->size = src->size; // HACK: Needed for ff_decode_frame_props(). + dst->data = (void*)1; // HACK: Needed for IS_EMPTY(). + + return sizeof(*dst); +} + static int extract_packet_props(AVCodecInternal *avci, AVPacket *pkt) { int ret = 0; - ret = avpriv_packet_list_put(&avci->pkt_props, &avci->pkt_props_tail, pkt, - av_packet_copy_props, 0); + if (av_fifo_space(avci->pkt_props) < sizeof(*pkt)) { + ret = av_fifo_grow(avci->pkt_props, sizeof(*pkt)); + if (ret < 0) + return ret; + } + + ret = av_fifo_generic_write(avci->pkt_props, pkt, sizeof(*pkt), copy_packet_props); if (ret < 0) return ret; - avci->pkt_props_tail->pkt.size = pkt->size; // HACK: Needed for ff_decode_frame_props(). - avci->pkt_props_tail->pkt.data = (void*)1; // HACK: Needed for IS_EMPTY(). + av_assert0(ret == sizeof(*pkt)); if (IS_EMPTY(avci->last_pkt_props)) { - ret = avpriv_packet_list_get(&avci->pkt_props, - &avci->pkt_props_tail, - avci->last_pkt_props); - av_assert0(ret != AVERROR(EAGAIN)); + ret = av_fifo_generic_read(avci->pkt_props, avci->last_pkt_props, + sizeof(*avci->last_pkt_props), NULL); } return ret; } @@ -1721,10 +1739,9 @@ int ff_decode_frame_props(AVCodecContext *avctx, AVFrame *frame) { AV_PKT_DATA_S12M_TIMECODE, AV_FRAME_DATA_S12M_TIMECODE }, }; - if (IS_EMPTY(pkt)) - avpriv_packet_list_get(&avctx->internal->pkt_props, - &avctx->internal->pkt_props_tail, - pkt); + if (IS_EMPTY(pkt) && av_fifo_size(avctx->internal->pkt_props) >= sizeof(*pkt)) + av_fifo_generic_read(avctx->internal->pkt_props, + pkt, sizeof(*pkt), NULL); if (pkt) { frame->pts = pkt->pts; diff --git a/libavcodec/internal.h b/libavcodec/internal.h index 17defb9b50..8a51bca2df 100644 --- a/libavcodec/internal.h +++ b/libavcodec/internal.h @@ -28,6 +28,7 @@ #include "libavutil/buffer.h" #include "libavutil/channel_layout.h" +#include "libavutil/fifo.h" #include "libavutil/mathematics.h" #include "libavutil/pixfmt.h" #include "avcodec.h" @@ -145,8 +146,7 @@ typedef struct AVCodecInternal { * for decoding. */ AVPacket *last_pkt_props; - AVPacketList *pkt_props; - AVPacketList *pkt_props_tail; + AVFifoBuffer *pkt_props; /** * temporary buffer used for encoders to store their bitstream diff --git a/libavcodec/utils.c b/libavcodec/utils.c index 45295dd3ce..c81cc972dc 100644 --- a/libavcodec/utils.c +++ b/libavcodec/utils.c @@ -593,10 +593,12 @@ int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *code avci->es.in_frame = av_frame_alloc(); avci->ds.in_pkt = av_packet_alloc(); avci->last_pkt_props = av_packet_alloc(); + avci->pkt_props = av_fifo_alloc(sizeof(AVPacket)); if (!avci->compat_decode_frame || !avci->compat_encode_packet || !avci->buffer_frame || !avci->buffer_pkt || !avci->es.in_frame || !avci->ds.in_pkt || - !avci->to_free || !avci->last_pkt_props) { + !avci->to_free || !avci->last_pkt_props || + !avci->pkt_props) { ret = AVERROR(ENOMEM); goto free_and_end; } @@ -1076,6 +1078,7 @@ FF_ENABLE_DEPRECATION_WARNINGS av_packet_free(&avci->compat_encode_packet); av_packet_free(&avci->buffer_pkt); av_packet_free(&avci->last_pkt_props); + av_fifo_freep(&avci->pkt_props); av_packet_free(&avci->ds.in_pkt); av_frame_free(&avci->es.in_frame); @@ -1116,8 +1119,7 @@ void avcodec_flush_buffers(AVCodecContext *avctx) av_packet_unref(avci->buffer_pkt); av_packet_unref(avci->last_pkt_props); - avpriv_packet_list_free(&avci->pkt_props, - &avci->pkt_props_tail); + av_fifo_reset(avci->pkt_props); av_frame_unref(avci->es.in_frame); av_packet_unref(avci->ds.in_pkt); @@ -1180,8 +1182,7 @@ av_cold int avcodec_close(AVCodecContext *avctx) av_packet_free(&avctx->internal->compat_encode_packet); av_packet_free(&avctx->internal->buffer_pkt); av_packet_free(&avctx->internal->last_pkt_props); - avpriv_packet_list_free(&avctx->internal->pkt_props, - &avctx->internal->pkt_props_tail); + av_fifo_freep(&avctx->internal->pkt_props); av_packet_free(&avctx->internal->ds.in_pkt); av_frame_free(&avctx->internal->es.in_frame);
Signed-off-by: James Almer <jamrial@gmail.com> --- The idea of making avpriv_packet_list_* public was not liked, and it was suggested to just use AVFifoBuffer instead. After this, the avpriv_packet_list_* functions can be made local to libavformat again. libavcodec/decode.c | 41 +++++++++++++++++++++++++++++------------ libavcodec/internal.h | 4 ++-- libavcodec/utils.c | 11 ++++++----- 3 files changed, 37 insertions(+), 19 deletions(-)