Bug 217820 – Support EXIF image resolution in CoreGraphics (macOS/iOS)

• Component: Images
• Status: RESOLVED FIXED

[Noam Rosenthal]

See spec proposal https://github.com/whatwg/html/pull/5574
and master bug https://bugs.webkit.org/show_bug.cgi?id=212405

[Noam Rosenthal]

Created attachment 411568 [details]
Patch

[Noam Rosenthal]

Recently landed in Chromium with the same web platform tests: https://chromium.googlesource.com/chromium/src/+/86ae1272b4a60b42439bc115777bb916fa3a6720

[Noam Rosenthal]

Created attachment 412005 [details]
Patch

[Darin Adler]

Comment on attachment 412005 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=412005&action=review

Thanks for taking this on. This is a neat feature.

review- because of the commented out code; requires at least one more turn of the crank

> Source/WebCore/ChangeLog:3
> +        Support EXIF image resolution in CoreGraphics (OSX/iOS)

OSX is not the correct name for anything; in the past "OS X" might have been, but nowadays, likely macOS is what you mean.

> Source/WebCore/platform/graphics/BitmapImage.cpp:154
> +NativeImagePtr BitmapImage::preTransformedNativeImageForCurrentFrame(ImageOrientation respectOrientation, const GraphicsContext* targetContext)

I’m not sure I understand how an ImageOrientation object is a "respect orientation". I think we’re using this just for whether it’s "none" or not?

> Source/WebCore/platform/graphics/BitmapImage.cpp:158
> +    ImageOrientation orientation = respectOrientation == ImageOrientation::None ? ImageOrientation(ImageOrientation::None) : orientationForCurrentFrame();

auto

> Source/WebCore/platform/graphics/BitmapImage.cpp:168
> +    FloatSize srcSize = sourceSize();

auto. WebKit coding style frowns in abbreviations like "src", and it’s particularly strange here to distinguish these just by spelling differences in the same name. I would have written:

    auto sourceSize = this->sourceSize();

> Source/WebCore/platform/graphics/ImageSource.cpp:571
> +    Optional<IntSize> size = frameMetadataAtIndexCacheIfNeeded<Optional<IntSize>>(0, (&ImageFrame::densityCorrectedSize), &m_densityCorrectedSize, ImageFrame::Caching::Metadata);

auto

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:454
>      NativeImagePtr tileImage;
> -    if (options.orientation() == ImageOrientation::FromImage)
> -        tileImage = image.nativeImageForCurrentFrameRespectingOrientation();
> -    else
> -        tileImage = image.nativeImageForCurrentFrame();
> +    tileImage = image.preTransformedNativeImageForCurrentFrame(options.orientation());

Should merge this into one line, and use auto.

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:70
> +    RetainPtr<CFMutableDictionaryRef> options = createImageSourceOptions();

auto

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:180
> +    CFDictionaryRef exifDictionary = (CFDictionaryRef)CFDictionaryGetValue(imageProperties, kCGImagePropertyExifDictionary);
> +    CFDictionaryRef tiffDictionary = (CFDictionaryRef)CFDictionaryGetValue(imageProperties, kCGImagePropertyTIFFDictionary);

I suggest auto instead of listing CFDictionaryRef twice on each line.

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:191
> +    CFNumberRef widthProperty = (CFNumberRef)CFDictionaryGetValue(imageProperties, kCGImagePropertyPixelWidth);
> +    CFNumberRef heightProperty = (CFNumberRef)CFDictionaryGetValue(imageProperties, kCGImagePropertyPixelHeight);
> +    CFNumberRef preferredWidthProperty = (CFNumberRef)CFDictionaryGetValue(exifDictionary, kCGImagePropertyExifPixelXDimension);
> +    CFNumberRef preferredHeightProperty = (CFNumberRef)CFDictionaryGetValue(exifDictionary, kCGImagePropertyExifPixelYDimension);
> +    CFNumberRef resolutionXProperty = (CFNumberRef)CFDictionaryGetValue(imageProperties, kCGImagePropertyDPIWidth);
> +    CFNumberRef resolutionYProperty = (CFNumberRef)CFDictionaryGetValue(imageProperties, kCGImagePropertyDPIHeight);
> +    CFNumberRef resolutionUnitProperty = (CFNumberRef)CFDictionaryGetValue(tiffDictionary, kCGImagePropertyTIFFResolutionUnit);

I suggest auto instead of listing CFNumberRef twice on each line.

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:197
> +    const unsigned defaultResolution = 72;
> +    const unsigned inchResolutionUnit = 2;

