diff mbox series

[FFmpeg-devel,4/5] avcodec/encode: Set AV_PKT_FLAG_KEY based upon AV_CODEC_PROP_INTRA_ONLY

Message ID AM7PR03MB66603816AC7833EA4C36C1D48FA19@AM7PR03MB6660.eurprd03.prod.outlook.com
State Accepted
Commit 3f11eac75741888c7b2b6f93c458766f2613bab5
Headers show
Series [FFmpeg-devel,1/5] avcodec/mlpenc: Set AV_PKT_FLAG_KEY manually
Related show

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

Andreas Rheinhardt Sept. 21, 2021, 10:18 p.m. UTC
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.rheinhardt@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(-)

Comments

James Almer Sept. 21, 2021, 10:30 p.m. UTC | #1
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.rheinhardt@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.

>       }
>   
>       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?

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

> +        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;
>
Andreas Rheinhardt Sept. 21, 2021, 10:35 p.m. UTC | #2
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.rheinhardt@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.
> 

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.

> 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;
>>
>
James Almer Sept. 21, 2021, 10:38 p.m. UTC | #3
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.rheinhardt@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-request@ffmpeg.org with subject "unsubscribe".
>
diff mbox series

Patch

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