| 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 | expand |
| 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 |
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; >
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; >> >
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 --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;
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(-)