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 <michae...@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
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