I suggest constexpr here. Also, since we are using these in float computations, how about having these be float, not unsigned. Or maybe "short" for the resolution unit. Why isn’t the inch value coming from a header?

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:199
> +    short widthValue, heightValue, resUnitValue, preferredWidthValue, preferredHeightValue;

How did we settle on short for these? CFNumber converts between sizes and since we are using these as floats, it seems a little strange to use 16-bit integers for the intermediate values. This is especially risky if we don’t check the return value from CFNumberGetValue. I think it should be all floats.

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:207
> +    CFNumberGetValue(widthProperty, kCFNumberShortType, &widthValue);
> +    CFNumberGetValue(heightProperty, kCFNumberShortType, &heightValue);
> +    CFNumberGetValue(preferredWidthProperty, kCFNumberShortType, &preferredWidthValue);
> +    CFNumberGetValue(preferredHeightProperty, kCFNumberShortType, &preferredHeightValue);
> +    CFNumberGetValue(resolutionXProperty, kCFNumberFloat32Type, &resXValue);
> +    CFNumberGetValue(resolutionYProperty, kCFNumberFloat32Type, &resYValue);
> +    CFNumberGetValue(resolutionUnitProperty, kCFNumberShortType, &resUnitValue);

Risky to call CFNumberGetValue without checking the boolean return value or initializing the passed in value. Could end up with uninitialized values if there is a type mismatch. Just like we check for null pointers above, we should be checking here.

Also, I think we could consider leaving off the "Value" suffix from all these local variable names.

Perhaps a helper function that combines both CFDictionaryGetValue and CFNumberGetValue could make this function less verbose and a little easier to understand.

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:215
> +    IntSize preferredSize(preferredWidthValue, preferredHeightValue);
> +    if (roundedIntSize(sizeFromResolution) == preferredSize)

Why is one size here rounded and the other truncated?

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:432
> +    RetainPtr<CFDictionaryRef> properties = adoptCF(CGImageSourceCopyPropertiesAtIndex(m_nativeDecoder.get(), index, createImageSourceMetadataOptions().get()));

Please use auto here.

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:514
> +// static CGImageRef resizeImage(CGImageRef image, const IntSize& size)
> +// {
> +//     RetainPtr<CGColorSpace> colorspace = CGImageGetColorSpace(image);
> +//     RetainPtr<CGContextRef> context = CGBitmapContextCreate(
> +//         nullptr, size.width(), size.height(),
> +//         CGImageGetBitsPerComponent(image),
> +//         CGImageGetBytesPerRow(image),
> +//         colorspace.get(),
> +//         CGImageGetAlphaInfo(image));
> +
> +//     if (!context)
> +//         return image;
> +
> +//     CGContextDrawImage(context.get(), CGRectMake(0, 0, size.width(), size.height()), image);
> +//     return CGBitmapContextCreateImage(context.get());
> +// }

Please don’t check in a commented-out function. Or is there some reason for that here?

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:542
> +        // auto originalSize = frameOriginalSizeAtIndex(index, SubsamplingLevel::Default);
> +        // if (size != originalSize)
> +        //    image = resizeImage(image.get(), size);

More commented out code. Why?

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:548
> -    // CGContextDrawImage. We now tell CG to cache the drawn images. See also <rdar://problem/14366755> -
> +    // CGContextDrawImage. We now tell CG 0to cache the drawn images. See also <rdar://problem/14366755> -

Oops, that doesn’t look right.

[Noam Rosenthal]

Comment on attachment 412005 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=412005&action=review

Thanks! Will revise and resubmit.

>> Source/WebCore/ChangeLog:3
>> +        Support EXIF image resolution in CoreGraphics (OSX/iOS)
> 
> OSX is not the correct name for anything; in the past "OS X" might have been, but nowadays, likely macOS is what you mean.

Gotcha, old habit.

>> Source/WebCore/platform/graphics/BitmapImage.cpp:154
>> +NativeImagePtr BitmapImage::preTransformedNativeImageForCurrentFrame(ImageOrientation respectOrientation, const GraphicsContext* targetContext)
> 
> I’m not sure I understand how an ImageOrientation object is a "respect orientation". I think we’re using this just for whether it’s "none" or not?

The Orientation enum (since before this patch) is used to describe two things, the orientation itself and whether or not it's respected.
I don't like it but didn't tackle it in this patch. Should I?

>> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:514
>> +// }
> 
> Please don’t check in a commented-out function. Or is there some reason for that here?

Oops

>> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:542
>> +        //    image = resizeImage(image.get(), size);
> 
> More commented out code. Why?

Leftovers, ditto re. oops

[Darin Adler]

Comment on attachment 412005 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=412005&action=review

>>> Source/WebCore/ChangeLog:3
>>> +        Support EXIF image resolution in CoreGraphics (OSX/iOS)
>> 
>> OSX is not the correct name for anything; in the past "OS X" might have been, but nowadays, likely macOS is what you mean.
> 
> Gotcha, old habit.

Oh, also, to be pedantic, this also adds this support to the other Apple platforms that WebKit supports, watchOS and tvOS. No need to take any different action or retitle because of that, I suppose.

[Darin Adler]

Comment on attachment 412005 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=412005&action=review

>>> Source/WebCore/platform/graphics/BitmapImage.cpp:154
>>> +NativeImagePtr BitmapImage::preTransformedNativeImageForCurrentFrame(ImageOrientation respectOrientation, const GraphicsContext* targetContext)
>> 
>> I’m not sure I understand how an ImageOrientation object is a "respect orientation". I think we’re using this just for whether it’s "none" or not?
> 
> The Orientation enum (since before this patch) is used to describe two things, the orientation itself and whether or not it's respected.
> I don't like it but didn't tackle it in this patch. Should I?

Well, when it’s used for both, that’s a good thing. When it’s used only for whether it’s respected, then I suppose it’s weak. I think there is a true naming challenge if you want to say "orientation value that only determines whether to respect orientation", and I don’t think naming it "respectOrientation" does the job. A boolean named "respectOrientation" would make more sense. But an ImageOrientation named that is simply too confusing. I think one way to repair that would be to change this argument into a boolean and put the "!= None" at the call site. As long as no one is literally passing "false" or "true" I can’t think of any downside to making that change. But there may be other even better solutions.

[Noam Rosenthal]

Comment on attachment 412005 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=412005&action=review

>> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:70
>> +    RetainPtr<CFMutableDictionaryRef> options = createImageSourceOptions();
> 
> auto

With auto it won't retain...

>> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:432
>> +    RetainPtr<CFDictionaryRef> properties = adoptCF(CGImageSourceCopyPropertiesAtIndex(m_nativeDecoder.get(), index, createImageSourceMetadataOptions().get()));
> 
> Please use auto here.

Again, how would it be retained? The rest of the code does exactly this...

[Noam Rosenthal]

Created attachment 412076 [details]
Patch

[Noam Rosenthal]

Created attachment 412077 [details]
Patch

[Noam Rosenthal]

(In reply to Noam Rosenthal from comment #8)
> Comment on attachment 412005 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412005&action=review
> 
> >> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:70
> >> +    RetainPtr<CFMutableDictionaryRef> options = createImageSourceOptions();
> > 
> > auto
> 
> With auto it won't retain...
> 
> >> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:432
> >> +    RetainPtr<CFDictionaryRef> properties = adoptCF(CGImageSourceCopyPropertiesAtIndex(m_nativeDecoder.get(), index, createImageSourceMetadataOptions().get()));
> > 
> > Please use auto here.
> 
> Again, how would it be retained? The rest of the code does exactly this...

Scratch this comment :) See new patch.

[Noam Rosenthal]

Created attachment 412084 [details]
Patch

[Noam Rosenthal]

Created attachment 412089 [details]
Patch

[Noam Rosenthal]

Created attachment 412094 [details]
Patch

[Noam Rosenthal]

Created attachment 412096 [details]
Patch

[Darin Adler]

Comment on attachment 412096 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=412096&action=review

> Source/WebCore/platform/graphics/BitmapImage.cpp:159
> +    ImageOrientation orientation = respectOrientation ? orientationForCurrentFrame() : ImageOrientation(ImageOrientation::None);

auto

> Source/WebCore/platform/graphics/BitmapImage.cpp:164
> +    FloatSize correctedSizeFloat(correctedSize ? FloatSize(correctedSize.value()) : size());

auto correctedSizeFloat = correctedSize ? FloatSize(correctedSize.value()) : size();

> Source/WebCore/platform/graphics/BitmapImage.cpp:169
> +    FloatSize srcSize = sourceSize();

Did you see my comment about src vs. source? Did you respond? I understand that you might not agree, but was hoping to see your response.

> Source/WebCore/platform/graphics/BitmapImage.cpp:235
> +    FloatRect srcRect(requestedSrcRect);

auto srcRect = requestedSrcRect;

