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

[Tomas Härdin]

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 at 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 at 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