[FFmpeg-devel,2/2] avcodec/librav1e: Pass through timestamps as opaque user data

avcodec has no facilities to generate timestamps properly from
output frame numbers (and it would be wrong for VFR anyway),
so pass through the timestamps using rav1e's opaque user data
feature, which was added in v0.4.0.

This bumps the minimum librav1e version to 0.4.0.

Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
---
 configure             |  2 +-
 libavcodec/librav1e.c | 12 +++++++++++-
 2 files changed, 12 insertions(+), 2 deletions(-)

On 1/14/2021 10:55 AM, Derek Buitenhuis wrote:
> avcodec has no facilities to generate timestamps properly from
> output frame numbers (and it would be wrong for VFR anyway),
> so pass through the timestamps using rav1e's opaque user data
> feature, which was added in v0.4.0.
> 
> This bumps the minimum librav1e version to 0.4.0.
> 
> Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
> ---
>   configure             |  2 +-
>   libavcodec/librav1e.c | 12 +++++++++++-
>   2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/configure b/configure
> index 900505756b..54fbbd6b5f 100755
> --- a/configure
> +++ b/configure
> @@ -6408,7 +6408,7 @@ enabled libopus           && {
>   }
>   enabled libpulse          && require_pkg_config libpulse libpulse pulse/pulseaudio.h pa_context_new
>   enabled librabbitmq       && require_pkg_config librabbitmq "librabbitmq >= 0.7.1" amqp.h amqp_new_connection
> -enabled librav1e          && require_pkg_config librav1e "rav1e >= 0.1.0" rav1e.h rav1e_context_new
> +enabled librav1e          && require_pkg_config librav1e "rav1e >= 0.4.0" rav1e.h rav1e_context_new
>   enabled librsvg           && require_pkg_config librsvg librsvg-2.0 librsvg-2.0/librsvg/rsvg.h rsvg_handle_render_cairo
>   enabled librtmp           && require_pkg_config librtmp librtmp librtmp/rtmp.h RTMP_Socket
>   enabled librubberband     && require_pkg_config librubberband "rubberband >= 1.8.1" rubberband/rubberband-c.h rubberband_new -lstdc++ && append librubberband_extralibs "-lstdc++"
> diff --git a/libavcodec/librav1e.c b/libavcodec/librav1e.c
> index 46071bcdac..c1c0de45a6 100644
> --- a/libavcodec/librav1e.c
> +++ b/libavcodec/librav1e.c
> @@ -445,10 +445,18 @@ static int librav1e_receive_packet(AVCodecContext *avctx, AVPacket *pkt)
>           if (frame->buf[0]) {
>               const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(frame->format);
>   
> +            int64_t *pts = av_malloc(sizeof(int64_t));
> +            if (!pts) {
> +                av_log(avctx, AV_LOG_ERROR, "Could not allocate PTS buffer.\n");
> +                return AVERROR(ENOMEM);
> +            }
> +            *pts = frame->pts;
> +
>               rframe = rav1e_frame_new(ctx->ctx);
>               if (!rframe) {
>                   av_log(avctx, AV_LOG_ERROR, "Could not allocate new rav1e frame.\n");
>                   av_frame_unref(frame);
> +                av_freep(&pts);
>                   return AVERROR(ENOMEM);
>               }
>   
> @@ -461,6 +469,7 @@ static int librav1e_receive_packet(AVCodecContext *avctx, AVPacket *pkt)
>               }
>               av_frame_unref(frame);
>           }
> +        rav1e_frame_set_opaque(rframe, pts, av_free);
>       }
>   
>       ret = rav1e_send_frame(ctx->ctx, rframe);
> @@ -535,7 +544,8 @@ retry:
>       if (rpkt->frame_type == RA_FRAME_TYPE_KEY)
>           pkt->flags |= AV_PKT_FLAG_KEY;
>   
> -    pkt->pts = pkt->dts = rpkt->input_frameno * avctx->ticks_per_frame;
> +    pkt->pts = pkt->dts = *((int64_t *) rpkt->opaque);
> +    av_free(rpkt->opaque);

Is the ownership of the opaque user data moved from the RaFrame to the 
RaPacket to ensure this is safe? If so, why does 
rav1e_frame_set_opaque() take a free callback argument? When does it 
trigger?

