| Message ID | 20200118082346.4573-1-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Matroska muxer patches | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
Am Sa., 18. Jan. 2020 um 09:46 Uhr schrieb Andreas Rheinhardt <andreas.rheinhardt@gmail.com>: > > Up until now, the Matroska muxer would mark a track as default if it had > the disposition AV_DISPOSITION_DEFAULT or if there was no track with > AV_DISPOSITION_DEFAULT set; in the latter case even more than one track > of a kind (audio, video, subtitles) was marked as default which is not > sensible. It would not help only to change this strange behaviour? > This commit changes the logic used to mark tracks as default. There are > now three modes for this: > a) In the "infer" mode the first track of every type (audio, video, > subtitles) with default disposition set will be marked as default; if > there is no such track (for a given type), then the first track of this > type (if existing) will be marked as default. This behaviour is inspired > by mkvmerge. It ensures that the default flags will be set in a sensible > way even if the input comes from containers that lack the concept of > default flags. This mode is the default mode. > b) The "infer_no_subs" mode is similar to the "infer" mode; the > difference is that if no subtitle track with default disposition exists, > no subtitle track will be marked as default at all. > c) The "passthrough" mode: Here the track will be marked as default if > and only the corresponding input stream had disposition default. > > This fixes ticket #8173 (the passthrough mode is ideal for this) as > well as ticket #8416 (the "infer_no_subs" mode leads to the desired > output). Doesn't this duplicate the cli option for disposition? (Just asking, I know this is a can of worms) I am wondering if a new option is necessary and useful. Carl Eugen
On Tue, Jan 21, 2020 at 12:11 AM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > Am Sa., 18. Jan. 2020 um 09:46 Uhr schrieb Andreas Rheinhardt > <andreas.rheinhardt@gmail.com>: > > > > Up until now, the Matroska muxer would mark a track as default if it had > > the disposition AV_DISPOSITION_DEFAULT or if there was no track with > > AV_DISPOSITION_DEFAULT set; in the latter case even more than one track > > of a kind (audio, video, subtitles) was marked as default which is not > > sensible. > > It would not help only to change this strange behaviour? > > So you mean: If no stream with disposition default exist at all, then only mark the first track of each kind as default; otherwise pass the disposition through unchanged? That would be possible, of course; and it would be better than the current behaviour. But it would not allow to create files with no track marked as default at all (requested in #8173) and if one muxed several tracks from different Matroska files, each of which is marked as default in its source, one would still have multiple tracks of the same kind marked as default in the output. This should not be the default behaviour. > > This commit changes the logic used to mark tracks as default. There are > > now three modes for this: > > a) In the "infer" mode the first track of every type (audio, video, > > subtitles) with default disposition set will be marked as default; if > > there is no such track (for a given type), then the first track of this > > type (if existing) will be marked as default. This behaviour is inspired > > by mkvmerge. It ensures that the default flags will be set in a sensible > > way even if the input comes from containers that lack the concept of > > default flags. This mode is the default mode. > > b) The "infer_no_subs" mode is similar to the "infer" mode; the > > difference is that if no subtitle track with default disposition exists, > > no subtitle track will be marked as default at all. > > c) The "passthrough" mode: Here the track will be marked as default if > > and only the corresponding input stream had disposition default. > > > > This fixes ticket #8173 (the passthrough mode is ideal for this) as > > well as ticket #8416 (the "infer_no_subs" mode leads to the desired > > output). > > Doesn't this duplicate the cli option for disposition? > (Just asking, I know this is a can of worms) > > I am wondering if a new option is necessary and useful. > > Without a new option, one could not distinguish the case where the user explicitly wants no dispositions at all and the case where one remuxes from a source container that does not have the concept of default streams. Furthermore, having an option allows to distinguish the infer and "infer_no_subs" cases (the latter makes (some) players not show the subtitles unless one explicitly enables them which is what (some) users (including myself and apparently the guy behind #8416) want; I am actually unsure whether infer_no_subs would make a better default value). - Andreas
Am Di., 21. Jan. 2020 um 00:43 Uhr schrieb Andreas Rheinhardt <andreas.rheinhardt@gmail.com>: > > On Tue, Jan 21, 2020 at 12:11 AM Carl Eugen Hoyos <ceffmpeg@gmail.com> > wrote: > > > Am Sa., 18. Jan. 2020 um 09:46 Uhr schrieb Andreas Rheinhardt > > <andreas.rheinhardt@gmail.com>: > > > > > > Up until now, the Matroska muxer would mark a track as default if it had > > > the disposition AV_DISPOSITION_DEFAULT or if there was no track with > > > AV_DISPOSITION_DEFAULT set; in the latter case even more than one track > > > of a kind (audio, video, subtitles) was marked as default which is not > > > sensible. > > > > It would not help only to change this strange behaviour? > > > > So you mean: If no stream with disposition default exist at all, then only > mark the first track of each kind as default; otherwise pass the > disposition through unchanged? That would be possible, of course; and it > would be better than the current behaviour. But it would not allow to > create files with no track marked as default at all (requested in #8173) > and if one muxed several tracks from different Matroska files, each of > which is marked as default in its source, one would still have multiple > tracks of the same kind marked as default in the output. This should not be > the default behaviour. (The behaviour could be to only mark one stream as default, I always considered that as the only sane approach and never understood what the intention was with the code we have now...) > > > This commit changes the logic used to mark tracks as default. There are > > > now three modes for this: > > > a) In the "infer" mode the first track of every type (audio, video, > > > subtitles) with default disposition set will be marked as default; if > > > there is no such track (for a given type), then the first track of this > > > type (if existing) will be marked as default. This behaviour is inspired > > > by mkvmerge. It ensures that the default flags will be set in a sensible > > > way even if the input comes from containers that lack the concept of > > > default flags. This mode is the default mode. > > > b) The "infer_no_subs" mode is similar to the "infer" mode; the > > > difference is that if no subtitle track with default disposition exists, > > > no subtitle track will be marked as default at all. > > > c) The "passthrough" mode: Here the track will be marked as default if > > > and only the corresponding input stream had disposition default. > > > > > > This fixes ticket #8173 (the passthrough mode is ideal for this) as > > > well as ticket #8416 (the "infer_no_subs" mode leads to the desired > > > output). > > > > Doesn't this duplicate the cli option for disposition? > > (Just asking, I know this is a can of worms) > > > > I am wondering if a new option is necessary and useful. > > > > Without a new option, one could not distinguish the case where the user > explicitly wants no dispositions at all and the case where one remuxes from > a source container that does not have the concept of default streams. > Furthermore, having an option allows to distinguish the infer and > "infer_no_subs" cases (the latter makes (some) players not show the > subtitles unless one explicitly enables them which is what (some) users > (including myself and apparently the guy behind #8416) want; I am actually > unsure whether infer_no_subs would make a better default value). Thank you for the explanation, I do not immediately see a better solution. Carl Eugen
Andreas Rheinhardt (12020-01-21): > Without a new option, one could not distinguish the case where the user > explicitly wants no dispositions at all and the case where one remuxes from > a source container that does not have the concept of default streams. > Furthermore, having an option allows to distinguish the infer and > "infer_no_subs" cases (the latter makes (some) players not show the > subtitles unless one explicitly enables them which is what (some) users > (including myself and apparently the guy behind #8416) want; I am actually > unsure whether infer_no_subs would make a better default value). Hi. I am replying to this old discussion because I just lost a little time on an encode: I had "-disposition:2 -default" in my command line, and the track was still marked default, I was missing "-default_mode passthrough". Silently ignoring or overriding user-given options is really not a good idea. How can we enhance that? The obvious solution would be to make passthrough the default. That would make matroskaenc similar to other muxers. But I suppose you had a good reason to make infer the default? We could try to find an heuristic to detect if the disposition was set, and print a warning if infer is used and it gets overridden. I do not like that much, because warnings are not a satisfactory solution. We can try to find a real solution to detect if the disposition is set or not reliably, and use it (and then deprecate -default_mode). For example: /** * Set by demuxers when they know the disposition is reliable. * Used by muxers to detect if they use the disposition or guess it. */ #define AV_DISPOSITION_SET 0x10000000 If none of these solution work, then we need to write "for Matroska, -default_mode passthrough is needed" in the documentation for -disposition. In blinking lights. What do you think? Regards,
Nicolas George: > Andreas Rheinhardt (12020-01-21): >> Without a new option, one could not distinguish the case where the user >> explicitly wants no dispositions at all and the case where one remuxes from >> a source container that does not have the concept of default streams. >> Furthermore, having an option allows to distinguish the infer and >> "infer_no_subs" cases (the latter makes (some) players not show the >> subtitles unless one explicitly enables them which is what (some) users >> (including myself and apparently the guy behind #8416) want; I am actually >> unsure whether infer_no_subs would make a better default value). > > Hi. I am replying to this old discussion because I just lost a little > time on an encode: I had "-disposition:2 -default" in my command line, > and the track was still marked default, I was missing "-default_mode > passthrough". > > Silently ignoring or overriding user-given options is really not a good > idea. How can we enhance that? > > The obvious solution would be to make passthrough the default. That > would make matroskaenc similar to other muxers. But I suppose you had a > good reason to make infer the default? > At the time of said patch, the definition of the default flag was: "Set if that track (audio, video or subs) **SHOULD** be active if no language found matches the user preference." This implies that if no language found matches the user preference, then all default tracks should be played simultaneously, so that it makes very little sense to mark more than one track of a given type as default (there were also notes to the spec which contradicted the main definition and said that in this case the *first* default track should be used). For this reason mkvmerge disallowed setting multiple default tracks of the same type. And given that mkvmerge is the de-facto reference implementation, this meant that simply passing the disposition through would have created files of dubious validity. So I imitated what mkvmerge was doing. The whole situation changed now; the meaning of the default flag has been revised in a way that leaves no doubt that it is perfectly valid to set the default flag for as many tracks as one wishes. Therefore I have sent a patchset implementing this: https://ffmpeg.org/pipermail/ffmpeg-devel/2021-August/284066.html > We could try to find an heuristic to detect if the disposition was set, > and print a warning if infer is used and it gets overridden. I do not > like that much, because warnings are not a satisfactory solution. > > We can try to find a real solution to detect if the disposition is set > or not reliably, and use it (and then deprecate -default_mode). For > example: > > /** > * Set by demuxers when they know the disposition is reliable. > * Used by muxers to detect if they use the disposition or guess it. > */ > #define AV_DISPOSITION_SET 0x10000000 > I pondered doing such a thing then and now; I didn't do it last year because a system as you propose would not have solved the main "problem" (as I considered it at the time) of creating files where multiple tracks of the same type are marked as default. I didn't do it now, because the default course of action should always be just to preserve what the user indicated. But if there are complaints, then I will probably implement something like this. (mkvmerge's strategy is btw: If the user set it explicitly on the commandline, use this; else if the input file provides a default flag, use this; else treat it as default track.) - Andreas PS: Sorry for taking so long to answer.
diff --git a/doc/muxers.texi b/doc/muxers.texi index 521d99140c..48f3896146 100644 --- a/doc/muxers.texi +++ b/doc/muxers.texi @@ -1341,6 +1341,25 @@ the end. A safe size for most use cases should be about 50kB per hour of video. Note that cues are only written if the output is seekable and this option will have no effect if it is not. +@item default_mode +This option controls how the FlagDefault of the output tracks will be set. +It influences which tracks players should play by default. The default mode +is @samp{infer}. +@table @samp +@item infer +In this mode, for each type of track (audio, video or subtitle), if there is +a track with disposition default of this type, then the first such track +(i.e. the one with the lowest index) will be marked as default; if no such +track exists, the first track of this type will be marked as default instead +(if existing). This ensures that the default flag is set in a sensible way even +if the input originated from containers that lack the concept of default tracks. +@item infer_no_subs +This mode is the same as infer except that if no subtitle track with +disposition default exists, no subtitle track will be marked as default. +@item passthrough +In this mode the FlagDefault is set if and only if the AV_DISPOSITION_DEFAULT +flag is set in the disposition of the corresponding stream. +@end table @end table @anchor{md5} diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c index 2331e21ff8..5065b2adbd 100644 --- a/libavformat/matroskaenc.c +++ b/libavformat/matroskaenc.c @@ -61,6 +61,12 @@ * Info, Tracks, Chapters, Attachments, Tags and Cues */ #define MAX_SEEKHEAD_ENTRIES 6 +enum { + DEFAULT_MODE_INFER, + DEFAULT_MODE_INFER_NO_SUBS, + DEFAULT_MODE_PASSTHROUGH, +}; + typedef struct ebml_master { int64_t pos; ///< absolute offset in the containing AVIOContext where the master's elements start int sizebytes; ///< how many bytes were reserved for the size @@ -160,6 +166,7 @@ typedef struct MatroskaMuxContext { int wrote_chapters; int allow_raw_vfw; + int default_mode; } MatroskaMuxContext; /** 2 bytes * 7 for EBML IDs, 7 1-byte EBML lengths, 6 1-byte uint, @@ -1085,7 +1092,7 @@ static int mkv_write_stereo_mode(AVFormatContext *s, AVIOContext *pb, } static int mkv_write_track(AVFormatContext *s, MatroskaMuxContext *mkv, - int i, AVIOContext *pb, int default_stream_exists) + int i, AVIOContext *pb, int is_default) { AVStream *st = s->streams[i]; AVCodecParameters *par = st->codecpar; @@ -1127,8 +1134,8 @@ static int mkv_write_track(AVFormatContext *s, MatroskaMuxContext *mkv, // The default value for TRACKFLAGDEFAULT is 1, so add element // if we need to clear it. - if (default_stream_exists && !(st->disposition & AV_DISPOSITION_DEFAULT)) - put_ebml_uint(pb, MATROSKA_ID_TRACKFLAGDEFAULT, !!(st->disposition & AV_DISPOSITION_DEFAULT)); + if (!is_default) + put_ebml_uint(pb, MATROSKA_ID_TRACKFLAGDEFAULT, 0); if (st->disposition & AV_DISPOSITION_FORCED) put_ebml_uint(pb, MATROSKA_ID_TRACKFLAGFORCED, 1); @@ -1358,7 +1365,7 @@ static int mkv_write_tracks(AVFormatContext *s) { MatroskaMuxContext *mkv = s->priv_data; AVIOContext *pb = s->pb; - int i, ret, default_stream_exists = 0; + int i, ret, video_default_idx, audio_default_idx, subtitle_default_idx; mkv_add_seekhead_entry(mkv, MATROSKA_ID_TRACKS, avio_tell(pb)); @@ -1366,12 +1373,41 @@ static int mkv_write_tracks(AVFormatContext *s) if (ret < 0) return ret; - for (i = 0; i < s->nb_streams; i++) { - AVStream *st = s->streams[i]; - default_stream_exists |= st->disposition & AV_DISPOSITION_DEFAULT; + if (mkv->default_mode != DEFAULT_MODE_PASSTHROUGH) { + int video_idx, audio_idx, subtitle_idx; + + video_idx = video_default_idx = + audio_idx = audio_default_idx = + subtitle_idx = subtitle_default_idx = -1; + + for (i = s->nb_streams - 1; i >= 0; i--) { + AVStream *st = s->streams[i]; + + switch (st->codecpar->codec_type) { +#define CASE(type, variable) \ + case AVMEDIA_TYPE_ ## type: \ + variable ## _idx = i; \ + if (st->disposition & AV_DISPOSITION_DEFAULT) \ + variable ## _default_idx = i; \ + break; + CASE(VIDEO, video) + CASE(AUDIO, audio) + CASE(SUBTITLE, subtitle) +#undef CASE + } + } + + video_default_idx = FFMAX(video_default_idx, video_idx); + audio_default_idx = FFMAX(audio_default_idx, audio_idx); + if (mkv->default_mode != DEFAULT_MODE_INFER_NO_SUBS) + subtitle_default_idx = FFMAX(subtitle_default_idx, subtitle_idx); } for (i = 0; i < s->nb_streams; i++) { - ret = mkv_write_track(s, mkv, i, mkv->tracks_bc, default_stream_exists); + int is_default = mkv->default_mode == DEFAULT_MODE_PASSTHROUGH ? + s->streams[i]->disposition & AV_DISPOSITION_DEFAULT : + i == video_default_idx || i == audio_default_idx || + i == subtitle_default_idx; + ret = mkv_write_track(s, mkv, i, mkv->tracks_bc, is_default); if (ret < 0) return ret; } @@ -2696,6 +2732,10 @@ static const AVOption options[] = { { "live", "Write files assuming it is a live stream.", OFFSET(is_live), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, FLAGS }, { "allow_raw_vfw", "allow RAW VFW mode", OFFSET(allow_raw_vfw), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, FLAGS }, { "write_crc32", "write a CRC32 element inside every Level 1 element", OFFSET(write_crc), AV_OPT_TYPE_BOOL, { .i64 = 1 }, 0, 1, FLAGS }, + { "default_mode", "Controls how a track's FlagDefault is inferred", OFFSET(default_mode), AV_OPT_TYPE_INT, { .i64 = DEFAULT_MODE_INFER }, DEFAULT_MODE_INFER, DEFAULT_MODE_PASSTHROUGH, FLAGS, "default_mode" }, + { "infer", "For each track type, mark the first track of disposition default as default; if none exists, mark the first track as default.", 0, AV_OPT_TYPE_CONST, { .i64 = DEFAULT_MODE_INFER }, 0, 0, FLAGS, "default_mode" }, + { "infer_no_subs", "For each track type, mark the first track of disposition default as default; for audio and video: if none exists, mark the first track as default.", 0, AV_OPT_TYPE_CONST, { .i64 = DEFAULT_MODE_INFER_NO_SUBS }, 0, 0, FLAGS, "default_mode" }, + { "passthrough", "Use the disposition flag as-is", 0, AV_OPT_TYPE_CONST, { .i64 = DEFAULT_MODE_PASSTHROUGH }, 0, 0, FLAGS, "default_mode" }, { NULL }, };
Up until now, the Matroska muxer would mark a track as default if it had the disposition AV_DISPOSITION_DEFAULT or if there was no track with AV_DISPOSITION_DEFAULT set; in the latter case even more than one track of a kind (audio, video, subtitles) was marked as default which is not sensible. This commit changes the logic used to mark tracks as default. There are now three modes for this: a) In the "infer" mode the first track of every type (audio, video, subtitles) with default disposition set will be marked as default; if there is no such track (for a given type), then the first track of this type (if existing) will be marked as default. This behaviour is inspired by mkvmerge. It ensures that the default flags will be set in a sensible way even if the input comes from containers that lack the concept of default flags. This mode is the default mode. b) The "infer_no_subs" mode is similar to the "infer" mode; the difference is that if no subtitle track with default disposition exists, no subtitle track will be marked as default at all. c) The "passthrough" mode: Here the track will be marked as default if and only the corresponding input stream had disposition default. This fixes ticket #8173 (the passthrough mode is ideal for this) as well as ticket #8416 (the "infer_no_subs" mode leads to the desired output). Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- Better naming suggestions than "infer_no_subs" welcome. Furthermore, I'd appreciate suggestions on how to shorten the AVOption documentation (without losing relevant information). doc/muxers.texi | 19 +++++++++++++ libavformat/matroskaenc.c | 56 +++++++++++++++++++++++++++++++++------ 2 files changed, 67 insertions(+), 8 deletions(-)