diff mbox series

[FFmpeg-devel] ffmpeg_opts: remove lowres check

Message ID 20210109174717.535-1-jamrial@gmail.com
State Accepted
Commit a423bc9dc294c32e6162b900b58b1cc2d3c3328d
Headers show
Series [FFmpeg-devel] ffmpeg_opts: remove lowres check
Related show

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

James Almer Jan. 9, 2021, 5:47 p.m. UTC
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(-)

Comments

James Almer Jan. 21, 2021, 1:56 a.m. UTC | #1
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.
Anton Khirnov Jan. 21, 2021, 12:59 p.m. UTC | #2
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.
James Almer Jan. 21, 2021, 1:29 p.m. UTC | #3
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.
Anton Khirnov Jan. 23, 2021, 6:17 p.m. UTC | #4
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?
James Almer Jan. 23, 2021, 6:38 p.m. UTC | #5
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.
Anton Khirnov Jan. 24, 2021, 1:44 p.m. UTC | #6
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 mbox series

Patch

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;