[FFmpeg-devel] [PATCH 1/3] lavc/encode: factor audio/video-specific parts out of ff_encode_preinit()

[FFmpeg-devel] [PATCH 1/3] lavc/encode: factor audio/video-specific parts out of ff_encode_preinit()

[Anton Khirnov]

---
 libavcodec/encode.c | 127 ++++++++++++++++++++++++++------------------
 1 file changed, 75 insertions(+), 52 deletions(-)

diff --git a/libavcodec/encode.c b/libavcodec/encode.c
index 618be0573d..6543dda5bd 100644
--- a/libavcodec/encode.c
+++ b/libavcodec/encode.c
@@ -406,15 +406,67 @@ int attribute_align_arg avcodec_receive_packet(AVCodecContext *avctx, AVPacket *
     return 0;
 }
 
-int ff_encode_preinit(AVCodecContext *avctx)
+static int encode_preinit_video(AVCodecContext *avctx)
 {
+        const AVPixFmtDescriptor *pixdesc = av_pix_fmt_desc_get(avctx->pix_fmt);
     int i;
 
-    if (avctx->time_base.num <= 0 || avctx->time_base.den <= 0) {
-        av_log(avctx, AV_LOG_ERROR, "The encoder timebase is not set.\n");
-        return AVERROR(EINVAL);
+    if (avctx->codec->pix_fmts) {
+        for (i = 0; avctx->codec->pix_fmts[i] != AV_PIX_FMT_NONE; i++)
+            if (avctx->pix_fmt == avctx->codec->pix_fmts[i])
+                break;
+        if (avctx->codec->pix_fmts[i] == AV_PIX_FMT_NONE) {
+            char buf[128];
+            snprintf(buf, sizeof(buf), "%d", avctx->pix_fmt);
+            av_log(avctx, AV_LOG_ERROR, "Specified pixel format %s is invalid or not supported\n",
+                   (char *)av_x_if_null(av_get_pix_fmt_name(avctx->pix_fmt), buf));
+            return AVERROR(EINVAL);
+        }
+        if (avctx->codec->pix_fmts[i] == AV_PIX_FMT_YUVJ420P ||
+            avctx->codec->pix_fmts[i] == AV_PIX_FMT_YUVJ411P ||
+            avctx->codec->pix_fmts[i] == AV_PIX_FMT_YUVJ422P ||
+            avctx->codec->pix_fmts[i] == AV_PIX_FMT_YUVJ440P ||
+            avctx->codec->pix_fmts[i] == AV_PIX_FMT_YUVJ444P)
+            avctx->color_range = AVCOL_RANGE_JPEG;
+    }
+
+        if (    avctx->bits_per_raw_sample < 0
+            || (avctx->bits_per_raw_sample > 8 && pixdesc->comp[0].depth <= 8)) {
+            av_log(avctx, AV_LOG_WARNING, "Specified bit depth %d not possible with the specified pixel formats depth %d\n",
+                avctx->bits_per_raw_sample, pixdesc->comp[0].depth);
+            avctx->bits_per_raw_sample = pixdesc->comp[0].depth;
+        }
+        if (avctx->width <= 0 || avctx->height <= 0) {
+            av_log(avctx, AV_LOG_ERROR, "dimensions not set\n");
+            return AVERROR(EINVAL);
+        }
+
+    if (avctx->hw_frames_ctx) {
+        AVHWFramesContext *frames_ctx = (AVHWFramesContext*)avctx->hw_frames_ctx->data;
+        if (frames_ctx->format != avctx->pix_fmt) {
+            av_log(avctx, AV_LOG_ERROR,
+                   "Mismatching AVCodecContext.pix_fmt and AVHWFramesContext.format\n");
+            return AVERROR(EINVAL);
+        }
+        if (avctx->sw_pix_fmt != AV_PIX_FMT_NONE &&
+            avctx->sw_pix_fmt != frames_ctx->sw_format) {
+            av_log(avctx, AV_LOG_ERROR,
+                   "Mismatching AVCodecContext.sw_pix_fmt (%s) "
+                   "and AVHWFramesContext.sw_format (%s)\n",
+                   av_get_pix_fmt_name(avctx->sw_pix_fmt),
+                   av_get_pix_fmt_name(frames_ctx->sw_format));
+            return AVERROR(EINVAL);
+        }
+        avctx->sw_pix_fmt = frames_ctx->sw_format;
     }
 
+    return 0;
+}
+
+static int encode_preinit_audio(AVCodecContext *avctx)
+{
+    int i;
+
     if (avctx->codec->sample_fmts) {
         for (i = 0; avctx->codec->sample_fmts[i] != AV_SAMPLE_FMT_NONE; i++) {
             if (avctx->sample_fmt == avctx->codec->sample_fmts[i])
@@ -434,24 +486,6 @@ int ff_encode_preinit(AVCodecContext *avctx)
             return AVERROR(EINVAL);
         }
     }
-    if (avctx->codec->pix_fmts) {
-        for (i = 0; avctx->codec->pix_fmts[i] != AV_PIX_FMT_NONE; i++)
-            if (avctx->pix_fmt == avctx->codec->pix_fmts[i])
-                break;
-        if (avctx->codec->pix_fmts[i] == AV_PIX_FMT_NONE) {
-            char buf[128];
-            snprintf(buf, sizeof(buf), "%d", avctx->pix_fmt);
-            av_log(avctx, AV_LOG_ERROR, "Specified pixel format %s is invalid or not supported\n",
-                   (char *)av_x_if_null(av_get_pix_fmt_name(avctx->pix_fmt), buf));
-            return AVERROR(EINVAL);
-        }
-        if (avctx->codec->pix_fmts[i] == AV_PIX_FMT_YUVJ420P ||
-            avctx->codec->pix_fmts[i] == AV_PIX_FMT_YUVJ411P ||
-            avctx->codec->pix_fmts[i] == AV_PIX_FMT_YUVJ422P ||
-            avctx->codec->pix_fmts[i] == AV_PIX_FMT_YUVJ440P ||
-            avctx->codec->pix_fmts[i] == AV_PIX_FMT_YUVJ444P)
-            avctx->color_range = AVCOL_RANGE_JPEG;
-    }
     if (avctx->codec->supported_samplerates) {
         for (i = 0; avctx->codec->supported_samplerates[i] != 0; i++)
             if (avctx->sample_rate == avctx->codec->supported_samplerates[i])
@@ -500,19 +534,26 @@ int ff_encode_preinit(AVCodecContext *avctx)
                 avctx->channels);
         return AVERROR(EINVAL);
     }
-    if(avctx->codec_type == AVMEDIA_TYPE_VIDEO) {
-        const AVPixFmtDescriptor *pixdesc = av_pix_fmt_desc_get(avctx->pix_fmt);
-        if (    avctx->bits_per_raw_sample < 0
-            || (avctx->bits_per_raw_sample > 8 && pixdesc->comp[0].depth <= 8)) {
-            av_log(avctx, AV_LOG_WARNING, "Specified bit depth %d not possible with the specified pixel formats depth %d\n",
-                avctx->bits_per_raw_sample, pixdesc->comp[0].depth);
-            avctx->bits_per_raw_sample = pixdesc->comp[0].depth;
-        }
-        if (avctx->width <= 0 || avctx->height <= 0) {
-            av_log(avctx, AV_LOG_ERROR, "dimensions not set\n");
-            return AVERROR(EINVAL);
-        }
+
+    return 0;
+}
+
+int ff_encode_preinit(AVCodecContext *avctx)
+{
+    int ret = 0;
+
+    if (avctx->time_base.num <= 0 || avctx->time_base.den <= 0) {
+        av_log(avctx, AV_LOG_ERROR, "The encoder timebase is not set.\n");
+        return AVERROR(EINVAL);
     }
+
+    switch (avctx->codec_type) {
+    case AVMEDIA_TYPE_VIDEO: ret = encode_preinit_video(avctx); break;
+    case AVMEDIA_TYPE_AUDIO: ret = encode_preinit_audio(avctx); break;
+    }
+    if (ret < 0)
+        return ret;
+
     if (   (avctx->codec_type == AVMEDIA_TYPE_VIDEO || avctx->codec_type == AVMEDIA_TYPE_AUDIO)
         && avctx->bit_rate>0 && avctx->bit_rate<1000) {
         av_log(avctx, AV_LOG_WARNING, "Bitrate %"PRId64" is extremely low, maybe you mean %"PRId64"k\n", avctx->bit_rate, avctx->bit_rate);
@@ -531,24 +572,6 @@ int ff_encode_preinit(AVCodecContext *avctx)
         return AVERROR(EINVAL);
     }
 
-    if (avctx->hw_frames_ctx) {
-        AVHWFramesContext *frames_ctx = (AVHWFramesContext*)avctx->hw_frames_ctx->data;
-        if (frames_ctx->format != avctx->pix_fmt) {
-            av_log(avctx, AV_LOG_ERROR,
-                   "Mismatching AVCodecContext.pix_fmt and AVHWFramesContext.format\n");
-            return AVERROR(EINVAL);
-        }
-        if (avctx->sw_pix_fmt != AV_PIX_FMT_NONE &&
-            avctx->sw_pix_fmt != frames_ctx->sw_format) {
-            av_log(avctx, AV_LOG_ERROR,
-                   "Mismatching AVCodecContext.sw_pix_fmt (%s) "
-                   "and AVHWFramesContext.sw_format (%s)\n",
-                   av_get_pix_fmt_name(avctx->sw_pix_fmt),
-                   av_get_pix_fmt_name(frames_ctx->sw_format));
-            return AVERROR(EINVAL);
-        }
-        avctx->sw_pix_fmt = frames_ctx->sw_format;
-    }
     if (avctx->codec_descriptor->props & AV_CODEC_PROP_INTRA_ONLY)
         avctx->internal->intra_only_flag = AV_PKT_FLAG_KEY;
 
-- 
2.33.0

_______________________________________________
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 2/3] lavc/encode: reindent

[Anton Khirnov]

---
 libavcodec/encode.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/libavcodec/encode.c b/libavcodec/encode.c
index 6543dda5bd..b6f81d1458 100644
--- a/libavcodec/encode.c
+++ b/libavcodec/encode.c
@@ -408,7 +408,7 @@ int attribute_align_arg avcodec_receive_packet(AVCodecContext *avctx, AVPacket *
 
 static int encode_preinit_video(AVCodecContext *avctx)
 {
-        const AVPixFmtDescriptor *pixdesc = av_pix_fmt_desc_get(avctx->pix_fmt);
+    const AVPixFmtDescriptor *pixdesc = av_pix_fmt_desc_get(avctx->pix_fmt);
     int i;
 
     if (avctx->codec->pix_fmts) {
@@ -430,16 +430,16 @@ static int encode_preinit_video(AVCodecContext *avctx)
             avctx->color_range = AVCOL_RANGE_JPEG;
     }
 
-        if (    avctx->bits_per_raw_sample < 0
-            || (avctx->bits_per_raw_sample > 8 && pixdesc->comp[0].depth <= 8)) {
-            av_log(avctx, AV_LOG_WARNING, "Specified bit depth %d not possible with the specified pixel formats depth %d\n",
-                avctx->bits_per_raw_sample, pixdesc->comp[0].depth);
-            avctx->bits_per_raw_sample = pixdesc->comp[0].depth;
-        }
-        if (avctx->width <= 0 || avctx->height <= 0) {
-            av_log(avctx, AV_LOG_ERROR, "dimensions not set\n");
-            return AVERROR(EINVAL);
-        }
+    if (    avctx->bits_per_raw_sample < 0
+        || (avctx->bits_per_raw_sample > 8 && pixdesc->comp[0].depth <= 8)) {
+        av_log(avctx, AV_LOG_WARNING, "Specified bit depth %d not possible with the specified pixel formats depth %d\n",
+            avctx->bits_per_raw_sample, pixdesc->comp[0].depth);
+        avctx->bits_per_raw_sample = pixdesc->comp[0].depth;
+    }
+    if (avctx->width <= 0 || avctx->height <= 0) {
+        av_log(avctx, AV_LOG_ERROR, "dimensions not set\n");
+        return AVERROR(EINVAL);
+    }
 
     if (avctx->hw_frames_ctx) {
         AVHWFramesContext *frames_ctx = (AVHWFramesContext*)avctx->hw_frames_ctx->data;
-- 
2.33.0

_______________________________________________
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 3/3] lavc/encode: pick a sane default for bits_per_raw_sample if it's not set

[Anton Khirnov]

Fixes #9563.
---
 libavcodec/encode.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/libavcodec/encode.c b/libavcodec/encode.c
index b6f81d1458..44ab81af3f 100644
--- a/libavcodec/encode.c
+++ b/libavcodec/encode.c
@@ -535,6 +535,9 @@ static int encode_preinit_audio(AVCodecContext *avctx)
         return AVERROR(EINVAL);
     }
 
+    if (!avctx->bits_per_raw_sample)
+        avctx->bits_per_raw_sample = 8 * av_get_bytes_per_sample(avctx->sample_fmt);
+
     return 0;
 }
 
-- 
2.33.0

_______________________________________________
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 3/3] lavc/encode: pick a sane default for bits_per_raw_sample if it's not set

