Re: [FFmpeg-devel] [PATCH] libsvtav1: Enable 2-pass encoding

Re: [FFmpeg-devel] [PATCH] libsvtav1: Enable 2-pass encoding

[Fredrik Lundkvist via ffmpeg-devel]

> > + ret = av_reallocp(&param->rc_stats_buffer.buf, param->rc_stats_buffer.sz);

> Why realloc instead of malloc?

As far as I gathered, rc_stats_buffer.buf is already allocated by SVT-AV1; if malloc is preferred, I can change it and test to make sure it doesn’t cause any obvious issues.


> Wouldn't it be safer to free rc_stats_buffer.buf and set rc_stats_buffer.sz to 0 on failure here too?

Absolutely agree on setting rc_stats_buffer.sz to 0, not quite sure about freeing rc_stats_buffer_buf it is allocated by SVT-AV1.

Thank you for the review, the other comments will be fixed in V2.

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

Re: [FFmpeg-devel] [PATCH] libsvtav1: Enable 2-pass encoding

[Fredrik Lundkvist via ffmpeg-devel]

To continue this discussion.


>> +                av_base64_encode(avctx->stats_out, b64_size, first_pass_stats.buf, first_pass_stats.sz);

> Unchecked return value.


I don’t see any non-test instance in the code base where the return value of av_base64_encode is actually checked. Unchecked calls are however found in many places, f.ex. libaomenc, librav1e, and matroskadec. IMO it should be consistent across the code base, and thus stay as is in this patch.


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

Re: [FFmpeg-devel] [PATCH] libsvtav1: Enable 2-pass encoding

[Andreas Rheinhardt]

Fredrik Lundkvist via ffmpeg-devel:
>>> + ret = av_reallocp(&param->rc_stats_buffer.buf, param->rc_stats_buffer.sz);
> 
>> Why realloc instead of malloc?
> 
> As far as I gathered, rc_stats_buffer.buf is already allocated by SVT-AV1; if malloc is preferred, I can change it and test to make sure it doesn’t cause any obvious issues.
> 

https://gitlab.com/AOMediaCodec/SVT-AV1/-/blob/master/Source/API/EbSvtAv1Enc.h?ref_type=heads#L163
says that this pointer is a non-ownership pointer. Which would imply
that you can't realloc the initial pointer at all and have to simply
replace it with something of our own.
(That we would probably also need to free on our own.)

> 
>> Wouldn't it be safer to free rc_stats_buffer.buf and set rc_stats_buffer.sz to 0 on failure here too?
> 
> Absolutely agree on setting rc_stats_buffer.sz to 0, not quite sure about freeing rc_stats_buffer_buf it is allocated by SVT-AV1.
> 
> Thank you for the review, the other comments will be fixed in V2.
> 
> /Fredrik
> _______________________________________________
> 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".

[FFmpeg-devel] [PATCH] libsvtav1: Enable 2-pass encoding

[Fredrik Lundkvist via ffmpeg-devel]

Currently, libsvtav1 does not send pass number or stat buffer to SVT-AV1, which means that 2-pass encoding is not possible using FFMPEG;
if a user wants to do 2-pass encoding using SVT-AV1, they have to use SvtAv1EncApp.
This patch adds 2-pass encoding support to libsvtav1 using the stats buffers provided by the AVCodecContext.
When encoding the first pass using SVT-AV1, the following should be logged:
Svt[info]: SVT [config]: preset  : Pass 1
Indicating that the encoder knows that it is performing the first pass, activating a special preset.
With the patch applied:
ffmpeg -i input.mp4 -c:v libsvtav1 -an  -pass 1 -b:v 1M -f null /dev/null
[...]
Svt[info]: -------------------------------------------
Svt[info]: SVT [config]: preset  : Pass 1
Svt[info]: -------------------------------------------
[...]

Signed-off-by: Fredrik Lundkvist <fredrik.lundkvist@svt.se<mailto:fredrik.lundkvist@svt.se>>
---
libavcodec/libsvtav1.c | 47 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 46 insertions(+), 1 deletion(-)

diff --git a/libavcodec/libsvtav1.c b/libavcodec/libsvtav1.c
index 9bc165f0cf..be2189399f 100644
--- a/libavcodec/libsvtav1.c
+++ b/libavcodec/libsvtav1.c
@@ -25,6 +25,7 @@
#include <EbSvtAv1Enc.h>
#include <EbSvtAv1Metadata.h>

+#include "libavutil/base64.h"
#include "libavutil/common.h"
#include "libavutil/frame.h"
#include "libavutil/imgutils.h"
@@ -226,6 +227,34 @@ static int config_enc_params(EbSvtAv1EncConfiguration *param,
        param->max_qp_allowed       = avctx->qmax;
        param->min_qp_allowed       = avctx->qmin;
    }
