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

[zhentan feng]

Hi

2008/7/17 Reimar Döffinger <Reimar.Doeffinger at stud.uni-karlsruhe.de>:

> 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.
>

Thanks very much for your reviewing.
I'll modified it ASAP.



-- 
Best wishes~