[Martijn van Beurden]

Op do 20 jan. 2022 om 17:05 schreef Anton Khirnov <anton@khirnov.net>:

> Fixes #9563.
> ---
>  libavcodec/encode.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/libavcodec/encode.c b/libavcodec/encode.c
> index b6f81d1458..44ab81af3f 100644
> --- a/libavcodec/encode.c
> +++ b/libavcodec/encode.c
> @@ -535,6 +535,9 @@ static int encode_preinit_audio(AVCodecContext *avctx)
>          return AVERROR(EINVAL);
>      }
>
> +    if (!avctx->bits_per_raw_sample)
> +        avctx->bits_per_raw_sample = 8 *
> av_get_bytes_per_sample(avctx->sample_fmt);
> +
>      return 0;
>  }
>

This creates a new regression: now 24-bit WAV files are converted to 32-bit
WavPack files. I think I found the cause of the problem though: the issue
is that wavpack uses sample format s32p and 24-bit wav files use s32.
Therefore, a aresample filter is auto-inserted to convert s32p to s32,
which causes ost->filter->graph->is_meta to be false.
_______________________________________________
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 3/3] lavc/encode: pick a sane default for bits_per_raw_sample if it's not set

[Anton Khirnov]

Quoting Martijn van Beurden (2022-01-20 19:58:54)
> Op do 20 jan. 2022 om 17:05 schreef Anton Khirnov <anton@khirnov.net>:
> 
> > Fixes #9563.
> > ---
> >  libavcodec/encode.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/libavcodec/encode.c b/libavcodec/encode.c
> > index b6f81d1458..44ab81af3f 100644
> > --- a/libavcodec/encode.c
> > +++ b/libavcodec/encode.c
> > @@ -535,6 +535,9 @@ static int encode_preinit_audio(AVCodecContext *avctx)
> >          return AVERROR(EINVAL);
> >      }
> >
> > +    if (!avctx->bits_per_raw_sample)
> > +        avctx->bits_per_raw_sample = 8 *
> > av_get_bytes_per_sample(avctx->sample_fmt);
> > +
> >      return 0;
> >  }
> >
> 
> This creates a new regression: now 24-bit WAV files are converted to 32-bit
> WavPack files. I think I found the cause of the problem though: the issue
> is that wavpack uses sample format s32p and 24-bit wav files use s32.
> Therefore, a aresample filter is auto-inserted to convert s32p to s32,
> which causes ost->filter->graph->is_meta to be false.

