WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
217820
Support EXIF image resolution in CoreGraphics (macOS/iOS)
https://bugs.webkit.org/show_bug.cgi?id=217820
Summary
Support EXIF image resolution in CoreGraphics (macOS/iOS)
Noam Rosenthal
Reported
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
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
Show Obsolete
(8)
View All
Add attachment
proposed patch, testcase, etc.
Noam Rosenthal
Comment 1
2020-10-16 06:55:37 PDT
Created
attachment 411568
[details]
Patch
Noam Rosenthal
Comment 2
2020-10-16 06:59:31 PDT
Recently landed in Chromium with the same web platform tests:
https://chromium.googlesource.com/chromium/src/+/86ae1272b4a60b42439bc115777bb916fa3a6720
Noam Rosenthal
Comment 3
2020-10-21 10:10:25 PDT
Created
attachment 412005
[details]
Patch
Darin Adler
Comment 4
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.
Noam Rosenthal
Comment 5
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
Darin Adler
Comment 6
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.
Darin Adler
Comment 7
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.
Noam Rosenthal
Comment 8
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...
Noam Rosenthal
Comment 9
2020-10-22 02:08:01 PDT
Created
attachment 412076
[details]
Patch
Noam Rosenthal
Comment 10
2020-10-22 02:18:56 PDT
Created
attachment 412077
[details]
Patch
Noam Rosenthal
Comment 11
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.
Noam Rosenthal
Comment 12
2020-10-22 04:20:42 PDT
Created
attachment 412084
[details]
Patch
Noam Rosenthal
Comment 13
2020-10-22 06:19:34 PDT
Created
attachment 412089
[details]
Patch
Noam Rosenthal
Comment 14
2020-10-22 07:29:56 PDT
Created
attachment 412094
[details]
Patch
Noam Rosenthal
Comment 15
2020-10-22 07:53:12 PDT
Created
attachment 412096
[details]
Patch
Darin Adler
Comment 16
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
Noam Rosenthal
Comment 17
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!
Noam Rosenthal
Comment 18
2020-10-22 12:43:09 PDT
Created
attachment 412122
[details]
Patch for landing
EWS
Comment 19
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]
.
Radar WebKit Bug Importer
Comment 20
2020-10-22 13:21:33 PDT
<
rdar://problem/70586608
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug