On 9/21/2021 7:35 PM, Andreas Rheinhardt wrote:
James Almer:
On 9/21/2021 7:18 PM, Andreas Rheinhardt wrote:
Currently, the AV_PKT_FLAG_KEY is automatically set for audio encoders;
yet this is wrong, as both MLP and TrueHD have non-keyframes. So set it
based upon AV_CODEC_PROP_INTRA_ONLY (from the corresponding
AVCodecDescriptor) instead. This also sets it for some video codecs,
which is intended.

Signed-off-by: Andreas Rheinhardt <andreas.rheinha...@outlook.com>
---
I was surprised that this did not necessitate FATE changes.

Btw: AVCodecContext.codec_descriptor is always set by avcodec_open2(),
yet marked as unused for encoding in its doxy. Shall I make document
the current behaviour?

   libavcodec/encode.c   | 7 +++----
   libavcodec/internal.h | 7 +++++++
   2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/libavcodec/encode.c b/libavcodec/encode.c
index 98dfbfdff3..dd25cf999b 100644
--- a/libavcodec/encode.c
+++ b/libavcodec/encode.c
@@ -235,12 +235,9 @@ static int encode_simple_internal(AVCodecContext
*avctx, AVPacket *avpkt)
               }
           }
           if (avctx->codec->type == AVMEDIA_TYPE_AUDIO) {
-            /* NOTE: if we add any audio encoders which output
non-keyframe packets,
-             *       this needs to be moved to the encoders, but for
now we can do it
-             *       here to simplify things */
-            avpkt->flags |= AV_PKT_FLAG_KEY;
               avpkt->dts = avpkt->pts;
           }
+        avpkt->flags |= avci->intra_only_flag;

You could do the check you added to ff_encode_preinit() here, inside the
audio media type check, instead of adding another single use field to
AVCodecInternal.

I don't consider this a problem, but doing so would make patch 5/5 not possible, so i guess it's fine as is.



That would add a dereference and a branch more for every packet; the
above avoids this.

       }
         if (avci->draining && !got_packet)
@@ -553,6 +550,8 @@ int ff_encode_preinit(AVCodecContext *avctx)
           }
           avctx->sw_pix_fmt = frames_ctx->sw_format;
       }
+    if (avctx->codec_descriptor->props & AV_CODEC_PROP_INTRA_ONLY)

fwiw, the doxy for AVCodecContext says codec_descriptor is unused for
encoding, but i see it's set unconditionally in avcodec_open2(). Should
the doxy be amended?


You overlooked my above comment, I presume.

Ah, I did, yes. My bad.


It also kinda feels like a superfluous field. Anyone can do
avcodec_descriptor_get(avctx->codec_id) if required.


Agreed.

+        avctx->internal->intra_only_flag = AV_PKT_FLAG_KEY;
         return 0;
   }
diff --git a/libavcodec/internal.h b/libavcodec/internal.h
index dc60e4bf08..8df622968c 100644
--- a/libavcodec/internal.h
+++ b/libavcodec/internal.h
@@ -155,6 +155,13 @@ typedef struct AVCodecInternal {
       uint8_t *byte_buffer;
       unsigned int byte_buffer_size;
   +    /**
+     * This is set to AV_PKT_FLAG_KEY for encoders that encode
intra-only
+     * formats (i.e. whose codec descriptor has
AV_CODEC_PROP_INTRA_ONLY set).
+     * This is used to set said flag generically for said encoders.
+     */
+    int intra_only_flag;
+
       void *frame_thread_encoder;
         EncodeSimpleContext es;


_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".


_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Reply via email to