diff mbox series

[FFmpeg-devel,1/7] lavc: factor out encoder init/validation from avcodec_open2()

Message ID 20210310120332.27225-1-anton@khirnov.net
State Accepted
Headers show
Series [FFmpeg-devel,1/7] lavc: factor out encoder init/validation from avcodec_open2() | expand

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

Anton Khirnov March 10, 2021, 12:03 p.m. UTC 
avcodec_open2() is massive, splitting it makes it more readable.

Also, add a missing error code to ticks_per_frame sanity check.
---
 libavcodec/encode.c | 157 +++++++++++++++++++++++++++++++++++++++++
 libavcodec/encode.h |   6 ++
 libavcodec/utils.c  | 166 +-------------------------------------------
 3 files changed, 166 insertions(+), 163 deletions(-)

Comments

James Almer March 10, 2021, 3:17 p.m. UTC | #1 
On 3/10/2021 9:03 AM, Anton Khirnov wrote:
> avcodec_open2() is massive, splitting it makes it more readable.
> 
> Also, add a missing error code to ticks_per_frame sanity check.
> ---
>   libavcodec/encode.c | 157 +++++++++++++++++++++++++++++++++++++++++
>   libavcodec/encode.h |   6 ++
>   libavcodec/utils.c  | 166 +-------------------------------------------
>   3 files changed, 166 insertions(+), 163 deletions(-)
> 
> diff --git a/libavcodec/encode.c b/libavcodec/encode.c
> index 282337e453..bbf03d62fc 100644
> --- a/libavcodec/encode.c
> +++ b/libavcodec/encode.c
> @@ -462,3 +462,160 @@ int attribute_align_arg avcodec_encode_video2(AVCodecContext *avctx,
>       return ret;
>   }
>   #endif
> +
> +int ff_encode_preinit(AVCodecContext *avctx)

nit: Would prefer if this and ff_decode_preinit() could stay in the same 
file as avcodec_open2() as static functions. This includes moving 
decode_bsfs_init() there, too.
decode.c and encode.c seem to me that they should contain functions used 
during decoding and encoding, and not initialization.

That being said, not related to this set and not really a priority, but 
avcodec_open2() is not a "utility" function as much as a core lavc 
function. av_get_bits_per_sample() for example is a util, as are 
av_get_audio_frame_duration() and avcodec_align_dimensions2(). So 
perhaps it, avcodec_alloc_context3() and avcodec_free_context() should 
be together (options.c is also not exactly the best name for the file 
currently hosting the latter two, so maybe it could be renamed to 
avcodec.c while at it).
Anton Khirnov March 16, 2021, 9:50 a.m. UTC | #2 
Quoting James Almer (2021-03-10 16:17:35)
> On 3/10/2021 9:03 AM, Anton Khirnov wrote:
> > avcodec_open2() is massive, splitting it makes it more readable.
> > 
> > Also, add a missing error code to ticks_per_frame sanity check.
> > ---
> >   libavcodec/encode.c | 157 +++++++++++++++++++++++++++++++++++++++++
> >   libavcodec/encode.h |   6 ++
> >   libavcodec/utils.c  | 166 +-------------------------------------------
> >   3 files changed, 166 insertions(+), 163 deletions(-)
> > 
> > diff --git a/libavcodec/encode.c b/libavcodec/encode.c
> > index 282337e453..bbf03d62fc 100644
> > --- a/libavcodec/encode.c
> > +++ b/libavcodec/encode.c
> > @@ -462,3 +462,160 @@ int attribute_align_arg avcodec_encode_video2(AVCodecContext *avctx,
> >       return ret;
> >   }
> >   #endif
> > +
> > +int ff_encode_preinit(AVCodecContext *avctx)
> 
> nit: Would prefer if this and ff_decode_preinit() could stay in the same 
> file as avcodec_open2() as static functions. This includes moving 
> decode_bsfs_init() there, too.
> decode.c and encode.c seem to me that they should contain functions used 
> during decoding and encoding, and not initialization.

For the record: discussed this on IRC and James dropped his objection to
the move

