diff mbox series

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

Message ID 20201014085311.14728-2-anton@khirnov.net
State New
Headers show
Series [FFmpeg-devel,1/3] ffmpeg.c: rename 'area' to 'score'
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished

Commit Message

Anton Khirnov Oct. 14, 2020, 8:53 a.m. UTC
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(-)

Comments

Michael Niedermayer Oct. 14, 2020, 5:09 p.m. UTC | #1
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

[...]
James Almer Oct. 14, 2020, 5:26 p.m. UTC | #2
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.
Gyan Doshi Oct. 14, 2020, 6:43 p.m. UTC | #3
On 14-10-2020 02:23 pm, 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(-)
>
> diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c
> index a0d1b06f2d..afef23919c 100644
> --- a/fftools/ffmpeg_opt.c
> +++ b/fftools/ffmpeg_opt.c
> @@ -19,6 +19,7 @@
>    * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>    */
>   
> +#include <float.h>
>   #include <stdint.h>
>   
>   #include "ffmpeg.h"
> @@ -2208,15 +2209,27 @@ static int open_output_file(OptionsContext *o, const char *filename)
>           char *subtitle_codec_name = NULL;
>           /* pick the "best" stream of each type */
>   
> -        /* video: highest resolution */
> +        /* video */
>           if (!o->video_disable && av_guess_codec(oc->oformat, NULL, filename, NULL, AVMEDIA_TYPE_VIDEO) != AV_CODEC_ID_NONE) {
> -            int best_score = 0, idx = -1;
> +            double best_score = 0.0;
> +            int idx = -1;
>               int qcr = avformat_query_codec(oc->oformat, oc->oformat->video_codec, 0);
>               for (i = 0; i < nb_input_streams; i++) {
> -                int score;
> +                double score;
>                   ist = input_streams[i];
> -                score = ist->st->codecpar->width * ist->st->codecpar->height + 100000000*!!ist->st->codec_info_nb_frames
> -                           + 5000000*!!(ist->st->disposition & AV_DISPOSITION_DEFAULT);
> +
> +                /* base score is just the area in pixels */
> +                score = (double)ist->st->codecpar->width * ist->st->codecpar->height;
> +                /* add a fractional part favoring higher bitrate among same-area streams */
> +                if (ist->st->codecpar->bit_rate) {
> +                    const double bitrate_max = 100e6; // cap at 100Mb/s
> +                    const double bitrate = FFMIN(ist->st->codecpar->bit_rate, bitrate_max - 1.0);
> +                    score += bitrate / bitrate_max;
> +                }

> +                /* default streams get max score */
> +                if (ist->st->disposition & AV_DISPOSITION_DEFAULT)
> +                    score = DBL_MAX;

This is actually a mistake that shouldn't ever been committed.

A default disposition only has relevance among companion video streams 
from the same input. It does and should not apply to inter-input 
comparisons.
Only a handful of demuxers set disposition. Default audio selection is 
already broken with respect to its documented behaviour for, I believe, 
a couple of years now.

The saner way is to select best stream from each input and then select 
from that shortlist without considering disposition.


> +
>                   if (ist->user_set_discard == AVDISCARD_ALL)
>                       continue;
>                   if((qcr!=MKTAG('A', 'P', 'I', 'C')) && (ist->st->disposition & AV_DISPOSITION_ATTACHED_PIC))

Regards,
Gyan
Michael Niedermayer Oct. 14, 2020, 7:24 p.m. UTC | #4
On Wed, Oct 14, 2020 at 02:26:51PM -0300, James Almer wrote:
> 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.

or a av_multi_fmt_find_best_stream() which isnt limited to one AVFormatContext.

thx

[...]
Anton Khirnov Oct. 19, 2020, 2:42 p.m. UTC | #5
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.

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.
Anton Khirnov Oct. 19, 2020, 2:44 p.m. UTC | #6
Quoting Gyan Doshi (2020-10-14 20:43:59)
> 
> 
> On 14-10-2020 02:23 pm, 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(-)
> >
> > diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c
> > index a0d1b06f2d..afef23919c 100644
> > --- a/fftools/ffmpeg_opt.c
> > +++ b/fftools/ffmpeg_opt.c
> > @@ -19,6 +19,7 @@
> >    * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> >    */
> >   
> > +#include <float.h>
> >   #include <stdint.h>
> >   
> >   #include "ffmpeg.h"
> > @@ -2208,15 +2209,27 @@ static int open_output_file(OptionsContext *o, const char *filename)
> >           char *subtitle_codec_name = NULL;
> >           /* pick the "best" stream of each type */
> >   
> > -        /* video: highest resolution */
> > +        /* video */
> >           if (!o->video_disable && av_guess_codec(oc->oformat, NULL, filename, NULL, AVMEDIA_TYPE_VIDEO) != AV_CODEC_ID_NONE) {
> > -            int best_score = 0, idx = -1;
> > +            double best_score = 0.0;
> > +            int idx = -1;
> >               int qcr = avformat_query_codec(oc->oformat, oc->oformat->video_codec, 0);
> >               for (i = 0; i < nb_input_streams; i++) {
> > -                int score;
> > +                double score;
> >                   ist = input_streams[i];
> > -                score = ist->st->codecpar->width * ist->st->codecpar->height + 100000000*!!ist->st->codec_info_nb_frames
> > -                           + 5000000*!!(ist->st->disposition & AV_DISPOSITION_DEFAULT);
> > +
> > +                /* base score is just the area in pixels */
> > +                score = (double)ist->st->codecpar->width * ist->st->codecpar->height;
> > +                /* add a fractional part favoring higher bitrate among same-area streams */
> > +                if (ist->st->codecpar->bit_rate) {
> > +                    const double bitrate_max = 100e6; // cap at 100Mb/s
> > +                    const double bitrate = FFMIN(ist->st->codecpar->bit_rate, bitrate_max - 1.0);
> > +                    score += bitrate / bitrate_max;
> > +                }
> 
> > +                /* default streams get max score */
> > +                if (ist->st->disposition & AV_DISPOSITION_DEFAULT)
> > +                    score = DBL_MAX;
> 
> This is actually a mistake that shouldn't ever been committed.
> 
> A default disposition only has relevance among companion video streams 
> from the same input. It does and should not apply to inter-input 
> comparisons.
> Only a handful of demuxers set disposition. Default audio selection is 
> already broken with respect to its documented behaviour for, I believe, 
> a couple of years now.
> 
> The saner way is to select best stream from each input and then select 
> from that shortlist without considering disposition.

Watches pelcome. I am not bringing about world peace here, I just want
ffmpeg.c to be a well-behaved user application and stop accessing lavf
internals.
Jean-Baptiste Kempf Oct. 19, 2020, 2:47 p.m. UTC | #7
On Mon, 19 Oct 2020, at 16:44, Anton Khirnov wrote:
> > The saner way is to select best stream from each input and then select 
> > from that shortlist without considering disposition.
> 
> Watches pelcome. I am not bringing about world peace here, I just want
> ffmpeg.c to be a well-behaved user application and stop accessing lavf
> internals.

Yes, please, please, please.
Gyan Doshi Oct. 20, 2020, 4:11 a.m. UTC | #8
On 19-10-2020 08:14 pm, Anton Khirnov wrote:
> Quoting Gyan Doshi (2020-10-14 20:43:59)
>>
>> On 14-10-2020 02:23 pm, 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(-)
>>>
>>> diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c
>>> index a0d1b06f2d..afef23919c 100644
>>> --- a/fftools/ffmpeg_opt.c
>>> +++ b/fftools/ffmpeg_opt.c
>>> @@ -19,6 +19,7 @@
>>>     * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>>>     */
>>>    
>>> +#include <float.h>
>>>    #include <stdint.h>
>>>    
>>>    #include "ffmpeg.h"
>>> @@ -2208,15 +2209,27 @@ static int open_output_file(OptionsContext *o, const char *filename)
>>>            char *subtitle_codec_name = NULL;
>>>            /* pick the "best" stream of each type */
>>>    
>>> -        /* video: highest resolution */
>>> +        /* video */
>>>            if (!o->video_disable && av_guess_codec(oc->oformat, NULL, filename, NULL, AVMEDIA_TYPE_VIDEO) != AV_CODEC_ID_NONE) {
>>> -            int best_score = 0, idx = -1;
>>> +            double best_score = 0.0;
>>> +            int idx = -1;
>>>                int qcr = avformat_query_codec(oc->oformat, oc->oformat->video_codec, 0);
>>>                for (i = 0; i < nb_input_streams; i++) {
>>> -                int score;
>>> +                double score;
>>>                    ist = input_streams[i];
>>> -                score = ist->st->codecpar->width * ist->st->codecpar->height + 100000000*!!ist->st->codec_info_nb_frames
>>> -                           + 5000000*!!(ist->st->disposition & AV_DISPOSITION_DEFAULT);
>>> +
>>> +                /* base score is just the area in pixels */
>>> +                score = (double)ist->st->codecpar->width * ist->st->codecpar->height;
>>> +                /* add a fractional part favoring higher bitrate among same-area streams */
>>> +                if (ist->st->codecpar->bit_rate) {
>>> +                    const double bitrate_max = 100e6; // cap at 100Mb/s
>>> +                    const double bitrate = FFMIN(ist->st->codecpar->bit_rate, bitrate_max - 1.0);
>>> +                    score += bitrate / bitrate_max;
>>> +                }
>>> +                /* default streams get max score */
>>> +                if (ist->st->disposition & AV_DISPOSITION_DEFAULT)
>>> +                    score = DBL_MAX;
>> This is actually a mistake that shouldn't ever been committed.
>>
>> A default disposition only has relevance among companion video streams
>> from the same input. It does and should not apply to inter-input
>> comparisons.
>> Only a handful of demuxers set disposition. Default audio selection is
>> already broken with respect to its documented behaviour for, I believe,
>> a couple of years now.
>>
>> The saner way is to select best stream from each input and then select
>> from that shortlist without considering disposition.
> Watches pelcome. I am not bringing about world peace here, I just want
> ffmpeg.c to be a well-behaved user application and stop accessing lavf
> internals.

Will do in a couple of weeks.  So, this patch LGTM then.

Gyan
James Almer Oct. 20, 2020, 4:41 a.m. UTC | #9
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.
Michael Niedermayer Oct. 21, 2020, 11:17 a.m. UTC | #10
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 ?
./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

[...]
James Almer Oct. 21, 2020, 1:32 p.m. UTC | #11
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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Anton Khirnov Oct. 25, 2020, 1:09 p.m. UTC | #12
Quoting Michael Niedermayer (2020-10-21 13:17:46)
> 
> are we talking about the same thing ?
> ./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 ;)

Do you have any actual suggestions as to what a "clean" solution should
look like? "Not break any samples" is not a vary practical one, since I
don't have all your samples and besides is too restrictive, since any
change can potentially break some workflow [1].

How about exporting a per-AVStream flag that indicates whether there
were every any packets seen for that stream? That would allow to keep
current functionality in ffmpeg.c and still avoid exporting too much
detail about the inner workings of avformat_find_stream_info().

[1] https://xkcd.com/1172/
Andreas Rheinhardt Oct. 25, 2020, 1:17 p.m. UTC | #13
Anton Khirnov:
> Quoting Michael Niedermayer (2020-10-21 13:17:46)
>>
>> are we talking about the same thing ?
>> ./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 ;)
> 
> Do you have any actual suggestions as to what a "clean" solution should
> look like? "Not break any samples" is not a vary practical one, since I
> don't have all your samples and besides is too restrictive, since any
> change can potentially break some workflow [1].
> 
> How about exporting a per-AVStream flag that indicates whether there
> were every any packets seen for that stream? That would allow to keep
> current functionality in ffmpeg.c and still avoid exporting too much
> detail about the inner workings of avformat_find_stream_info().
> 
Where would you add this flag to AVStream? Given that ffmpeg uses
internal fields there is no good solution to this.

- Andreas
Anton Khirnov Oct. 25, 2020, 1:26 p.m. UTC | #14
Quoting Andreas Rheinhardt (2020-10-25 14:17:06)
> Anton Khirnov:
> > Quoting Michael Niedermayer (2020-10-21 13:17:46)
> >>
> >> are we talking about the same thing ?
> >> ./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 ;)
> > 
> > Do you have any actual suggestions as to what a "clean" solution should
> > look like? "Not break any samples" is not a vary practical one, since I
> > don't have all your samples and besides is too restrictive, since any
> > change can potentially break some workflow [1].
> > 
> > How about exporting a per-AVStream flag that indicates whether there
> > were every any packets seen for that stream? That would allow to keep
> > current functionality in ffmpeg.c and still avoid exporting too much
> > detail about the inner workings of avformat_find_stream_info().
> > 
> Where would you add this flag to AVStream? Given that ffmpeg uses
> internal fields there is no good solution to this.

Yes, we are now officially dumpster fire, The major bump cannot come any
sooner.
Except these patches are a part my work towards the bump, so things get
a bit circular.

One option is to make it an event flag. That might even be usable for a
wider number of use cases, though I cannot think of any right now.
James Almer Oct. 25, 2020, 1:33 p.m. UTC | #15
On 10/25/2020 10:26 AM, Anton Khirnov wrote:
> Quoting Andreas Rheinhardt (2020-10-25 14:17:06)
>> Anton Khirnov:
>>> Quoting Michael Niedermayer (2020-10-21 13:17:46)
>>>>
>>>> are we talking about the same thing ?
>>>> ./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 ;)
>>>
>>> Do you have any actual suggestions as to what a "clean" solution should
>>> look like? "Not break any samples" is not a vary practical one, since I
>>> don't have all your samples and besides is too restrictive, since any
>>> change can potentially break some workflow [1].
>>>
>>> How about exporting a per-AVStream flag that indicates whether there
>>> were every any packets seen for that stream? That would allow to keep
>>> current functionality in ffmpeg.c and still avoid exporting too much
>>> detail about the inner workings of avformat_find_stream_info().
>>>
>> Where would you add this flag to AVStream? Given that ffmpeg uses
>> internal fields there is no good solution to this.
> 
> Yes, we are now officially dumpster fire, The major bump cannot come any
> sooner.

Lets cut a 4.4 release before this work goes in (assuming we want a last
one using the current soname/s), then simply don't worry if you break
offsets in any struct, as it will not make it to a release and the bump
will come soon afterwards.

> Except these patches are a part my work towards the bump, so things get
> a bit circular.
> 
> One option is to make it an event flag. That might even be usable for a
> wider number of use cases, though I cannot think of any right now.
>
diff mbox series

Patch

diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c
index a0d1b06f2d..afef23919c 100644
--- a/fftools/ffmpeg_opt.c
+++ b/fftools/ffmpeg_opt.c
@@ -19,6 +19,7 @@ 
  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
  */
 
+#include <float.h>
 #include <stdint.h>
 
 #include "ffmpeg.h"
@@ -2208,15 +2209,27 @@  static int open_output_file(OptionsContext *o, const char *filename)
         char *subtitle_codec_name = NULL;
         /* pick the "best" stream of each type */
 
-        /* video: highest resolution */
+        /* video */
         if (!o->video_disable && av_guess_codec(oc->oformat, NULL, filename, NULL, AVMEDIA_TYPE_VIDEO) != AV_CODEC_ID_NONE) {
-            int best_score = 0, idx = -1;
+            double best_score = 0.0;
+            int idx = -1;
             int qcr = avformat_query_codec(oc->oformat, oc->oformat->video_codec, 0);
             for (i = 0; i < nb_input_streams; i++) {
-                int score;
+                double score;
                 ist = input_streams[i];
-                score = ist->st->codecpar->width * ist->st->codecpar->height + 100000000*!!ist->st->codec_info_nb_frames
-                           + 5000000*!!(ist->st->disposition & AV_DISPOSITION_DEFAULT);
+
+                /* base score is just the area in pixels */
+                score = (double)ist->st->codecpar->width * ist->st->codecpar->height;
+                /* add a fractional part favoring higher bitrate among same-area streams */
+                if (ist->st->codecpar->bit_rate) {
+                    const double bitrate_max = 100e6; // cap at 100Mb/s
+                    const double bitrate = FFMIN(ist->st->codecpar->bit_rate, bitrate_max - 1.0);
+                    score += bitrate / bitrate_max;
+                }
+                /* default streams get max score */
+                if (ist->st->disposition & AV_DISPOSITION_DEFAULT)
+                    score = DBL_MAX;
+
                 if (ist->user_set_discard == AVDISCARD_ALL)
                     continue;
                 if((qcr!=MKTAG('A', 'P', 'I', 'C')) && (ist->st->disposition & AV_DISPOSITION_ATTACHED_PIC))