[FFmpeg-devel] [PATCH 2/2] avcodec/decode: use a packet list to store packet properties

[Andreas Rheinhardt]

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?

> 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).

> +    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);
>