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

Michael Niedermayer michaelni at gmx.at
Thu May 21 12:59:38 CEST 2015


On Thu, May 21, 2015 at 09:00:56AM +0200, Tomas Härdin wrote:
> 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?

https://ffmpeg.org/pipermail/ffmpeg-user/2015-March/025730.html


> What's your test for that use case?

i would ask the person who asked me to implement this if it works for
him


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

nor am i, ill try to move it into common code

thanks

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Breaking DRM is a little like attempting to break through a door even
though the window is wide open and the only thing in the house is a bunch
of things you dont want and which you would get tomorrow for free anyway
-------------- 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/20150521/287156e4/attachment.asc>


More information about the ffmpeg-devel mailing list