+
+    if (avctx->flags & AV_CODEC_FLAG_PASS2) {
+        int decode_size, ret;
+        if(!avctx->stats_in) {
+            av_log(avctx, AV_LOG_ERROR, "No stats file for second pass\n");
+            return AVERROR_INVALIDDATA;
+        }
+        param->rc_stats_buffer.sz = strlen(avctx->stats_in) * 3 / 4;
+        ret                       = av_reallocp(&param->rc_stats_buffer.buf, param->rc_stats_buffer.sz);
+        if (ret < 0) {
+            av_log(avctx, AV_LOG_ERROR,
+                    "Stat buffer alloc (%"SIZE_SPECIFIER" bytes) failed \n",
+                    param->rc_stats_buffer.sz
+            );
+            param->rc_stats_buffer.sz = 0;
+            return ret;
+        }
+        decode_size = av_base64_decode(param->rc_stats_buffer.buf, avctx->stats_in, param->rc_stats_buffer.sz);
+        if (decode_size < 0) {
+            av_log(avctx, AV_LOG_ERROR, "Stat buffer decode failed\n");
+            return AVERROR_INVALIDDATA;
+        }
+        param->rc_stats_buffer.sz = decode_size;
+        param->pass = 2;
+    } else if (avctx->flags & AV_CODEC_FLAG_PASS1) {
+        param->pass = 1;
+    }
+
    param->max_bit_rate             = avctx->rc_max_rate;
    if ((avctx->bit_rate > 0 || avctx->rc_max_rate > 0) && avctx->rc_buffer_size)
        param->maximum_buffer_size_ms =
@@ -618,6 +647,23 @@ static int eb_receive_packet(AVCodecContext *avctx, AVPacket *pkt)
    if (headerPtr->flags & EB_BUFFERFLAG_EOS) {
         svt_enc->eos_flag = EOS_RECEIVED;
         svt_av1_enc_release_out_buffer(&headerPtr);
+         if (avctx->flags & AV_CODEC_FLAG_PASS1) {
+            SvtAv1FixedBuf first_pass_stats;
+            EbErrorType ret = svt_av1_enc_get_stream_info(svt_enc->svt_handle, SVT_AV1_STREAM_INFO_FIRST_PASS_STATS_OUT, &first_pass_stats);
+            if (ret == EB_ErrorNone) {
+                size_t b64_size = AV_BASE64_SIZE(first_pass_stats.sz);
+
+                avctx->stats_out = av_malloc(b64_size);
+                if(!avctx->stats_out) {
+                    av_log(avctx, AV_LOG_ERROR, "Stat buffer alloc (%"SIZE_SPECIFIER" bytes) failed\n",
+                            b64_size);
+                    return AVERROR(ENOMEM);
+                }
+                av_base64_encode(avctx->stats_out, b64_size, first_pass_stats.buf, first_pass_stats.sz);
+            } else {
+                av_log(avctx, AV_LOG_ERROR, "Failed to get stream info");
+            }
+         }
         return AVERROR_EOF;
    }
#endif
@@ -681,7 +727,6 @@ static av_cold int eb_enc_close(AVCodecContext *avctx)
        svt_metadata_array_free(&svt_enc->in_buf->metadata);
        av_freep(&svt_enc->in_buf);
    }
-
    av_buffer_pool_uninit(&svt_enc->pool);
    av_frame_free(&svt_enc->frame);
    ff_dovi_ctx_unref(&svt_enc->dovi);
--
2.39.3 (Apple Git-145)
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] libsvtav1: Enable 2-pass encoding

[James Almer]