You can work around this by using the -bits_per_raw_sample option.
Handling this in a more automagic manner would be complicated - we could
propagate bits_per_raw_sample through lavfi or add it to AVFrame.

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

Re: [FFmpeg-devel] [PATCH 3/3] lavc/encode: pick a sane default for bits_per_raw_sample if it's not set

[Martijn van Beurden]

Op do 20 jan. 2022 om 19:58 schreef Martijn van Beurden <mvanb1@gmail.com>:

> This creates a new regression: now 24-bit WAV files are converted to
> 32-bit WavPack files. I think I found the cause of the problem though: the
> issue is that wavpack uses sample format s32p and 24-bit wav files use s32.
> Therefore, a aresample filter is auto-inserted to convert s32p to s32,
> which causes ost->filter->graph->is_meta to be false.
>

Also, I see an abuffer is also added to the filtergraph which too causes
ost->filter->graph->is_meta to be false.
_______________________________________________
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 3/3] lavc/encode: pick a sane default for bits_per_raw_sample if it's not set

[Martijn van Beurden]

Op do 20 jan. 2022 22:18 schreef Anton Khirnov <anton@khirnov.net>:

> Quoting Martijn van Beurden (2022-01-20 19:58:54)
> > Op do 20 jan. 2022 om 17:05 schreef Anton Khirnov <anton@khirnov.net>:
> >
> > > Fixes #9563.
> > > ---
> > >  libavcodec/encode.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/libavcodec/encode.c b/libavcodec/encode.c
> > > index b6f81d1458..44ab81af3f 100644
> > > --- a/libavcodec/encode.c
> > > +++ b/libavcodec/encode.c
> > > @@ -535,6 +535,9 @@ static int encode_preinit_audio(AVCodecContext
> *avctx)
> > >          return AVERROR(EINVAL);
> > >      }
> > >
> > > +    if (!avctx->bits_per_raw_sample)
> > > +        avctx->bits_per_raw_sample = 8 *
> > > av_get_bytes_per_sample(avctx->sample_fmt);
> > > +
> > >      return 0;
> > >  }
> > >
> >
> > This creates a new regression: now 24-bit WAV files are converted to
> 32-bit
> > WavPack files. I think I found the cause of the problem though: the issue
> > is that wavpack uses sample format s32p and 24-bit wav files use s32.
> > Therefore, a aresample filter is auto-inserted to convert s32p to s32,
> > which causes ost->filter->graph->is_meta to be false.
>
> You can work around this by using the -bits_per_raw_sample option.
> Handling this in a more automagic manner would be complicated - we could
> propagate bits_per_raw_sample through lavfi or add it to AVFrame.
>

