[FFmpeg-devel] [PATCH 2/2] avcodec/decode: use a packet list to store packet properties
James Almer
jamrial at gmail.com
Tue Sep 1 22:34:49 EEST 2020
On 9/1/2020 4:16 PM, Andreas Rheinhardt wrote:
> James Almer:
>> Keeping only the latest packet fed to the decoder works only for decoders that
>> return a frame immediately after every consumed packet. Decoders that consume
>> several packets before they return a frame will fill said frame with properties
>> taken from the last consumed packet instead of the earliest.
>>
>
> Then why does this patch not restrict usage of this list to decoders
> with AV_CODEC_CAP_DELAY?
To avoid adding extra checks, since the list works for all cases.
>
>> Signed-off-by: James Almer <jamrial at gmail.com>
>> ---
>> This essentially enables decoders implementing the AVCodec.receive_frame API to
>> not require having to deal with timestamps and such properties internally, and
>> letting ff_decode_frame_props() do all the work instead (See libdav1d, which
>> i'll adapt once this goes in).
>>
>> As an example, this fixes the timestamps for the audio track in
>> bink/binkaudio_rdft.bik from the FATE suite, where the decoder wrongly assumed
>> ff_decode_frame_props() alone would handle them right.
>> The reason the relevant fate test is unaffected is because it just compares the
>> decoded output with a reference pcm file.
>>
>> Haven't looked too much into it, but i assume this change could simplify some
>> AV_CODEC_CAP_DELAY decoders that keep track of timestamps, even those using the
>> old AVCodec.decode() API.
>>
>> libavcodec/decode.c | 34 +++++++++++++++++++++++++---------
>> libavcodec/internal.h | 2 ++
>> libavcodec/utils.c | 3 +++
>> 3 files changed, 30 insertions(+), 9 deletions(-)
>>
>> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
>> index da587ac1f6..de36e28d3c 100644
>> --- a/libavcodec/decode.c
>> +++ b/libavcodec/decode.c
>> @@ -43,6 +43,7 @@
>> #include "decode.h"
>> #include "hwconfig.h"
>> #include "internal.h"
>> +#include "packet_internal.h"
>> #include "thread.h"
>>
>> typedef struct FramePool {
>> @@ -142,15 +143,22 @@ fail2:
>> return 0;
>> }
>>
>> -static int extract_packet_props(AVCodecInternal *avci, const AVPacket *pkt)
>> +#define IS_EMPTY(pkt) (!(pkt)->data && !(pkt)->side_data_elems)
>> +
>> +static int extract_packet_props(AVCodecInternal *avci, AVPacket *pkt)
>> {
>> int ret = 0;
>>
>> - av_packet_unref(avci->last_pkt_props);
>> - if (pkt) {
>> - ret = av_packet_copy_props(avci->last_pkt_props, pkt);
>> - if (!ret)
>> - avci->last_pkt_props->size = pkt->size; // HACK: Needed for ff_decode_frame_props().
>> + ret = avpriv_packet_list_put(&avci->pkt_props, &avci->pkt_props_tail, pkt,
>> + FF_PACKETLIST_FLAG_REF_PACKET);
>
> This will make a reference to the pkt's data and therefore potentially
> (probably?) increase its lifetime. Maybe one should add a new
> FF_PACKETLIST_FLAG that says that only non-data fields should be copied?
> (Ideally, the new flag together with FF_PACKETLIST_FLAG_REF_PACKET would
> amount to av_packet_copy_props, whereas the new flag without it would
> amount to moving the side data to the list packet and resetting the
> non-data fields of src; but we have no function and no need for the
> latter, so this can remain unimplemented).
I don't think it's an issue since the amount of packets that would be
queued by this list is minimal, but yes, i can add that flag, if
anything to reduce the overhead from unnecessary AVBufferRef allocations
when we only want the props.
>
>> + if (ret < 0)
>> + return ret;
>> +
>> + 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));
>> }
>> return ret;
>> }
>> @@ -512,6 +520,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
>>
>> if (ret >= pkt->size || ret < 0) {
>> av_packet_unref(pkt);
>> + av_packet_unref(avci->last_pkt_props);
>> } else {
>> int consumed = ret;
>>
>> @@ -550,9 +559,11 @@ static int decode_receive_frame_internal(AVCodecContext *avctx, AVFrame *frame)
>>
>> av_assert0(!frame->buf[0]);
>>
>> - if (avctx->codec->receive_frame)
>> + if (avctx->codec->receive_frame) {
>> ret = avctx->codec->receive_frame(avctx, frame);
>> - else
>> + if (ret != AVERROR(EAGAIN))
>> + av_packet_unref(avci->last_pkt_props);
>> + } else
>> ret = decode_simple_receive_frame(avctx, frame);
>>
>> if (ret == AVERROR_EOF)
>> @@ -1683,7 +1694,7 @@ static int add_metadata_from_side_data(const AVPacket *avpkt, AVFrame *frame)
>>
>> int ff_decode_frame_props(AVCodecContext *avctx, AVFrame *frame)
>> {
>> - const AVPacket *pkt = avctx->internal->last_pkt_props;
>> + AVPacket *pkt = avctx->internal->last_pkt_props;
>> int i;
>> static const struct {
>> enum AVPacketSideDataType packet;
>> @@ -1701,6 +1712,11 @@ 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 (pkt) {
>> frame->pts = pkt->pts;
>> #if FF_API_PKT_PTS
>> diff --git a/libavcodec/internal.h b/libavcodec/internal.h
>> index 5d0e6e7831..ce4dbbc2b9 100644
>> --- a/libavcodec/internal.h
>> +++ b/libavcodec/internal.h
>> @@ -145,6 +145,8 @@ typedef struct AVCodecInternal {
>> * for decoding.
>> */
>> AVPacket *last_pkt_props;
>> + AVPacketList *pkt_props;
>> + AVPacketList *pkt_props_tail;
>>
>> /**
>> * temporary buffer used for encoders to store their bitstream
>> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
>> index 14cb5cf1aa..4a180a1500 100644
>> --- a/libavcodec/utils.c
>> +++ b/libavcodec/utils.c
>> @@ -50,6 +50,7 @@
>> #include "thread.h"
>> #include "frame_thread_encoder.h"
>> #include "internal.h"
>> +#include "packet_internal.h"
>> #include "put_bits.h"
>> #include "raw.h"
>> #include "bytestream.h"
>> @@ -1149,6 +1150,8 @@ 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_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