Read JPEG dimensions properly

Question

I need to extract the dimensions (width and height) of a jpeg file for inserting in an html document. According to the JPEG image compression FAQ, cautions against what's commonly out there,

Some people have recommended just searching for the byte pair representing SOFn, without paying attention to the marker block structure. This is unsafe because a prior marker might contain the SOFn pattern, either by chance or because it contains a JPEG-compressed thumbnail image.

It said to look up rdjpgcom.c, which is part of the open-source ImageMagick. It seems to agree with the advice on this answer. However, this file prints out jpeg comments, so I wrote one to specifically get the dimensions.

#include <stdlib.h> /* EXIT */
#include <stdio.h>  /* [f]printf fopen fclose fread */
#include <assert.h> /* assert */
#include <errno.h>  /* errno */

/** Attempt to read the size of a jpeg.
 @param[file] File that has been opened in binary mode and rewound; required.
 @param[width, height] Pointers that get overwritten on success; required.
 @return Success, otherwise `errno` will (probably) be set.
 @license <http://www.faqs.org/faqs/jpeg-faq/part1/> said to look up
 [rdjpgcom.c](https://github.com/ImageMagick/jpeg-turbo/blob/master/rdjpgcom.c),
 which has
 [IJG License](https://github.com/ImageMagick/jpeg-turbo/blob/master/README.ijg)
 and possibly related [ImageMagick](https://imagemagick.org/script/license.php).
 I used as a reference to write this function. */
static int jpeg_dim(FILE *const fp, unsigned *const width,
    unsigned *const height) {
    unsigned char f[8];
    unsigned skip;
    assert(fp && width && height);
    /* The start of the file has to be an `SOI`. */
    if(fread(f, 2, 1, fp) != 1) return 0;
    if(f[0] != 0xFF || f[1] != 0xD8) return errno = EDOM, 0;
    for( ; ; ) {
        /* Discard up until the last `0xFF`, then that is the marker type. */
        do { if(fread(f, 1, 1, fp) != 1) return 0; } while(f[0] != 0xFF);
        do { if(fread(f, 1, 1, fp) != 1) return 0; } while(f[0] == 0xFF);
        switch(f[0]) {
        case 0xC0: case 0xC1: case 0xC2: case 0xC3: case 0xC5: /* _sic_ */
        case 0xC6: case 0xC7: case 0xC9: /* _sic_ */ case 0xCA: case 0xCB:
        case 0xCD: /* _sic_ */ case 0xCE: case 0xCF:
            /* `SOF` markers. */
            if(fread(f, 8, 1, fp) != 1) return 0;
            if((skip = (f[0] << 8) | f[1]) != 8u + 3 * f[7])
                return errno = EDOM, 0;
            *width  = (f[5] << 8) | f[6];
            *height = (f[3] << 8) | f[4];
            return 1;
        case 0xD8: case 0xD9:
            /* Image data `SOS, EOI` without image size. */
            return errno = EDOM, 0;
        default:
            /* Skip the rest by reading it's size. */
            if(fread(f, 2, 1, fp) != 1) return 0;
            if((skip = (f[0] << 8) | f[1]) < 2) return errno = EDOM, 0;
            if(fseek(fp, skip - 2, SEEK_CUR) != 0) return 0;
        }
    }
}

int main(int argc, char **argv) {
    FILE *fp = 0;
    unsigned width, height;
    int success = EXIT_FAILURE;
    if(argc != 2 || !(fp = fopen(argv[1], "rb"))
        || !jpeg_dim(fp, &width, &height)) goto catch;
    printf("Jpeg width: %u, height: %u.\n", width, height);
    success = EXIT_SUCCESS;
    goto finally;
catch:
    if(errno) {
        perror("jpeg size");
    } else {
        fprintf(stderr, "Use with jpeg filename to see the size.\n");
    }
finally:
    if(fp) fclose(fp);
    return success;
}

It seems to work, but I'd like to review it to make sure that it's correct.

Answer 1

(kind of nitpicking here, it looks quite solid to me)

Code:

    /* Discard up until the last `0xFF`, then that is the marker type. */
    do { if(fread(f, 1, 1, fp) != 1) return 0; } while(f[0] != 0xFF);
    do { if(fread(f, 1, 1, fp) != 1) return 0; } while(f[0] == 0xFF);

While we might have extra 0xff fill to read, I'm not sure we should be reading and discarding non-0xff bytes. If the data in the file doesn't follow the expected format our assumptions about the marker / chunk structure might be wrong too, which could lead to invalid results.

Outputting the wrong size could cause more problems than returning an error, so I'd be inclined to pick the latter instead.

