On Tue, May 19, 2015 at 03:35:42PM +0100, tim nicholson wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > 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 <[email protected]> > >>> --- > >>> 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 ? > > > > 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.
ok, ill change it to unconditionally store the value then in absence
of other comments
>
> >
> >>
> >>> // 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_m
> inus1 + 1;
> >>> sc->h_chroma_sub_sample = 1 << pix_desc->log2_chrom
> a_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.
>
> I cannot get the ascii art to make sense to me at all, it just doesn't
> click. I do no see how you can represent "co-siting" in ascii art with a
> single character and the "3" position certainly doesn't do it for me!
ascii art improved
does it make sense now ?
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
No human being will ever know the Truth, for even if they happen to say it
by chance, they would not even known they had done so. -- Xenophanes
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list [email protected] http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
