Message ID | 20200818195332.9442-3-jamrial@gmail.com |
---|---|
State | Accepted |
Headers | show |
Series | avcodec/decode: use a packet list for last_pkt_props | expand |
Context | Check | Description |
---|---|---|
andriy/default | pending | |
andriy/make | success | Make finished |
andriy/make_fate | success | Make fate finished |
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. > > Signed-off-by: James Almer <jamrial@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); > + 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); > + The check below indicates that pkt might be NULL, yet IS_EMPTY(pkt) already dereferences it. > 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); >
On 8/18/2020 5:15 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. >> >> Signed-off-by: James Almer <jamrial@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); >> + 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); >> + > > The check below indicates that pkt might be NULL, yet IS_EMPTY(pkt) > already dereferences it. That check is bogus even in the current state, anyway. pkt is always true because avctx->internal->last_pkt_props is allocated and freed with the AVCodecContext. > >> 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@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: > 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@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); >
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@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@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 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); + 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);
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. Signed-off-by: James Almer <jamrial@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(-)