[webkit-reviews] review denied: [Bug 217820] Support EXIF image resolution in CoreGraphics (macOS/iOS) : [Attachment 412005] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Noam Rosenthal <noam at webkit.org>'s
request for review:
Bug 217820: Support EXIF image resolution in CoreGraphics (macOS/iOS)
https://bugs.webkit.org/show_bug.cgi?id=217820

Attachment 412005: Patch

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




--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 412005
  --> https://bugs.webkit.org/attachment.cgi?id=412005
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.