> 
> That being said, not related to this set and not really a priority, but 
> avcodec_open2() is not a "utility" function as much as a core lavc 
> function. av_get_bits_per_sample() for example is a util, as are 
> av_get_audio_frame_duration() and avcodec_align_dimensions2(). So 
> perhaps it, avcodec_alloc_context3() and avcodec_free_context() should 
> be together (options.c is also not exactly the best name for the file 
> currently hosting the latter two, so maybe it could be renamed to 
> avcodec.c while at it).

I would certainly be in favor of that. utils.c is way too huge.
diff mbox series

Patch

diff --git a/libavcodec/encode.c b/libavcodec/encode.c
index 282337e453..bbf03d62fc 100644
--- a/libavcodec/encode.c
+++ b/libavcodec/encode.c
@@ -462,3 +462,160 @@  int attribute_align_arg avcodec_encode_video2(AVCodecContext *avctx,
     return ret;
 }
 #endif
+
+int ff_encode_preinit(AVCodecContext *avctx)
+{
+        int i;
+#if FF_API_CODED_FRAME
+FF_DISABLE_DEPRECATION_WARNINGS
+        avctx->coded_frame = av_frame_alloc();
+        if (!avctx->coded_frame) {
+            return AVERROR(ENOMEM);
+        }
+FF_ENABLE_DEPRECATION_WARNINGS
+#endif
+
+        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->sample_fmts) {
+            for (i = 0; avctx->codec->sample_fmts[i] != AV_SAMPLE_FMT_NONE; i++) {
+                if (avctx->sample_fmt == avctx->codec->sample_fmts[i])
+                    break;
+                if (avctx->channels == 1 &&
+                    av_get_planar_sample_fmt(avctx->sample_fmt) ==
+                    av_get_planar_sample_fmt(avctx->codec->sample_fmts[i])) {
+                    avctx->sample_fmt = avctx->codec->sample_fmts[i];
+                    break;
+                }
+            }
+            if (avctx->codec->sample_fmts[i] == AV_SAMPLE_FMT_NONE) {
+                char buf[128];
+                snprintf(buf, sizeof(buf), "%d", avctx->sample_fmt);
+                av_log(avctx, AV_LOG_ERROR, "Specified sample format %s is invalid or not supported\n",
+                       (char *)av_x_if_null(av_get_sample_fmt_name(avctx->sample_fmt), buf));
+                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
+                && !((avctx->codec_id == AV_CODEC_ID_MJPEG || avctx->codec_id == AV_CODEC_ID_LJPEG)
+                     && avctx->strict_std_compliance <= FF_COMPLIANCE_UNOFFICIAL)) {
+                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])
+                    break;
+            if (avctx->codec->supported_samplerates[i] == 0) {
+                av_log(avctx, AV_LOG_ERROR, "Specified sample rate %d is not supported\n",
+                       avctx->sample_rate);
+                return AVERROR(EINVAL);
+            }
+        }
+        if (avctx->sample_rate < 0) {
+            av_log(avctx, AV_LOG_ERROR, "Specified sample rate %d is not supported\n",
+                    avctx->sample_rate);
+            return AVERROR(EINVAL);
+        }
+        if (avctx->codec->channel_layouts) {
+            if (!avctx->channel_layout) {
+                av_log(avctx, AV_LOG_WARNING, "Channel layout not specified\n");
+            } else {
+                for (i = 0; avctx->codec->channel_layouts[i] != 0; i++)
+                    if (avctx->channel_layout == avctx->codec->channel_layouts[i])
+                        break;
+                if (avctx->codec->channel_layouts[i] == 0) {
+                    char buf[512];
+                    av_get_channel_layout_string(buf, sizeof(buf), -1, avctx->channel_layout);
+                    av_log(avctx, AV_LOG_ERROR, "Specified channel layout '%s' is not supported\n", buf);
+                    return AVERROR(EINVAL);
+                }
+            }
+        }
+        if (avctx->channel_layout && avctx->channels) {
+            int channels = av_get_channel_layout_nb_channels(avctx->channel_layout);
+            if (channels != avctx->channels) {
+                char buf[512];
+                av_get_channel_layout_string(buf, sizeof(buf), -1, avctx->channel_layout);
+                av_log(avctx, AV_LOG_ERROR,
+                       "Channel layout '%s' with %d channels does not match number of specified channels %d\n",
+                       buf, channels, avctx->channels);
+                return AVERROR(EINVAL);
+            }
+        } else if (avctx->channel_layout) {
+            avctx->channels = av_get_channel_layout_nb_channels(avctx->channel_layout);
+        }
+        if (avctx->channels < 0) {
+            av_log(avctx, AV_LOG_ERROR, "Specified number of channels %d is not supported\n",
+                    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);
+            }
+        }
+        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);
+        }
+
+        if (!avctx->rc_initial_buffer_occupancy)
+            avctx->rc_initial_buffer_occupancy = avctx->rc_buffer_size * 3LL / 4;
+
+        if (avctx->ticks_per_frame && avctx->time_base.num &&
+            avctx->ticks_per_frame > INT_MAX / avctx->time_base.num) {
+            av_log(avctx, AV_LOG_ERROR,
+                   "ticks_per_frame %d too large for the timebase %d/%d.",
+                   avctx->ticks_per_frame,
+                   avctx->time_base.num,
+                   avctx->time_base.den);
+            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;
+}
diff --git a/libavcodec/encode.h b/libavcodec/encode.h
index dfa9cb2d97..9df5a13152 100644
--- a/libavcodec/encode.h
+++ b/libavcodec/encode.h
@@ -36,4 +36,10 @@ 
  */
 int ff_encode_get_frame(AVCodecContext *avctx, AVFrame *frame);
 
+/**
+ * Perform encoder initialization and validation.
+ * Called when opening the encoder, before the AVCodec.init() call.
+ */
+int ff_encode_preinit(AVCodecContext *avctx);
+
 #endif /* AVCODEC_ENCODE_H */
diff --git a/libavcodec/utils.c b/libavcodec/utils.c
index 01b603e2ea..9ccafb9e77 100644
--- a/libavcodec/utils.c
+++ b/libavcodec/utils.c
@@ -44,6 +44,7 @@ 
 #include "libavutil/thread.h"
 #include "avcodec.h"
 #include "decode.h"
+#include "encode.h"
 #include "hwconfig.h"
 #include "libavutil/opt.h"
 #include "mpegvideo.h"
@@ -546,7 +547,6 @@  int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *code
     int ret = 0;
     int codec_init_ok = 0;
     AVDictionary *tmp = NULL;
-    const AVPixFmtDescriptor *pixdesc;
     AVCodecInternal *avci;
 
     if (avcodec_is_open(avctx))
@@ -771,169 +771,9 @@  FF_ENABLE_DEPRECATION_WARNINGS
     }
 
     if (av_codec_is_encoder(avctx->codec)) {
-        int i;
-#if FF_API_CODED_FRAME
-FF_DISABLE_DEPRECATION_WARNINGS
-        avctx->coded_frame = av_frame_alloc();
-        if (!avctx->coded_frame) {
-            ret = AVERROR(ENOMEM);
-            goto free_and_end;
-        }
-FF_ENABLE_DEPRECATION_WARNINGS
-#endif
-
-        if (avctx->time_base.num <= 0 || avctx->time_base.den <= 0) {
-            av_log(avctx, AV_LOG_ERROR, "The encoder timebase is not set.\n");
-            ret = AVERROR(EINVAL);
-            goto free_and_end;
-        }
-
-        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])
-                    break;
-                if (avctx->channels == 1 &&
-                    av_get_planar_sample_fmt(avctx->sample_fmt) ==
-                    av_get_planar_sample_fmt(avctx->codec->sample_fmts[i])) {
-                    avctx->sample_fmt = avctx->codec->sample_fmts[i];
-                    break;
-                }
-            }
-            if (avctx->codec->sample_fmts[i] == AV_SAMPLE_FMT_NONE) {
-                char buf[128];
-                snprintf(buf, sizeof(buf), "%d", avctx->sample_fmt);
-                av_log(avctx, AV_LOG_ERROR, "Specified sample format %s is invalid or not supported\n",
-                       (char *)av_x_if_null(av_get_sample_fmt_name(avctx->sample_fmt), buf));
-                ret = AVERROR(EINVAL);
-                goto free_and_end;
-            }
-        }
-        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
-                && !((avctx->codec_id == AV_CODEC_ID_MJPEG || avctx->codec_id == AV_CODEC_ID_LJPEG)
-                     && avctx->strict_std_compliance <= FF_COMPLIANCE_UNOFFICIAL)) {
-                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));
-                ret = AVERROR(EINVAL);
-                goto free_and_end;
-            }
-            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])
-                    break;
-            if (avctx->codec->supported_samplerates[i] == 0) {
-                av_log(avctx, AV_LOG_ERROR, "Specified sample rate %d is not supported\n",
-                       avctx->sample_rate);
-                ret = AVERROR(EINVAL);
-                goto free_and_end;
-            }
-        }
-        if (avctx->sample_rate < 0) {
-            av_log(avctx, AV_LOG_ERROR, "Specified sample rate %d is not supported\n",
-                    avctx->sample_rate);
-            ret = AVERROR(EINVAL);
-            goto free_and_end;
-        }
-        if (avctx->codec->channel_layouts) {
-            if (!avctx->channel_layout) {
-                av_log(avctx, AV_LOG_WARNING, "Channel layout not specified\n");
-            } else {
-                for (i = 0; avctx->codec->channel_layouts[i] != 0; i++)
-                    if (avctx->channel_layout == avctx->codec->channel_layouts[i])
-                        break;
-                if (avctx->codec->channel_layouts[i] == 0) {
-                    char buf[512];
-                    av_get_channel_layout_string(buf, sizeof(buf), -1, avctx->channel_layout);
-                    av_log(avctx, AV_LOG_ERROR, "Specified channel layout '%s' is not supported\n", buf);
-                    ret = AVERROR(EINVAL);
-                    goto free_and_end;
-                }
-            }
-        }
-        if (avctx->channel_layout && avctx->channels) {
-            int channels = av_get_channel_layout_nb_channels(avctx->channel_layout);
-            if (channels != avctx->channels) {
-                char buf[512];
-                av_get_channel_layout_string(buf, sizeof(buf), -1, avctx->channel_layout);
-                av_log(avctx, AV_LOG_ERROR,
-                       "Channel layout '%s' with %d channels does not match number of specified channels %d\n",
-                       buf, channels, avctx->channels);
-                ret = AVERROR(EINVAL);
-                goto free_and_end;
-            }
-        } else if (avctx->channel_layout) {
-            avctx->channels = av_get_channel_layout_nb_channels(avctx->channel_layout);
-        }
-        if (avctx->channels < 0) {
-            av_log(avctx, AV_LOG_ERROR, "Specified number of channels %d is not supported\n",
-                    avctx->channels);
-            ret = AVERROR(EINVAL);
-            goto free_and_end;
-        }
-        if(avctx->codec_type == AVMEDIA_TYPE_VIDEO) {
-            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");
-                ret = AVERROR(EINVAL);
-                goto free_and_end;
-            }
-        }
-        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);
-        }
-
-        if (!avctx->rc_initial_buffer_occupancy)
-            avctx->rc_initial_buffer_occupancy = avctx->rc_buffer_size * 3LL / 4;
-
-        if (avctx->ticks_per_frame && avctx->time_base.num &&
-            avctx->ticks_per_frame > INT_MAX / avctx->time_base.num) {
-            av_log(avctx, AV_LOG_ERROR,
-                   "ticks_per_frame %d too large for the timebase %d/%d.",
-                   avctx->ticks_per_frame,
-                   avctx->time_base.num,
-                   avctx->time_base.den);
+        ret = ff_encode_preinit(avctx);
+        if (ret < 0)
             goto free_and_end;
-        }
-
-        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");
-                ret = AVERROR(EINVAL);
-                goto free_and_end;
-            }
-            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));
-                ret = AVERROR(EINVAL);
-                goto free_and_end;
-            }
-            avctx->sw_pix_fmt = frames_ctx->sw_format;
-        }
     }
 
     avctx->pts_correction_num_faulty_pts =