diff mbox series

[FFmpeg-devel,25/25] avformat/matroskaenc: Redo handling of FlagDefault

Message ID 20200118082346.4573-1-andreas.rheinhardt@gmail.com
State Accepted
Headers show
Series Matroska muxer patches
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Andreas Rheinhardt Jan. 18, 2020, 8:23 a.m. UTC
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(-)

Comments

Carl Eugen Hoyos Jan. 20, 2020, 11:11 p.m. UTC | #1
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
Andreas Rheinhardt Jan. 20, 2020, 11:43 p.m. UTC | #2
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
Carl Eugen Hoyos Jan. 20, 2020, 11:48 p.m. UTC | #3
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
diff mbox series

Patch

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 },
 };