| Message ID | 20210109174717.535-1-jamrial@gmail.com |
|---|---|
| State | Accepted |
| Commit | a423bc9dc294c32e6162b900b58b1cc2d3c3328d |
| Headers | show |
| Series | [FFmpeg-devel] ffmpeg_opts: remove lowres check | expand |
| 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 |
On 1/9/2021 2:47 PM, James Almer wrote: > The st->codec values are updated based on the lowres factor by > avformat_find_stream_info() when it runs an instance of the decoder internally, > and the same thing happens in ffmpeg.c when we open ist->dec_ctx with > avcodec_open2(), so these assignments are redundant. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > This chunk here is not properly wrapped with the relevant pre-processor check > for AVStream->codec, and seeing it's ultimately redundant, i figured we might > as well delete it now. > > For that matter, the deprecation of lowres in avcodec.h is in a very strange > state (the field is not removed, its offset is changed instead). Once the value > of FF_API_LOWRES is flipped, neither the field, the AVOption, or the usage > within decoders will be removed, but some code in libavformat/utils.c will be > disabled, and that may result in unexpected behavior. > > fftools/ffmpeg_opt.c | 9 --------- > 1 file changed, 9 deletions(-) > > diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c > index c295514401..dec523d621 100644 > --- a/fftools/ffmpeg_opt.c > +++ b/fftools/ffmpeg_opt.c > @@ -867,15 +867,6 @@ static void add_input_streams(OptionsContext *o, AVFormatContext *ic) > case AVMEDIA_TYPE_VIDEO: > if(!ist->dec) > ist->dec = avcodec_find_decoder(par->codec_id); > -#if FF_API_LOWRES > - if (st->codec->lowres) { > - ist->dec_ctx->lowres = st->codec->lowres; > - ist->dec_ctx->width = st->codec->width; > - ist->dec_ctx->height = st->codec->height; > - ist->dec_ctx->coded_width = st->codec->coded_width; > - ist->dec_ctx->coded_height = st->codec->coded_height; > - } > -#endif > > // avformat_find_stream_info() doesn't set this for us anymore. > ist->dec_ctx->framerate = st->avg_frame_rate; Will apply soon.
Quoting James Almer (2021-01-09 18:47:17) > The st->codec values are updated based on the lowres factor by > avformat_find_stream_info() when it runs an instance of the decoder internally, > and the same thing happens in ffmpeg.c when we open ist->dec_ctx with > avcodec_open2(), so these assignments are redundant. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > This chunk here is not properly wrapped with the relevant pre-processor check > for AVStream->codec, and seeing it's ultimately redundant, i figured we might > as well delete it now. > > For that matter, the deprecation of lowres in avcodec.h is in a very strange > state (the field is not removed, its offset is changed instead). Once the value > of FF_API_LOWRES is flipped, neither the field, the AVOption, or the usage > within decoders will be removed, but some code in libavformat/utils.c will be > disabled, and that may result in unexpected behavior. IMO it should just be made a codec-private option. And lavf has no business treating it specially. Patch fine with me.
On 1/21/2021 9:59 AM, Anton Khirnov wrote: > Quoting James Almer (2021-01-09 18:47:17) >> The st->codec values are updated based on the lowres factor by >> avformat_find_stream_info() when it runs an instance of the decoder internally, >> and the same thing happens in ffmpeg.c when we open ist->dec_ctx with >> avcodec_open2(), so these assignments are redundant. >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> This chunk here is not properly wrapped with the relevant pre-processor check >> for AVStream->codec, and seeing it's ultimately redundant, i figured we might >> as well delete it now. >> >> For that matter, the deprecation of lowres in avcodec.h is in a very strange >> state (the field is not removed, its offset is changed instead). Once the value >> of FF_API_LOWRES is flipped, neither the field, the AVOption, or the usage >> within decoders will be removed, but some code in libavformat/utils.c will be >> disabled, and that may result in unexpected behavior. > > IMO it should just be made a codec-private option. And lavf has no > business treating it specially. Technically speaking, there's no reason for lavf to check for lowres to do what it's currently doing. The code > int orig_w = st->codecpar->width; > int orig_h = st->codecpar->height; > ret = avcodec_parameters_from_context(st->codecpar, st->internal->avctx); > if (ret < 0) > goto find_stream_info_err; > ret = add_coded_side_data(st, st->internal->avctx); > if (ret < 0) > goto find_stream_info_err; > #if FF_API_LOWRES > // The decoder might reduce the video size by the lowres factor. > if (st->internal->avctx->lowres && orig_w) { > st->codecpar->width = orig_w; > st->codecpar->height = orig_h; > } > #endif Could be simplified to unconditionally set st->codecpar dimensions to the backed up ones. If you agree, i can send a patch. So based on the above, turning it into a codec specific option sounds good to me. We could repeat the FF_API_PRIVATE_OPT deprecation mechanism for it, but the global option/field needs to stay for another two years since neither were truly deprecated (same for AVCodec->max_lowres), just wrapped in a pre-processor check. > > Patch fine with me. > Pushed, thanks.
Quoting James Almer (2021-01-21 14:29:22) > On 1/21/2021 9:59 AM, Anton Khirnov wrote: > > Quoting James Almer (2021-01-09 18:47:17) > >> The st->codec values are updated based on the lowres factor by > >> avformat_find_stream_info() when it runs an instance of the decoder internally, > >> and the same thing happens in ffmpeg.c when we open ist->dec_ctx with > >> avcodec_open2(), so these assignments are redundant. > >> > >> Signed-off-by: James Almer <jamrial@gmail.com> > >> --- > >> This chunk here is not properly wrapped with the relevant pre-processor check > >> for AVStream->codec, and seeing it's ultimately redundant, i figured we might > >> as well delete it now. > >> > >> For that matter, the deprecation of lowres in avcodec.h is in a very strange > >> state (the field is not removed, its offset is changed instead). Once the value > >> of FF_API_LOWRES is flipped, neither the field, the AVOption, or the usage > >> within decoders will be removed, but some code in libavformat/utils.c will be > >> disabled, and that may result in unexpected behavior. > > > > IMO it should just be made a codec-private option. And lavf has no > > business treating it specially. > > Technically speaking, there's no reason for lavf to check for lowres to > do what it's currently doing. > > The code > > > int orig_w = st->codecpar->width; > > int orig_h = st->codecpar->height; > > ret = avcodec_parameters_from_context(st->codecpar, st->internal->avctx); > > if (ret < 0) > > goto find_stream_info_err; > > ret = add_coded_side_data(st, st->internal->avctx); > > if (ret < 0) > > goto find_stream_info_err; > > #if FF_API_LOWRES > > // The decoder might reduce the video size by the lowres factor. > > if (st->internal->avctx->lowres && orig_w) { > > st->codecpar->width = orig_w; > > st->codecpar->height = orig_h; > > } > > #endif > > Could be simplified to unconditionally set st->codecpar dimensions to > the backed up ones. If you agree, i can send a patch. I'm not sure that won't have other side effects - the decoder might have different ideas about dimensions than the demuxer, which might change something in some obscure cases. I guess try and see if FATE passes?
On 1/23/2021 3:17 PM, Anton Khirnov wrote: > Quoting James Almer (2021-01-21 14:29:22) >> On 1/21/2021 9:59 AM, Anton Khirnov wrote: >>> Quoting James Almer (2021-01-09 18:47:17) >>>> The st->codec values are updated based on the lowres factor by >>>> avformat_find_stream_info() when it runs an instance of the decoder internally, >>>> and the same thing happens in ffmpeg.c when we open ist->dec_ctx with >>>> avcodec_open2(), so these assignments are redundant. >>>> >>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>> --- >>>> This chunk here is not properly wrapped with the relevant pre-processor check >>>> for AVStream->codec, and seeing it's ultimately redundant, i figured we might >>>> as well delete it now. >>>> >>>> For that matter, the deprecation of lowres in avcodec.h is in a very strange >>>> state (the field is not removed, its offset is changed instead). Once the value >>>> of FF_API_LOWRES is flipped, neither the field, the AVOption, or the usage >>>> within decoders will be removed, but some code in libavformat/utils.c will be >>>> disabled, and that may result in unexpected behavior. >>> >>> IMO it should just be made a codec-private option. And lavf has no >>> business treating it specially. >> >> Technically speaking, there's no reason for lavf to check for lowres to >> do what it's currently doing. >> >> The code >> >>> int orig_w = st->codecpar->width; >>> int orig_h = st->codecpar->height; >>> ret = avcodec_parameters_from_context(st->codecpar, st->internal->avctx); >>> if (ret < 0) >>> goto find_stream_info_err; >>> ret = add_coded_side_data(st, st->internal->avctx); >>> if (ret < 0) >>> goto find_stream_info_err; >>> #if FF_API_LOWRES >>> // The decoder might reduce the video size by the lowres factor. >>> if (st->internal->avctx->lowres && orig_w) { >>> st->codecpar->width = orig_w; >>> st->codecpar->height = orig_h; >>> } >>> #endif >> >> Could be simplified to unconditionally set st->codecpar dimensions to >> the backed up ones. If you agree, i can send a patch. > > I'm not sure that won't have other side effects - the decoder might have > different ideas about dimensions than the demuxer, which might change > something in some obscure cases. I guess try and see if FATE passes? If i apply the following, the output of three remuxing (codec copy) tests change > diff --git a/libavformat/utils.c b/libavformat/utils.c > index 8ac6bc04b8..0cdf3156a6 100644 > --- a/libavformat/utils.c > +++ b/libavformat/utils.c > @@ -4110,13 +4110,12 @@ FF_ENABLE_DEPRECATION_WARNINGS > ret = add_coded_side_data(st, st->internal->avctx); > if (ret < 0) > goto find_stream_info_err; > -#if FF_API_LOWRES > - // The decoder might reduce the video size by the lowres factor. > - if (st->internal->avctx->lowres && orig_w) { > + > + // The decoder might change the video size. > + if (orig_w && orig_h) { > st->codecpar->width = orig_w; > st->codecpar->height = orig_h; > } > -#endif > } What i assume happens is that without this change st->codecpar is being populated with dimensions set by whatever decoder was used during probing, and then propagated to the muxer codecpar, whereas with this change the dimensions from the source container are kept unchanged. Case in point > diff --git a/tests/ref/fate/cbs-vp9-vp90-2-05-resize b/tests/ref/fate/cbs-vp9-vp90-2-05-resize > index 8f036bba81..37a37ff1ea 100644 > --- a/tests/ref/fate/cbs-vp9-vp90-2-05-resize > +++ b/tests/ref/fate/cbs-vp9-vp90-2-05-resize > @@ -1 +1 @@ > -6838422ebb45df353a2bad62b9aff8e9 > +1c39300b93fe110e1db30974e5d3479d > diff --git a/tests/ref/fate/redcode-demux b/tests/ref/fate/redcode-demux > index 45119ec71e..c6e0b6de5c 100644 > --- a/tests/ref/fate/redcode-demux > +++ b/tests/ref/fate/redcode-demux > @@ -1,7 +1,7 @@ > #tb 0: 1/240000 > #media_type 0: video > #codec_id 0: jpeg2000 > -#dimensions 0: 2048x1152 > +#dimensions 0: 4096x2304 > #sar 0: 0/1 > #tb 1: 1/240000 > #media_type 1: audio > diff --git a/tests/ref/fate/wtv-demux b/tests/ref/fate/wtv-demux > index abe85a4ab6..39d395699c 100644 > --- a/tests/ref/fate/wtv-demux > +++ b/tests/ref/fate/wtv-demux > @@ -3,7 +3,7 @@ > #tb 0: 1/10000000 > #media_type 0: video > #codec_id 0: mpeg2video > -#dimensions 0: 720x576 > +#dimensions 0: 704x480 > #sar 0: 64/45 > #tb 1: 1/10000000 > #media_type 1: audio > -- I personally think that for a codec copy scenario (Where you could have no decoders at all), this behavior is more consistent. Some samples have different resolution per frame, like that vp9 one, and a decoder could return the one from the last frame probed.
Quoting James Almer (2021-01-23 19:38:57) > > If i apply the following, the output of three remuxing (codec copy) > tests change > > > diff --git a/libavformat/utils.c b/libavformat/utils.c > > index 8ac6bc04b8..0cdf3156a6 100644 > > --- a/libavformat/utils.c > > +++ b/libavformat/utils.c > > @@ -4110,13 +4110,12 @@ FF_ENABLE_DEPRECATION_WARNINGS > > ret = add_coded_side_data(st, st->internal->avctx); > > if (ret < 0) > > goto find_stream_info_err; > > -#if FF_API_LOWRES > > - // The decoder might reduce the video size by the lowres factor. > > - if (st->internal->avctx->lowres && orig_w) { > > + > > + // The decoder might change the video size. > > + if (orig_w && orig_h) { > > st->codecpar->width = orig_w; > > st->codecpar->height = orig_h; > > } > > -#endif > > } > > What i assume happens is that without this change st->codecpar is being > populated with dimensions set by whatever decoder was used during > probing, and then propagated to the muxer codecpar, whereas with this > change the dimensions from the source container are kept unchanged. > > Case in point > > > diff --git a/tests/ref/fate/cbs-vp9-vp90-2-05-resize b/tests/ref/fate/cbs-vp9-vp90-2-05-resize > > index 8f036bba81..37a37ff1ea 100644 > > --- a/tests/ref/fate/cbs-vp9-vp90-2-05-resize > > +++ b/tests/ref/fate/cbs-vp9-vp90-2-05-resize > > @@ -1 +1 @@ > > -6838422ebb45df353a2bad62b9aff8e9 > > +1c39300b93fe110e1db30974e5d3479d > > diff --git a/tests/ref/fate/redcode-demux b/tests/ref/fate/redcode-demux > > index 45119ec71e..c6e0b6de5c 100644 > > --- a/tests/ref/fate/redcode-demux > > +++ b/tests/ref/fate/redcode-demux > > @@ -1,7 +1,7 @@ > > #tb 0: 1/240000 > > #media_type 0: video > > #codec_id 0: jpeg2000 > > -#dimensions 0: 2048x1152 > > +#dimensions 0: 4096x2304 > > #sar 0: 0/1 > > #tb 1: 1/240000 > > #media_type 1: audio > > diff --git a/tests/ref/fate/wtv-demux b/tests/ref/fate/wtv-demux > > index abe85a4ab6..39d395699c 100644 > > --- a/tests/ref/fate/wtv-demux > > +++ b/tests/ref/fate/wtv-demux > > @@ -3,7 +3,7 @@ > > #tb 0: 1/10000000 > > #media_type 0: video > > #codec_id 0: mpeg2video > > -#dimensions 0: 720x576 > > +#dimensions 0: 704x480 > > #sar 0: 64/45 > > #tb 1: 1/10000000 > > #media_type 1: audio > > -- > > I personally think that for a codec copy scenario (Where you could have > no decoders at all), this behavior is more consistent. Some samples have > different resolution per frame, like that vp9 one, and a decoder could > return the one from the last frame probed. I tend to agree, though in the specific case of wtv-demux the container dimensions seem broken. But then again I don't think it's the job of find_stream_info() to engage in such heuristics.
diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c index c295514401..dec523d621 100644 --- a/fftools/ffmpeg_opt.c +++ b/fftools/ffmpeg_opt.c @@ -867,15 +867,6 @@ static void add_input_streams(OptionsContext *o, AVFormatContext *ic) case AVMEDIA_TYPE_VIDEO: if(!ist->dec) ist->dec = avcodec_find_decoder(par->codec_id); -#if FF_API_LOWRES - if (st->codec->lowres) { - ist->dec_ctx->lowres = st->codec->lowres; - ist->dec_ctx->width = st->codec->width; - ist->dec_ctx->height = st->codec->height; - ist->dec_ctx->coded_width = st->codec->coded_width; - ist->dec_ctx->coded_height = st->codec->coded_height; - } -#endif // avformat_find_stream_info() doesn't set this for us anymore. ist->dec_ctx->framerate = st->avg_frame_rate;
The st->codec values are updated based on the lowres factor by avformat_find_stream_info() when it runs an instance of the decoder internally, and the same thing happens in ffmpeg.c when we open ist->dec_ctx with avcodec_open2(), so these assignments are redundant. Signed-off-by: James Almer <jamrial@gmail.com> --- This chunk here is not properly wrapped with the relevant pre-processor check for AVStream->codec, and seeing it's ultimately redundant, i figured we might as well delete it now. For that matter, the deprecation of lowres in avcodec.h is in a very strange state (the field is not removed, its offset is changed instead). Once the value of FF_API_LOWRES is flipped, neither the field, the AVOption, or the usage within decoders will be removed, but some code in libavformat/utils.c will be disabled, and that may result in unexpected behavior. fftools/ffmpeg_opt.c | 9 --------- 1 file changed, 9 deletions(-)