-----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 <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 ?
>
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.
>
>>
>>> // 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!
Not that I think your mappings are wrong, I just can't make sense of it
in my head.
> mapping it to the mxf descriptions isnt all that easy, IMHO because
> the mxf descriptions are rather bad
Funny really 'cos I find them fairly straightforward.
Oh how different brains work!
>
>
> [...]
>>
>>> + 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.
> [...]
- --
Tim.
Key Fingerprint 38CF DB09 3ED0 F607 8B67 6CED 0C0B FC44 8B0B FC83
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)
iQEcBAEBAgAGBQJVW0o+AAoJEAwL/ESLC/yDrngH/2xrMgQmm15hNI81dM6yGm/N
64zffNoeVwDZM4wJKqmfYQlMlvPZn84agXef65XBjB3YG52RVqu+0dHaAZKdw4Ep
NEXMC1pvRVwEu5UNFDeNplhAMbu+pkOMM6v3qLX3ZruEG9pGsvCqO7mojA31fmPw
x4YU76zYOMzLhzQQFuGHZ6GlORad4Qr3OYxLQu/CjtBgaaiXKUEbYfYemQHtro6V
LkzE+YcYwTJwhHaPOjiKWZdFCHA4IW+Yt4zO8wy3YhM3WaLnMBqs6H6fB6KowTLb
O1Nb/+ToOd27oGtoAo8+jeNzBT2Kniez0SsR4frWXrAFae2pLuNitIvy6vfPEqo=
=97ZX
-----END PGP SIGNATURE-----
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
