diff mbox series

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

Message ID 20210114135558.58119-2-derek.buitenhuis@gmail.com
State Superseded
Headers show
Series [FFmpeg-devel,1/2] avcodec/librav1e: Fix indentation
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Derek Buitenhuis Jan. 14, 2021, 1:55 p.m. UTC
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(-)

Comments

James Almer Jan. 14, 2021, 2:09 p.m. UTC | #1
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) {
>
Derek Buitenhuis Jan. 14, 2021, 2:12 p.m. UTC | #2
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
James Almer Jan. 16, 2021, 4:53 p.m. UTC | #3
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) {
>
Derek Buitenhuis Jan. 17, 2021, 12:39 p.m. UTC | #4
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
diff mbox series

Patch

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);
     rav1e_packet_unref(rpkt);
 
     if (avctx->flags & AV_CODEC_FLAG_GLOBAL_HEADER) {