[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