[FFmpeg-devel] [PATCH] avformat/mxfenc: Add color siting element
Michael Niedermayer
michaelni at gmx.at
Tue May 19 15:11:12 CEST 2015
On Tue, May 19, 2015 at 12:07:24PM +0100, tim nicholson wrote:
> On 19/05/15 01:33, Michael Niedermayer wrote:
> > The default is assumed to be 0xFF, which is what the 2009 spec lists,
> > the older version i have lists 0 as the default.
> >
> > Signed-off-by: Michael Niedermayer <michaelni at gmx.at>
> > ---
> > libavformat/mxfenc.c | 28 +++++++++++++++++++++++++
> > tests/ref/lavf/mxf | 12 +++++------
> > tests/ref/lavf/mxf_d10 | 2 +-
> > tests/ref/lavf/mxf_opatom | 2 +-
> > tests/ref/lavf/mxf_opatom_audio | 2 +-
> > tests/ref/seek/lavf-mxf | 44 +++++++++++++++++++--------------------
> > 6 files changed, 59 insertions(+), 31 deletions(-)
> >
> > diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> > index 659c34f..0af3db1 100644
> > --- a/libavformat/mxfenc.c
> > +++ b/libavformat/mxfenc.c
> > @@ -79,6 +79,7 @@ typedef struct MXFStreamContext {
> > int interlaced; ///< whether picture is interlaced
> > int field_dominance; ///< tff=1, bff=2
> > int component_depth;
> > + int color_siting;
> > int h_chroma_sub_sample;
> > int temporal_reordering;
> > AVRational aspect_ratio; ///< display aspect ratio
> > @@ -416,6 +417,7 @@ static const MXFLocalTagPair mxf_local_tag_batch[] = {
> > // CDCI Picture Essence Descriptor
> > { 0x3301, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x02,0x04,0x01,0x05,0x03,0x0A,0x00,0x00,0x00}}, /* Component Depth */
> > { 0x3302, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x01,0x04,0x01,0x05,0x01,0x05,0x00,0x00,0x00}}, /* Horizontal Subsampling */
> > + { 0x3303, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x01,0x04,0x01,0x05,0x01,0x06,0x00,0x00,0x00}}, /* Color Siting */
> > // Generic Sound Essence Descriptor
> > { 0x3D02, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x04,0x04,0x02,0x03,0x01,0x04,0x00,0x00,0x00}}, /* Locked/Unlocked */
> > { 0x3D03, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x05,0x04,0x02,0x03,0x01,0x01,0x01,0x00,0x00}}, /* Audio sampling rate */
> > @@ -996,6 +998,8 @@ static void mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID ke
> > unsigned desc_size = size+8+8+8+8+8+8+8+5+16+sc->interlaced*4+12+20;
> > if (sc->interlaced && sc->field_dominance)
> > desc_size += 5;
> > + if (sc->color_siting != 0xFF)
> > + desc_size += 5;
> >
> > mxf_write_generic_desc(s, st, key, desc_size);
> >
> > @@ -1030,6 +1034,12 @@ static void mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID ke
> > mxf_write_local_tag(pb, 4, 0x3302);
> > avio_wb32(pb, sc->h_chroma_sub_sample);
> >
> > + if (sc->color_siting != 0xFF) {
> > + // color siting
> > + mxf_write_local_tag(pb, 1, 0x3303);
> > + avio_w8(pb, sc->color_siting);
> > + }
> > +
>
> If the value as far as we can determine is "unknown", should we not
> write that value (0xFF), rather than leave the metadata out, and hope
> that any reader will assume the 2009 default rather than the previous,
> different default? It would seem to me to be more universal.
>
> I'm generally wary of leaving out values just because they are some
> default especially if that default is subject to change.
i was trying to avoid breaking things by favoring not writing it
as is done in git currently.
But maybe iam too carefull here, do you think we can safely write
0xFF always ? (which could happen if the user does not provide any
information about the chroma location or pixel format
or should this be tested with some applications? if so with what
applications ?
>
> > // frame layout
> > mxf_write_local_tag(pb, 1, 0x320C);
> > avio_w8(pb, sc->interlaced);
> > @@ -2037,11 +2047,29 @@ static int mxf_write_header(AVFormatContext *s)
> > // Default component depth to 8
> > sc->component_depth = 8;
> > sc->h_chroma_sub_sample = 2;
> > + sc->color_siting = 0xFF;
> >
> > if (pix_desc) {
> > sc->component_depth = pix_desc->comp[0].depth_minus1 + 1;
> > sc->h_chroma_sub_sample = 1 << pix_desc->log2_chroma_w;
> > }
> > + switch (st->codec->chroma_sample_location) {
> > + case AVCHROMA_LOC_TOPLEFT: sc->color_siting = 0; break;
> > + case AVCHROMA_LOC_LEFT: sc->color_siting = 6; break;
> > + case AVCHROMA_LOC_TOP: sc->color_siting = 1; break;
> > + case AVCHROMA_LOC_CENTER: sc->color_siting = 3; break;
>
> If these mappings are correct, then the AVCHROMA_LOC_ names are
> certainly not intuitive (and the comments unhelpful), and bear little
> relation to the way SMPTE S377 describes the positioning!
the avchroma locations are explained by the ascii art above the enum:
* X X 3 4 X X are luma samples,
* 1 2 1-6 are possible chroma positions
* X X 5 6 X 0 is undefined/unknown position
and from this the english directions are exactly where the chroma
samples are located, this also matches how H264 defines chroma
locations.
mapping it to the mxf descriptions isnt all that easy, IMHO because
the mxf descriptions are rather bad
[...]
>
> > + case AVCHROMA_LOC_UNSPECIFIED:
> > + if (pix_desc) {
> > + if (pix_desc->log2_chroma_h == 0) {
> > + sc->color_siting = 0;
> > + } else if (pix_desc->log2_chroma_w == 1 && pix_desc->log2_chroma_h == 1) {
> > + switch (st->codec->codec_id) {
> > + case AV_CODEC_ID_MPEG2VIDEO: sc->color_siting = 6; break;
>
> Only true for 4:2:0 mpeg2, what about 4:2:2 (XDCAM-HD) or does the 'if'
> filter that out? (I'm not that familiar with the pix_desc struct).
the if implies 4:2:0
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
In fact, the RIAA has been known to suggest that students drop out
of college or go to community college in order to be able to afford
settlements. -- The RIAA
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150519/a930c6f8/attachment.asc>
More information about the ffmpeg-devel
mailing list