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

Wed Oct 21 16:32:26 EEST 2020

On 10/21/2020 8:17 AM, Michael Niedermayer wrote:
> On Tue, Oct 20, 2020 at 01:41:50AM -0300, James Almer wrote:
>> 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().
> 
> are we talking about the same thing ?

No. I was talking about "./ffmpeg -ss 2:16 -i
~/tickets/2687/Thailand-Wave.wmv last2sec-of-source.asf", which was
fixed by the current version of this change, but is nonetheless an
scenario that could be replicated with another sample if it has streams
with similar dimensions and bitrate, yet the "best" one (as chosen
without taking in mind frame count) has no more frames at the point
seeking drops you at.

> ./ffmpeg  -i ~/tickets/4496/08_lect_01.rm  file.avi
> 
> produces a output over 1 hour long before the patch, theres no seek to before EOF
> 
> ./ffmpeg  -i ~/tickets/4496/08_lect_01.rm  file-new.avi
> 
> after the patch the output is empty
> 
> IMHO, its important to remove access to internal fields from applications.
> its important to cleanup code but its also important to do the cleanup
> cleanly. And just droping everything thats in the way instead of spending
> the hour(s) to do it properly. That is IMHO tainted cleanup, as if you
> wash a dirty surface with dirty water ;)
> 
> thx
> 
> [...]
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
> 