[FFmpeg-devel,1/3] lavc/encode: make sure frame timebase matches encoder, when set

AVFrame.time_base has been added recently, but is currently not used for
anything. Prepare for its use in encoders by rejecting frames where
time_base is set, but differs from the AVCodecContext one.
---
 libavcodec/avcodec.h | 4 ++++
 libavcodec/encode.c  | 8 ++++++++
 libavutil/frame.h    | 5 +++--
 3 files changed, 15 insertions(+), 2 deletions(-)

On Wed, 28 Sep 2022, Anton Khirnov wrote:

> AVFrame.time_base has been added recently, but is currently not used for
> anything. Prepare for its use in encoders by rejecting frames where
> time_base is set, but differs from the AVCodecContext one.

How is that not an API break? Users can encode AVFrames with anything in 
the AVFrame->time_base right now, if you change that behaviour, that will 
surely break some code. That is why it was explicitly documented that 
it will be ignored by encoders by default.

It is the second time people make this mistake, and it is exactly this 
confusion why I was hesitant to accept adding the time base to AVFrame.

Regards,
Marton

> ---
> libavcodec/avcodec.h | 4 ++++
> libavcodec/encode.c  | 8 ++++++++
> libavutil/frame.h    | 5 +++--
> 3 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 7365eb5cc0..4030675d4f 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -2645,6 +2645,10 @@ int avcodec_receive_frame(AVCodecContext *avctx, AVFrame *frame);
>  *                  packets are ignored, and sending frames will return
>  *                  AVERROR_EOF.
>  *
> + *                  frame->time_base should be set to the same value as
> + *                  avctx->time_base. This is not required yet, but may be in
> + *                  the future.
> + *
>  *                  For audio:
>  *                  If AV_CODEC_CAP_VARIABLE_FRAME_SIZE is set, then each frame
>  *                  can have any number of samples.
> diff --git a/libavcodec/encode.c b/libavcodec/encode.c
> index b275344bd1..0f78012747 100644
> --- a/libavcodec/encode.c
> +++ b/libavcodec/encode.c
> @@ -386,6 +386,14 @@ static int encode_send_frame_internal(AVCodecContext *avctx, const AVFrame *src)
>     AVFrame *dst = avci->buffer_frame;
>     int ret;
>
> +    /* make sure the frame's timebase (if set) matches the encoder one */
> +    if (src->time_base.num &&
> +        (src->time_base.num != avctx->time_base.num ||
> +         src->time_base.den != avctx->time_base.den)) {
> +        av_log(avctx, AV_LOG_ERROR, "Mismatching frame/encoder time base\n");
> +        return AVERROR(EINVAL);
> +    }
> +
>     if (avctx->codec->type == AVMEDIA_TYPE_AUDIO) {
>         /* extract audio service type metadata */
>         AVFrameSideData *sd = av_frame_get_side_data(src, AV_FRAME_DATA_AUDIO_SERVICE_TYPE);
> diff --git a/libavutil/frame.h b/libavutil/frame.h
> index 6d9563bc5d..c5e2de85b3 100644
> --- a/libavutil/frame.h
> +++ b/libavutil/frame.h
> @@ -441,8 +441,9 @@ typedef struct AVFrame {
>     /**
>      * Time base for the timestamps in this frame.
>      * In the future, this field may be set on frames output by decoders or
> -     * filters, but its value will be by default ignored on input to encoders
> -     * or filters.
> +     * filters; its value will be by default ignored on input to filters.
> +     * For frames sent to encoders, it should be set by the user to the same
> +     * value as AVCodecContext.time_base.
>      */
>     AVRational time_base;
>
> -- 
> 2.35.1
>
> _______________________________________________
> 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".
>

Quoting Marton Balint (2022-09-28 21:54:11)
> 
> 
> On Wed, 28 Sep 2022, Anton Khirnov wrote:
> 
> > AVFrame.time_base has been added recently, but is currently not used for
> > anything. Prepare for its use in encoders by rejecting frames where
> > time_base is set, but differs from the AVCodecContext one.
> 
> How is that not an API break? Users can encode AVFrames with anything in 
> the AVFrame->time_base right now, if you change that behaviour, that will 
> surely break some code. That is why it was explicitly documented that 
> it will be ignored by encoders by default.

Why would there be anything in that field? No code we have currently
sets that field or does anything with it. There is no valid reason for
the users to be setting it on the frames they send to lavc.

As for "it would have worked before', there are many precedents where
some nonsensical parameter combination would "work", but then we'd add a
check and it would start returning errors. Callers should not be setting
random fields to random values and expect things to work.

Would applying this patch after a major bump alleviate your concerns? We
wanted to have one for a few months already.

On Tue, 4 Oct 2022, Anton Khirnov wrote:

> Quoting Marton Balint (2022-09-28 21:54:11)
>>
>>
>> On Wed, 28 Sep 2022, Anton Khirnov wrote:
>>
>>> AVFrame.time_base has been added recently, but is currently not used for
>>> anything. Prepare for its use in encoders by rejecting frames where
>>> time_base is set, but differs from the AVCodecContext one.
>>
>> How is that not an API break? Users can encode AVFrames with anything in
>> the AVFrame->time_base right now, if you change that behaviour, that will
>> surely break some code. That is why it was explicitly documented that
>> it will be ignored by encoders by default.
>
> Why would there be anything in that field? No code we have currently
> sets that field or does anything with it.

It is a public field which was explicitly documented to be ignored by 
filters or encoders. The user could store any data in it, because the 
documentation of the field ensured it will not be a problem.

If you read back the old threads which added AVFrame->time_base 
you will find the reasoning behind the original comments, in fact,
you suggested the actual wording for the documentation of the field, and 
now you want now to change the semantics of the field which contradicts 
the existing documentation... Usually we introduce a new field and 
deprecate the old if we want to do something like this.

One could argue that this break is "small" enough, to not dance around it, 
but I don't really see the benefit of the change in the first place. So 
the real question is why do you want to start using AVFrame->time_base in 
encoders, and what is the feature which is undoable with the current 
AVCodecContext->time_base?

Thanks,
Marton

> There is no valid reason for
> the users to be setting it on the frames they send to lavc.
>
> As for "it would have worked before', there are many precedents where
> some nonsensical parameter combination would "work", but then we'd add a
> check and it would start returning errors. Callers should not be setting
> random fields to random values and expect things to work.
>
> Would applying this patch after a major bump alleviate your concerns? We
> wanted to have one for a few months already.
>
> -- 
> Anton Khirnov
> _______________________________________________
> 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".
>

Quoting Marton Balint (2022-10-05 20:54:46)
> 
> 
> On Tue, 4 Oct 2022, Anton Khirnov wrote:
> 
> > Quoting Marton Balint (2022-09-28 21:54:11)
> >>
> >>
> >> On Wed, 28 Sep 2022, Anton Khirnov wrote:
> >>
> >>> AVFrame.time_base has been added recently, but is currently not used for
> >>> anything. Prepare for its use in encoders by rejecting frames where
> >>> time_base is set, but differs from the AVCodecContext one.
> >>
> >> How is that not an API break? Users can encode AVFrames with anything in
> >> the AVFrame->time_base right now, if you change that behaviour, that will
> >> surely break some code. That is why it was explicitly documented that
> >> it will be ignored by encoders by default.
> >
> > Why would there be anything in that field? No code we have currently
> > sets that field or does anything with it.
> 
> It is a public field which was explicitly documented to be ignored by 
> filters or encoders. The user could store any data in it, because the 
> documentation of the field ensured it will not be a problem.
> 
> If you read back the old threads which added AVFrame->time_base 
> you will find the reasoning behind the original comments, in fact,
> you suggested the actual wording for the documentation of the field, and 
> now you want now to change the semantics of the field which contradicts 
> the existing documentation... Usually we introduce a new field and 
> deprecate the old if we want to do something like this.

Honestly I do not remember why I wrote it that way and it now seems like
a mistake to me. We did not add this field as random-number storage for
our callers, we added it to use it in the libraries.

> One could argue that this break is "small" enough, to not dance around it, 
> but I don't really see the benefit of the change in the first place. So 
> the real question is why do you want to start using AVFrame->time_base in 
> encoders, and what is the feature which is undoable with the current 
> AVCodecContext->time_base?

Where I'm going with this is duration handling in encoders. libavcodec
currently disregards AVFrame.duration completely, while properly it
should set output AVPacket.duration from its corresponding
AVFrame.duration. The problem with that is that we cannot just use
AVFrame.duration, because there is no guarantee that the caller is aware
of it and set in the correct timebase. E.g. ffmpeg.c currently does not
touch AVFrame.duration, so whatever value it acquired along the way
(from the decoder or some filter) will linger there in whatever timebase
it was originally in, which is not necessarily equal to the encoder
timebase.

So my idea was to use the timebase being set as an indicator of the
caller being duration-aware. But on more thought, this seems rather
obscure and an explicit option is probably better.