Bug 217820 - Support EXIF image resolution in CoreGraphics (macOS/iOS)
Summary: Support EXIF image resolution in CoreGraphics (macOS/iOS)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Noam Rosenthal
URL:
Keywords: InRadar
Depends on:
Blocks: 212405
  Show dependency treegraph
 
Reported: 2020-10-16 06:37 PDT by Noam Rosenthal
Modified: 2020-10-22 13:21 PDT (History)
13 users (show)

See Also:


Attachments
Patch (182.44 KB, patch)
2020-10-16 06:55 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (183.25 KB, patch)
2020-10-21 10:10 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (191.20 KB, patch)
2020-10-22 02:08 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (191.25 KB, patch)
2020-10-22 02:18 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (190.87 KB, patch)
2020-10-22 04:20 PDT, Noam Rosenthal
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (191.36 KB, patch)
2020-10-22 06:19 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (191.36 KB, patch)
2020-10-22 07:29 PDT, Noam Rosenthal
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (191.21 KB, patch)
2020-10-22 07:53 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch for landing (191.20 KB, patch)
2020-10-22 12:43 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Noam Rosenthal 2020-10-16 06:37:22 PDT
See spec proposal https://github.com/whatwg/html/pull/5574
and master bug https://bugs.webkit.org/show_bug.cgi?id=212405
Comment 1 Noam Rosenthal 2020-10-16 06:55:37 PDT
Created attachment 411568 [details]
Patch
Comment 2 Noam Rosenthal 2020-10-16 06:59:31 PDT
Recently landed in Chromium with the same web platform tests: https://chromium.googlesource.com/chromium/src/+/86ae1272b4a60b42439bc115777bb916fa3a6720
Comment 3 Noam Rosenthal 2020-10-21 10:10:25 PDT
Created attachment 412005 [details]
Patch
Comment 4 Darin Adler 2020-10-21 11:29:55 PDT
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.
Comment 5 Noam Rosenthal 2020-10-21 12:26:11 PDT
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
Comment 6 Darin Adler 2020-10-21 12:51:37 PDT
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.
Comment 7 Darin Adler 2020-10-21 12:54:13 PDT
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.
Comment 8 Noam Rosenthal 2020-10-22 00:22:34 PDT
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...
Comment 9 Noam Rosenthal 2020-10-22 02:08:01 PDT
Created attachment 412076 [details]
Patch
Comment 10 Noam Rosenthal 2020-10-22 02:18:56 PDT
Created attachment 412077 [details]
Patch
Comment 11 Noam Rosenthal 2020-10-22 02:36:32 PDT
(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.
Comment 12 Noam Rosenthal 2020-10-22 04:20:42 PDT
Created attachment 412084 [details]
Patch
Comment 13 Noam Rosenthal 2020-10-22 06:19:34 PDT
Created attachment 412089 [details]
Patch
Comment 14 Noam Rosenthal 2020-10-22 07:29:56 PDT
Created attachment 412094 [details]
Patch
Comment 15 Noam Rosenthal 2020-10-22 07:53:12 PDT
Created attachment 412096 [details]
Patch
Comment 16 Darin Adler 2020-10-22 12:23:08 PDT
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
Comment 17 Noam Rosenthal 2020-10-22 12:34:55 PDT
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!
Comment 18 Noam Rosenthal 2020-10-22 12:43:09 PDT
Created attachment 412122 [details]
Patch for landing
Comment 19 EWS 2020-10-22 13:20:56 PDT
Committed r268886: <https://trac.webkit.org/changeset/268886>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 412122 [details].
Comment 20 Radar WebKit Bug Importer 2020-10-22 13:21:33 PDT
<rdar://problem/70586608>