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

Submitted by Mark Reid on Dec. 5, 2017, 4:46 a.m.

Details

Message ID 20171205044623.77938-3-mindmark@gmail.com
State Accepted
Commit 901d87aa83aa1686f02bb4750c74d4d7cb6086db
Headers show

Commit Message

Mark Reid Dec. 5, 2017, 4:46 a.m.
---
 libavformat/mxf.h    |  1 +
 libavformat/mxfenc.c | 42 +++++++++++++++++++++++++++++++++++-------
 2 files changed, 36 insertions(+), 7 deletions(-)

Comments

Tomas Härdin Dec. 7, 2017, 3:45 p.m.
On 2017-12-05 05:46, Mark Reid wrote:
> ---
>   libavformat/mxf.h    |  1 +
>   libavformat/mxfenc.c | 42 +++++++++++++++++++++++++++++++++++-------
>   2 files changed, 36 insertions(+), 7 deletions(-)
>
>
> @@ -1476,6 +1495,15 @@ static int mxf_write_header_metadata_sets(AVFormatContext *s)
>           }
>       }
>   
> +    entry = av_dict_get(s->metadata, "reel_name", NULL, 0);
> +    if (entry) {
> +        packages[2].name = entry->value;
> +        packages[2].type = SourcePackage;
> +        packages[2].instance = 2;
> +        packages[1].ref = &packages[2];
> +        package_count = 3;
> +    }

I guess we can have it check track metadata later if we feel like it. 
Did a patch moving reel_name into AVFormatContext make it into mxfdec 
yet? mxfenc's output surviving a roundtrip through mxfdec + mxfenc might 
be desirable:

     ffmpeg -i somefile_with_reel_name.mkv output.mxf
     ffmpeg -i output.mxf -vcodec copy -acodec copy remuxed.mxf

Ideally remuxed.mxf is identical to output.mxf

/Tomas
Mark Reid Dec. 7, 2017, 10:45 p.m.
On Dec 7, 2017 7:45 AM, "Tomas Härdin" <tjoppen@acc.umu.se> wrote:

On 2017-12-05 05:46, Mark Reid wrote:

> ---
>   libavformat/mxf.h    |  1 +
>   libavformat/mxfenc.c | 42 +++++++++++++++++++++++++++++++++++-------
>   2 files changed, 36 insertions(+), 7 deletions(-)
>
>
> @@ -1476,6 +1495,15 @@ static int mxf_write_header_metadata_sets(AVFormatContext
> *s)
>           }
>       }
>   +    entry = av_dict_get(s->metadata, "reel_name", NULL, 0);
> +    if (entry) {
> +        packages[2].name = entry->value;
> +        packages[2].type = SourcePackage;
> +        packages[2].instance = 2;
> +        packages[1].ref = &packages[2];
> +        package_count = 3;
> +    }
>

I guess we can have it check track metadata later if we feel like it. Did a
patch moving reel_name into AVFormatContext make it into mxfdec yet?
mxfenc's output surviving a roundtrip through mxfdec + mxfenc might be
desirable:

    ffmpeg -i somefile_with_reel_name.mkv output.mxf
    ffmpeg -i output.mxf -vcodec copy -acodec copy remuxed.mxf

Ideally remuxed.mxf is identical to output.mxf

/Tomas


Yes I agree that such behaviour is desirable.
I haven't taken a look at mxfdec yet. Looking in steams for reel_name might
be necessary, as I believe that's where mov,MP4 store it. But I was trying
to keep it simple at first and addressed this specific issue a future patch.
Tomas Härdin Dec. 7, 2017, 11:19 p.m.
On Thu, 2017-12-07 at 14:45 -0800, Mark Reid wrote:
> On Dec 7, 2017 7:45 AM, "Tomas Härdin" <tjoppen@acc.umu.se> wrote:
> 
> On 2017-12-05 05:46, Mark Reid wrote:
> 
> > ---
> >   libavformat/mxf.h    |  1 +
> >   libavformat/mxfenc.c | 42 +++++++++++++++++++++++++++++++++++--
> > -----
> >   2 files changed, 36 insertions(+), 7 deletions(-)
> > 
> > 
> > @@ -1476,6 +1495,15 @@ static int
> > mxf_write_header_metadata_sets(AVFormatContext
> > *s)
> >           }
> >       }
> >   +    entry = av_dict_get(s->metadata, "reel_name", NULL, 0);
> > +    if (entry) {
> > +        packages[2].name = entry->value;
> > +        packages[2].type = SourcePackage;
> > +        packages[2].instance = 2;
> > +        packages[1].ref = &packages[2];
> > +        package_count = 3;
> > +    }
> > 
> 
> I guess we can have it check track metadata later if we feel like it.
> Did a
> patch moving reel_name into AVFormatContext make it into mxfdec yet?
> mxfenc's output surviving a roundtrip through mxfdec + mxfenc might
> be
> desirable:
> 
>     ffmpeg -i somefile_with_reel_name.mkv output.mxf
>     ffmpeg -i output.mxf -vcodec copy -acodec copy remuxed.mxf
> 
> Ideally remuxed.mxf is identical to output.mxf
> 
> /Tomas
> 
> 
> Yes I agree that such behaviour is desirable.
> I haven't taken a look at mxfdec yet. Looking in steams for reel_name
> might
> be necessary, as I believe that's where mov,MP4 store it. But I was
> trying
> to keep it simple at first and addressed this specific issue a future
> patch.

