[FFmpeg-devel] [PATCH] avformat/mxfenc: Add color siting element
Michael Niedermayer
michaelni at gmx.at
Tue May 19 17:50:55 CEST 2015
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 <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 ?
> >
>
> 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
-------------- 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/d1a1d619/attachment.asc>
More information about the ffmpeg-devel
mailing list