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

2017-11-27 Thread Mark Reid 
On Mon, Nov 27, 2017 at 2:14 AM, Tomas Härdin  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, [i]);
> > +for (i = 0; i < package_count; i++) {
> > +packages[i].uuid_offset = track_count;
> > +track_count += mxf_write_package(s, [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
>
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2017-11-27 Thread Tomas Härdin 
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, [i]);
> +for (i = 0; i < package_count; i++) {
> +packages[i].uuid_offset = track_count;
> +track_count += mxf_write_package(s, [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

signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2017-11-26 Thread Mark Reid 
---
 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(-)

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