[webkit-reviews] review denied: [Bug 100144] In the open-source jpeg decoder, read image orientation from the exif data : [Attachment 170199] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Oct 23 12:46:19 PDT 2012


Eric Seidel <eric at webkit.org> has denied Nico Weber <thakis at chromium.org>'s
request for review:
Bug 100144: In the open-source jpeg decoder, read image orientation from the
exif data
https://bugs.webkit.org/show_bug.cgi?id=100144

Attachment 170199: Patch
https://bugs.webkit.org/attachment.cgi?id=170199&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=170199&action=review


I think the primary concern with patches like this is readability.  I think
you've done a fine job, but I listed a few notes below which might help make it
even easier to understand what's going on here.  (Which helps us make sure
there are no security holes or errors in the future.)

One more round please. :)

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:151
> +static bool
> +checkExifHeader(jpeg_saved_marker_ptr marker, bool* isBigEndian, unsigned*
idfOffset)

I believe these normally go on the same line in webKit.

If isBigEndian/idfOffset aren't optional, seems they should be & instead of *.

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:156
> +    // 'M', 'M' (motorola / big endian byte order), folled by (uint16_t)42,

folled

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:166
> +	   && marker->data[0] == 'E'
> +	   && marker->data[1] == 'x'
> +	   && marker->data[2] == 'i'
> +	   && marker->data[3] == 'f'
> +	   && marker->data[4] == '\0'

We have some (primative) utilities for string-based parsing (as opposed to
character-based, as I might describe this approach) in ParserUtilities.h.  But
I suspect the type of marker->data is not UChar*, so even those primitives
won't work here.  The CSSParser is another example of a UChar* parser in
WebKit.

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:189
> +	   idfOffset += 6; // Account for 'Exif\0\0' header.

You could also use sizeof("Exif\0") if you wanted.  I'm not sure it's more
clear in this instance until you're using string-based parsing throught the
rest of the code.

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:190
> +	   if (idfOffset >= marker->data_length - 2)

No risk of underflow of an unsigned, right?

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:192
> +	   JOCTET* idf = marker->data + idfOffset;

idf?

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:195
> +	   idf += 2;

What are we skipping here?

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:197
> +	   for (unsigned i = 0; i < tagCount && end - idf >= 12; ++i, idf +=
12) {

12 should probably have a name.  Do you want to give a comment description of
the layout of what you're reading here?


More information about the webkit-reviews mailing list