| Message ID | 20201014085311.14728-2-anton@khirnov.net |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/3] ffmpeg.c: rename 'area' to 'score' | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
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 [...]
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.
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
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 [...]
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.
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.
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.
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
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.
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 [...]
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". >
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/
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
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.
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 --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))