| 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 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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
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 --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);