[Ffmpeg-devel] SMAF updates

[Michael Niedermayer]

Hi

On Tue, May 16, 2006 at 08:59:25AM +0200, Vidar Madsen wrote:
> Hi.
> 
> Here are some enhancements to SMAF / MMF format writing, as
> contributed by Stuart Slade.
> 
> Most notable is the addition of the required 2-byte CRC trailer, which
> prevented the generated files from working on several phone models.
> 
> Vidar

[...]
> +    char metadata[520];
[...]
> -    put_byte(pb, 0); /* status */
> -    put_byte(pb, 0); /* counts */
> +    put_byte(pb, copystatus & 0xff); /* copy status */
> +    put_byte(pb, copycounts & 0xff); /* copy counts */

the & 0xFF is redundant (but its no problem if you prefer it that way)


>      put_tag(pb, "VN:libavcodec,"); /* metadata ("ST:songtitle,VN:version,...") */
> +    if(s->title[0]     != '\0') { snprintf(metadata, 520, "ST:%s,", s->title);     put_tag(pb, metadata); }
> +    if(s->author[0]    != '\0') { snprintf(metadata, 520, "SW:%s,", s->author);    put_tag(pb, metadata); }
> +    if(s->copyright[copyoffset] != '\0') { snprintf(metadata, 520, "CR:%s,", &s->copyright[copyoffset]); put_tag(pb, metadata); }

maybe that should not all be on the same line for readability?
i would also strongly suggest to replace 520 by sizeof(metadata) so that if
its size changes nothing bad happens


[...]
> @@ -157,6 +176,28 @@
>          url_fseek(pb, pos, SEEK_SET);
>  
>          put_flush_packet(pb);
> +
> +        /* Store the location of the crc */
> +        crc_pos = url_ftell(pb);
> +
> +        /* Add a dummy CRC */
> +        put_byte(pb, 0);
> +        put_byte(pb, 0);

put_be16() should be used here


> +
> +        // Update header length counter to allow for the CRC
> +        end_tag_be(pb, 8);
> +
> +        /* Generate a good CRC */
> +        crc = mmf_make_crc(s);
> +
> +        /* Find where to store the CRC */
> +        url_fseek(pb, crc_pos, SEEK_SET);
> +
> +        /* Write the crc */
> +        put_byte(pb, crc >> 8);
> +        put_byte(pb, crc & 0xff);

put_be16() should be used here


[...]
> +static unsigned int mmf_crc_table[256];
> +
> +static void mmf_make_crc_table(void)
> +{
> +    int i;
> +    for(i = 0; i <= 255; i++) {
> +        int k = i << 8;
> +        int j;
> +        for(j = 0; j < 8; j++)
> +            if((k & 0x8000) != 0)
> +                k = k << 1 ^ 0x1021;
> +            else
> +                k <<= 1;
> +
> +        mmf_crc_table[i] = k & 0xffff;
> +    }
> +}

dupliating crc calculation code is not ok (use libavutil/crc.* instead,
crc.c contains some example code on how to use it)


> +
> +static int mmf_make_crc(AVFormatContext *s)
> +{
> +    int i = 65535;
> +    int k;
> +    FILE *fp;
> +    struct stat filestats;
> +    int stat_status;
> +    int length;
> +    
> +    mmf_make_crc_table();
> +    
> +    stat_status = stat(s->filename, &filestats);
> +    
> +    if(stat_status == 0) {
> +        fp = fopen(s->filename, "r");

direct accesses to files are not acceptable (=any *open *read *get, stat, FILE
from libc) instead init_checksum() and get_checksum() could be used


[...]

-- 
Michael

In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is