[FFmpeg-devel] [PATCH] avcodec/decode: port last_pkt_props to AVFifoBuffer
James Almer
jamrial at gmail.com
Fri Dec 4 21:50:54 EET 2020
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 at 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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>
More information about the ffmpeg-devel
mailing list