Re: [FFmpeg-devel] [PATCH v2 1/3] avformat/mxfenc: use track count to generate component instance uuid

[Mark Reid]

On Thu, Nov 30, 2017 at 9:01 AM, Tomas Härdin  wrote:

> On Wed, 2017-11-29 at 20:18 -0800, Mark Reid wrote:
> > On Wed, Nov 29, 2017 at 1:36 AM, Tomas Härdin 
> > wrote:
> >
> > > On 2017-11-29 05:11, Mark Reid wrote:
> > >
> > > > @@ -980,7 +980,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, mxf->track_uuid_offset);
> > > > PRINT_KEY(s, "structural component uid", pb->buf_ptr -
> > > > 16);
> > > >   mxf_write_common_fields(s, st);
> > > > @@ -1357,7 +1357,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 +1375,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, mxf->track_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,  mxf->track_uuid_offset + i +
> > > > 1);
> > > >
> > >
> > > Do these refer to tracks that are about to be written? Seems so as
> > > best I
> > > can tell, so I guess it works. I'd be happier if the referencing
> > > was done
> > > more explicitly rather than implicitly
> >
> >
> > yes they refer to the tracks about to be written. Those uuids need to
> > match
> > the uuid instance tag (0x3C0A) on a track written in mxf_write_track.
> > then
> > the uuid ref on the track needs to match the one on the sequence, and
> > then
> > the one the sequence needs to match the one on the component.
> >
> > would you rather the instance id be a argument to each of the write
> > functions? for example:
> > mxf_write_package(AVFormatContext *s, MXFPackage *package, int
> > track_uuid_offset)
> > mxf_write_track(AVFormatContext *s, AVStream *st, MXFPackage
> > *package, int
> > instance_id)
> > would that be more explicit?
>
> I'm not quite sure tbh. It's been a few years since I worked with MXF,
> so it's harder to say. Maybe a comment would be enough
>
>
okay I'll submit a new version with some more comments


> /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 v2 1/3] avformat/mxfenc: use track count to generate component instance uuid

[Tomas Härdin]

On Wed, 2017-11-29 at 20:18 -0800, Mark Reid wrote:
> On Wed, Nov 29, 2017 at 1:36 AM, Tomas Härdin 
> wrote:
> 
> > On 2017-11-29 05:11, Mark Reid wrote:
> > 
> > > @@ -980,7 +980,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, mxf->track_uuid_offset);
> > > PRINT_KEY(s, "structural component uid", pb->buf_ptr -
> > > 16);
> > >   mxf_write_common_fields(s, st);
> > > @@ -1357,7 +1357,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 +1375,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, mxf->track_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,  mxf->track_uuid_offset + i +
> > > 1);
> > > 
> > 
> > Do these refer to tracks that are about to be written? Seems so as
> > best I
> > can tell, so I guess it works. I'd be happier if the referencing
> > was done
> > more explicitly rather than implicitly
> 
> 
> yes they refer to the tracks about to be written. Those uuids need to
> match
> the uuid instance tag (0x3C0A) on a track written in mxf_write_track.
> then
> the uuid ref on the track needs to match the one on the sequence, and
> then
> the one the sequence needs to match the one on the component.
> 
> would you rather the instance id be a argument to each of the write
> functions? for example:
> mxf_write_package(AVFormatContext *s, MXFPackage *package, int
> track_uuid_offset)
> mxf_write_track(AVFormatContext *s, AVStream *st, MXFPackage
> *package, int
> instance_id)
> would that be more explicit?

I'm not quite sure tbh. It's been a few years since I worked with MXF,
so it's harder to say. Maybe a comment would be enough

/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

Re: [FFmpeg-devel] [PATCH v2 1/3] avformat/mxfenc: use track count to generate component instance uuid

[Mark Reid]

On Wed, Nov 29, 2017 at 1:36 AM, Tomas Härdin  wrote:

> On 2017-11-29 05:11, Mark Reid wrote:
>
>> @@ -980,7 +980,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, mxf->track_uuid_offset);
>> PRINT_KEY(s, "structural component uid", pb->buf_ptr - 16);
>>   mxf_write_common_fields(s, st);
>> @@ -1357,7 +1357,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 +1375,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, mxf->track_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,  mxf->track_uuid_offset + i + 1);
>>
>
> Do these refer to tracks that are about to be written? Seems so as best I
> can tell, so I guess it works. I'd be happier if the referencing was done
> more explicitly rather than implicitly


yes they refer to the tracks about to be written. Those uuids need to match
the uuid instance tag (0x3C0A) on a track written in mxf_write_track. then
the uuid ref on the track needs to match the one on the sequence, and then
the one the sequence needs to match the one on the component.

