diff mbox series

[FFmpeg-devel] avformat/mxfenc: Write color metadata to MXF

Message ID 38F7B904-4988-45CD-BC4C-68FDABA578BA@codex.online
State Accepted
Commit 64ff61b3c52af335e811fe04b85108775e1f2784
Headers show
Series [FFmpeg-devel] avformat/mxfenc: Write color metadata to MXF
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Harry Mallon Aug. 6, 2020, 2:27 p.m. UTC
I attach a patch to apply the colour metadata that was read in my previous commit during mxf encoding. ffmpeg previously wrote the transfer characteristic only, and not for all supported transfer curves. Now it will do transfer characteristic, colour primaries and colour space.

Best,
Harry

From de460620b73379d5a869baa98e49a5d0f67d9ebb Mon Sep 17 00:00:00 2001

From: Harry Mallon <harry.mallon@codex.online>
Date: Tue, 28 Jul 2020 10:33:19 +0100
Subject: [PATCH] avformat/mxfenc: Write color metadata to MXF

Writes color_primaries, color_trc and color_space to mxf
headers. ULs are from https://registry.smpte-ra.org/ site.

Signed-off-by: Harry Mallon <harry.mallon@codex.online>
---
 libavformat/mxfenc.c | 58 ++++++++++++++++----------------------------
 1 file changed, 21 insertions(+), 37 deletions(-)

Comments