>       rav1e_packet_unref(rpkt);
>   
>       if (avctx->flags & AV_CODEC_FLAG_GLOBAL_HEADER) {
>

On 14/01/2021 14:09, James Almer wrote:
> Is the ownership of the opaque user data moved from the RaFrame to the 
> RaPacket to ensure this is safe? If so, why does 
> rav1e_frame_set_opaque() take a free callback argument? When does it 
> trigger?

It's a bit of a weird API. The free callback that rav1e_frame_set_opaque takes
is only executed in case of a failue inside of rav1e - once the packet is output,
it's up to the API caller to free it.

- Derek

On 1/14/2021 10:55 AM, Derek Buitenhuis wrote:
> avcodec has no facilities to generate timestamps properly from
> output frame numbers (and it would be wrong for VFR anyway),
> so pass through the timestamps using rav1e's opaque user data
> feature, which was added in v0.4.0.
> 
> This bumps the minimum librav1e version to 0.4.0.
> 
> Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
> ---
>   configure             |  2 +-
>   libavcodec/librav1e.c | 12 +++++++++++-
>   2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/configure b/configure
> index 900505756b..54fbbd6b5f 100755
> --- a/configure
> +++ b/configure
> @@ -6408,7 +6408,7 @@ enabled libopus           && {
>   }
>   enabled libpulse          && require_pkg_config libpulse libpulse pulse/pulseaudio.h pa_context_new
>   enabled librabbitmq       && require_pkg_config librabbitmq "librabbitmq >= 0.7.1" amqp.h amqp_new_connection
> -enabled librav1e          && require_pkg_config librav1e "rav1e >= 0.1.0" rav1e.h rav1e_context_new
> +enabled librav1e          && require_pkg_config librav1e "rav1e >= 0.4.0" rav1e.h rav1e_context_new
>   enabled librsvg           && require_pkg_config librsvg librsvg-2.0 librsvg-2.0/librsvg/rsvg.h rsvg_handle_render_cairo
>   enabled librtmp           && require_pkg_config librtmp librtmp librtmp/rtmp.h RTMP_Socket
>   enabled librubberband     && require_pkg_config librubberband "rubberband >= 1.8.1" rubberband/rubberband-c.h rubberband_new -lstdc++ && append librubberband_extralibs "-lstdc++"
> diff --git a/libavcodec/librav1e.c b/libavcodec/librav1e.c
> index 46071bcdac..c1c0de45a6 100644
> --- a/libavcodec/librav1e.c
> +++ b/libavcodec/librav1e.c
> @@ -445,10 +445,18 @@ static int librav1e_receive_packet(AVCodecContext *avctx, AVPacket *pkt)
>           if (frame->buf[0]) {
>               const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(frame->format);
>   
> +            int64_t *pts = av_malloc(sizeof(int64_t));
> +            if (!pts) {
> +                av_log(avctx, AV_LOG_ERROR, "Could not allocate PTS buffer.\n");
> +                return AVERROR(ENOMEM);
> +            }
> +            *pts = frame->pts;
> +
>               rframe = rav1e_frame_new(ctx->ctx);
>               if (!rframe) {
>                   av_log(avctx, AV_LOG_ERROR, "Could not allocate new rav1e frame.\n");
>                   av_frame_unref(frame);
> +                av_freep(&pts);
>                   return AVERROR(ENOMEM);
>               }
>   
> @@ -461,6 +469,7 @@ static int librav1e_receive_packet(AVCodecContext *avctx, AVPacket *pkt)
>               }
>               av_frame_unref(frame);
>           }
> +        rav1e_frame_set_opaque(rframe, pts, av_free);

pts was declared inside the if (frame->buf[0]) block, so compilation 
fails. You need to move this line inside that block.

>       }
>   
>       ret = rav1e_send_frame(ctx->ctx, rframe);
> @@ -535,7 +544,8 @@ retry:
>       if (rpkt->frame_type == RA_FRAME_TYPE_KEY)
>           pkt->flags |= AV_PKT_FLAG_KEY;
>   
> -    pkt->pts = pkt->dts = rpkt->input_frameno * avctx->ticks_per_frame;
> +    pkt->pts = pkt->dts = *((int64_t *) rpkt->opaque);
> +    av_free(rpkt->opaque);
>       rav1e_packet_unref(rpkt);
>   
>       if (avctx->flags & AV_CODEC_FLAG_GLOBAL_HEADER) {
>

On 16/01/2021 16:53, James Almer wrote:
> pts was declared inside the if (frame->buf[0]) block, so compilation 
> fails. You need to move this line inside that block.

Guess who forgot to git commit --amend before sending his patch? This guy.
Woops - I had already fixed this locally before sending the set but didn't
amend it. v2 sent.

I've pushed patch 1/2 since it's just cosmetic, in the meantime.

- Derek