> Source/WebCore/platform/graphics/BitmapImage.cpp:243
>      FloatSize scaleFactorForDrawing = context.scaleFactorForDrawing(destRect, srcRect);

auto

> Source/WebCore/platform/graphics/BitmapImage.cpp:244
> +    IntSize sizeForDrawing = expandedIntSize(srcSize * scaleFactorForDrawing);

auto

> Source/WebCore/platform/graphics/ImageResolution.cpp:36
> +    FloatSize sizeFromResolution(sourceSize.width() * DefaultResolution / metadata.resolution.width(), sourceSize.height() * DefaultResolution / metadata.resolution.height());

auto sizeFromResolution = sourceSize * DefaultSize / metadata.resolution;

> Source/WebCore/platform/graphics/ImageResolution.h:35
> +    static const unsigned DefaultResolution = 72;

constexpr

> Source/WebCore/platform/graphics/ImageResolution.h:37
> +    // This range intentionally matches the resolution values from the EXIF spec.

OK, but why?

> Source/WebCore/platform/graphics/ImageSource.cpp:571
> +    Optional<IntSize> size = frameMetadataAtIndexCacheIfNeeded<Optional<IntSize>>(0, (&ImageFrame::densityCorrectedSize), &m_densityCorrectedSize, ImageFrame::Caching::Metadata);

auto

No need for parentheses around &ImageFrame::densityCorrectedSize and I don’t think they add readability.

> Source/WebCore/platform/graphics/ImageSource.cpp:578
> +    return orientation.usesWidthAsHeight() ? Optional<IntSize>(size.value().transposedSize()) : size;

Better to use makeOptional() rather than Optional<IntSize>().

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:213
> +    return ImageResolution::densityCorrectedSize(FloatSize(sourceWidth, sourceHeight), {
> +        FloatSize(preferredWidth, preferredHeight),
> +        FloatSize(resolutionWidth, resolutionHeight),
> +        static_cast<ImageResolution::ResolutionUnit>(resolutionUnit)
> +    });

Can probably use braces here instead of FloatSize(). So { sourceWidth, sourceHeight } instead of FloatSize(sourceWidth, sourceHeight). I like those better. What do you think?

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:428
> +    RetainPtr<CFDictionaryRef> properties = adoptCF(CGImageSourceCopyPropertiesAtIndex(m_nativeDecoder.get(), index, createImageSourceMetadataOptions().get()));

auto

[Noam Rosenthal]

Comment on attachment 412096 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=412096&action=review

>> Source/WebCore/platform/graphics/BitmapImage.cpp:169
>> +    FloatSize srcSize = sourceSize();
> 
> Did you see my comment about src vs. source? Did you respond? I understand that you might not agree, but was hoping to see your response.

Missed that comment. Will revise. I definitely agree.

>> Source/WebCore/platform/graphics/ImageResolution.cpp:36
>> +    FloatSize sizeFromResolution(sourceSize.width() * DefaultResolution / metadata.resolution.width(), sourceSize.height() * DefaultResolution / metadata.resolution.height());
> 
> auto sizeFromResolution = sourceSize * DefaultSize / metadata.resolution;

Ack

>> Source/WebCore/platform/graphics/ImageResolution.h:37
>> +    // This range intentionally matches the resolution values from the EXIF spec.
> 
> OK, but why?

Because it's a utility to be used with EXIF values, same as orientation. I can clarify in the comment.

>> Source/WebCore/platform/graphics/ImageSource.cpp:571
>> +    Optional<IntSize> size = frameMetadataAtIndexCacheIfNeeded<Optional<IntSize>>(0, (&ImageFrame::densityCorrectedSize), &m_densityCorrectedSize, ImageFrame::Caching::Metadata);
> 
> auto
> 
> No need for parentheses around &ImageFrame::densityCorrectedSize and I don’t think they add readability.

Ack

>> Source/WebCore/platform/graphics/ImageSource.cpp:578
>> +    return orientation.usesWidthAsHeight() ? Optional<IntSize>(size.value().transposedSize()) : size;
> 
> Better to use makeOptional() rather than Optional<IntSize>().

Ack

>> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:213
>> +    });
> 
> Can probably use braces here instead of FloatSize(). So { sourceWidth, sourceHeight } instead of FloatSize(sourceWidth, sourceHeight). I like those better. What do you think?

Sure!

[Noam Rosenthal]

Created attachment 412122 [details]
Patch for landing

[EWS]

Committed r268886: <https://trac.webkit.org/changeset/268886>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 412122 [details].

[Radar WebKit Bug Importer]

<rdar://problem/70586608>