diff mbox series

[FFmpeg-devel,05/18] lavc/dv: remove ff_dvvideo_init()

Message ID 20220824084318.333-5-anton@khirnov.net
State New
Headers show
Series [FFmpeg-devel,01/18] tests/fate/mov: add a test for dv audio demuxed through dv demuxer | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 fail Make fate failed
andriy/make_x86 success Make finished
andriy/make_fate_x86 fail Make fate failed

Commit Message

Anton Khirnov Aug. 24, 2022, 8:43 a.m. UTC 
The function contains only two assignments, which are simpler to
duplicate into decoder and encoder. Additionally, dvdec does not use
DVVideoContext.avctx at all.
---
 libavcodec/dv.c    | 10 ----------
 libavcodec/dv.h    |  2 --
 libavcodec/dvdec.c |  4 +++-
 libavcodec/dvenc.c |  5 ++++-
 4 files changed, 7 insertions(+), 14 deletions(-)

Comments

Andreas Rheinhardt Aug. 24, 2022, 12:27 p.m. UTC | #1 
Anton Khirnov:
> The function contains only two assignments, which are simpler to
> duplicate into decoder and encoder. Additionally, dvdec does not use
> DVVideoContext.avctx at all.
> ---
>  libavcodec/dv.c    | 10 ----------
>  libavcodec/dv.h    |  2 --
>  libavcodec/dvdec.c |  4 +++-
>  libavcodec/dvenc.c |  5 ++++-
>  4 files changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/libavcodec/dv.c b/libavcodec/dv.c
> index e2550c4cc1..9e05aa8927 100644
> --- a/libavcodec/dv.c
> +++ b/libavcodec/dv.c
> @@ -184,13 +184,3 @@ int ff_dv_init_dynamic_tables(DVVideoContext *ctx, const AVDVProfile *d)
>  
>      return 0;
>  }
> -
> -av_cold int ff_dvvideo_init(AVCodecContext *avctx)
> -{
> -    DVVideoContext *s = avctx->priv_data;
> -
> -    s->avctx = avctx;
> -    avctx->chroma_sample_location = AVCHROMA_LOC_TOPLEFT;
> -
> -    return 0;
> -}
> diff --git a/libavcodec/dv.h b/libavcodec/dv.h
> index 331b8e846a..2b082d0140 100644
> --- a/libavcodec/dv.h
> +++ b/libavcodec/dv.h
> @@ -97,8 +97,6 @@ enum dv_pack_type {
>  
>  int ff_dv_init_dynamic_tables(DVVideoContext *s, const AVDVProfile *d);
>  
> -int ff_dvvideo_init(AVCodecContext *avctx);
> -
>  static inline int dv_work_pool_size(const AVDVProfile *d)
>  {
>      int size = d->n_difchan * d->difseg_size * 27;
> diff --git a/libavcodec/dvdec.c b/libavcodec/dvdec.c
> index bad8419925..daee2347e6 100644
> --- a/libavcodec/dvdec.c
> +++ b/libavcodec/dvdec.c
> @@ -240,6 +240,8 @@ static av_cold int dvvideo_decode_init(AVCodecContext *avctx)
>      DVVideoContext *s = avctx->priv_data;
>      int i;
>  
> +    avctx->chroma_sample_location = AVCHROMA_LOC_TOPLEFT;
> +
>      ff_idctdsp_init(&s->idsp, avctx);
>  
>      for (i = 0; i < 64; i++)
> @@ -258,7 +260,7 @@ static av_cold int dvvideo_decode_init(AVCodecContext *avctx)
>  
>      ff_thread_once(&init_static_once, dv_init_static);
>  
> -    return ff_dvvideo_init(avctx);
> +    return 0;
>  }
>  
>  /* decode AC coefficients */
> diff --git a/libavcodec/dvenc.c b/libavcodec/dvenc.c
> index 5ba4de3213..8027feb9b3 100644
> --- a/libavcodec/dvenc.c
> +++ b/libavcodec/dvenc.c
> @@ -55,6 +55,9 @@ static av_cold int dvvideo_encode_init(AVCodecContext *avctx)
>      PixblockDSPContext pdsp;
>      int ret;
>  
> +    s->avctx = avctx;
> +    avctx->chroma_sample_location = AVCHROMA_LOC_TOPLEFT;
> +
>      s->sys = av_dv_codec_profile2(avctx->width, avctx->height, avctx->pix_fmt, avctx->time_base);
>      if (!s->sys) {
>          av_log(avctx, AV_LOG_ERROR, "Found no DV profile for %ix%i %s video. "
> @@ -91,7 +94,7 @@ static av_cold int dvvideo_encode_init(AVCodecContext *avctx)
>      }
>  #endif
>  
> -    return ff_dvvideo_init(avctx);
> +    return 0;
>  }
>  
>  /* bit budget for AC only in 5 MBs */

There is actually another issue here: Encoders are not supposed to set
chroma_sample_location at all according to the documentation. I sent a
patch to implement this
(https://ffmpeg.org/pipermail/ffmpeg-devel/2022-July/298518.html -- it
also inlines and removes ff_dvvideo_init()), but I am unsure whether it
is not the documentation that needs to be updated.

- Andreas
Anton Khirnov Aug. 25, 2022, 9:48 a.m. UTC | #2 
Quoting Andreas Rheinhardt (2022-08-24 14:27:43)
> There is actually another issue here: Encoders are not supposed to set
> chroma_sample_location at all according to the documentation. I sent a
> patch to implement this
> (https://ffmpeg.org/pipermail/ffmpeg-devel/2022-July/298518.html -- it
> also inlines and removes ff_dvvideo_init()), but I am unsure whether it
> is not the documentation that needs to be updated.

I would say update the documentation. It does make sense for encoders to
set this field.
Anton Khirnov Aug. 25, 2022, 9:55 a.m. UTC | #3 
Quoting Anton Khirnov (2022-08-25 11:48:27)
> Quoting Andreas Rheinhardt (2022-08-24 14:27:43)
> > There is actually another issue here: Encoders are not supposed to set
> > chroma_sample_location at all according to the documentation. I sent a
> > patch to implement this
> > (https://ffmpeg.org/pipermail/ffmpeg-devel/2022-July/298518.html -- it
> > also inlines and removes ff_dvvideo_init()), but I am unsure whether it
> > is not the documentation that needs to be updated.
> 
> I would say update the documentation. It does make sense for encoders to
> set this field.

Or on second thought, maybe no. Apparently no other encoders set it.
Though we might want to check whether the value matches and warn/error
out otherwise.
diff mbox series

Patch

diff --git a/libavcodec/dv.c b/libavcodec/dv.c
index e2550c4cc1..9e05aa8927 100644
--- a/libavcodec/dv.c
+++ b/libavcodec/dv.c
@@ -184,13 +184,3 @@  int ff_dv_init_dynamic_tables(DVVideoContext *ctx, const AVDVProfile *d)
 
     return 0;
 }