Fair enough. I guess we can commit this patch series then. Michael?

/Tomas
Michael Niedermayer Dec. 8, 2017, 6:43 p.m.
On Fri, Dec 08, 2017 at 12:19:21AM +0100, Tomas Härdin wrote:
> On Thu, 2017-12-07 at 14:45 -0800, Mark Reid wrote:
> > On Dec 7, 2017 7:45 AM, "Tomas Härdin" <tjoppen@acc.umu.se> wrote:
> > 
> > On 2017-12-05 05:46, Mark Reid wrote:
> > 
> > > ---
> > >   libavformat/mxf.h    |  1 +
> > >   libavformat/mxfenc.c | 42 +++++++++++++++++++++++++++++++++++--
> > > -----
> > >   2 files changed, 36 insertions(+), 7 deletions(-)
> > > 
> > > 
> > > @@ -1476,6 +1495,15 @@ static int
> > > mxf_write_header_metadata_sets(AVFormatContext
> > > *s)
> > >           }
> > >       }
> > >   +    entry = av_dict_get(s->metadata, "reel_name", NULL, 0);
> > > +    if (entry) {
> > > +        packages[2].name = entry->value;
> > > +        packages[2].type = SourcePackage;
> > > +        packages[2].instance = 2;
> > > +        packages[1].ref = &packages[2];
> > > +        package_count = 3;
> > > +    }
> > > 
> > 
> > I guess we can have it check track metadata later if we feel like it.
> > Did a
> > patch moving reel_name into AVFormatContext make it into mxfdec yet?
> > mxfenc's output surviving a roundtrip through mxfdec + mxfenc might
> > be
> > desirable:
> > 
> >     ffmpeg -i somefile_with_reel_name.mkv output.mxf
> >     ffmpeg -i output.mxf -vcodec copy -acodec copy remuxed.mxf
> > 
> > Ideally remuxed.mxf is identical to output.mxf
> > 
> > /Tomas
> > 
> > 
> > Yes I agree that such behaviour is desirable.
> > I haven't taken a look at mxfdec yet. Looking in steams for reel_name
> > might
> > be necessary, as I believe that's where mov,MP4 store it. But I was
> > trying
> > to keep it simple at first and addressed this specific issue a future
> > patch.
> 
> Fair enough. I guess we can commit this patch series then. Michael?

will apply

thanks

[...]

Patch hide | download patch | download mbox

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 c9694f2a4e..3bb70326fe 100644
--- a/libavformat/mxfenc.c
+++ b/libavformat/mxfenc.c
@@ -105,6 +105,7 @@  typedef struct MXFPackage {
     char *name;
     enum MXFMetadataSetType type;
     int instance;
+    struct MXFPackage *ref;
 } MXFPackage;
 
 enum ULIndex {
@@ -991,20 +992,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;
@@ -1388,13 +1402,17 @@  static void mxf_write_package(AVFormatContext *s, MXFPackage *package)
     }
 
     // write multiple descriptor reference
-    if (package->type == SourcePackage) {
+    if (package->type == SourcePackage && 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->type == SourcePackage && package->instance == 2) {
+        mxf_write_local_tag(pb, 16, 0x4701);
+        mxf_write_uuid(pb, TapeDescriptor, 0);
+        mxf_write_tape_descriptor(s);
     }
 
     /*
@@ -1418,7 +1436,7 @@  static void mxf_write_package(AVFormatContext *s, MXFPackage *package)
         mxf_write_structural_component(s, st, package);
         mxf->track_instance_count++;
 
-        if (package->type == SourcePackage) {
+        if (package->type == SourcePackage && package->instance == 1) {
             MXFStreamContext *sc = st->priv_data;
             mxf_essence_container_uls[sc->index].write_desc(s, st);
         }
@@ -1453,12 +1471,13 @@  static int mxf_write_header_metadata_sets(AVFormatContext *s)
     AVDictionaryEntry *entry = NULL;
     AVStream *st = NULL;
     int i;
-
-    MXFPackage packages[2] = {{0}};
+    MXFPackage packages[3] = {{0}};
     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;
@@ -1476,6 +1495,15 @@  static int mxf_write_header_metadata_sets(AVFormatContext *s)
         }
     }
 
+    entry = av_dict_get(s->metadata, "reel_name", NULL, 0);
+    if (entry) {
+        packages[2].name = entry->value;
+        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);