| Message ID | GV1P250MB07378A419F80D397A8C53F338F022@GV1P250MB0737.EURP250.PROD.OUTLOOK.COM |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/2] avcodec/mpegvideo_enc: Reject input incompatible with chroma subsampling | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Andreas Rheinhardt: > Fixes ticket #10952. > > Discovered by: Zeng Yunxiang > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > I am pretty sure that a lot of other encoders don't handle this well > either. Maybe we should handle this more generically in > ff_encode_preinit? > > libavcodec/mpegvideo_enc.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/libavcodec/mpegvideo_enc.c b/libavcodec/mpegvideo_enc.c > index d1b1917824..a65ecc6839 100644 > --- a/libavcodec/mpegvideo_enc.c > +++ b/libavcodec/mpegvideo_enc.c > @@ -314,6 +314,7 @@ av_cold int ff_mpv_encode_init(AVCodecContext *avctx) > AVCPBProperties *cpb_props; > int i, ret; > int mb_array_size, mv_table_size; > + int chroma_h_subsampling = 1, chroma_v_subsampling = 1; > > mpv_encode_defaults(s); > > @@ -325,14 +326,25 @@ av_cold int ff_mpv_encode_init(AVCodecContext *avctx) > case AV_PIX_FMT_YUVJ422P: > case AV_PIX_FMT_YUV422P: > s->chroma_format = CHROMA_422; > + chroma_h_subsampling = 2; > break; > case AV_PIX_FMT_YUVJ420P: > case AV_PIX_FMT_YUV420P: > default: > s->chroma_format = CHROMA_420; > + chroma_h_subsampling = 2; > + chroma_v_subsampling = 2; > break; > } > > + if (avctx->width & (chroma_h_subsampling - 1) || > + avctx->height & (chroma_v_subsampling - 1)) { > + av_log(avctx, AV_LOG_ERROR, > + "Dimensions %dx%d incompatible with chroma subsampling.\n", > + avctx->width, avctx->height); > + return AVERROR(EINVAL); > + } > + > avctx->bits_per_raw_sample = av_clip(avctx->bits_per_raw_sample, 0, 8); > > s->bit_rate = avctx->bit_rate; Will apply this patchset tomorrow unless there are objections. - Andreas
Quoting Andreas Rheinhardt (2024-04-06 12:23:49) > Fixes ticket #10952. > > Discovered by: Zeng Yunxiang > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > I am pretty sure that a lot of other encoders don't handle this well > either. Maybe we should handle this more generically in > ff_encode_preinit? We should.
On Mon, Apr 08, 2024 at 12:51:08AM +0200, Andreas Rheinhardt wrote: > Andreas Rheinhardt: > > Fixes ticket #10952. > > > > Discovered by: Zeng Yunxiang > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > > --- > > I am pretty sure that a lot of other encoders don't handle this well > > either. Maybe we should handle this more generically in > > ff_encode_preinit? > > > > libavcodec/mpegvideo_enc.c | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/libavcodec/mpegvideo_enc.c b/libavcodec/mpegvideo_enc.c > > index d1b1917824..a65ecc6839 100644 > > --- a/libavcodec/mpegvideo_enc.c > > +++ b/libavcodec/mpegvideo_enc.c > > @@ -314,6 +314,7 @@ av_cold int ff_mpv_encode_init(AVCodecContext *avctx) > > AVCPBProperties *cpb_props; > > int i, ret; > > int mb_array_size, mv_table_size; > > + int chroma_h_subsampling = 1, chroma_v_subsampling = 1; > > > > mpv_encode_defaults(s); > > > > @@ -325,14 +326,25 @@ av_cold int ff_mpv_encode_init(AVCodecContext *avctx) > > case AV_PIX_FMT_YUVJ422P: > > case AV_PIX_FMT_YUV422P: > > s->chroma_format = CHROMA_422; > > + chroma_h_subsampling = 2; > > break; > > case AV_PIX_FMT_YUVJ420P: > > case AV_PIX_FMT_YUV420P: > > default: > > s->chroma_format = CHROMA_420; > > + chroma_h_subsampling = 2; > > + chroma_v_subsampling = 2; > > break; > > } > > > > + if (avctx->width & (chroma_h_subsampling - 1) || > > + avctx->height & (chroma_v_subsampling - 1)) { > > + av_log(avctx, AV_LOG_ERROR, > > + "Dimensions %dx%d incompatible with chroma subsampling.\n", > > + avctx->width, avctx->height); > > + return AVERROR(EINVAL); > > + } > > + > > avctx->bits_per_raw_sample = av_clip(avctx->bits_per_raw_sample, 0, 8); > > > > s->bit_rate = avctx->bit_rate; > > Will apply this patchset tomorrow unless there are objections. this breaks ./ffmpeg -i 'samples.multimedia.cx/game-formats/sierra-vmd/lastdynasty/HG060808.VMD' -y test.avi [...]
On Mon, Apr 08, 2024 at 12:51:08AM +0200, Andreas Rheinhardt wrote: > Andreas Rheinhardt: > > Fixes ticket #10952. > > > > Discovered by: Zeng Yunxiang > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > > --- > > I am pretty sure that a lot of other encoders don't handle this well > > either. Maybe we should handle this more generically in > > ff_encode_preinit? > > > > libavcodec/mpegvideo_enc.c | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/libavcodec/mpegvideo_enc.c b/libavcodec/mpegvideo_enc.c > > index d1b1917824..a65ecc6839 100644 > > --- a/libavcodec/mpegvideo_enc.c > > +++ b/libavcodec/mpegvideo_enc.c > > @@ -314,6 +314,7 @@ av_cold int ff_mpv_encode_init(AVCodecContext *avctx) > > AVCPBProperties *cpb_props; > > int i, ret; > > int mb_array_size, mv_table_size; > > + int chroma_h_subsampling = 1, chroma_v_subsampling = 1; > > > > mpv_encode_defaults(s); > > > > @@ -325,14 +326,25 @@ av_cold int ff_mpv_encode_init(AVCodecContext *avctx) > > case AV_PIX_FMT_YUVJ422P: > > case AV_PIX_FMT_YUV422P: > > s->chroma_format = CHROMA_422; > > + chroma_h_subsampling = 2; > > break; > > case AV_PIX_FMT_YUVJ420P: > > case AV_PIX_FMT_YUV420P: > > default: > > s->chroma_format = CHROMA_420; > > + chroma_h_subsampling = 2; > > + chroma_v_subsampling = 2; > > break; > > } > > > > + if (avctx->width & (chroma_h_subsampling - 1) || > > + avctx->height & (chroma_v_subsampling - 1)) { > > + av_log(avctx, AV_LOG_ERROR, > > + "Dimensions %dx%d incompatible with chroma subsampling.\n", > > + avctx->width, avctx->height); > > + return AVERROR(EINVAL); > > + } > > + > > avctx->bits_per_raw_sample = av_clip(avctx->bits_per_raw_sample, 0, 8); > > > > s->bit_rate = avctx->bit_rate; > > Will apply this patchset tomorrow unless there are objections. Also please dont apply stuff like this with a 1 day warning, i just saw this now dropping support for odd resolutions is a major change and not ok thanks [...]
Quoting Michael Niedermayer (2024-04-08 18:07:31) > Also please dont apply stuff like this with a 1 day warning, i just saw this now > > dropping support for odd resolutions is a major change and not ok Maybe such things would happen less frequently if your private testsuite was less private.
diff --git a/libavcodec/mpegvideo_enc.c b/libavcodec/mpegvideo_enc.c index d1b1917824..a65ecc6839 100644 --- a/libavcodec/mpegvideo_enc.c +++ b/libavcodec/mpegvideo_enc.c @@ -314,6 +314,7 @@ av_cold int ff_mpv_encode_init(AVCodecContext *avctx) AVCPBProperties *cpb_props; int i, ret; int mb_array_size, mv_table_size; + int chroma_h_subsampling = 1, chroma_v_subsampling = 1; mpv_encode_defaults(s); @@ -325,14 +326,25 @@ av_cold int ff_mpv_encode_init(AVCodecContext *avctx) case AV_PIX_FMT_YUVJ422P: case AV_PIX_FMT_YUV422P: s->chroma_format = CHROMA_422; + chroma_h_subsampling = 2; break; case AV_PIX_FMT_YUVJ420P: case AV_PIX_FMT_YUV420P: default: s->chroma_format = CHROMA_420; + chroma_h_subsampling = 2; + chroma_v_subsampling = 2; break; } + if (avctx->width & (chroma_h_subsampling - 1) || + avctx->height & (chroma_v_subsampling - 1)) { + av_log(avctx, AV_LOG_ERROR, + "Dimensions %dx%d incompatible with chroma subsampling.\n", + avctx->width, avctx->height); + return AVERROR(EINVAL); + } + avctx->bits_per_raw_sample = av_clip(avctx->bits_per_raw_sample, 0, 8); s->bit_rate = avctx->bit_rate;
Fixes ticket #10952. Discovered by: Zeng Yunxiang Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- I am pretty sure that a lot of other encoders don't handle this well either. Maybe we should handle this more generically in ff_encode_preinit? libavcodec/mpegvideo_enc.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)