-
-av_cold int ff_dvvideo_init(AVCodecContext *avctx)
-{
-    DVVideoContext *s = avctx->priv_data;
-
-    s->avctx = avctx;
-    avctx->chroma_sample_location = AVCHROMA_LOC_TOPLEFT;
-
-    return 0;
-}
diff --git a/libavcodec/dv.h b/libavcodec/dv.h
index 331b8e846a..2b082d0140 100644
--- a/libavcodec/dv.h
+++ b/libavcodec/dv.h
@@ -97,8 +97,6 @@  enum dv_pack_type {
 
 int ff_dv_init_dynamic_tables(DVVideoContext *s, const AVDVProfile *d);
 
-int ff_dvvideo_init(AVCodecContext *avctx);
-
 static inline int dv_work_pool_size(const AVDVProfile *d)
 {
     int size = d->n_difchan * d->difseg_size * 27;
diff --git a/libavcodec/dvdec.c b/libavcodec/dvdec.c
index bad8419925..daee2347e6 100644
--- a/libavcodec/dvdec.c
+++ b/libavcodec/dvdec.c
@@ -240,6 +240,8 @@  static av_cold int dvvideo_decode_init(AVCodecContext *avctx)
     DVVideoContext *s = avctx->priv_data;
     int i;
 
+    avctx->chroma_sample_location = AVCHROMA_LOC_TOPLEFT;
+
     ff_idctdsp_init(&s->idsp, avctx);
 
     for (i = 0; i < 64; i++)
@@ -258,7 +260,7 @@  static av_cold int dvvideo_decode_init(AVCodecContext *avctx)
 
     ff_thread_once(&init_static_once, dv_init_static);
 
-    return ff_dvvideo_init(avctx);
+    return 0;
 }
 
 /* decode AC coefficients */
diff --git a/libavcodec/dvenc.c b/libavcodec/dvenc.c
index 5ba4de3213..8027feb9b3 100644
--- a/libavcodec/dvenc.c
+++ b/libavcodec/dvenc.c
@@ -55,6 +55,9 @@  static av_cold int dvvideo_encode_init(AVCodecContext *avctx)
     PixblockDSPContext pdsp;
     int ret;
 
+    s->avctx = avctx;
+    avctx->chroma_sample_location = AVCHROMA_LOC_TOPLEFT;
+
     s->sys = av_dv_codec_profile2(avctx->width, avctx->height, avctx->pix_fmt, avctx->time_base);
     if (!s->sys) {
         av_log(avctx, AV_LOG_ERROR, "Found no DV profile for %ix%i %s video. "
@@ -91,7 +94,7 @@  static av_cold int dvvideo_encode_init(AVCodecContext *avctx)
     }
 #endif
 
-    return ff_dvvideo_init(avctx);
+    return 0;
 }
 
 /* bit budget for AC only in 5 MBs */