[FFmpeg-devel] [PATCH] ffmpeg_opts: remove lowres check

[James Almer]

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 at 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.