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

[Baptiste Coudurier]

Hi,

Reimar Döffinger wrote:
> 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).

Yes.

>> +/* 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.

I absolutely agree with this, I'd though prefer the code to work before
spending time on 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.

Yes, that will be taken care of later, I'll extract common code.

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

Probably, if this is about uniqueness then pick on random at first, then
increment value.

> [...]
> 
>> +    ref = type == MaterialPackage ? set_ref->package : & set_ref->package[1];
> 
> ref = set_ref->package;
> if (type == MaterialPackage)
>   ref++;
> 
> seems much more readable to me.

Well, I'd prefer code to be, if correct and possible:

ref = &set_ref->package[type == MaterialPackage];

[...]

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
Smartjog USA Inc.                                http://www.smartjog.com
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA