[FFmpeg-devel] [PATCH] avformat/mxfenc: Add color siting element

[Tomas Härdin]

On Tue, 2015-05-19 at 15:35 +0100, tim nicholson wrote:
> On 19/05/15 14:11, Michael Niedermayer wrote:
> > 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,0x
> 05,0x03,0x0A,0x00,0x00,0x00}}, /* Component Depth */
> >>>      { 0x3302, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x01,0x04,0x01,0x
> 05,0x01,0x05,0x00,0x00,0x00}}, /* Horizontal Subsampling */
> >>> +    { 0x3303, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x01,0x04,0x01,0x
> 05,0x01,0x06,0x00,0x00,0x00}}, /* Color Siting */
> >>>      // Generic Sound Essence Descriptor
> >>>      { 0x3D02, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x04,0x04,0x02,0x
> 03,0x01,0x04,0x00,0x00,0x00}}, /* Locked/Unlocked */
> >>>      { 0x3D03, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x05,0x04,0x02,0x
> 03,0x01,0x01,0x01,0x00,0x00}}, /* Audio sampling rate */
> >>> @@ -996,6 +998,8 @@ static void mxf_write_cdci_common(AVFormatContex
> t *s, AVStream *st, const UID ke
> >>>      unsigned desc_size = size+8+8+8+8+8+8+8+5+16+sc->interlaced*4+1
> 2+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(AVFormatCon
> text *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
> 
> I should say so, but I wonder if Tomas has a view.
> 
> > 
> > or should this be tested with some applications? if so with what
> > applications ?

As patch submitter I assume you have an application in mind. MXF isn't
really something you just implement stuff for on a lark. What's your use
case? What's your test for that use case?

> Currently bmxlib reports an ffmpeg.mxf as:-
> 
> color_siting    : Unknown (value='255')
> 
> i.e, in the absence of the metadata entry it "assumes" the correct
> default, so forcibly setting it will make no difference here (or with
> any other up to date reader). Its only for anything conforming to the
> old standard of which I do not know a sample.

Sounds reasonable to me.

> > [...]
> >>
> >>> +            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_siti
> ng = 6; break;
> >>
> >> Only true for 4:2:0 mpeg2, what about 4:2:2 (XDCAM-HD) or does the 'i
> f'
> >> filter that out? (I'm not that familiar with the pix_desc struct).
> > 
> > the if implies 4:2:0
> > 
> 
> Ah fine then.

Why is this "guessing" code in mxfenc? This should be something that's
taken care of before calling any muxer (like in
avformat_write_header()), so everyone can benefit from it and so there's
less risk of duplication when it's needed elsewhere.

I've seen stunts like this pulled in more places than mxfenc, where
muxers will do dubious things like parse certain kinds of essence. I am
not a fan.

/Tomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150521/b7ec9f21/attachment.asc>