diff mbox series

[FFmpeg-devel,11/19] avformat/matroskadec: Support FlagOriginal

Message ID 20210217101356.1723370-11-andreas.rheinhardt@gmail.com
State New
Headers show
Series [FFmpeg-devel,01/19] fate/matroska: Add test for mastering display metadata
Related show

Checks

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

Commit Message

Andreas Rheinhardt Feb. 17, 2021, 10:13 a.m. UTC
Needs a CountedElement in order to distinguish the case of the element
not being present and the element being present with a value of zero.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavformat/matroska.h    | 1 +
 libavformat/matroskadec.c | 7 ++++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

Comments

Andreas Rheinhardt Feb. 22, 2021, 4:05 a.m. UTC | #1
Andreas Rheinhardt:
> Needs a CountedElement in order to distinguish the case of the element
> not being present and the element being present with a value of zero.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavformat/matroska.h    | 1 +
>  libavformat/matroskadec.c | 7 ++++++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/matroska.h b/libavformat/matroska.h
> index 191c4f6149..8ab87eff20 100644
> --- a/libavformat/matroska.h
> +++ b/libavformat/matroska.h
> @@ -100,6 +100,7 @@
>  #define MATROSKA_ID_TRACKFLAGDEFAULT 0x88
>  #define MATROSKA_ID_TRACKFLAGFORCED 0x55AA
>  #define MATROSKA_ID_TRACKFLAGLACING 0x9C
> +#define MATROSKA_ID_TRACKFLAGORIGINAL 0x55AE
>  #define MATROSKA_ID_TRACKMINCACHE 0x6DE7
>  #define MATROSKA_ID_TRACKMAXCACHE 0x6DF8
>  #define MATROSKA_ID_TRACKDEFAULTDURATION 0x23E383
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index fa266fcaec..f15bf8f9d2 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -251,6 +251,7 @@ typedef struct MatroskaTrack {
>      uint64_t flag_default;
>      uint64_t flag_forced;
>      uint64_t flag_comment;
> +    CountedElement flag_original;
>      uint64_t seek_preroll;
>      MatroskaTrackVideo video;
>      MatroskaTrackAudio audio;
> @@ -410,7 +411,7 @@ typedef struct MatroskaDemuxContext {
>  // incomplete type (6.7.2 in C90, 6.9.2 in C99).
>  // Removing the sizes breaks MSVC.
>  static EbmlSyntax ebml_syntax[3], matroska_segment[9], matroska_track_video_color[15], matroska_track_video[19],
> -                  matroska_track[28], matroska_track_encoding[6], matroska_track_encodings[2],
> +                  matroska_track[29], matroska_track_encoding[6], matroska_track_encodings[2],
>                    matroska_track_combine_planes[2], matroska_track_operation[2], matroska_tracks[2],
>                    matroska_attachments[2], matroska_chapter_entry[9], matroska_chapter[6], matroska_chapters[2],
>                    matroska_index_entry[3], matroska_index[2], matroska_tag[3], matroska_tags[2], matroska_seekhead[2],
> @@ -575,6 +576,7 @@ static EbmlSyntax matroska_track[] = {
>      { MATROSKA_ID_TRACKFLAGCOMMENTARY,   EBML_UINT,  0, 0, offsetof(MatroskaTrack, flag_comment), { .u = 0 } },
>      { MATROSKA_ID_TRACKFLAGDEFAULT,      EBML_UINT,  0, 0, offsetof(MatroskaTrack, flag_default), { .u = 1 } },
>      { MATROSKA_ID_TRACKFLAGFORCED,       EBML_UINT,  0, 0, offsetof(MatroskaTrack, flag_forced),  { .u = 0 } },
> +    { MATROSKA_ID_TRACKFLAGORIGINAL,     EBML_UINT,  1, 0, offsetof(MatroskaTrack, flag_original), {.u = 0 } },
>      { MATROSKA_ID_TRACKVIDEO,            EBML_NEST,  0, 0, offsetof(MatroskaTrack, video),        { .n = matroska_track_video } },
>      { MATROSKA_ID_TRACKAUDIO,            EBML_NEST,  0, 0, offsetof(MatroskaTrack, audio),        { .n = matroska_track_audio } },
>      { MATROSKA_ID_TRACKOPERATION,        EBML_NEST,  0, 0, offsetof(MatroskaTrack, operation),    { .n = matroska_track_operation } },
> @@ -2746,6 +2748,9 @@ static int matroska_parse_tracks(AVFormatContext *s)
>              st->disposition |= AV_DISPOSITION_FORCED;
>          if (track->flag_comment)
>              st->disposition |= AV_DISPOSITION_COMMENT;
> +        if (track->flag_original.count > 0)
> +            st->disposition |= track->flag_original.el.u ? AV_DISPOSITION_ORIGINAL
> +                                                         : AV_DISPOSITION_DUB;
>  
>          if (!st->codecpar->extradata) {
>              if (extradata) {
> 
Ridley Combs reviewed this set via IRC and approved it with one
exception: It makes no sense to use AV_DISPOSITION_DUB for something
else than an audio track, so if FlagOriginal is set to zero (meaning the
track is not in the content's original language), it should not be
exported at all. And the muxer should ignore AV_DISPOSITION_DUB for
non-audio-tracks. How do others think about this?

- Andreas
Anton Khirnov Feb. 23, 2021, 1:40 p.m. UTC | #2
Quoting Andreas Rheinhardt (2021-02-22 05:05:29)
> Andreas Rheinhardt:
> > Needs a CountedElement in order to distinguish the case of the element
> > not being present and the element being present with a value of zero.
> > 
> > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> > ---
> >  libavformat/matroska.h    | 1 +
> >  libavformat/matroskadec.c | 7 ++++++-
> >  2 files changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libavformat/matroska.h b/libavformat/matroska.h
> > index 191c4f6149..8ab87eff20 100644
> > --- a/libavformat/matroska.h
> > +++ b/libavformat/matroska.h
> > @@ -100,6 +100,7 @@
> >  #define MATROSKA_ID_TRACKFLAGDEFAULT 0x88
> >  #define MATROSKA_ID_TRACKFLAGFORCED 0x55AA
> >  #define MATROSKA_ID_TRACKFLAGLACING 0x9C
> > +#define MATROSKA_ID_TRACKFLAGORIGINAL 0x55AE
> >  #define MATROSKA_ID_TRACKMINCACHE 0x6DE7
> >  #define MATROSKA_ID_TRACKMAXCACHE 0x6DF8
> >  #define MATROSKA_ID_TRACKDEFAULTDURATION 0x23E383
> > diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> > index fa266fcaec..f15bf8f9d2 100644
> > --- a/libavformat/matroskadec.c
> > +++ b/libavformat/matroskadec.c
> > @@ -251,6 +251,7 @@ typedef struct MatroskaTrack {
> >      uint64_t flag_default;
> >      uint64_t flag_forced;
> >      uint64_t flag_comment;
> > +    CountedElement flag_original;
> >      uint64_t seek_preroll;
> >      MatroskaTrackVideo video;
> >      MatroskaTrackAudio audio;
> > @@ -410,7 +411,7 @@ typedef struct MatroskaDemuxContext {
> >  // incomplete type (6.7.2 in C90, 6.9.2 in C99).
> >  // Removing the sizes breaks MSVC.
> >  static EbmlSyntax ebml_syntax[3], matroska_segment[9], matroska_track_video_color[15], matroska_track_video[19],
> > -                  matroska_track[28], matroska_track_encoding[6], matroska_track_encodings[2],
> > +                  matroska_track[29], matroska_track_encoding[6], matroska_track_encodings[2],
> >                    matroska_track_combine_planes[2], matroska_track_operation[2], matroska_tracks[2],
> >                    matroska_attachments[2], matroska_chapter_entry[9], matroska_chapter[6], matroska_chapters[2],
> >                    matroska_index_entry[3], matroska_index[2], matroska_tag[3], matroska_tags[2], matroska_seekhead[2],
> > @@ -575,6 +576,7 @@ static EbmlSyntax matroska_track[] = {
> >      { MATROSKA_ID_TRACKFLAGCOMMENTARY,   EBML_UINT,  0, 0, offsetof(MatroskaTrack, flag_comment), { .u = 0 } },
> >      { MATROSKA_ID_TRACKFLAGDEFAULT,      EBML_UINT,  0, 0, offsetof(MatroskaTrack, flag_default), { .u = 1 } },
> >      { MATROSKA_ID_TRACKFLAGFORCED,       EBML_UINT,  0, 0, offsetof(MatroskaTrack, flag_forced),  { .u = 0 } },
> > +    { MATROSKA_ID_TRACKFLAGORIGINAL,     EBML_UINT,  1, 0, offsetof(MatroskaTrack, flag_original), {.u = 0 } },
> >      { MATROSKA_ID_TRACKVIDEO,            EBML_NEST,  0, 0, offsetof(MatroskaTrack, video),        { .n = matroska_track_video } },
> >      { MATROSKA_ID_TRACKAUDIO,            EBML_NEST,  0, 0, offsetof(MatroskaTrack, audio),        { .n = matroska_track_audio } },
> >      { MATROSKA_ID_TRACKOPERATION,        EBML_NEST,  0, 0, offsetof(MatroskaTrack, operation),    { .n = matroska_track_operation } },
> > @@ -2746,6 +2748,9 @@ static int matroska_parse_tracks(AVFormatContext *s)
> >              st->disposition |= AV_DISPOSITION_FORCED;
> >          if (track->flag_comment)
> >              st->disposition |= AV_DISPOSITION_COMMENT;
> > +        if (track->flag_original.count > 0)
> > +            st->disposition |= track->flag_original.el.u ? AV_DISPOSITION_ORIGINAL
> > +                                                         : AV_DISPOSITION_DUB;
> >  
> >          if (!st->codecpar->extradata) {
> >              if (extradata) {
> > 
> Ridley Combs reviewed this set via IRC and approved it with one
> exception: It makes no sense to use AV_DISPOSITION_DUB for something
> else than an audio track, so if FlagOriginal is set to zero (meaning the
> track is not in the content's original language), it should not be
> exported at all. And the muxer should ignore AV_DISPOSITION_DUB for
> non-audio-tracks. How do others think about this?

AV_DISPOSITION_DUB is not documented, so you can interpret it pretty
widely. One interpretation that I can think of is:
- there are two audio tracks - let's say japanese original and english
  dub
- there are also two english subtitle tracks:
    * one that translates everything, meant to be used with the original
      audio;
    * the other one translating just the text in the video (and possibly
      things that were meant to be subtitled in the original, like
      speech in Klingon), meant to be used with the english dub

Then it can make sense to tag the second subtitle track with
AV_DISPOSITION_DUB to indicate it is supposed to be used with the dubbed
audio track.
diff mbox series

Patch

diff --git a/libavformat/matroska.h b/libavformat/matroska.h
index 191c4f6149..8ab87eff20 100644
--- a/libavformat/matroska.h
+++ b/libavformat/matroska.h
@@ -100,6 +100,7 @@ 
 #define MATROSKA_ID_TRACKFLAGDEFAULT 0x88
 #define MATROSKA_ID_TRACKFLAGFORCED 0x55AA
 #define MATROSKA_ID_TRACKFLAGLACING 0x9C
+#define MATROSKA_ID_TRACKFLAGORIGINAL 0x55AE
 #define MATROSKA_ID_TRACKMINCACHE 0x6DE7
 #define MATROSKA_ID_TRACKMAXCACHE 0x6DF8
 #define MATROSKA_ID_TRACKDEFAULTDURATION 0x23E383
diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index fa266fcaec..f15bf8f9d2 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -251,6 +251,7 @@  typedef struct MatroskaTrack {
     uint64_t flag_default;
     uint64_t flag_forced;
     uint64_t flag_comment;
+    CountedElement flag_original;
     uint64_t seek_preroll;
     MatroskaTrackVideo video;
     MatroskaTrackAudio audio;
@@ -410,7 +411,7 @@  typedef struct MatroskaDemuxContext {
 // incomplete type (6.7.2 in C90, 6.9.2 in C99).
 // Removing the sizes breaks MSVC.
 static EbmlSyntax ebml_syntax[3], matroska_segment[9], matroska_track_video_color[15], matroska_track_video[19],
-                  matroska_track[28], matroska_track_encoding[6], matroska_track_encodings[2],
+                  matroska_track[29], matroska_track_encoding[6], matroska_track_encodings[2],
                   matroska_track_combine_planes[2], matroska_track_operation[2], matroska_tracks[2],
                   matroska_attachments[2], matroska_chapter_entry[9], matroska_chapter[6], matroska_chapters[2],
                   matroska_index_entry[3], matroska_index[2], matroska_tag[3], matroska_tags[2], matroska_seekhead[2],
@@ -575,6 +576,7 @@  static EbmlSyntax matroska_track[] = {
     { MATROSKA_ID_TRACKFLAGCOMMENTARY,   EBML_UINT,  0, 0, offsetof(MatroskaTrack, flag_comment), { .u = 0 } },
     { MATROSKA_ID_TRACKFLAGDEFAULT,      EBML_UINT,  0, 0, offsetof(MatroskaTrack, flag_default), { .u = 1 } },
     { MATROSKA_ID_TRACKFLAGFORCED,       EBML_UINT,  0, 0, offsetof(MatroskaTrack, flag_forced),  { .u = 0 } },
+    { MATROSKA_ID_TRACKFLAGORIGINAL,     EBML_UINT,  1, 0, offsetof(MatroskaTrack, flag_original), {.u = 0 } },
     { MATROSKA_ID_TRACKVIDEO,            EBML_NEST,  0, 0, offsetof(MatroskaTrack, video),        { .n = matroska_track_video } },
     { MATROSKA_ID_TRACKAUDIO,            EBML_NEST,  0, 0, offsetof(MatroskaTrack, audio),        { .n = matroska_track_audio } },
     { MATROSKA_ID_TRACKOPERATION,        EBML_NEST,  0, 0, offsetof(MatroskaTrack, operation),    { .n = matroska_track_operation } },
@@ -2746,6 +2748,9 @@  static int matroska_parse_tracks(AVFormatContext *s)
             st->disposition |= AV_DISPOSITION_FORCED;
         if (track->flag_comment)
             st->disposition |= AV_DISPOSITION_COMMENT;
+        if (track->flag_original.count > 0)
+            st->disposition |= track->flag_original.el.u ? AV_DISPOSITION_ORIGINAL
+                                                         : AV_DISPOSITION_DUB;
 
         if (!st->codecpar->extradata) {
             if (extradata) {