would you rather the instance id be a argument to each of the write
functions? for example:
mxf_write_package(AVFormatContext *s, MXFPackage *package, int
track_uuid_offset)
mxf_write_track(AVFormatContext *s, AVStream *st, MXFPackage *package, int
instance_id)
would that be more explicit?



>
> // write user comment refs
>>   if (mxf->store_user_comments) {
>> @@ -1402,12 +1401,14 @@ static void mxf_write_package(AVFormatContext
>> *s, MXFPackage *package)
>>   mxf_write_track(s, mxf->timecode_track, package);
>>   mxf_write_sequence(s, mxf->timecode_track, package);
>>   mxf_write_timecode_component(s, mxf->timecode_track, package);
>> +mxf->track_uuid_offset++;
>> for (i = 0; i < s->nb_streams; i++) {
>>   AVStream *st = s->streams[i];
>>   mxf_write_track(s, st, package);
>>   mxf_write_sequence(s, st, package);
>>   mxf_write_structural_component(s, st, package);
>> +mxf->track_uuid_offset++;
>>
>>
>
> /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 v2 1/3] avformat/mxfenc: use track count to generate component instance uuid

[Tomas Härdin]

On 2017-11-29 05:11, Mark Reid wrote:

@@ -980,7 +980,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, mxf->track_uuid_offset);
  
  PRINT_KEY(s, "structural component uid", pb->buf_ptr - 16);

  mxf_write_common_fields(s, st);
@@ -1357,7 +1357,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 +1375,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, mxf->track_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,  mxf->track_uuid_offset + i + 1);


Do these refer to tracks that are about to be written? Seems so as best 
I can tell, so I guess it works. I'd be happier if the referencing was 
done more explicitly rather than implicitly


  
  // write user comment refs

  if (mxf->store_user_comments) {
@@ -1402,12 +1401,14 @@ static void mxf_write_package(AVFormatContext *s, 
MXFPackage *package)
  mxf_write_track(s, mxf->timecode_track, package);
  mxf_write_sequence(s, mxf->timecode_track, package);
  mxf_write_timecode_component(s, mxf->timecode_track, package);
+mxf->track_uuid_offset++;
  
  for (i = 0; i < s->nb_streams; i++) {

  AVStream *st = s->streams[i];
  mxf_write_track(s, st, package);
  mxf_write_sequence(s, st, package);
  mxf_write_structural_component(s, st, package);
+mxf->track_uuid_offset++;
  


/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH v2 1/3] avformat/mxfenc: use track count to generate component instance uuid

[Mark Reid]

---
 libavformat/mxf.h   |  1 -
 libavformat/mxfenc.c| 30 +-
 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, 27 insertions(+), 24 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..baaeb8c617 100644
--- a/libavformat/mxfenc.c
+++ b/libavformat/mxfenc.c
@@ -386,6 +386,7 @@ typedef struct MXFContext {
 uint32_t tagged_value_count;
 AVRational audio_edit_rate;
 int store_user_comments;
+int track_uuid_offset;
 } MXFContext;
 
 static const uint8_t uuid_base[]= { 
0xAD,0xAB,0x44,0x24,0x2f,0x25,0x4d,0xc7,0x92,0xff,0x29,0xbd };
@@ -853,7 +854,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, mxf->track_uuid_offset);
 PRINT_KEY(s, "track uid", pb->buf_ptr - 16);
 
 // write track id
@@ -884,7 +885,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, mxf->track_uuid_offset);
 }
 
 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 
};
@@ -924,7 +925,7 @@ static void mxf_write_sequence(AVFormatContext *s, AVStream 
*st, MXFPackage *pac
 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, mxf->track_uuid_offset);
 
 PRINT_KEY(s, "sequence uid", pb->buf_ptr - 16);
 mxf_write_common_fields(s, st);
@@ -936,9 +937,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, mxf->track_uuid_offset);
 }
 
 static void mxf_write_timecode_component(AVFormatContext *s, AVStream *st, 
MXFPackage *package)
@@ -951,8 +951,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, mxf->track_uuid_offset);
 
 mxf_write_common_fields(s, st);
 
@@ -971,6 +970,7 @@ static void mxf_write_timecode_component(AVFormatContext 
*s, AVStream *st, MXFPa
 
 static void mxf_write_structural_component(AVFormatContext *s, AVStream *st, 
MXFPackage *package)
 {
+MXFContext *mxf = s->priv_data;
 AVIOContext *pb = s->pb;
 int i;
 
@@ -980,7 +980,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, mxf->track_uuid_offset);
 
 PRINT_KEY(s, "structural component uid", pb->buf_ptr - 16);
 mxf_write_common_fields(s, st);
@@ -1357,7 +1357,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 +1375,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, mxf->track_uuid_offset); // timecode