On 5/15/2024 11:14 AM, Fredrik Lundkvist via ffmpeg-devel wrote:
> Currently, libsvtav1 does not send pass number or stat buffer to SVT-AV1, which means that 2-pass encoding is not possible using FFMPEG;
> if a user wants to do 2-pass encoding using SVT-AV1, they have to use SvtAv1EncApp.
> This patch adds 2-pass encoding support to libsvtav1 using the stats buffers provided by the AVCodecContext.
> When encoding the first pass using SVT-AV1, the following should be logged:
> Svt[info]: SVT [config]: preset  : Pass 1
> Indicating that the encoder knows that it is performing the first pass, activating a special preset.
> With the patch applied:
> ffmpeg -i input.mp4 -c:v libsvtav1 -an  -pass 1 -b:v 1M -f null /dev/null
> [...]
> Svt[info]: -------------------------------------------
> Svt[info]: SVT [config]: preset  : Pass 1
> Svt[info]: -------------------------------------------
> [...]
> 
> Signed-off-by: Fredrik Lundkvist <fredrik.lundkvist@svt.se<mailto:fredrik.lundkvist@svt.se>>
> ---
> libavcodec/libsvtav1.c | 47 +++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 46 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/libsvtav1.c b/libavcodec/libsvtav1.c
> index 9bc165f0cf..be2189399f 100644
> --- a/libavcodec/libsvtav1.c
> +++ b/libavcodec/libsvtav1.c
> @@ -25,6 +25,7 @@
> #include <EbSvtAv1Enc.h>
> #include <EbSvtAv1Metadata.h>
> 
> +#include "libavutil/base64.h"
> #include "libavutil/common.h"
> #include "libavutil/frame.h"
> #include "libavutil/imgutils.h"
> @@ -226,6 +227,34 @@ static int config_enc_params(EbSvtAv1EncConfiguration *param,
>          param->max_qp_allowed       = avctx->qmax;
>          param->min_qp_allowed       = avctx->qmin;
>      }
> +
> +    if (avctx->flags & AV_CODEC_FLAG_PASS2) {
> +        int decode_size, ret;
> +        if(!avctx->stats_in) {

if (...

> +            av_log(avctx, AV_LOG_ERROR, "No stats file for second pass\n");
> +            return AVERROR_INVALIDDATA;
> +        }
> +        param->rc_stats_buffer.sz = strlen(avctx->stats_in) * 3 / 4;

nit: AV_BASE64_DECODE_SIZE

> +        ret                       = av_reallocp(&param->rc_stats_buffer.buf, param->rc_stats_buffer.sz);

Why realloc instead of malloc?

> +        if (ret < 0) {
> +            av_log(avctx, AV_LOG_ERROR,
> +                    "Stat buffer alloc (%"SIZE_SPECIFIER" bytes) failed \n",
> +                    param->rc_stats_buffer.sz
> +            );
> +            param->rc_stats_buffer.sz = 0;
> +            return ret;
> +        }
> +        decode_size = av_base64_decode(param->rc_stats_buffer.buf, avctx->stats_in, param->rc_stats_buffer.sz);
> +        if (decode_size < 0) {

Wouldn't it be safer to free rc_stats_buffer.buf and set 
rc_stats_buffer.sz to 0 on failure here too?


> +            av_log(avctx, AV_LOG_ERROR, "Stat buffer decode failed\n");
> +            return AVERROR_INVALIDDATA;
> +        }
> +        param->rc_stats_buffer.sz = decode_size;
> +        param->pass = 2;
> +    } else if (avctx->flags & AV_CODEC_FLAG_PASS1) {

No brackets for single line blocks.

> +        param->pass = 1;
> +    }
> +
>      param->max_bit_rate             = avctx->rc_max_rate;
>      if ((avctx->bit_rate > 0 || avctx->rc_max_rate > 0) && avctx->rc_buffer_size)
>          param->maximum_buffer_size_ms =
> @@ -618,6 +647,23 @@ static int eb_receive_packet(AVCodecContext *avctx, AVPacket *pkt)
>      if (headerPtr->flags & EB_BUFFERFLAG_EOS) {
>           svt_enc->eos_flag = EOS_RECEIVED;
>           svt_av1_enc_release_out_buffer(&headerPtr);
> +         if (avctx->flags & AV_CODEC_FLAG_PASS1) {
> +            SvtAv1FixedBuf first_pass_stats;
> +            EbErrorType ret = svt_av1_enc_get_stream_info(svt_enc->svt_handle, SVT_AV1_STREAM_INFO_FIRST_PASS_STATS_OUT, &first_pass_stats);

svt_ret. Don't shadow ret.

Also, consider splitting this line.

> +            if (ret == EB_ErrorNone) {
> +                size_t b64_size = AV_BASE64_SIZE(first_pass_stats.sz);
> +
> +                avctx->stats_out = av_malloc(b64_size);
> +                if(!avctx->stats_out) {

Missing space after the if again.

> +                    av_log(avctx, AV_LOG_ERROR, "Stat buffer alloc (%"SIZE_SPECIFIER" bytes) failed\n",
> +                            b64_size);
> +                    return AVERROR(ENOMEM);
> +                }
> +                av_base64_encode(avctx->stats_out, b64_size, first_pass_stats.buf, first_pass_stats.sz);

Unchecked return value.

> +            } else {

Single line block again.

> +                av_log(avctx, AV_LOG_ERROR, "Failed to get stream info");
> +            }
> +         }
>           return AVERROR_EOF;
>      }
> #endif
> @@ -681,7 +727,6 @@ static av_cold int eb_enc_close(AVCodecContext *avctx)
>          svt_metadata_array_free(&svt_enc->in_buf->metadata);
>          av_freep(&svt_enc->in_buf);
>      }
> -

Spurious change.

>      av_buffer_pool_uninit(&svt_enc->pool);
>      av_frame_free(&svt_enc->frame);
>      ff_dovi_ctx_unref(&svt_enc->dovi);
> --
> 2.39.3 (Apple Git-145)
> _______________________________________________
> 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".