| Message ID | 20200831190756.80515-2-harry.mallon@codex.online |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel,1/3] avformat/mxfdec: Read Mastering Display Colour Volume from MXF | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On Mon, 31 Aug 2020, Harry Mallon wrote: > Described in Annex B SMPTE ST 2067-21:2020 > > Signed-off-by: Harry Mallon <harry.mallon@codex.online> > --- > libavformat/mxfenc.c | 56 ++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 56 insertions(+) > > diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c > index e495b5ba0e..fe1ecb6705 100644 > --- a/libavformat/mxfenc.c > +++ b/libavformat/mxfenc.c > @@ -44,6 +44,7 @@ > #include "libavutil/random_seed.h" > #include "libavutil/timecode.h" > #include "libavutil/avassert.h" > +#include "libavutil/mastering_display_metadata.h" > #include "libavutil/pixdesc.h" > #include "libavutil/time_internal.h" > #include "libavcodec/bytestream.h" > @@ -421,6 +422,13 @@ static const MXFLocalTagPair mxf_user_comments_local_tag[] = { > { 0x5003, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x02,0x03,0x02,0x01,0x02,0x0A,0x01,0x00,0x00}}, /* Value */ > }; > > +static const MXFLocalTagPair mxf_mastering_display_local_tags[] = { > + { 0x8201, {0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0e,0x04,0x20,0x04,0x01,0x01,0x01,0x00,0x00}}, /* Mastering Display Primaries */ > + { 0x8202, {0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0e,0x04,0x20,0x04,0x01,0x01,0x02,0x00,0x00}}, /* Mastering Display White Point Chromaticity */ > + { 0x8203, {0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0e,0x04,0x20,0x04,0x01,0x01,0x03,0x00,0x00}}, /* Mastering Display Maximum Luminance */ > + { 0x8204, {0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0e,0x04,0x20,0x04,0x01,0x01,0x04,0x00,0x00}}, /* Mastering Display Minimum Luminance */ Ain't these collide with AVC subdescriptor local tags? > +}; > + > static void mxf_write_uuid(AVIOContext *pb, enum MXFMetadataSetType type, int value) > { > avio_write(pb, uuid_base, 12); > @@ -510,6 +518,7 @@ static void mxf_write_primer_pack(AVFormatContext *s) > AVIOContext *pb = s->pb; > int local_tag_number, i = 0; > int avc_tags_count = 0; > + int mastering_tags_count = 0; > > local_tag_number = FF_ARRAY_ELEMS(mxf_local_tag_batch); > local_tag_number += mxf->store_user_comments * FF_ARRAY_ELEMS(mxf_user_comments_local_tag); > @@ -522,6 +531,15 @@ static void mxf_write_primer_pack(AVFormatContext *s) > } > } > > + for (i = 0; i < s->nb_streams; i++) { > + uint8_t *side_data = av_stream_get_side_data(s->streams[i], AV_PKT_DATA_MASTERING_DISPLAY_METADATA, NULL); You can get rid of the side_data temporary variable and use the function call directly in the if condition. > + if (side_data) { > + mastering_tags_count = FF_ARRAY_ELEMS(mxf_mastering_display_local_tags); > + local_tag_number += mastering_tags_count; > + break; > + } > + } Push this into the previous loop. Only increase local_tag_number out of the loop, this way you can get rid of the break. (Similar change might make sense for the existing avc_tags_count code as well, because it is super confusing that it increases local_tag_count for every stream...) > + > avio_write(pb, primer_pack_key, 16); > klv_encode_ber_length(pb, local_tag_number * 18 + 8); > > @@ -539,6 +557,8 @@ static void mxf_write_primer_pack(AVFormatContext *s) > } > if (avc_tags_count > 0) > mxf_write_local_tags(pb, mxf_avc_subdescriptor_local_tags, avc_tags_count); > + if (mastering_tags_count > 0) > + mxf_write_local_tags(pb, mxf_mastering_display_local_tags, mastering_tags_count); > } > > static void mxf_write_local_tag(AVIOContext *pb, int size, int tag) > @@ -1048,6 +1068,11 @@ 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 inline int64_t rescale_mastering(AVRational q, int b) > +{ > + return av_rescale(q.num, b, q.den); Might make sense to av_clip_uint16 the result and change return type accordingly? > +} > + > static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID key) > { > MXFStreamContext *sc = st->priv_data; > @@ -1060,6 +1085,7 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID > const MXFCodecUL *color_trc_ul; > const MXFCodecUL *color_space_ul; > int64_t pos = mxf_write_generic_desc(s, st, key); > + uint8_t *side_data; > > 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); > @@ -1228,6 +1254,36 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID > mxf_write_local_tag(pb, 16, 0x3201); > avio_write(pb, *sc->codec_ul, 16); > > + // Mastering Display metadata > + side_data = av_stream_get_side_data(st, AV_PKT_DATA_MASTERING_DISPLAY_METADATA, NULL); > + if (side_data) { > + const AVMasteringDisplayMetadata *metadata = (const AVMasteringDisplayMetadata*)side_data; > + const int chroma_den = 50000; > + const int luma_den = 10000; > + if (metadata->has_primaries) { > + mxf_write_local_tag(pb, 12, 0x8201); > + avio_wb16(pb, rescale_mastering(metadata->display_primaries[0][0], chroma_den)); > + avio_wb16(pb, rescale_mastering(metadata->display_primaries[0][1], chroma_den)); > + avio_wb16(pb, rescale_mastering(metadata->display_primaries[1][0], chroma_den)); > + avio_wb16(pb, rescale_mastering(metadata->display_primaries[1][1], chroma_den)); > + avio_wb16(pb, rescale_mastering(metadata->display_primaries[2][0], chroma_den)); > + avio_wb16(pb, rescale_mastering(metadata->display_primaries[2][1], chroma_den)); > + mxf_write_local_tag(pb, 4, 0x8202); > + avio_wb16(pb, rescale_mastering(metadata->white_point[0], chroma_den)); > + avio_wb16(pb, rescale_mastering(metadata->white_point[1], chroma_den)); > + } else { > + av_log(NULL, AV_LOG_VERBOSE, "Not writing mastering display primaries. Missing data.\n"); > + } > + if (metadata->has_luminance) { > + mxf_write_local_tag(pb, 4, 0x8203); > + avio_wb32(pb, rescale_mastering(metadata->max_luminance, luma_den)); > + mxf_write_local_tag(pb, 4, 0x8204); > + avio_wb32(pb, rescale_mastering(metadata->min_luminance, luma_den)); > + } else { > + av_log(NULL, AV_LOG_VERBOSE, "Not writing mastering display luminances. Missing data.\n"); > + } > + } > + Thanks, Marton
mån 2020-08-31 klockan 20:07 +0100 skrev Harry Mallon: > +static const MXFLocalTagPair mxf_mastering_display_local_tags[] = { > + { 0x8201, {0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0e,0x04,0x20,0x04,0x01,0x01,0x01,0x00,0x00}}, /* Mastering Display Primaries */ > + { 0x8202, {0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0e,0x04,0x20,0x04,0x01,0x01,0x02,0x00,0x00}}, /* Mastering Display White Point Chromaticity */ > + { 0x8203, {0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0e,0x04,0x20,0x04,0x01,0x01,0x03,0x00,0x00}}, /* Mastering Display Maximum Luminance */ > + { 0x8204, {0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0e,0x04,0x20,0x04,0x01,0x01,0x04,0x00,0x00}}, /* Mastering Display Minimum Luminance */ > +}; These conflict with existing local tags, as Marton says. It also strikes me that these duplicate the keys already defined in patch 1. Consider having both mxfdec and mxfenc share tables for them. Maybe a better solution would be to have mxfenc automagically come up with local tags for custom ULs, so we avoid these kinds of collisions in the future. /Tomas
> On 3 Sep 2020, at 07:31, Marton Balint <cus@passwd.hu> wrote: > > On Mon, 31 Aug 2020, Harry Mallon wrote: > >> [..] >> }; >> +static const MXFLocalTagPair mxf_mastering_display_local_tags[] = { >> + { 0x8201, {0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0e,0x04,0x20,0x04,0x01,0x01,0x01,0x00,0x00}}, /* Mastering Display Primaries */ >> + { 0x8202, {0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0e,0x04,0x20,0x04,0x01,0x01,0x02,0x00,0x00}}, /* Mastering Display White Point Chromaticity */ >> + { 0x8203, {0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0e,0x04,0x20,0x04,0x01,0x01,0x03,0x00,0x00}}, /* Mastering Display Maximum Luminance */ >> + { 0x8204, {0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0e,0x04,0x20,0x04,0x01,0x01,0x04,0x00,0x00}}, /* Mastering Display Minimum Luminance */ > > Ain't these collide with AVC subdescriptor local tags? Yep, doh. Fixed in v2. > >> +}; >> + >> static void mxf_write_uuid(AVIOContext *pb, enum MXFMetadataSetType type, int value) >> { >> avio_write(pb, uuid_base, 12); >> @@ -510,6 +518,7 @@ static void mxf_write_primer_pack(AVFormatContext *s) >> AVIOContext *pb = s->pb; >> int local_tag_number, i = 0; >> int avc_tags_count = 0; >> + int mastering_tags_count = 0; >> >> local_tag_number = FF_ARRAY_ELEMS(mxf_local_tag_batch); >> local_tag_number += mxf->store_user_comments * FF_ARRAY_ELEMS(mxf_user_comments_local_tag); >> @@ -522,6 +531,15 @@ static void mxf_write_primer_pack(AVFormatContext *s) >> } >> } >> + for (i = 0; i < s->nb_streams; i++) { >> + uint8_t *side_data = av_stream_get_side_data(s->streams[i], AV_PKT_DATA_MASTERING_DISPLAY_METADATA, NULL); > > You can get rid of the side_data temporary variable and use the function call directly in the if condition. This and other comments fixed in v2 (also Tomas' comments) > >> + if (side_data) { >> + mastering_tags_count = FF_ARRAY_ELEMS(mxf_mastering_display_local_tags); >> + local_tag_number += mastering_tags_count; >> + break; >> + } >> + } > > [..] >> + Thanks for the review Best, Harry
diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c index e495b5ba0e..fe1ecb6705 100644 --- a/libavformat/mxfenc.c +++ b/libavformat/mxfenc.c @@ -44,6 +44,7 @@ #include "libavutil/random_seed.h" #include "libavutil/timecode.h" #include "libavutil/avassert.h" +#include "libavutil/mastering_display_metadata.h" #include "libavutil/pixdesc.h" #include "libavutil/time_internal.h" #include "libavcodec/bytestream.h" @@ -421,6 +422,13 @@ static const MXFLocalTagPair mxf_user_comments_local_tag[] = { { 0x5003, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x02,0x03,0x02,0x01,0x02,0x0A,0x01,0x00,0x00}}, /* Value */ }; +static const MXFLocalTagPair mxf_mastering_display_local_tags[] = { + { 0x8201, {0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0e,0x04,0x20,0x04,0x01,0x01,0x01,0x00,0x00}}, /* Mastering Display Primaries */ + { 0x8202, {0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0e,0x04,0x20,0x04,0x01,0x01,0x02,0x00,0x00}}, /* Mastering Display White Point Chromaticity */ + { 0x8203, {0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0e,0x04,0x20,0x04,0x01,0x01,0x03,0x00,0x00}}, /* Mastering Display Maximum Luminance */ + { 0x8204, {0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0e,0x04,0x20,0x04,0x01,0x01,0x04,0x00,0x00}}, /* Mastering Display Minimum Luminance */ +}; + static void mxf_write_uuid(AVIOContext *pb, enum MXFMetadataSetType type, int value) { avio_write(pb, uuid_base, 12); @@ -510,6 +518,7 @@ static void mxf_write_primer_pack(AVFormatContext *s) AVIOContext *pb = s->pb; int local_tag_number, i = 0; int avc_tags_count = 0; + int mastering_tags_count = 0; local_tag_number = FF_ARRAY_ELEMS(mxf_local_tag_batch); local_tag_number += mxf->store_user_comments * FF_ARRAY_ELEMS(mxf_user_comments_local_tag); @@ -522,6 +531,15 @@ static void mxf_write_primer_pack(AVFormatContext *s) } } + for (i = 0; i < s->nb_streams; i++) { + uint8_t *side_data = av_stream_get_side_data(s->streams[i], AV_PKT_DATA_MASTERING_DISPLAY_METADATA, NULL); + if (side_data) { + mastering_tags_count = FF_ARRAY_ELEMS(mxf_mastering_display_local_tags); + local_tag_number += mastering_tags_count; + break; + } + } + avio_write(pb, primer_pack_key, 16); klv_encode_ber_length(pb, local_tag_number * 18 + 8); @@ -539,6 +557,8 @@ static void mxf_write_primer_pack(AVFormatContext *s) } if (avc_tags_count > 0) mxf_write_local_tags(pb, mxf_avc_subdescriptor_local_tags, avc_tags_count); + if (mastering_tags_count > 0) + mxf_write_local_tags(pb, mxf_mastering_display_local_tags, mastering_tags_count); } static void mxf_write_local_tag(AVIOContext *pb, int size, int tag) @@ -1048,6 +1068,11 @@ 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 inline int64_t rescale_mastering(AVRational q, int b) +{ + return av_rescale(q.num, b, q.den); +} + static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID key) { MXFStreamContext *sc = st->priv_data; @@ -1060,6 +1085,7 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID const MXFCodecUL *color_trc_ul; const MXFCodecUL *color_space_ul; int64_t pos = mxf_write_generic_desc(s, st, key); + uint8_t *side_data; 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); @@ -1228,6 +1254,36 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID mxf_write_local_tag(pb, 16, 0x3201); avio_write(pb, *sc->codec_ul, 16); + // Mastering Display metadata + side_data = av_stream_get_side_data(st, AV_PKT_DATA_MASTERING_DISPLAY_METADATA, NULL); + if (side_data) { + const AVMasteringDisplayMetadata *metadata = (const AVMasteringDisplayMetadata*)side_data; + const int chroma_den = 50000; + const int luma_den = 10000; + if (metadata->has_primaries) { + mxf_write_local_tag(pb, 12, 0x8201); + avio_wb16(pb, rescale_mastering(metadata->display_primaries[0][0], chroma_den)); + avio_wb16(pb, rescale_mastering(metadata->display_primaries[0][1], chroma_den)); + avio_wb16(pb, rescale_mastering(metadata->display_primaries[1][0], chroma_den)); + avio_wb16(pb, rescale_mastering(metadata->display_primaries[1][1], chroma_den)); + avio_wb16(pb, rescale_mastering(metadata->display_primaries[2][0], chroma_den)); + avio_wb16(pb, rescale_mastering(metadata->display_primaries[2][1], chroma_den)); + mxf_write_local_tag(pb, 4, 0x8202); + avio_wb16(pb, rescale_mastering(metadata->white_point[0], chroma_den)); + avio_wb16(pb, rescale_mastering(metadata->white_point[1], chroma_den)); + } else { + av_log(NULL, AV_LOG_VERBOSE, "Not writing mastering display primaries. Missing data.\n"); + } + if (metadata->has_luminance) { + mxf_write_local_tag(pb, 4, 0x8203); + avio_wb32(pb, rescale_mastering(metadata->max_luminance, luma_den)); + mxf_write_local_tag(pb, 4, 0x8204); + avio_wb32(pb, rescale_mastering(metadata->min_luminance, luma_den)); + } else { + av_log(NULL, AV_LOG_VERBOSE, "Not writing mastering display luminances. Missing data.\n"); + } + } + if (sc->interlaced && sc->field_dominance) { mxf_write_local_tag(pb, 1, 0x3212); avio_w8(pb, sc->field_dominance);
Described in Annex B SMPTE ST 2067-21:2020 Signed-off-by: Harry Mallon <harry.mallon@codex.online> --- libavformat/mxfenc.c | 56 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+)