I think this patch shouldn't be applied. Without it 32 bit audio is
transcoded to 24 bit audio, which is not lossless. However, with this patch
24 bit audio is transcoded to 32 bit audio which is lossless, but the
resulting files are understood by few software decoder and probably no
hardware decoders.

Perhaps adding a warning instead of setting a default would be better?

>
_______________________________________________
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 3/3] lavc/encode: pick a sane default for bits_per_raw_sample if it's not set

[Anton Khirnov]

Quoting Martijn van Beurden (2022-01-23 14:15:53)
> I think this patch shouldn't be applied. Without it 32 bit audio is
> transcoded to 24 bit audio, which is not lossless. However, with this patch
> 24 bit audio is transcoded to 32 bit audio which is lossless, but the
> resulting files are understood by few software decoder and probably no
> hardware decoders.

Do you know which decoders do not support 32bit audio and how widely
used they are? I'm not very familiar with wavpack, but 24bit and 32bit
decoding don't seem significantly different in the decoder.

> Perhaps adding a warning instead of setting a default would be better?

IMO the encoder should strive to be lossless, so it should not encode as
24bit unless it was explicitly told the input data is 24bit.
bits_per_raw_sample=0 is not a meaningful value, so the encoder should
not treat it as 24bit.

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

Re: [FFmpeg-devel] [PATCH 3/3] lavc/encode: pick a sane default for bits_per_raw_sample if it's not set

[Anton Khirnov]

ping

any more opinions on this?

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

Re: [FFmpeg-devel] [PATCH 3/3] lavc/encode: pick a sane default for bits_per_raw_sample if it's not set

[Paul B Mahol]

On Sun, Feb 20, 2022 at 3:19 PM Anton Khirnov <anton@khirnov.net> wrote:

> ping
>
> any more opinions on this?
>

I'm OK with this patch.

If 24bit is still needed it can be set in usual already known way, by
setting additional flag.


>
> --
> 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".
>
_______________________________________________
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 3/3] lavc/encode: pick a sane default for bits_per_raw_sample if it's not set

[Paul B Mahol]

Ping for this. Current state is imho bad.
_______________________________________________
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 3/3] lavc/encode: pick a sane default for bits_per_raw_sample if it's not set

[Anton Khirnov]

Quoting Paul B Mahol (2022-04-06 13:24:35)
> Ping for this. Current state is imho bad.

pushed

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