---

        if((skip = (f[0] << 8) | f[1]) != 8u + 3 * f[7])
            return errno = EDOM, 0;

No need to set skip here, since we return in both branches.

We're checking the SOF chunk length matches with the number of image components? It might be nice to have a comment or something to mention this.

---

Using errno seems unnecessary if we return success / failure in the return value (and the only error code used EDOM).

---

Style:

Some of the compound statements seem a bit forced. e.g.

e.g.:

if(argc != 2 || !(fp = fopen(argv[1], "rb"))
    || !jpeg_dim(fp, &width, &height)) goto catch;

If we split this up, the flow becomes much simpler, we can introduce local variables later, and we don't need multiple branches in the error handling:

int main(int argc, char **argv) {

    if (argc != 2) {
        fprintf(stderr, "Use with jpeg filename to see the size.\n");
        return EXIT_FAILURE;
    }

    FILE* fp = fopen(argv[1], "rb"); // note: we only need this if argc was correct
    if (!fp) {
        perror("jpeg size"); // note: fopen() may not set errno on non-POSIX platforms
        return EXIT_FAILURE;
    }

    unsigned width, height; // note: we only need these if fopen worked
    if (!jpeg_dim(fp, &width, &height))
    {
        perror("jpeg size"); // note: errno isn't very helpful here... we could return an error code instead
        return EXIT_FAILURE;
    }

    printf("Jpeg width: %u, height: %u.\n", width, height);

    fclose(fp);

    return EXIT_SUCCESS;
}

There are quite a few repeated statements in jpeg_dim (fread and stuff like ((f[5] << 8) | f[6]). These are quite concise, but should probably still be factored into separate functions.

I'd suggest something like a jpeg_read_u8, jpeg_read_u16 and jpeg_skip:

static int jpeg_read_u8(FILE *const fp, unsigned char *const u8) {
    assert(u8);

    unsigned char b;
    if (fread(&b, 1, 1, fp) != 1)
        return 0;

    *u8 = b;
    return 1;
}

static int jpeg_read_u16(FILE *const fp, unsigned short *const u16) {
    assert(u16);

    unsigned char b[2];
    if (fread(&b, 1, 2, fp) != 2)
        return 0;

    *u16 = (b[0] << 8) | b[1];
    return 1;
}

static int jpeg_skip(FILE *const fp, unsigned bytes) {
    return fseek(fp, bytes, SEEK_CUR) == 0;
}

static int jpeg_dim(FILE *const fp, unsigned *const width, unsigned *const height) {
    assert(fp);
    assert(width && height);

    /* The start of the file has to be an `SOI`. */
    unsigned char soi0; if (!jpeg_read_u8(fp, &soi0) || soi0 != 0xff) return 0;
    unsigned char soi1; if (!jpeg_read_u8(fp, &soi1) || soi1 != 0xd8) return 0;

    for (; ; ) {
        /* Read first byte of next marker */
        unsigned char m0; if (!jpeg_read_u8(fp, &m0) || m0 != 0xff) return 0;
        /* Discard fill characters */
        unsigned char m1; do { if (!jpeg_read_u8(fp, &m1)) return 0; } while (m1 == 0xff);

        switch (m1) {
        case 0xC0: case 0xC1: case 0xC2: case 0xC3: case 0xC5: /* _sic_ */
        case 0xC6: case 0xC7: case 0xC9: /* _sic_ */ case 0xCA: case 0xCB:
        case 0xCD: /* _sic_ */ case 0xCE: case 0xCF: {
            /* `SOF` markers. */
            unsigned short Lf; if (!jpeg_read_u16(fp, &Lf)) return 0;
            if (!jpeg_skip(fp, 1)) return 0; /* skip Precision */
            unsigned short Y;  if (!jpeg_read_u16(fp, &Y)) return 0;
            unsigned short X;  if (!jpeg_read_u16(fp, &X)) return 0;
            unsigned char Nf;  if (!jpeg_read_u8(fp, &Nf)) return 0;
            /* Check that the chunk length (Lf) matches the number of image components (Nf) */
            if (Lf != 8u + 3 * Nf) return 0;
            *width = X; *height = Y;
            return 1;
        }
        case 0xD8: case 0xD9:
            /* Image data `SOS, EOI` without image size. */
            return 0;
        default: {
            /* Skip the rest by reading it's size. */
            unsigned short Lf;
            if (!jpeg_read_u16(fp, &Lf) || Lf < 2) return 0;
            if (!jpeg_skip(fp, Lf - 2)) return 0;
        }
        }
    }
}