See spec proposal https://github.com/whatwg/html/pull/5574 and master bug https://bugs.webkit.org/show_bug.cgi?id=212405
Created attachment 411568 [details] Patch
Recently landed in Chromium with the same web platform tests: https://chromium.googlesource.com/chromium/src/+/86ae1272b4a60b42439bc115777bb916fa3a6720
Created attachment 412005 [details] Patch
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 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 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 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 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...
Created attachment 412076 [details] Patch
Created attachment 412077 [details] Patch
(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.
Created attachment 412084 [details] Patch
Created attachment 412089 [details] Patch
Created attachment 412094 [details] Patch
Created attachment 412096 [details] Patch
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 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!
Created attachment 412122 [details] Patch for landing
Committed r268886: <https://trac.webkit.org/changeset/268886> All reviewed patches have been landed. Closing bug and clearing flags on attachment 412122 [details].
<rdar://problem/70586608>