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

[Marton Balint <cus@passwd.hu>]

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".
>
_______________________________________________
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".