[FFmpeg-soc] [soc]: r2802 - mxf/mxfenc.c

[Reimar Döffinger]

Hello,
I know this is a first version, and maybe you intend to change most
things anyway, but I think I better point them out since fixing them
later when they appear in even more place sure won't be easier.

On Wed, Jul 16, 2008 at 06:39:42PM +0200, spyfeng wrote:
> +static const uint8_t umid_base[] = {0x06, 0x0a, 0x2b, 0x34, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x0f, 0x00, 0x13, 0x00, 0x00, 0x00};//16 bytes

Useless comment. People can count, or if you want to make it explicit,
write it in the [] (you could then also drop the last 0 elements).

> +/* complete key */
> +static const uint8_t op1a_ul[]            = { 0x06, 0x0e, 0x2b, 0x34, 0x04, 0x01, 0x01, 0x01, 0x0d, 0x01, 0x02, 0x01, 0x01, 0x01, 0x01, 0x00 };
> +static const uint8_t header_partition_key[]            = { 0x06, 0x0e, 0x2b, 0x34, 0x02, 0x05, 0x01, 0x01, 0x0d, 0x01, 0x02, 0x01, 0x01, 0x02, 0x04, 0x00 };//ClosedComplete
> +static const uint8_t footer_partition_key[] = {0x06, 0x0e, 0x2b, 0x34, 0x02, 0x05, 0x01, 0x01, 0x0d, 0x01, 0x02, 0x01, 0x01, 0x04, 0x04, 0x00};//ClosedComplete

Comments should be doxygen-compatible. Also, a one-word comment almost
never has any value, at most for the author and that only for at most
one year.
There are also some very weird spaces, preferably it should be aligned
so the '=' match.

> +/* partial key for header metadata */

Just repeating once more to make it clear ;-): (almost) all comments should be converted to be
doxygen-compatible. Almost is the exception for comments that doxygen
does not support, like inside functions.
Also, (almost) all functions should have doxygen comments.

> +#define PRINT_KEY(pc, s, x) dprintf(pc, "%s %02X %02X %02X %02X %02X %02X %02X %02X %02X %02X %02X %02X %02X %02X %02X %02X\n", s, \
> +                             (x)[0], (x)[1], (x)[2], (x)[3], (x)[4], (x)[5], (x)[6], (x)[7], (x)[8], (x)[9], (x)[10], (x)[11], (x)[12], (x)[13], (x)[14], (x)[15])

I think this is also in the mxf demuxer (and probably some other stuff).
That should be moved into some shared file.

> +static void mxf_generate_uuid(AVFormatContext *s, UID uuid)
> +{
> +    MXFContext *mxf = s->priv_data;
> +    int rand_num, i;
> +
> +    for (i = 0; i < 16; i++) {
> +        rand_num = av_random(&mxf->random_state);
> +        rand_num = rand_num%256 + 1;

That probably will need some checking. I have some doubts that av_random
is suitable for this.
With a constant initializer it certainly is quite pointless, and without
it needs some extra case for bitexact.

> +    refs = av_mallocz(ref_count * sizeof(UID));
> +    if (!refs)
> +        return -1;

You should probably use AVERROR(ENOMEM) and try to propagate that up.

> +    p = refs;
> +    for (i = 0; i < ref_count; i++) {
> +        mxf_generate_uuid(s, *p);
> +        p ++;
> +    }
> +    p = 0;
> +    return 0;

p = 0 seems pointless?

> +static int klv_encode_ber_length(ByteIOContext *pb, uint64_t len)
> +{
> +    // Determine the best BER size
> +    int size = 0, i;
> +    if (len < 128) {
> +        //short form
> +        size = 1;
> +        put_byte(pb, len);
> +        return size;
> +    }
> +
> +    while (len /= 256)
> +        size ++;

Seems like a case for av_log2, or possibly even a new av_log2_64bit

> +static int mxf_write_local_tag(ByteIOContext *pb, int value_size, int tag)
> +{
> +    put_be16(pb, tag);
> +    put_be16(pb, value_size);
> +    return 0;
> +}

Any reason why these have a return value.

> +static int utf8len(const uint8_t *b){
> +    int len=0;
> +    int val;
> +    while(*b){
> +        GET_UTF8(val, *b++, return -1;)
> +        len++;
> +    }
> +    return len;
> +}

Uh, are you really, really sure that is needed?

> +    //write KLV
> +    klv->key[13] = 0x01;
> +    klv->key[14] = 0x2f;
> +    klv->key[15] = 0x00;

AV_WB24 might look nicer...

> +    company_name_len = utf8len("FFmpeg") + 1;
> +    product_name_len = utf8len("OP1a Muxer") + 1;
> +    version_string_len = utf8len("version 0.0.1") + 1;

First, using utf8len on purely ASCII strings sure is overkill.

> +    mxf_write_local_tag(dyn_bc, company_name_len, 0x3C01);
> +    put_buffer(dyn_bc, "FFmpeg", company_name_len);

And I can not really imagine that MXF wants you to write the number of
UTF8 characters, actually I am quite sure that the length of (local)
tags is always given in bytes, so you must use strlen, not utf8len.
And sorry for not reading the spec myself, but it seems unusual to me that they
should want the strings to be 0-terminated.

> +    klv->key[13] = 0x01;
> +    klv->key[14] = 0x30;
> +    klv->key[15] = 0x00;

Maybe AV_WB24?

> +    klv->key[13] = 0x01;
> +    klv->key[14] = 0x18;
> +    klv->key[15] = 0x00;

Same

> +    ref = type == MaterialPackage ? set_ref->package : & set_ref->package[1];

ref = set_ref->package;
if (type == MaterialPackage)
  ref++;

seems much more readable to me.

> +    klv->key[13] = 0x01;
> +    klv->key[14] = type == MaterialPackage ? 0x36 : 0x37;
> +    klv->key[15] = 0x00;

As above.

> +    put_buffer(pb, klv->key, 16);
> +    klv_encode_ber_length(pb, dyn_size);
> +    put_buffer(pb, dyn_buf, dyn_size);

And actually, since that whole sequence appears so often, a extra
function for it might be a good idea.

Greetings,
Reimar Döffinger