diff mbox series

[FFmpeg-devel,2/5] Revert "avcodec/decode: use a packet list to store packet properties"

Message ID 20221204215227.4186-2-jamrial@gmail.com
State New
Headers show
Series [FFmpeg-devel,1/5] avcodec/binkaudio: clear pts when returning more than one frame per input packet | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

James Almer Dec. 4, 2022, 9:52 p.m. UTC 
The idea behind last_pkt_props was to store the properties of the last packet
fed to the decoder. Any sort of queueing required by decoders that consume
several packets before they start outputting frames should be done by the
decoders in question. An example of this is in the libdav1d wrapper.

This is required to maintain its contents during flush, and for the following
commits that will fix last_pkt_props in frame threading scenarios.

This revers commit 022a12b306ab2096e6ac9fc9b149828a849d65b2.

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavcodec/avcodec.c    | 10 ---------
 libavcodec/decode.c     | 47 ++++++-----------------------------------
 libavcodec/internal.h   |  1 -
 tests/ref/fate/flcl1905 |  2 +-
 4 files changed, 8 insertions(+), 52 deletions(-)

Comments

Michael Niedermayer Dec. 5, 2022, 9:26 p.m. UTC | #1 
On Sun, Dec 04, 2022 at 06:52:24PM -0300, James Almer wrote:
> The idea behind last_pkt_props was to store the properties of the last packet
> fed to the decoder. Any sort of queueing required by decoders that consume
> several packets before they start outputting frames should be done by the
> decoders in question. An example of this is in the libdav1d wrapper.
> 
> This is required to maintain its contents during flush, and for the following
> commits that will fix last_pkt_props in frame threading scenarios.
> 
> This revers commit 022a12b306ab2096e6ac9fc9b149828a849d65b2.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavcodec/avcodec.c    | 10 ---------
>  libavcodec/decode.c     | 47 ++++++-----------------------------------
>  libavcodec/internal.h   |  1 -
>  tests/ref/fate/flcl1905 |  2 +-
>  4 files changed, 8 insertions(+), 52 deletions(-)

This causes a wrong looking final timestamps
for example with V-codecs/WMVP/Arlington.wmv
./ffmpeg -i Arlington.wmv  -f framecrc -

 0,       4410,       4410,        1,   115200, 0x12ea3f01
 0,       4411,       4411,        1,   115200, 0xb376241c
 1,    8105977,    8105977,    10240,    40960, 0xbbacee03
+1,    8105977,    8105977,     2048,     8192, 0x985d6153
 0,       4412,       4412,        1,   115200, 0xc214049c
 0,       4413,       4413,        1,   115200, 0xcdcddf9c
 0,       4414,       4414,        1,   115200, 0xcbd5c21d
 0,       4415,       4415,        1,   115200, 0xed32b414
-1,    8116217,    8116217,     2048,     8192, 0x985d6153


[...]
diff mbox series

Patch

diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
index a85d3c2309..efa76d2740 100644
--- a/libavcodec/avcodec.c
+++ b/libavcodec/avcodec.c
@@ -386,9 +386,6 @@  void avcodec_flush_buffers(AVCodecContext *avctx)
             av_frame_unref(avci->recon_frame);
     } else {
         av_packet_unref(avci->last_pkt_props);
-        while (av_fifo_read(avci->pkt_props, avci->last_pkt_props, 1) >= 0)
-            av_packet_unref(avci->last_pkt_props);
-
         av_packet_unref(avci->in_pkt);
 
         avctx->pts_correction_last_pts =
@@ -453,13 +450,6 @@  av_cold int avcodec_close(AVCodecContext *avctx)
         av_freep(&avci->byte_buffer);
         av_frame_free(&avci->buffer_frame);
         av_packet_free(&avci->buffer_pkt);
-        if (avci->pkt_props) {
-            while (av_fifo_can_read(avci->pkt_props)) {
-                av_packet_unref(avci->last_pkt_props);
-                av_fifo_read(avci->pkt_props, avci->last_pkt_props, 1);
-            }
-            av_fifo_freep2(&avci->pkt_props);
-        }
         av_packet_free(&avci->last_pkt_props);
 
         av_packet_free(&avci->in_pkt);
diff --git a/libavcodec/decode.c b/libavcodec/decode.c
index 6be2d3d6ed..c94d9aa33c 100644
--- a/libavcodec/decode.c
+++ b/libavcodec/decode.c
@@ -132,38 +132,16 @@  fail2:
     return 0;
 }
 
-#define IS_EMPTY(pkt) (!(pkt)->data)
-
-static int copy_packet_props(AVPacket *dst, const AVPacket *src)
-{
-    int 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 0;
-}
-
 static int extract_packet_props(AVCodecInternal *avci, const AVPacket *pkt)
 {
-    AVPacket tmp = { 0 };
     int ret = 0;
 
-    if (IS_EMPTY(avci->last_pkt_props)) {
-        if (av_fifo_read(avci->pkt_props, avci->last_pkt_props, 1) < 0)
-            return copy_packet_props(avci->last_pkt_props, pkt);
+    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 = copy_packet_props(&tmp, pkt);
-    if (ret < 0)
-        return ret;
-
-    ret = av_fifo_write(avci->pkt_props, &tmp, 1);
-    if (ret < 0)
-        av_packet_unref(&tmp);
-
     return ret;
 }
 
@@ -483,7 +461,6 @@  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;
 
@@ -578,8 +555,6 @@  static int decode_receive_frame_internal(AVCodecContext *avctx, AVFrame *frame)
 
     if (codec->cb_type == FF_CODEC_CB_TYPE_RECEIVE_FRAME) {
         ret = codec->cb.receive_frame(avctx, frame);
-        if (ret != AVERROR(EAGAIN))
-            av_packet_unref(avci->last_pkt_props);
     } else
         ret = decode_simple_receive_frame(avctx, frame);
 
@@ -593,12 +568,6 @@  static int decode_receive_frame_internal(AVCodecContext *avctx, AVFrame *frame)
         return ok;
     }
 
