diff mbox

[FFmpeg-devel,2/4] avformat/mxfenc: use track count to generate component instance uuid

Message ID 20171127054219.37859-3-mindmark@gmail.com
State Superseded
Headers show

Commit Message

Mark Reid Nov. 27, 2017, 5:42 a.m. UTC 
---
 libavformat/mxf.h               |  1 -
 libavformat/mxfenc.c            | 45 +++++++++++++++++++++++++----------------
 tests/ref/fate/copy-trac4914    |  2 +-
 tests/ref/fate/time_base        |  2 +-
 tests/ref/lavf/mxf              |  6 +++---
 tests/ref/lavf/mxf_d10          |  2 +-
 tests/ref/lavf/mxf_dv25         |  2 +-
 tests/ref/lavf/mxf_dvcpro50     |  2 +-
 tests/ref/lavf/mxf_opatom       |  2 +-
 tests/ref/lavf/mxf_opatom_audio |  2 +-
 10 files changed, 38 insertions(+), 28 deletions(-)

Comments

Tomas Härdin Nov. 27, 2017, 10:14 a.m. UTC | #1 
On Sun, 2017-11-26 at 21:42 -0800, Mark Reid wrote:
> ---
>  libavformat/mxf.h               |  1 -
>  libavformat/mxfenc.c            | 45 +++++++++++++++++++++++++----
> ------------
>  tests/ref/fate/copy-trac4914    |  2 +-
>  tests/ref/fate/time_base        |  2 +-
>  tests/ref/lavf/mxf              |  6 +++---
>  tests/ref/lavf/mxf_d10          |  2 +-
>  tests/ref/lavf/mxf_dv25         |  2 +-
>  tests/ref/lavf/mxf_dvcpro50     |  2 +-
>  tests/ref/lavf/mxf_opatom       |  2 +-
>  tests/ref/lavf/mxf_opatom_audio |  2 +-
>  10 files changed, 38 insertions(+), 28 deletions(-)
> [...]
> @@ -846,6 +847,10 @@ static void
> mxf_write_track(AVFormatContext *s, AVStream *st, MXFPackage *packag
>      MXFContext *mxf = s->priv_data;
>      AVIOContext *pb = s->pb;
>      MXFStreamContext *sc = st->priv_data;
> +    int instance = package->uuid_offset;
> +
> +    if (st != mxf->timecode_track)
> +        instance += st->index + 1;
>  
>      mxf_write_metadata_key(pb, 0x013b00);
>      PRINT_KEY(s, "track key", pb->buf_ptr - 16);
> 
>  static int mxf_write_essence_container_data(AVFormatContext *s)
> @@ -1443,11 +1451,12 @@ static int
> mxf_write_header_metadata_sets(AVFormatContext *s)
>      AVDictionaryEntry *entry = NULL;
>      AVStream *st = NULL;
>      int i;
> -
> +    int track_count = 0;
>      MXFPackage packages[2] = {};
>      int package_count = 2;
>      packages[0].type = MaterialPackage;
>      packages[1].type = SourcePackage;
> +    packages[1].instance = 1;
>  
>      if (entry = av_dict_get(s->metadata, "material_package_name",
> NULL, 0))
>         packages[0].name = entry->value;
> @@ -1468,8 +1477,10 @@ static int
> mxf_write_header_metadata_sets(AVFormatContext *s)
>      mxf_write_preface(s);
>      mxf_write_identification(s);
>      mxf_write_content_storage(s, packages, package_count);
> -    for (i = 0; i < package_count; i++)
> -        mxf_write_package(s, &packages[i]);
> +    for (i = 0; i < package_count; i++) {
> +        packages[i].uuid_offset = track_count;
> +        track_count += mxf_write_package(s, &packages[i]);
> +    }

I see st->index + 1 when deriving instance from uuid_offset, are you
sure there isn't a potential off-by-one error here? An MP track and SP
track getting the same UUID or something. I guess type is enough to
differentiate, but then why does instance need uuid_offset?

/Tomas
Mark Reid Nov. 28, 2017, 3:58 a.m. UTC | #2 
On Mon, Nov 27, 2017 at 2:14 AM, Tomas Härdin <tjoppen@acc.umu.se> wrote:

> On Sun, 2017-11-26 at 21:42 -0800, Mark Reid wrote:
> > ---
> >  libavformat/mxf.h               |  1 -
> >  libavformat/mxfenc.c            | 45 +++++++++++++++++++++++++----
> > ------------
> >  tests/ref/fate/copy-trac4914    |  2 +-
> >  tests/ref/fate/time_base        |  2 +-
> >  tests/ref/lavf/mxf              |  6 +++---
> >  tests/ref/lavf/mxf_d10          |  2 +-
> >  tests/ref/lavf/mxf_dv25         |  2 +-
> >  tests/ref/lavf/mxf_dvcpro50     |  2 +-
> >  tests/ref/lavf/mxf_opatom       |  2 +-
> >  tests/ref/lavf/mxf_opatom_audio |  2 +-
> >  10 files changed, 38 insertions(+), 28 deletions(-)
> > [...]
> > @@ -846,6 +847,10 @@ static void
> > mxf_write_track(AVFormatContext *s, AVStream *st, MXFPackage *packag
> >      MXFContext *mxf = s->priv_data;
> >      AVIOContext *pb = s->pb;
> >      MXFStreamContext *sc = st->priv_data;
> > +    int instance = package->uuid_offset;
> > +
> > +    if (st != mxf->timecode_track)
> > +        instance += st->index + 1;
> >
> >      mxf_write_metadata_key(pb, 0x013b00);
> >      PRINT_KEY(s, "track key", pb->buf_ptr - 16);
> >
> >  static int mxf_write_essence_container_data(AVFormatContext *s)
> > @@ -1443,11 +1451,12 @@ static int
> > mxf_write_header_metadata_sets(AVFormatContext *s)
> >      AVDictionaryEntry *entry = NULL;
> >      AVStream *st = NULL;
> >      int i;
> > -
> > +    int track_count = 0;
> >      MXFPackage packages[2] = {};
> >      int package_count = 2;
> >      packages[0].type = MaterialPackage;
> >      packages[1].type = SourcePackage;
> > +    packages[1].instance = 1;
> >
> >      if (entry = av_dict_get(s->metadata, "material_package_name",
> > NULL, 0))
> >         packages[0].name = entry->value;
> > @@ -1468,8 +1477,10 @@ static int
> > mxf_write_header_metadata_sets(AVFormatContext *s)
> >      mxf_write_preface(s);
> >      mxf_write_identification(s);
> >      mxf_write_content_storage(s, packages, package_count);
> > -    for (i = 0; i < package_count; i++)
> > -        mxf_write_package(s, &packages[i]);
> > +    for (i = 0; i < package_count; i++) {
> > +        packages[i].uuid_offset = track_count;
> > +        track_count += mxf_write_package(s, &packages[i]);
> > +    }
>
> I see st->index + 1 when deriving instance from uuid_offset, are you
> sure there isn't a potential off-by-one error here? An MP track and SP
> track getting the same UUID or something. I guess type is enough to
> differentiate, but then why does instance need uuid_offset?
>

I don't think so unless AVStream->index could get set incorrectly, is this
possible?
I had to change the old method because it falls apart when you add the 2nd
source package.
Basically the idea is since every 1 track has 1 sequence and 1 component
they can share the same instance number
Rethinking it a little bit I think I have come up with something simple
that achieves the same result, I'll send a new patch.


>
> /Tomas
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
diff mbox

Patch

diff --git a/libavformat/mxf.h b/libavformat/mxf.h
index f3db1f939b..2d5b44943b 100644
--- a/libavformat/mxf.h
+++ b/libavformat/mxf.h
@@ -45,7 +45,6 @@  enum MXFMetadataSetType {
     SubDescriptor,
     IndexTableSegment,
     EssenceContainerData,
-    TypeBottom,// add metadata type before this
     EssenceGroup,
     TaggedValue,
 };
diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
index ed6ecbf541..d573586fe4 100644
--- a/libavformat/mxfenc.c
+++ b/libavformat/mxfenc.c
@@ -105,6 +105,7 @@  typedef struct MXFPackage {
     char *name;
     enum MXFMetadataSetType type;
     int instance;
+    int uuid_offset;
 } MXFPackage;
 
 enum ULIndex {
@@ -846,6 +847,10 @@  static void mxf_write_track(AVFormatContext *s, AVStream *st, MXFPackage *packag
     MXFContext *mxf = s->priv_data;
     AVIOContext *pb = s->pb;
     MXFStreamContext *sc = st->priv_data;
+    int instance = package->uuid_offset;
+
+    if (st != mxf->timecode_track)
+        instance += st->index + 1;
 
     mxf_write_metadata_key(pb, 0x013b00);
     PRINT_KEY(s, "track key", pb->buf_ptr - 16);
@@ -853,7 +858,7 @@  static void mxf_write_track(AVFormatContext *s, AVStream *st, MXFPackage *packag
 
     // write track uid
     mxf_write_local_tag(pb, 16, 0x3C0A);
-    mxf_write_uuid(pb, package->type == MaterialPackage ? Track : Track + TypeBottom, st->index);
+    mxf_write_uuid(pb, Track, instance);
     PRINT_KEY(s, "track uid", pb->buf_ptr - 16);
 
     // write track id
@@ -884,7 +889,7 @@  static void mxf_write_track(AVFormatContext *s, AVStream *st, MXFPackage *packag
 
     // write sequence refs
     mxf_write_local_tag(pb, 16, 0x4803);
-    mxf_write_uuid(pb, package->type == MaterialPackage ? Sequence: Sequence + TypeBottom, st->index);
+    mxf_write_uuid(pb, Sequence, instance);
 }
 
 static const uint8_t smpte_12m_timecode_track_data_ul[] = { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x01,0x03,0x02,0x01,0x01,0x00,0x00,0x00 };
@@ -918,13 +923,17 @@  static void mxf_write_sequence(AVFormatContext *s, AVStream *st, MXFPackage *pac
     MXFContext *mxf = s->priv_data;
     AVIOContext *pb = s->pb;
     enum MXFMetadataSetType component;
+    int instance = package->uuid_offset;
+
+    if (st != mxf->timecode_track)
+        instance += st->index + 1;
 
     mxf_write_metadata_key(pb, 0x010f00);
     PRINT_KEY(s, "sequence key", pb->buf_ptr - 16);
     klv_encode_ber_length(pb, 80);
 
     mxf_write_local_tag(pb, 16, 0x3C0A);
-    mxf_write_uuid(pb, package->type == MaterialPackage ? Sequence: Sequence + TypeBottom, st->index);
+    mxf_write_uuid(pb, Sequence, instance);
 
     PRINT_KEY(s, "sequence uid", pb->buf_ptr - 16);
     mxf_write_common_fields(s, st);
@@ -936,9 +945,8 @@  static void mxf_write_sequence(AVFormatContext *s, AVStream *st, MXFPackage *pac
         component = TimecodeComponent;
     else
         component = SourceClip;
-    if (package->type == SourcePackage)
-        component += TypeBottom;
-    mxf_write_uuid(pb, component, st->index);
+
+    mxf_write_uuid(pb, component, instance);
 }
 
 static void mxf_write_timecode_component(AVFormatContext *s, AVStream *st, MXFPackage *package)
@@ -951,8 +959,7 @@  static void mxf_write_timecode_component(AVFormatContext *s, AVStream *st, MXFPa
 
     // UID
     mxf_write_local_tag(pb, 16, 0x3C0A);
-    mxf_write_uuid(pb, package->type == MaterialPackage ? TimecodeComponent :
-                   TimecodeComponent + TypeBottom, st->index);
+    mxf_write_uuid(pb, TimecodeComponent, package->uuid_offset);
 
     mxf_write_common_fields(s, st);
 
@@ -973,6 +980,7 @@  static void mxf_write_structural_component(AVFormatContext *s, AVStream *st, MXF
 {
     AVIOContext *pb = s->pb;
     int i;
+    int instance = package->uuid_offset + st->index + 1;
 
     mxf_write_metadata_key(pb, 0x011100);
     PRINT_KEY(s, "sturctural component key", pb->buf_ptr - 16);
@@ -980,7 +988,7 @@  static void mxf_write_structural_component(AVFormatContext *s, AVStream *st, MXF
 
     // write uid
     mxf_write_local_tag(pb, 16, 0x3C0A);
-    mxf_write_uuid(pb, package->type == MaterialPackage ? SourceClip: SourceClip + TypeBottom, st->index);
+    mxf_write_uuid(pb, SourceClip, instance);
 
     PRINT_KEY(s, "structural component uid", pb->buf_ptr - 16);
     mxf_write_common_fields(s, st);
@@ -1329,7 +1337,7 @@  static int mxf_write_user_comments(AVFormatContext *s, const AVDictionary *m)
     return count;
 }
 
-static void mxf_write_package(AVFormatContext *s, MXFPackage *package)
+static int mxf_write_package(AVFormatContext *s, MXFPackage *package)
 {
     MXFContext *mxf = s->priv_data;
     AVIOContext *pb = s->pb;
@@ -1357,7 +1365,7 @@  static void mxf_write_package(AVFormatContext *s, MXFPackage *package)
 
     // write package umid
     mxf_write_local_tag(pb, 32, 0x4401);
-    mxf_write_umid(s, package->type == SourcePackage);
+    mxf_write_umid(s, package->instance);
     PRINT_KEY(s, "package umid second part", pb->buf_ptr - 16);
 
     // package name
@@ -1375,10 +1383,9 @@  static void mxf_write_package(AVFormatContext *s, MXFPackage *package)
     // write track refs
     mxf_write_local_tag(pb, track_count*16 + 8, 0x4403);
     mxf_write_refs_count(pb, track_count);
-    mxf_write_uuid(pb, package->type == MaterialPackage ? Track :
-                   Track + TypeBottom, -1); // timecode track
+    mxf_write_uuid(pb, Track, package->uuid_offset); // timecode track
     for (i = 0; i < s->nb_streams; i++)
-        mxf_write_uuid(pb, package->type == MaterialPackage ? Track : Track + TypeBottom, i);
+        mxf_write_uuid(pb, Track, package->uuid_offset + i + 1);
 
     // write user comment refs
     if (mxf->store_user_comments) {
@@ -1414,6 +1421,7 @@  static void mxf_write_package(AVFormatContext *s, MXFPackage *package)
             mxf_essence_container_uls[sc->index].write_desc(s, st);
         }
     }
+    return track_count;
 }
 
 static int mxf_write_essence_container_data(AVFormatContext *s)
@@ -1443,11 +1451,12 @@  static int mxf_write_header_metadata_sets(AVFormatContext *s)
     AVDictionaryEntry *entry = NULL;
     AVStream *st = NULL;
     int i;
-
+    int track_count = 0;
     MXFPackage packages[2] = {};
     int package_count = 2;
     packages[0].type = MaterialPackage;
     packages[1].type = SourcePackage;
+    packages[1].instance = 1;
 
     if (entry = av_dict_get(s->metadata, "material_package_name", NULL, 0))
        packages[0].name = entry->value;
@@ -1468,8 +1477,10 @@  static int mxf_write_header_metadata_sets(AVFormatContext *s)
     mxf_write_preface(s);
     mxf_write_identification(s);
     mxf_write_content_storage(s, packages, package_count);
-    for (i = 0; i < package_count; i++)
-        mxf_write_package(s, &packages[i]);
+    for (i = 0; i < package_count; i++) {
+        packages[i].uuid_offset = track_count;
+        track_count += mxf_write_package(s, &packages[i]);
+    }
     mxf_write_essence_container_data(s);
     return 0;
 }
diff --git a/tests/ref/fate/copy-trac4914 b/tests/ref/fate/copy-trac4914
index e0864a0035..a8f287fafa 100644
--- a/tests/ref/fate/copy-trac4914
+++ b/tests/ref/fate/copy-trac4914
@@ -1,4 +1,4 @@ 
-d51f6bcc96885a2ce8517ae8c774f610 *tests/data/fate/copy-trac4914.mxf
+05fdc4a6e28abb2c26e96224682d2684 *tests/data/fate/copy-trac4914.mxf
 560697 tests/data/fate/copy-trac4914.mxf
 #tb 0: 1001/30000
 #media_type 0: video
diff --git a/tests/ref/fate/time_base b/tests/ref/fate/time_base
index 7923556b35..4dd14084d3 100644
--- a/tests/ref/fate/time_base
+++ b/tests/ref/fate/time_base
@@ -1 +1 @@ 
-d26a35b141551b36c5b8bd716451cfcb
+f97551f884df5ab709c5869c66c7b9bc
diff --git a/tests/ref/lavf/mxf b/tests/ref/lavf/mxf
index b9c37334a9..7318447ecb 100644
--- a/tests/ref/lavf/mxf
+++ b/tests/ref/lavf/mxf
@@ -1,9 +1,9 @@ 
-1c06a9d69b6e309579784db5ecb0b69f *./tests/data/lavf/lavf.mxf
+d4140129463dec64bdb4a7d7ad1b0c82 *./tests/data/lavf/lavf.mxf
 525369 ./tests/data/lavf/lavf.mxf
 ./tests/data/lavf/lavf.mxf CRC=0x8dddfaab
-50b4f9ca0493e6d83f4c52dc3aa2b7a5 *./tests/data/lavf/lavf.mxf
+a27bb8cd5e185ea13b0a8daa4eb221cd *./tests/data/lavf/lavf.mxf
 560697 ./tests/data/lavf/lavf.mxf
 ./tests/data/lavf/lavf.mxf CRC=0xf21b1b48
-4b71b154ae37364c8028cb50850a54c5 *./tests/data/lavf/lavf.mxf
+395bf0047c97ceca96935357166b94c7 *./tests/data/lavf/lavf.mxf
 525369 ./tests/data/lavf/lavf.mxf
 ./tests/data/lavf/lavf.mxf CRC=0x8dddfaab
diff --git a/tests/ref/lavf/mxf_d10 b/tests/ref/lavf/mxf_d10
index 134db876d5..2384d427b0 100644
--- a/tests/ref/lavf/mxf_d10
+++ b/tests/ref/lavf/mxf_d10
@@ -1,3 +1,3 @@ 
-73c0cb416548c33d0651c59519a8f7e2 *./tests/data/lavf/lavf.mxf_d10
+f4694941b0cd5b5e3c91064d84dbd345 *./tests/data/lavf/lavf.mxf_d10
 5330989 ./tests/data/lavf/lavf.mxf_d10
 ./tests/data/lavf/lavf.mxf_d10 CRC=0x6c74d488
diff --git a/tests/ref/lavf/mxf_dv25 b/tests/ref/lavf/mxf_dv25
index 85094828d1..e836b14240 100644
--- a/tests/ref/lavf/mxf_dv25
+++ b/tests/ref/lavf/mxf_dv25
@@ -1,3 +1,3 @@ 
-1871bd11947924116776201f24fd0adf *./tests/data/lavf/lavf.mxf_dv25
+1ca8143bf6cf322fd39f6e856959d502 *./tests/data/lavf/lavf.mxf_dv25
 3833389 ./tests/data/lavf/lavf.mxf_dv25
 ./tests/data/lavf/lavf.mxf_dv25 CRC=0xbdaf7f52
diff --git a/tests/ref/lavf/mxf_dvcpro50 b/tests/ref/lavf/mxf_dvcpro50
index 1d0cf79996..bb3d6b928a 100644
--- a/tests/ref/lavf/mxf_dvcpro50
+++ b/tests/ref/lavf/mxf_dvcpro50
@@ -1,3 +1,3 @@ 
-6c9cb62911ac16c3b55f0ad0b052c05b *./tests/data/lavf/lavf.mxf_dvcpro50
+987fd4b2abb36433fba0e35f4092efc6 *./tests/data/lavf/lavf.mxf_dvcpro50
 7430189 ./tests/data/lavf/lavf.mxf_dvcpro50
 ./tests/data/lavf/lavf.mxf_dvcpro50 CRC=0xe3bbe4b4
diff --git a/tests/ref/lavf/mxf_opatom b/tests/ref/lavf/mxf_opatom
index ea1190c06a..1cc612e627 100644
--- a/tests/ref/lavf/mxf_opatom
+++ b/tests/ref/lavf/mxf_opatom
@@ -1,3 +1,3 @@ 
-962c2cd582340f8961a8283636093abf *./tests/data/lavf/lavf.mxf_opatom
+b8fe60f7457b83709f33357d04c8db0c *./tests/data/lavf/lavf.mxf_opatom
 4717113 ./tests/data/lavf/lavf.mxf_opatom
 ./tests/data/lavf/lavf.mxf_opatom CRC=0xf55aa22a
diff --git a/tests/ref/lavf/mxf_opatom_audio b/tests/ref/lavf/mxf_opatom_audio
index 953df9094f..deed55e526 100644
--- a/tests/ref/lavf/mxf_opatom_audio
+++ b/tests/ref/lavf/mxf_opatom_audio
@@ -1,3 +1,3 @@ 
-d4ad5a0faf410a9d9e99b3328143e89d *./tests/data/lavf/lavf.mxf_opatom_audio
+e7da52bd591e6eddb4e1af381a4e5bd4 *./tests/data/lavf/lavf.mxf_opatom_audio
 101945 ./tests/data/lavf/lavf.mxf_opatom_audio
 ./tests/data/lavf/lavf.mxf_opatom_audio CRC=0xd155c6ff