Tomas Härdin Aug. 11, 2020, 1:15 p.m. UTC | #1
tor 2020-08-06 klockan 15:27 +0100 skrev Harry Mallon:
> I attach a patch to apply the colour metadata that was read in my
> previous commit during mxf encoding. ffmpeg previously wrote the
> transfer characteristic only, and not for all supported transfer
> curves. Now it will do transfer characteristic, colour primaries and
> colour space.
> 
> Best,
> Harry
> 
> From de460620b73379d5a869baa98e49a5d0f67d9ebb Mon Sep 17 00:00:00 2001
> 
> From: Harry Mallon <harry.mallon@codex.online>
> Date: Tue, 28 Jul 2020 10:33:19 +0100
> Subject: [PATCH] avformat/mxfenc: Write color metadata to MXF
> 
> Writes color_primaries, color_trc and color_space to mxf
> headers. ULs are from https://registry.smpte-ra.org/ site.
> 
> Signed-off-by: Harry Mallon <harry.mallon@codex.online>
> ---
>  libavformat/mxfenc.c | 58 ++++++++++++++++----------------------------
>  1 file changed, 21 insertions(+), 37 deletions(-)
> 
> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> index 5a3a609bf6..a38fa6b983 100644
> --- a/libavformat/mxfenc.c
> +++ b/libavformat/mxfenc.c
> @@ -553,11 +553,10 @@ static void mxf_write_metadata_key(AVIOContext *pb, unsigned int value)
>      avio_wb24(pb, value);
>  }
>  
> -static const MXFCodecUL *mxf_get_data_definition_ul(int type)
> +static const MXFCodecUL *mxf_get_codec_ul_by_id(const MXFCodecUL *uls, int id)
>  {
> -    const MXFCodecUL *uls = ff_mxf_data_definition_uls;
>      while (uls->uid[0]) {
> -        if (type == uls->id)
> +        if (id == uls->id)
>              break;
>          uls++;
>      }

I feel like this and mxf_get_codec_ul() could be moved into mxf.* since
they both look up a MXFCodecUL*. One does lookup on id, the other on
ul. Doesn't need to hold up this patch though.

> @@ -1085,10 +1056,14 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
>      int stored_height = (st->codecpar->height+15)/16*16;
>      int display_height;
>      int f1, f2;
> -    UID transfer_ul = {0};
> +    const MXFCodecUL *color_primaries_ul;
> +    const MXFCodecUL *color_trc_ul;
> +    const MXFCodecUL *color_space_ul;
>      int64_t pos = mxf_write_generic_desc(s, st, key);
>  
> -    get_trc(transfer_ul, st->codecpar->color_trc);
> +    color_primaries_ul = mxf_get_codec_ul_by_id(ff_mxf_color_primaries_uls, st->codecpar->color_primaries);
> +    color_trc_ul       = mxf_get_codec_ul_by_id(ff_mxf_color_trc_uls, st->codecpar->color_trc);
> +    color_space_ul     = mxf_get_codec_ul_by_id(ff_mxf_color_space_uls, st->codecpar->color_space);
>  
>      if (st->codecpar->codec_id == AV_CODEC_ID_DVVIDEO) {
>          if (st->codecpar->height == 1080)
> @@ -1235,10 +1210,19 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
>      avio_wb32(pb, sc->aspect_ratio.num);
>      avio_wb32(pb, sc->aspect_ratio.den);
>  
> -    //Transfer characteristic
> -    if (transfer_ul[0]) {
> +    if (color_primaries_ul->uid[0]) {
> +        mxf_write_local_tag(pb, 16, 0x3219);

Maybe we should add local_tag to MXFCodecUL. Would simplify a lot of
code like this.

Patch looks OK and makes the code neater. I'll wait a day or two before
pushing in case anyone else has opinions.

/Tomas
Tomas Härdin Aug. 12, 2020, 2:18 p.m. UTC | #2
tis 2020-08-11 klockan 15:15 +0200 skrev Tomas Härdin:
> tor 2020-08-06 klockan 15:27 +0100 skrev Harry Mallon:
> > I attach a patch to apply the colour metadata that was read in my
> > previous commit during mxf encoding. ffmpeg previously wrote the
> > transfer characteristic only, and not for all supported transfer
> > curves. Now it will do transfer characteristic, colour primaries and
> > colour space.
> > 
> > Best,
> > Harry
> > 
> > From de460620b73379d5a869baa98e49a5d0f67d9ebb Mon Sep 17 00:00:00 2001
> > 
> > From: Harry Mallon <harry.mallon@codex.online>
> > Date: Tue, 28 Jul 2020 10:33:19 +0100
> > Subject: [PATCH] avformat/mxfenc: Write color metadata to MXF
> > 
> > Writes color_primaries, color_trc and color_space to mxf
> > headers. ULs are from https://registry.smpte-ra.org/ site.
> > 
> > Signed-off-by: Harry Mallon <harry.mallon@codex.online>
> > ---
> >  libavformat/mxfenc.c | 58 ++++++++++++++++----------------------------
> >  1 file changed, 21 insertions(+), 37 deletions(-)
> > 
> > diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> > index 5a3a609bf6..a38fa6b983 100644
> > --- a/libavformat/mxfenc.c
> > +++ b/libavformat/mxfenc.c
> > @@ -553,11 +553,10 @@ static void mxf_write_metadata_key(AVIOContext *pb, unsigned int value)
> >      avio_wb24(pb, value);
> >  }
> >  
> > -static const MXFCodecUL *mxf_get_data_definition_ul(int type)
> > +static const MXFCodecUL *mxf_get_codec_ul_by_id(const MXFCodecUL *uls, int id)
> >  {
> > -    const MXFCodecUL *uls = ff_mxf_data_definition_uls;
> >      while (uls->uid[0]) {
> > -        if (type == uls->id)
> > +        if (id == uls->id)
> >              break;
> >          uls++;
> >      }
> 
> I feel like this and mxf_get_codec_ul() could be moved into mxf.* since
> they both look up a MXFCodecUL*. One does lookup on id, the other on
> ul. Doesn't need to hold up this patch though.
> 
> > @@ -1085,10 +1056,14 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
> >      int stored_height = (st->codecpar->height+15)/16*16;
> >      int display_height;
> >      int f1, f2;
> > -    UID transfer_ul = {0};
> > +    const MXFCodecUL *color_primaries_ul;
> > +    const MXFCodecUL *color_trc_ul;
> > +    const MXFCodecUL *color_space_ul;
> >      int64_t pos = mxf_write_generic_desc(s, st, key);
> >  
> > -    get_trc(transfer_ul, st->codecpar->color_trc);
> > +    color_primaries_ul = mxf_get_codec_ul_by_id(ff_mxf_color_primaries_uls, st->codecpar->color_primaries);
> > +    color_trc_ul       = mxf_get_codec_ul_by_id(ff_mxf_color_trc_uls, st->codecpar->color_trc);
> > +    color_space_ul     = mxf_get_codec_ul_by_id(ff_mxf_color_space_uls, st->codecpar->color_space);
> >  
> >      if (st->codecpar->codec_id == AV_CODEC_ID_DVVIDEO) {
> >          if (st->codecpar->height == 1080)
> > @@ -1235,10 +1210,19 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
> >      avio_wb32(pb, sc->aspect_ratio.num);
> >      avio_wb32(pb, sc->aspect_ratio.den);
> >  
> > -    //Transfer characteristic
> > -    if (transfer_ul[0]) {
> > +    if (color_primaries_ul->uid[0]) {
> > +        mxf_write_local_tag(pb, 16, 0x3219);
> 
> Maybe we should add local_tag to MXFCodecUL. Would simplify a lot of
> code like this.
> 
> Patch looks OK and makes the code neater. I'll wait a day or two before
> pushing in case anyone else has opinions.

Pushed

/Tomas
diff mbox series

Patch

diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
index 5a3a609bf6..a38fa6b983 100644
--- a/libavformat/mxfenc.c
+++ b/libavformat/mxfenc.c
@@ -553,11 +553,10 @@  static void mxf_write_metadata_key(AVIOContext *pb, unsigned int value)
     avio_wb24(pb, value);
 }
 
-static const MXFCodecUL *mxf_get_data_definition_ul(int type)
+static const MXFCodecUL *mxf_get_codec_ul_by_id(const MXFCodecUL *uls, int id)
 {
-    const MXFCodecUL *uls = ff_mxf_data_definition_uls;
     while (uls->uid[0]) {
-        if (type == uls->id)
+        if (id == uls->id)
             break;
         uls++;
     }
@@ -847,7 +846,7 @@  static void mxf_write_common_fields(AVFormatContext *s, AVStream *st)
     if (st == mxf->timecode_track)
         avio_write(pb, smpte_12m_timecode_track_data_ul, 16);
     else {
-        const MXFCodecUL *data_def_ul = mxf_get_data_definition_ul(st->codecpar->codec_type);
+        const MXFCodecUL *data_def_ul = mxf_get_codec_ul_by_id(ff_mxf_data_definition_uls, st->codecpar->codec_type);
         avio_write(pb, data_def_ul->uid, 16);
     }
 
@@ -1049,34 +1048,6 @@  static const UID mxf_generic_sound_descriptor_key = { 0x06,0x0E,0x2B,0x34,0x02,0
 
 static const UID mxf_avc_subdescriptor_key = { 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x6E,0x00 };
 
-static int get_trc(UID ul, enum AVColorTransferCharacteristic trc)
-{
-    switch (trc){
-    case AVCOL_TRC_GAMMA28   :
-    case AVCOL_TRC_GAMMA22   :
-        memcpy(ul, ((UID){0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x04,0x01,0x01,0x01,0x01,0x01,0x00,0x00}), 16);
-        return 0;
-    case AVCOL_TRC_BT709     :
-    case AVCOL_TRC_SMPTE170M :
-        memcpy(ul, ((UID){0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x04,0x01,0x01,0x01,0x01,0x02,0x00,0x00}), 16);
-        return 0;
-    case AVCOL_TRC_SMPTE240M :
-        memcpy(ul, ((UID){0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x04,0x01,0x01,0x01,0x01,0x03,0x00,0x00}), 16);
-        return 0;
-    case AVCOL_TRC_BT1361_ECG:
-        memcpy(ul, ((UID){0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x06,0x04,0x01,0x01,0x01,0x01,0x05,0x00,0x00}), 16);
-        return 0;
-    case AVCOL_TRC_LINEAR    :
-        memcpy(ul, ((UID){0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x06,0x04,0x01,0x01,0x01,0x01,0x06,0x00,0x00}), 16);
-        return 0;
-    case AVCOL_TRC_SMPTE428  :
-        memcpy(ul, ((UID){0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x08,0x04,0x01,0x01,0x01,0x01,0x07,0x00,0x00}), 16);
-        return 0;
-    default:
-        return -1;
-    }
-}
-
 static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID key)
 {
     MXFStreamContext *sc = st->priv_data;
@@ -1085,10 +1056,14 @@  static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
     int stored_height = (st->codecpar->height+15)/16*16;
     int display_height;
     int f1, f2;
-    UID transfer_ul = {0};
+    const MXFCodecUL *color_primaries_ul;
+    const MXFCodecUL *color_trc_ul;
+    const MXFCodecUL *color_space_ul;
     int64_t pos = mxf_write_generic_desc(s, st, key);
 
-    get_trc(transfer_ul, st->codecpar->color_trc);
+    color_primaries_ul = mxf_get_codec_ul_by_id(ff_mxf_color_primaries_uls, st->codecpar->color_primaries);
+    color_trc_ul       = mxf_get_codec_ul_by_id(ff_mxf_color_trc_uls, st->codecpar->color_trc);
+    color_space_ul     = mxf_get_codec_ul_by_id(ff_mxf_color_space_uls, st->codecpar->color_space);
 
     if (st->codecpar->codec_id == AV_CODEC_ID_DVVIDEO) {
         if (st->codecpar->height == 1080)
@@ -1235,10 +1210,19 @@  static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
     avio_wb32(pb, sc->aspect_ratio.num);
     avio_wb32(pb, sc->aspect_ratio.den);
 
-    //Transfer characteristic
-    if (transfer_ul[0]) {
+    if (color_primaries_ul->uid[0]) {
+        mxf_write_local_tag(pb, 16, 0x3219);
+        avio_write(pb, color_primaries_ul->uid, 16);
+    };
+
+    if (color_trc_ul->uid[0]) {
         mxf_write_local_tag(pb, 16, 0x3210);
-        avio_write(pb, transfer_ul, 16);
+        avio_write(pb, color_trc_ul->uid, 16);
+    };
+
+    if (color_space_ul->uid[0]) {
+        mxf_write_local_tag(pb, 16, 0x321A);
+        avio_write(pb, color_space_ul->uid, 16);
     };
 
     mxf_write_local_tag(pb, 16, 0x3201);