-    if (!(codec->caps_internal & FF_CODEC_CAP_SETS_FRAME_PROPS) &&
-        IS_EMPTY(avci->last_pkt_props)) {
-        // May fail if the FIFO is empty.
-        av_fifo_read(avci->pkt_props, avci->last_pkt_props, 1);
-    }
-
     if (!ret) {
         frame->best_effort_timestamp = guess_correct_pts(avctx,
                                                          frame->pts,
@@ -1293,7 +1262,7 @@  static int add_metadata_from_side_data(const AVPacket *avpkt, AVFrame *frame)
 
 int ff_decode_frame_props(AVCodecContext *avctx, AVFrame *frame)
 {
-    AVPacket *pkt = avctx->internal->last_pkt_props;
+    const AVPacket *pkt = avctx->internal->last_pkt_props;
     static const struct {
         enum AVPacketSideDataType packet;
         enum AVFrameSideDataType frame;
@@ -1651,9 +1620,7 @@  FF_ENABLE_DEPRECATION_WARNINGS
 
     avci->in_pkt         = av_packet_alloc();
     avci->last_pkt_props = av_packet_alloc();
-    avci->pkt_props      = av_fifo_alloc2(1, sizeof(*avci->last_pkt_props),
-                                          AV_FIFO_FLAG_AUTO_GROW);
-    if (!avci->in_pkt || !avci->last_pkt_props || !avci->pkt_props)
+    if (!avci->in_pkt || !avci->last_pkt_props)
         return AVERROR(ENOMEM);
 
     ret = decode_bsfs_init(avctx);
diff --git a/libavcodec/internal.h b/libavcodec/internal.h
index 76a6ea6bc6..a283c52e01 100644
--- a/libavcodec/internal.h
+++ b/libavcodec/internal.h
@@ -88,7 +88,6 @@  typedef struct AVCodecInternal {
      * for decoding.
      */
     AVPacket *last_pkt_props;
-    struct AVFifo *pkt_props;
 
     /**
      * temporary buffer used for encoders to store their bitstream
diff --git a/tests/ref/fate/flcl1905 b/tests/ref/fate/flcl1905
index 59cee95459..4500117993 100644
--- a/tests/ref/fate/flcl1905
+++ b/tests/ref/fate/flcl1905
@@ -189,4 +189,4 @@  frame|media_type=audio|stream_index=0|key_frame=1|pts=N/A|pts_time=N/A|pkt_dts=N
 frame|media_type=audio|stream_index=0|key_frame=1|pts=N/A|pts_time=N/A|pkt_dts=N/A|pkt_dts_time=N/A|best_effort_timestamp=N/A|best_effort_timestamp_time=N/A|pkt_duration=22528|pkt_duration_time=0.510839|duration=22528|duration_time=0.510839|pkt_pos=61436|pkt_size=744|sample_fmt=fltp|nb_samples=2048|channels=2|channel_layout=unknown
 frame|media_type=audio|stream_index=0|key_frame=1|pts=N/A|pts_time=N/A|pkt_dts=N/A|pkt_dts_time=N/A|best_effort_timestamp=N/A|best_effort_timestamp_time=N/A|pkt_duration=22528|pkt_duration_time=0.510839|duration=22528|duration_time=0.510839|pkt_pos=61436|pkt_size=372|sample_fmt=fltp|nb_samples=2048|channels=2|channel_layout=unknown
 packet|codec_type=audio|stream_index=0|pts=360448|pts_time=8.173424|dts=360448|dts_time=8.173424|duration=44|duration_time=0.000998|size=8|pos=65528|flags=K_
-frame|media_type=audio|stream_index=0|key_frame=1|pts=N/A|pts_time=N/A|pkt_dts=N/A|pkt_dts_time=N/A|best_effort_timestamp=N/A|best_effort_timestamp_time=N/A|pkt_duration=N/A|pkt_duration_time=N/A|duration=N/A|duration_time=N/A|pkt_pos=N/A|pkt_size=0|sample_fmt=fltp|nb_samples=2048|channels=2|channel_layout=unknown
+frame|media_type=audio|stream_index=0|key_frame=1|pts=360448|pts_time=8.173424|pkt_dts=N/A|pkt_dts_time=N/A|best_effort_timestamp=360448|best_effort_timestamp_time=8.173424|pkt_duration=44|pkt_duration_time=0.000998|duration=44|duration_time=0.000998|pkt_pos=65528|pkt_size=8|sample_fmt=fltp|nb_samples=2048|channels=2|channel_layout=unknown