diff mbox

[FFmpeg-devel,3/4] avformat/mxfenc: write reel_name if metadata key is present

Message ID 20171127054219.37859-4-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 | 52 ++++++++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 47 insertions(+), 6 deletions(-)

Comments

Tomas Härdin Nov. 27, 2017, 10:40 a.m. UTC | #1 
On Sun, 2017-11-26 at 21:42 -0800, Mark Reid wrote:
> @@ -1396,13 +1410,17 @@ static int mxf_write_package(AVFormatContext
> *s, MXFPackage *package)
>      }
>  
>      // write multiple descriptor reference
> -    if (package->type == SourcePackage) {
> +    if (package->instance == 1) {

Would only ever SourcePackage have instance != 0? What if we have
multiple MaterialPackage?
Saying (package->type == SourcePackage && package->instance == 1) might
be appropriate

>          mxf_write_local_tag(pb, 16, 0x4701);
>          if (s->nb_streams > 1) {
>              mxf_write_uuid(pb, MultipleDescriptor, 0);
>              mxf_write_multi_descriptor(s);
>          } else
>              mxf_write_uuid(pb, SubDescriptor, 0);
> +    } else if (package->instance == 2) {

Same here

> +        mxf_write_local_tag(pb, 16, 0x4701);
> +        mxf_write_uuid(pb, TapeDescriptor, 0);
> +        mxf_write_tape_descriptor(s);
>      }
>  
>      // write timecode track
> @@ -1416,7 +1434,7 @@ static int mxf_write_package(AVFormatContext
> *s, MXFPackage *package)
>          mxf_write_sequence(s, st, package);
>          mxf_write_structural_component(s, st, package);
>  
> -        if (package->type == SourcePackage) {
> +        if (package->instance == 1) {

And here

> @@ -1474,6 +1494,26 @@ static int
> mxf_write_header_metadata_sets(AVFormatContext *s)
>          }
>      }
>  
> +    if (entry = av_dict_get(s->metadata, "reel_name", NULL, 0)) {

Parenthesis around this and maybe an equality check. Or move the
assignment outside the if.

> +        packages[2].name = entry->value;
> +    } else {
> +        /* check if any of the streams contain a reel_name */
> +        for (i = 0; i < s->nb_streams; i++) {
> +            st = s->streams[i];
> +            if (entry = av_dict_get(st->metadata, "reel_name", NULL,
> 0)) {
> +                packages[2].name = entry->value;
> +                break;

Is it possible to set more than one reel_name? Conflicting values
should probably result in an error (both s->metadata and st->metadata).

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

> On Sun, 2017-11-26 at 21:42 -0800, Mark Reid wrote:
> > @@ -1396,13 +1410,17 @@ static int mxf_write_package(AVFormatContext
> > *s, MXFPackage *package)
> >      }
> >
> >      // write multiple descriptor reference
> > -    if (package->type == SourcePackage) {
> > +    if (package->instance == 1) {
>
> Would only ever SourcePackage have instance != 0? What if we have
> multiple MaterialPackage?
> Saying (package->type == SourcePackage && package->instance == 1) might
> be appropriate
>

simple enough, I'll do that


>
> >          mxf_write_local_tag(pb, 16, 0x4701);
> >          if (s->nb_streams > 1) {
> >              mxf_write_uuid(pb, MultipleDescriptor, 0);
> >              mxf_write_multi_descriptor(s);
> >          } else
> >              mxf_write_uuid(pb, SubDescriptor, 0);
> > +    } else if (package->instance == 2) {
>
> Same here
>
> > +        mxf_write_local_tag(pb, 16, 0x4701);
> > +        mxf_write_uuid(pb, TapeDescriptor, 0);
> > +        mxf_write_tape_descriptor(s);
> >      }
> >
> >      // write timecode track
> > @@ -1416,7 +1434,7 @@ static int mxf_write_package(AVFormatContext
> > *s, MXFPackage *package)
> >          mxf_write_sequence(s, st, package);
> >          mxf_write_structural_component(s, st, package);
> >
> > -        if (package->type == SourcePackage) {
> > +        if (package->instance == 1) {
>
> And here
>
> > @@ -1474,6 +1494,26 @@ static int
> > mxf_write_header_metadata_sets(AVFormatContext *s)
> >          }
> >      }
> >
> > +    if (entry = av_dict_get(s->metadata, "reel_name", NULL, 0)) {
>
> Parenthesis around this and maybe an equality check. Or move the
> assignment outside the if.
>
>
Ok, I'll move it outside the if

> +        packages[2].name = entry->value;
> > +    } else {
> > +        /* check if any of the streams contain a reel_name */
> > +        for (i = 0; i < s->nb_streams; i++) {
> > +            st = s->streams[i];
> > +            if (entry = av_dict_get(st->metadata, "reel_name", NULL,
> > 0)) {
> > +                packages[2].name = entry->value;
> > +                break;
>
> Is it possible to set more than one reel_name? Conflicting values
> should probably result in an error (both s->metadata and st->metadata).
>
>
yes this is a bit messy,  mxfdec puts the reel_names on streams. Even if
the all the streams source the same reel package,  I'd like to try and fix
that in mxfdec and put them on format level. How about for mxfenc being
strict and only accepting reel_name metadata at the format level for now?



> /Tomas
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
Tomas Härdin Nov. 28, 2017, 9:55 a.m. UTC | #3 
On 2017-11-28 05:35, Mark Reid wrote:
> On Mon, Nov 27, 2017 at 2:40 AM, Tomas Härdin <tjoppen@acc.umu.se> wrote:
>
>> On Sun, 2017-11-26 at 21:42 -0800, Mark Reid wrote:
>>> @@ -1396,13 +1410,17 @@ static int mxf_write_package(AVFormatContext
>>> *s, MXFPackage *package)
>>>       }
>>>
>>>       // write multiple descriptor reference
>>> -    if (package->type == SourcePackage) {
>>> +    if (package->instance == 1) {
>> Would only ever SourcePackage have instance != 0? What if we have
>> multiple MaterialPackage?
>> Saying (package->type == SourcePackage && package->instance == 1) might
>> be appropriate
>>
> simple enough, I'll do that
>
>
>>>           mxf_write_local_tag(pb, 16, 0x4701);
>>>           if (s->nb_streams > 1) {
>>>               mxf_write_uuid(pb, MultipleDescriptor, 0);
>>>               mxf_write_multi_descriptor(s);
>>>           } else
>>>               mxf_write_uuid(pb, SubDescriptor, 0);
>>> +    } else if (package->instance == 2) {
>> Same here
>>
>>> +        mxf_write_local_tag(pb, 16, 0x4701);
>>> +        mxf_write_uuid(pb, TapeDescriptor, 0);
>>> +        mxf_write_tape_descriptor(s);
>>>       }
>>>
>>>       // write timecode track
>>> @@ -1416,7 +1434,7 @@ static int mxf_write_package(AVFormatContext
>>> *s, MXFPackage *package)
>>>           mxf_write_sequence(s, st, package);
>>>           mxf_write_structural_component(s, st, package);
>>>
>>> -        if (package->type == SourcePackage) {
>>> +        if (package->instance == 1) {
>> And here
>>
>>> @@ -1474,6 +1494,26 @@ static int
>>> mxf_write_header_metadata_sets(AVFormatContext *s)
>>>           }
>>>       }
>>>
>>> +    if (entry = av_dict_get(s->metadata, "reel_name", NULL, 0)) {
>> Parenthesis around this and maybe an equality check. Or move the
>> assignment outside the if.
>>
>>
> Ok, I'll move it outside the if
>
>> +        packages[2].name = entry->value;
>>> +    } else {
>>> +        /* check if any of the streams contain a reel_name */
>>> +        for (i = 0; i < s->nb_streams; i++) {
>>> +            st = s->streams[i];
>>> +            if (entry = av_dict_get(st->metadata, "reel_name", NULL,
>>> 0)) {
>>> +                packages[2].name = entry->value;
>>> +                break;
>> Is it possible to set more than one reel_name? Conflicting values
>> should probably result in an error (both s->metadata and st->metadata).
>>
>>
> yes this is a bit messy,  mxfdec puts the reel_names on streams. Even if
> the all the streams source the same reel package,  I'd like to try and fix
> that in mxfdec and put them on format level. How about for mxfenc being
> strict and only accepting reel_name metadata at the format level for now?

Yes, strictness is good. Can always be relaxed later, unlike the opposite

/Tomas
diff mbox

Patch

diff --git a/libavformat/mxf.h b/libavformat/mxf.h
index 2d5b44943b..ffcc429a8b 100644
--- a/libavformat/mxf.h
+++ b/libavformat/mxf.h
@@ -47,6 +47,7 @@  enum MXFMetadataSetType {
     EssenceContainerData,
     EssenceGroup,
     TaggedValue,
+    TapeDescriptor,
 };
 
 enum MXFFrameLayout {
diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
index d573586fe4..8280f0236e 100644
--- a/libavformat/mxfenc.c
+++ b/libavformat/mxfenc.c
@@ -106,6 +106,7 @@  typedef struct MXFPackage {
     enum MXFMetadataSetType type;
     int instance;
     int uuid_offset;
+    struct MXFPackage *ref;
 } MXFPackage;
 
 enum ULIndex {
@@ -999,20 +1000,33 @@  static void mxf_write_structural_component(AVFormatContext *s, AVStream *st, MXF
 
     // write source package uid, end of the reference
     mxf_write_local_tag(pb, 32, 0x1101);
-    if (package->type == SourcePackage) {
+    if (!package->ref) {
         for (i = 0; i < 4; i++)
             avio_wb64(pb, 0);
     } else
-        mxf_write_umid(s, 1);
+        mxf_write_umid(s, package->ref->instance);
 
     // write source track id
     mxf_write_local_tag(pb, 4, 0x1102);
-    if (package->type == SourcePackage)
+    if (package->type == SourcePackage && !package->ref)
         avio_wb32(pb, 0);
     else
         avio_wb32(pb, st->index+2);
 }
 
+static void mxf_write_tape_descriptor(AVFormatContext *s)
+{
+    AVIOContext *pb = s->pb;
+
+    mxf_write_metadata_key(pb, 0x012e00);
+    PRINT_KEY(s, "tape descriptor key", pb->buf_ptr - 16);
+    klv_encode_ber_length(pb, 20);
+    mxf_write_local_tag(pb, 16, 0x3C0A);
+    mxf_write_uuid(pb, TapeDescriptor, 0);
+    PRINT_KEY(s, "tape_desc uid", pb->buf_ptr - 16);
+}
+
+
 static void mxf_write_multi_descriptor(AVFormatContext *s)
 {
     MXFContext *mxf = s->priv_data;
@@ -1396,13 +1410,17 @@  static int mxf_write_package(AVFormatContext *s, MXFPackage *package)
     }
 
     // write multiple descriptor reference
-    if (package->type == SourcePackage) {
+    if (package->instance == 1) {
         mxf_write_local_tag(pb, 16, 0x4701);
         if (s->nb_streams > 1) {
             mxf_write_uuid(pb, MultipleDescriptor, 0);
             mxf_write_multi_descriptor(s);
         } else
             mxf_write_uuid(pb, SubDescriptor, 0);
+    } else if (package->instance == 2) {
+        mxf_write_local_tag(pb, 16, 0x4701);
+        mxf_write_uuid(pb, TapeDescriptor, 0);
+        mxf_write_tape_descriptor(s);
     }
 
     // write timecode track
@@ -1416,7 +1434,7 @@  static int mxf_write_package(AVFormatContext *s, MXFPackage *package)
         mxf_write_sequence(s, st, package);
         mxf_write_structural_component(s, st, package);
 
-        if (package->type == SourcePackage) {
+        if (package->instance == 1) {
             MXFStreamContext *sc = st->priv_data;
             mxf_essence_container_uls[sc->index].write_desc(s, st);
         }
@@ -1452,11 +1470,13 @@  static int mxf_write_header_metadata_sets(AVFormatContext *s)
     AVStream *st = NULL;
     int i;
     int track_count = 0;
-    MXFPackage packages[2] = {};
+    MXFPackage packages[3] = {};
     int package_count = 2;
     packages[0].type = MaterialPackage;
     packages[1].type = SourcePackage;
     packages[1].instance = 1;
+    packages[0].ref = &packages[1];
+
 
     if (entry = av_dict_get(s->metadata, "material_package_name", NULL, 0))
        packages[0].name = entry->value;
@@ -1474,6 +1494,26 @@  static int mxf_write_header_metadata_sets(AVFormatContext *s)
         }
     }
 
+    if (entry = av_dict_get(s->metadata, "reel_name", NULL, 0)) {
+        packages[2].name = entry->value;
+    } else {
+        /* check if any of the streams contain a reel_name */
+        for (i = 0; i < s->nb_streams; i++) {
+            st = s->streams[i];
+            if (entry = av_dict_get(st->metadata, "reel_name", NULL, 0)) {
+                packages[2].name = entry->value;
+                break;
+            }
+        }
+    }
+
+    if (packages[2].name) {
+        packages[2].type = SourcePackage;
+        packages[2].instance = 2;
+        packages[1].ref = &packages[2];
+        package_count = 3;
+    }
+
     mxf_write_preface(s);
     mxf_write_identification(s);
     mxf_write_content_storage(s, packages, package_count);