| Message ID | GV1P250MB07371C1E0C33E95C5433E4EA8FE62@GV1P250MB0737.EURP250.PROD.OUTLOOK.COM |
|---|---|
| State | Accepted |
| Commit | eee88ba0dc67b5beaa31199d5bf1e7fc324e0652 |
| Headers | show |
| Series | [FFmpeg-devel,1/5] avcodec/mlpdec: Set AV_FRAME_FLAG_KEY explicitly | expand |
tor 2024-05-09 klockan 04:04 +0200 skrev Andreas Rheinhardt: > This commit is the analog of 3f11eac75741888c7b2b6f93c458766f2613bab5 > for decoding: It sets the AV_FRAME_FLAG_KEY and (for video decoders) > also pict_type to AV_PICTURE_TYPE_I. It furthermore stops setting > audio frames as always being key frames -- it is wrong for e.g. > TrueHD/MLP. The latter also affects TAK and DFPWM. > > The change already improves output for several decoders where > it has been forgotten to set e.g. pict_type like speedhq, wnv1 > or tiff. The latter is the reason for the change to the exif-image- > tiff > FATE test reference file. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libavcodec/decode.c | 29 +++++++++++++++++++++++++++-- > libavcodec/pthread_frame.c | 17 ++++++++++++++--- > tests/ref/fate/exif-image-tiff | 2 +- > 3 files changed, 42 insertions(+), 6 deletions(-) > > diff --git a/libavcodec/decode.c b/libavcodec/decode.c > index d031b1ca17..0ca5344ef5 100644 > --- a/libavcodec/decode.c > +++ b/libavcodec/decode.c > @@ -57,6 +57,20 @@ > typedef struct DecodeContext { > AVCodecInternal avci; > > + /** > + * This is set to AV_FRAME_FLAG_KEY for decoders of intra-only > formats > + * (those whose codec descriptor has AV_CODEC_PROP_INTRA_ONLY > set) > + * to set the flag generically. > + */ > + int intra_only_flag; > + > + /** > + * This is set to AV_PICTURE_TYPE_I for intra only video > decoders > + * and to AV_PICTURE_TYPE_NONE for other decoders. It is used to > set > + * the AVFrame's pict_type before the decoder receives it. > + */ > + enum AVPictureType initial_pict_type; Carrying this around as state seems unnecessary when a small static function could do the same? > @@ -108,6 +106,10 @@ typedef struct PerThreadContext { > int hwaccel_threadsafe; > > atomic_int debug_threads; ///< Set if the FF_DEBUG_THREADS > option is set. > + > + /// The following two fields have the same semantics as the > DecodeContext field > + int intra_only_flag; > + enum AVPictureType initial_pict_type; Same here /Tomas
Tomas Härdin: > tor 2024-05-09 klockan 04:04 +0200 skrev Andreas Rheinhardt: >> This commit is the analog of 3f11eac75741888c7b2b6f93c458766f2613bab5 >> for decoding: It sets the AV_FRAME_FLAG_KEY and (for video decoders) >> also pict_type to AV_PICTURE_TYPE_I. It furthermore stops setting >> audio frames as always being key frames -- it is wrong for e.g. >> TrueHD/MLP. The latter also affects TAK and DFPWM. >> >> The change already improves output for several decoders where >> it has been forgotten to set e.g. pict_type like speedhq, wnv1 >> or tiff. The latter is the reason for the change to the exif-image- >> tiff >> FATE test reference file. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >> --- >> libavcodec/decode.c | 29 +++++++++++++++++++++++++++-- >> libavcodec/pthread_frame.c | 17 ++++++++++++++--- >> tests/ref/fate/exif-image-tiff | 2 +- >> 3 files changed, 42 insertions(+), 6 deletions(-) >> >> diff --git a/libavcodec/decode.c b/libavcodec/decode.c >> index d031b1ca17..0ca5344ef5 100644 >> --- a/libavcodec/decode.c >> +++ b/libavcodec/decode.c >> @@ -57,6 +57,20 @@ >> typedef struct DecodeContext { >> AVCodecInternal avci; >> >> + /** >> + * This is set to AV_FRAME_FLAG_KEY for decoders of intra-only >> formats >> + * (those whose codec descriptor has AV_CODEC_PROP_INTRA_ONLY >> set) >> + * to set the flag generically. >> + */ >> + int intra_only_flag; >> + >> + /** >> + * This is set to AV_PICTURE_TYPE_I for intra only video >> decoders >> + * and to AV_PICTURE_TYPE_NONE for other decoders. It is used to >> set >> + * the AVFrame's pict_type before the decoder receives it. >> + */ >> + enum AVPictureType initial_pict_type; > > Carrying this around as state seems unnecessary when a small static > function could do the same? > The aim of this is to avoid branches for every frame. - Andreas
mån 2024-05-13 klockan 10:54 +0200 skrev Andreas Rheinhardt: > Tomas Härdin: > > tor 2024-05-09 klockan 04:04 +0200 skrev Andreas Rheinhardt: > > > This commit is the analog of > > > 3f11eac75741888c7b2b6f93c458766f2613bab5 > > > for decoding: It sets the AV_FRAME_FLAG_KEY and (for video > > > decoders) > > > also pict_type to AV_PICTURE_TYPE_I. It furthermore stops setting > > > audio frames as always being key frames -- it is wrong for e.g. > > > TrueHD/MLP. The latter also affects TAK and DFPWM. > > > > > > The change already improves output for several decoders where > > > it has been forgotten to set e.g. pict_type like speedhq, wnv1 > > > or tiff. The latter is the reason for the change to the exif- > > > image- > > > tiff > > > FATE test reference file. > > > > > > Signed-off-by: Andreas Rheinhardt > > > <andreas.rheinhardt@outlook.com> > > > --- > > > libavcodec/decode.c | 29 +++++++++++++++++++++++++++- > > > - > > > libavcodec/pthread_frame.c | 17 ++++++++++++++--- > > > tests/ref/fate/exif-image-tiff | 2 +- > > > 3 files changed, 42 insertions(+), 6 deletions(-) > > > > > > diff --git a/libavcodec/decode.c b/libavcodec/decode.c > > > index d031b1ca17..0ca5344ef5 100644 > > > --- a/libavcodec/decode.c > > > +++ b/libavcodec/decode.c > > > @@ -57,6 +57,20 @@ > > > typedef struct DecodeContext { > > > AVCodecInternal avci; > > > > > > + /** > > > + * This is set to AV_FRAME_FLAG_KEY for decoders of intra- > > > only > > > formats > > > + * (those whose codec descriptor has > > > AV_CODEC_PROP_INTRA_ONLY > > > set) > > > + * to set the flag generically. > > > + */ > > > + int intra_only_flag; > > > + > > > + /** > > > + * This is set to AV_PICTURE_TYPE_I for intra only video > > > decoders > > > + * and to AV_PICTURE_TYPE_NONE for other decoders. It is > > > used to > > > set > > > + * the AVFrame's pict_type before the decoder receives it. > > > + */ > > > + enum AVPictureType initial_pict_type; > > > > Carrying this around as state seems unnecessary when a small static > > function could do the same? > > > > The aim of this is to avoid branches for every frame. Checking a single value that will likely reside in cache once per frame is hardly expensive, especially when the branch predictor will predict it correctly. When you carry state around you're carrying around multiple sources of truth, which is something that tends to cause bugs.. /Tomas
Tomas Härdin: > mån 2024-05-13 klockan 10:54 +0200 skrev Andreas Rheinhardt: >> Tomas Härdin: >>> tor 2024-05-09 klockan 04:04 +0200 skrev Andreas Rheinhardt: >>>> This commit is the analog of >>>> 3f11eac75741888c7b2b6f93c458766f2613bab5 >>>> for decoding: It sets the AV_FRAME_FLAG_KEY and (for video >>>> decoders) >>>> also pict_type to AV_PICTURE_TYPE_I. It furthermore stops setting >>>> audio frames as always being key frames -- it is wrong for e.g. >>>> TrueHD/MLP. The latter also affects TAK and DFPWM. >>>> >>>> The change already improves output for several decoders where >>>> it has been forgotten to set e.g. pict_type like speedhq, wnv1 >>>> or tiff. The latter is the reason for the change to the exif- >>>> image- >>>> tiff >>>> FATE test reference file. >>>> >>>> Signed-off-by: Andreas Rheinhardt >>>> <andreas.rheinhardt@outlook.com> >>>> --- >>>> libavcodec/decode.c | 29 +++++++++++++++++++++++++++- >>>> - >>>> libavcodec/pthread_frame.c | 17 ++++++++++++++--- >>>> tests/ref/fate/exif-image-tiff | 2 +- >>>> 3 files changed, 42 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/libavcodec/decode.c b/libavcodec/decode.c >>>> index d031b1ca17..0ca5344ef5 100644 >>>> --- a/libavcodec/decode.c >>>> +++ b/libavcodec/decode.c >>>> @@ -57,6 +57,20 @@ >>>> typedef struct DecodeContext { >>>> AVCodecInternal avci; >>>> >>>> + /** >>>> + * This is set to AV_FRAME_FLAG_KEY for decoders of intra- >>>> only >>>> formats >>>> + * (those whose codec descriptor has >>>> AV_CODEC_PROP_INTRA_ONLY >>>> set) >>>> + * to set the flag generically. >>>> + */ >>>> + int intra_only_flag; >>>> + >>>> + /** >>>> + * This is set to AV_PICTURE_TYPE_I for intra only video >>>> decoders >>>> + * and to AV_PICTURE_TYPE_NONE for other decoders. It is >>>> used to >>>> set >>>> + * the AVFrame's pict_type before the decoder receives it. >>>> + */ >>>> + enum AVPictureType initial_pict_type; >>> >>> Carrying this around as state seems unnecessary when a small static >>> function could do the same? >>> >> >> The aim of this is to avoid branches for every frame. > > Checking a single value that will likely reside in cache once per frame > is hardly expensive, especially when the branch predictor will predict > it correctly. > > When you carry state around you're carrying around multiple sources of > truth, which is something that tends to cause bugs.. > Given that this is immutable after init, it is not really "state". - Andreas
mån 2024-05-13 klockan 17:55 +0200 skrev Andreas Rheinhardt: > Tomas Härdin: > > mån 2024-05-13 klockan 10:54 +0200 skrev Andreas Rheinhardt: > > > Tomas Härdin: > > > > tor 2024-05-09 klockan 04:04 +0200 skrev Andreas Rheinhardt: > > > > > This commit is the analog of > > > > > 3f11eac75741888c7b2b6f93c458766f2613bab5 > > > > > for decoding: It sets the AV_FRAME_FLAG_KEY and (for video > > > > > decoders) > > > > > also pict_type to AV_PICTURE_TYPE_I. It furthermore stops > > > > > setting > > > > > audio frames as always being key frames -- it is wrong for > > > > > e.g. > > > > > TrueHD/MLP. The latter also affects TAK and DFPWM. > > > > > > > > > > The change already improves output for several decoders where > > > > > it has been forgotten to set e.g. pict_type like speedhq, > > > > > wnv1 > > > > > or tiff. The latter is the reason for the change to the exif- > > > > > image- > > > > > tiff > > > > > FATE test reference file. > > > > > > > > > > Signed-off-by: Andreas Rheinhardt > > > > > <andreas.rheinhardt@outlook.com> > > > > > --- > > > > > libavcodec/decode.c | 29 > > > > > +++++++++++++++++++++++++++- > > > > > - > > > > > libavcodec/pthread_frame.c | 17 ++++++++++++++--- > > > > > tests/ref/fate/exif-image-tiff | 2 +- > > > > > 3 files changed, 42 insertions(+), 6 deletions(-) > > > > > > > > > > diff --git a/libavcodec/decode.c b/libavcodec/decode.c > > > > > index d031b1ca17..0ca5344ef5 100644 > > > > > --- a/libavcodec/decode.c > > > > > +++ b/libavcodec/decode.c > > > > > @@ -57,6 +57,20 @@ > > > > > typedef struct DecodeContext { > > > > > AVCodecInternal avci; > > > > > > > > > > + /** > > > > > + * This is set to AV_FRAME_FLAG_KEY for decoders of > > > > > intra- > > > > > only > > > > > formats > > > > > + * (those whose codec descriptor has > > > > > AV_CODEC_PROP_INTRA_ONLY > > > > > set) > > > > > + * to set the flag generically. > > > > > + */ > > > > > + int intra_only_flag; > > > > > + > > > > > + /** > > > > > + * This is set to AV_PICTURE_TYPE_I for intra only video > > > > > decoders > > > > > + * and to AV_PICTURE_TYPE_NONE for other decoders. It is > > > > > used to > > > > > set > > > > > + * the AVFrame's pict_type before the decoder receives > > > > > it. > > > > > + */ > > > > > + enum AVPictureType initial_pict_type; > > > > > > > > Carrying this around as state seems unnecessary when a small > > > > static > > > > function could do the same? > > > > > > > > > > The aim of this is to avoid branches for every frame. > > > > Checking a single value that will likely reside in cache once per > > frame > > is hardly expensive, especially when the branch predictor will > > predict > > it correctly. > > > > When you carry state around you're carrying around multiple sources > > of > > truth, which is something that tends to cause bugs.. > > > > Given that this is immutable after init, it is not really "state". State tends to live in memory.. Perhaps this borders on bikeshedding, but I have said my peace and look forward to cashing in accrued I-told-you-so's in the future should my fears come to pass. /Tomas
diff --git a/libavcodec/decode.c b/libavcodec/decode.c index d031b1ca17..0ca5344ef5 100644 --- a/libavcodec/decode.c +++ b/libavcodec/decode.c @@ -57,6 +57,20 @@ typedef struct DecodeContext { AVCodecInternal avci; + /** + * This is set to AV_FRAME_FLAG_KEY for decoders of intra-only formats + * (those whose codec descriptor has AV_CODEC_PROP_INTRA_ONLY set) + * to set the flag generically. + */ + int intra_only_flag; + + /** + * This is set to AV_PICTURE_TYPE_I for intra only video decoders + * and to AV_PICTURE_TYPE_NONE for other decoders. It is used to set + * the AVFrame's pict_type before the decoder receives it. + */ + enum AVPictureType initial_pict_type; + /* to prevent infinite loop on errors when draining */ int nb_draining_errors; @@ -382,6 +396,7 @@ static int discard_samples(AVCodecContext *avctx, AVFrame *frame, int64_t *disca static inline int decode_simple_internal(AVCodecContext *avctx, AVFrame *frame, int64_t *discarded_samples) { AVCodecInternal *avci = avctx->internal; + DecodeContext *dc = decode_ctx(avci); AVPacket *const pkt = avci->in_pkt; const FFCodec *const codec = ffcodec(avctx->codec); int got_frame, consumed; @@ -409,6 +424,8 @@ static inline int decode_simple_internal(AVCodecContext *avctx, AVFrame *frame, if (HAVE_THREADS && avctx->active_thread_type & FF_THREAD_FRAME) { consumed = ff_thread_decode_frame(avctx, frame, &got_frame, pkt); } else { + frame->pict_type = dc->initial_pict_type; + frame->flags |= dc->intra_only_flag; consumed = codec->cb.decode(avctx, frame, &got_frame, pkt); if (!(codec->caps_internal & FF_CODEC_CAP_SETS_PKT_DTS)) @@ -597,6 +614,8 @@ static int decode_receive_frame_internal(AVCodecContext *avctx, AVFrame *frame) av_assert0(!frame->buf[0]); if (codec->cb_type == FF_CODEC_CB_TYPE_RECEIVE_FRAME) { + frame->pict_type = dc->initial_pict_type; + frame->flags |= dc->intra_only_flag; ret = codec->cb.receive_frame(avctx, frame); emms_c(); if (!ret) { @@ -626,8 +645,7 @@ static int decode_receive_frame_internal(AVCodecContext *avctx, AVFrame *frame) frame->width = avctx->width; if (!frame->height) frame->height = avctx->height; - } else - frame->flags |= AV_FRAME_FLAG_KEY; + } ret = fill_frame_props(avctx, frame); if (ret < 0) { @@ -1793,6 +1811,13 @@ int ff_decode_preinit(AVCodecContext *avctx) DecodeContext *dc = decode_ctx(avci); int ret = 0; + dc->initial_pict_type = AV_PICTURE_TYPE_NONE; + if (avctx->codec_descriptor->props & AV_CODEC_PROP_INTRA_ONLY) { + dc->intra_only_flag = AV_FRAME_FLAG_KEY; + if (avctx->codec_type == AVMEDIA_TYPE_VIDEO) + dc->initial_pict_type = AV_PICTURE_TYPE_I; + } + /* if the decoder init function was already called previously, * free the already allocated subtitle_header before overwriting it */ av_freep(&avctx->subtitle_header); diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c index 67f09c1f48..982e4a64c5 100644 --- a/libavcodec/pthread_frame.c +++ b/libavcodec/pthread_frame.c @@ -22,13 +22,11 @@ * @see doc/multithreading.txt */ -#include "config.h" - #include <stdatomic.h> -#include <stdint.h> #include "avcodec.h" #include "avcodec_internal.h" +#include "codec_desc.h" #include "codec_internal.h" #include "decode.h" #include "hwaccel_internal.h" @@ -108,6 +106,10 @@ typedef struct PerThreadContext { int hwaccel_threadsafe; atomic_int debug_threads; ///< Set if the FF_DEBUG_THREADS option is set. + + /// The following two fields have the same semantics as the DecodeContext field + int intra_only_flag; + enum AVPictureType initial_pict_type; } PerThreadContext; /** @@ -220,6 +222,8 @@ static attribute_align_arg void *frame_worker_thread(void *arg) av_frame_unref(p->frame); p->got_frame = 0; + p->frame->pict_type = p->initial_pict_type; + p->frame->flags |= p->intra_only_flag; p->result = codec->cb.decode(avctx, p->frame, &p->got_frame, p->avpkt); if ((p->result < 0 || !p->got_frame) && p->frame->buf[0]) @@ -763,6 +767,13 @@ static av_cold int init_thread(PerThreadContext *p, int *threads_to_free, AVCodecContext *copy; int err; + p->initial_pict_type = AV_PICTURE_TYPE_NONE; + if (avctx->codec_descriptor->props & AV_CODEC_PROP_INTRA_ONLY) { + p->intra_only_flag = AV_FRAME_FLAG_KEY; + if (avctx->codec_type == AVMEDIA_TYPE_VIDEO) + p->initial_pict_type = AV_PICTURE_TYPE_I; + } + atomic_init(&p->state, STATE_INPUT_READY); copy = av_memdup(avctx, sizeof(*avctx)); diff --git a/tests/ref/fate/exif-image-tiff b/tests/ref/fate/exif-image-tiff index 887c039df9..f5ff4dc16c 100644 --- a/tests/ref/fate/exif-image-tiff +++ b/tests/ref/fate/exif-image-tiff @@ -20,7 +20,7 @@ crop_left=0 crop_right=0 pix_fmt=rgb24 sample_aspect_ratio=1:1 -pict_type=? +pict_type=I interlaced_frame=0 top_field_first=0 repeat_pict=0
This commit is the analog of 3f11eac75741888c7b2b6f93c458766f2613bab5 for decoding: It sets the AV_FRAME_FLAG_KEY and (for video decoders) also pict_type to AV_PICTURE_TYPE_I. It furthermore stops setting audio frames as always being key frames -- it is wrong for e.g. TrueHD/MLP. The latter also affects TAK and DFPWM. The change already improves output for several decoders where it has been forgotten to set e.g. pict_type like speedhq, wnv1 or tiff. The latter is the reason for the change to the exif-image-tiff FATE test reference file. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- libavcodec/decode.c | 29 +++++++++++++++++++++++++++-- libavcodec/pthread_frame.c | 17 ++++++++++++++--- tests/ref/fate/exif-image-tiff | 2 +- 3 files changed, 42 insertions(+), 6 deletions(-)