[FFmpeg-devel] [PATCH 2/3] ffmpeg.c: refine picking default video stream

[James Almer]

On 10/19/2020 11:42 AM, Anton Khirnov wrote:
> Quoting James Almer (2020-10-14 19:26:51)
>> On 10/14/2020 2:09 PM, Michael Niedermayer wrote:
>>> On Wed, Oct 14, 2020 at 10:53:10AM +0200, Anton Khirnov wrote:
>>>> Use a floating-point score value to take into account bitrate, when
>>>> multiple streams with the same resolution are present.
>>>>
>>>> Stop accessing private AVStream.codec_info_nb_frames field, as the
>>>> sample in question
>>>> http://samples.ffmpeg.org/ffmpeg-bugs/trac/ticket2639/Thailand-Wave.wmv
>>>> is now handled by the above change.
>>>> ---
>>>>  fftools/ffmpeg_opt.c | 23 ++++++++++++++++++-----
>>>>  1 file changed, 18 insertions(+), 5 deletions(-)
>>>
>>> Breaks:
>>> -i tickets/4496/08_lect_01.rm  file.avi
>>>
>>> as refernce here is what ffmpeg shows about the streams:
>>>     Stream #0:0: Audio: sipr (sipr / 0x72706973), 8000 Hz, mono, flt, 4 kb/s
>>>     Stream #0:1: Audio: sipr (sipr / 0x72706973), 8000 Hz, mono, flt, 8 kb/s
>>>     Stream #0:2: Audio: cook (cook / 0x6B6F6F63), 22050 Hz, mono, fltp, 4 kb/s
>>>     Stream #0:3: Audio: sipr (sipr / 0x72706973), 8000 Hz, mono, flt, 4 kb/s
>>>     Stream #0:4: Video: rv20 (RV20 / 0x30325652), yuv420p, 320x240, 252 kb/s, 15 fps, 15 tbr, 1k tbn, 1k tbc
>>>     Stream #0:5: Video: rv20 (RV20 / 0x30325652), yuv420p, 320x240, 252 kb/s, 15 fps, 15 tbr, 1k tbn, 1k tbc
>>>     Stream #0:6: Video: rv20 (RV20 / 0x30325652), yuv420p, 320x240, 252 kb/s, 15 fps, 15 tbr, 1k tbn, 1k tbc
>>>     Stream #0:7: Video: rv20 (RV20 / 0x30325652), yuv420p, 320x240, 252 kb/s, 15 fps, 15 tbr, 1k tbn, 1k tbc
>>>     Stream #0:8: Video: rv20 (RV20 / 0x30325652), yuv420p, 320x240, 252 kb/s, 15 fps, 15 tbr, 1k tbn, 1k tbc
>>>     Stream #0:9: Video: rv20 (RV20 / 0x30325652), yuv420p, 320x240, 252 kb/s, 15 fps, 15 tbr, 1k tbn, 1k tbc
>>>     Stream #0:10: Video: rv20 (RV20 / 0x30325652), yuv420p, 320x240, 252 kb/s, 15 fps, 15 tbr, 1k tbn, 1k tbc
>>>     Stream #0:11: Video: rv20 (RV20 / 0x30325652), yuv420p, 320x240, 252 kb/s, 15 fps, 15 tbr, 1k tbn, 1k tbc
>>>     Stream #0:12: Video: rv20 (RV20 / 0x30325652), yuv420p, 320x240, 252 kb/s, 15 fps, 15 tbr, 1k tbn, 1k tbc
>>>     Stream #0:13: Video: rv20 (RV20 / 0x30325652), yuv420p, 320x240, 252 kb/s, 15 fps, 15 tbr, 1k tbn, 1k tbc
>>>
>>> sample should be here according to the ticket:
>>> https://samples.ffmpeg.org/ffmpeg-bugs/trac/ticket2152/
>>>
>>> I dont think you can reliably detect non empty streams from the header alone
>>> in every case.
>>>
>>> thx
>>
>> This whole chunk of code is apparently done because
>> av_find_best_stream() can't be used here (The list of input streams is
>> in a custom struct InputStream instead of an AVFormatContext).
>> codec_info_nb_frames is evidently needed to detect non-empty streams, so
>> either making it or a replacement public or hand crafting a dummy
>> AVFormatContext with the input streams then using av_find_best_stream()
>> are two options for this.
>>
>> I personally think the latter option is a good solution.
>> av_find_best_stream() does not seem to care about its contents beyond
>> nb_streams and the streams list.
> 
> That seems immensely hacky to me tbh.

If it prevents regressions like these while using public API... It'd not
be the only case of dummy contexts used for some specific purpose in the
codebase, too. But i agree it's kinda hacky and probably not ideal for
what's technically the main reference library implementation.

> 
> I am also not very comfortable with making codec_info_nb_frames
> officially public, as that hardcodes internal workings of
> find_stream_info() into the public API, making it harder to change
> later.
> 
> More generally, I do not think it is reasonable to demand that the
> automatic mapping code in ffmpeg.c magically choose the best stream for
> all pathological cases. Files with empty streams are IMO pathological,
> and the user can always override the mapping manually.

It's not just an empty stream per se. The first command line Michael
posted did a seek to a few seconds before EOF. At that point the stream
that would be chosen by looking at dimensions and bitrate alone had no
frames left, but other video streams did. codec_info_nb_frames ensured
one of them would be chosen instead of the otherwise best stream, as
would have done av_find_best_stream().

That being said, using internal fields is a problem, so I'll not be
against a solution like this patch that does a best effort attempt
without frame count knowledge.