Return Styles: Pseud0ch, Terminal, Valhalla, NES, Geocities, Blue Moon. Entire thread

Wut ? C

Name: Anonymous 2007-11-16 12:09

AviStruct* GetVideoInfo(char*file)
{
    AviStruct* MyVideo = (AviStruct*)s_alloc(sizeof(AviStruct));
    FILE*f = fopen(file,"rb");
    if (f)
    {
        unsigned int dwRate;
        unsigned int dwScale;
        unsigned int dwLength;
        fseek(f,0x84, SEEK_SET );
        fread(&dwRate,4,1,f);
        fseek(f,0x80, SEEK_SET );
        fread(&dwScale,4,1,f);
        fseek(f,0x8C, SEEK_SET );
        fread(&dwLength,4,1,f);
        fseek(f,0x40, SEEK_SET );
        fread(&MyVideo->width,4,1,f);
        fseek(f,0x44, SEEK_SET );
        fread(&MyVideo->height,4,1,f);
        MyVideo->fFps = (float)((float)(dwRate)/(float)(dwScale));
        MyVideo->fDuration = (float)((float)(dwLength)/(float)(MyVideo->fFps));
        return MyVideo;

    }
    return 0x0;
}

Name: Anonymous 2007-11-17 13:41

There are piles of things wrong with >>1's code post. First, it puts main functionality inside an "if successful" branch, when obviously error handling should be in the further indented bit. Second, it pointlessly abuses fseek() to read bits and pieces of a header when a read into a byte buffer would be clearer (and a little bit faster too).

Third, and most important, it reads integers in a non-portable way. INTEGER ENDIANNESS MOTHERFUCKER, have you heard of it?

I'd do the same somewhat like this:


unsigned int uintFromFile(FILE *f, off_t offset)
{
    int n = fseek(f, offset, SEEK_SET);
    if(n != 0) goto iofail;
    uint8_t buf[sizeof(unsigned int)];
    n = fread(buf, sizeof(unsigned int), 1, f);
    if(n != 1) goto iofail;
    // assuming little-endian, i.e. RIFF byte order
    return buf[0] | (((unsigned)buf[1]) << 8) | (((unsigned)buf[2]) << 16) | (((unsigned)buf[3]) << 24);

iofail:
    if(errno == 0) {
        fprintf(stderr, "%s: short read\n", __FUNCTION__);
    } else {
        fprintf(stderr, "%s: IO failure: %s\n", __FUNCTION__, strerror(errno));
    }
    exit(EXIT_FAILURE);
    // or some other appropriate error handling mechanism   
}


This moves error handling out of the main functionality and into a reusable function (we'll assume that you'd like to read other bits from the AVI header too), and narrows down the error cases so that the caller doesn't need to bother their little noggin too much.

You could also change the prototype to int uintFromFile(unsigned int *result_p, FILE *f, off_t offset) and store the output integer in *result_p and return an error indicator (0 on success, errno on failure, for instance).

Newer Posts
Don't change these.
Name: Email:
Entire Thread Thread List