Bug 155422

Created attachment 273927 [details] Patch

Created attachment 273928 [details] Patch

Created attachment 273932 [details] Patch

Created attachment 273935 [details] Patch

Created attachment 273936 [details] Patch

Created attachment 273940 [details] patch for review

Comment on attachment 273940 [details] patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=273940&action=review > Source/WebCore/ChangeLog:9 > + Move the image decoding code from ImageSourceCG.cpp out to a new class named > + ImageDecoder. The purpose of this split is to have the CG ImageSource similar Why do this just for CG, and not the other platforms? > Source/WebCore/platform/graphics/BitmapImage.h:285 > + mutable SubsamplingLevel m_maximumSubsamplingLevel { 0 }; Would be nice to change the other data members to use{ } initialization too. > Source/WebCore/platform/graphics/cg/ImageDecoder.cpp:-9 > -// > -// ImageDecoder.cpp > -// WebCore > -// > -// Created by Said Abou-Hallawa on 3/13/16. > -// > -// > - > -#include "ImageDecoder.hpp" We should not see this in this patch. > Source/WebCore/platform/graphics/cg/ImageDecoder.h:-7 > -// > -// ImageDecoder.hpp > -// WebCore > -// > -// Created by Said Abou-Hallawa on 3/13/16. > -// > -// Ditto. > Source/WebCore/platform/graphics/cg/ImageDecoder.h:43 > + virtual ~ImageDecoder() { } Why virtual? > Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:69 > + delete m_decoder; Use unique_ptr > Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:84 > + m_decoder = new ImageDecoder(); No bare new!

Created attachment 274969 [details] Patch

Created attachment 274970 [details] Patch

Created attachment 274994 [details] Patch

Comment on attachment 273940 [details] patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=273940&action=review >> Source/WebCore/ChangeLog:9 >> + ImageDecoder. The purpose of this split is to have the CG ImageSource similar > > Why do this just for CG, and not the other platforms? Because there are image decoders for other platforms. They are in Source/WebCore/platform/image-decoders/. >> Source/WebCore/platform/graphics/BitmapImage.h:285 >> + mutable SubsamplingLevel m_maximumSubsamplingLevel { 0 }; > > Would be nice to change the other data members to use{ } initialization too. I will do this clean up in a separate patch. >> Source/WebCore/platform/graphics/cg/ImageDecoder.cpp:-9 >> -#include "ImageDecoder.hpp" > > We should not see this in this patch. I will move the ImageDecoder class to separate files in a separate patch. >> Source/WebCore/platform/graphics/cg/ImageDecoder.h:-7 >> -// > > Ditto. I will move the ImageDecoder class to separate files in a separate patch. >> Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:69 >> + delete m_decoder; > > Use unique_ptr Done. >> Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:84 >> + m_decoder = new ImageDecoder(); > > No bare new! Done.

Comment on attachment 274994 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274994&action=review r+ but I'd like to see some more testing of image subsampling (maybe plumb through settings so we can enable it for testing on Mac). > Source/WebCore/ChangeLog:11 > + class named ImageDecoder. The purpose of this split is to have the CG > + ImageSource similar to the none CG ImageSource where there is a separate > + class for decoding and it is named ImageDecoder also. This sentence is hard to follow. > Source/WebCore/platform/graphics/BitmapImage.cpp:-719 > - if (allowSubsampling()) > - ts.dumpProperty("allow-subsampling", allowSubsampling()); > if (m_isSolidColor) > ts.dumpProperty("solid-color", m_isSolidColor); > - > - if (m_imageOrientation != OriginTopLeft) > - ts.dumpProperty("orientation", m_imageOrientation); We're losing information in the dump now. I think you should add dumping for the source to make up for this. > Source/WebCore/platform/graphics/ImageSource.cpp:65 > + if (m_decoder) > + m_decoder = nullptr; No need for the if () > Source/WebCore/platform/graphics/ImageSource.h:126 > - // Always original size, without subsampling. > - IntSize size(ImageOrientationDescription = ImageOrientationDescription()) const; > - // Size of optionally subsampled frame. > - IntSize frameSizeAtIndex(size_t, SubsamplingLevel = 0, ImageOrientationDescription = ImageOrientationDescription()) const; > + IntSize size() const; > + IntSize sizeRespectingOrientation() const; > + IntSize frameSizeAtIndex(size_t, SubsamplingLevel = 0, RespectImageOrientationEnum = DoNotRespectImageOrientation) const; Please preserve the comments. > Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:164 > + : m_nativeDecoder(CGImageSourceCreateIncremental(0)) nullptr > Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:170 > + CFRelease(m_nativeDecoder); Why isn't this a RetainPtr<>? > Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:218 > +IntSize ImageDecoder::size() const > +{ > + if (m_size.isEmpty()) > + m_size = frameSizeAtIndex(0, 0); > + return m_size; > +} Is this supposed to return the original size, or the subsampled size? > Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:570 > +IntSize ImageSource::size() const > +{ > + return frameSizeAtIndex(0, 0); > +} Original size, or subsampled size? > Source/WebCore/platform/image-decoders/ImageDecoder.cpp:107 > + return std::unique_ptr<ImageDecoder>(new GIFImageDecoder(alphaOption, gammaAndColorProfileOption)); These should all use std::make_unique<>

Created attachment 275066 [details] Patch

Comment on attachment 274994 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274994&action=review I will follow up with another patch which adds an option in the settings for the image subsampling. And I will include also tests which enable this option when displaying the image. >> Source/WebCore/ChangeLog:11 >> + class for decoding and it is named ImageDecoder also. > > This sentence is hard to follow. I rephrased it. >> Source/WebCore/platform/graphics/BitmapImage.cpp:-719 >> - ts.dumpProperty("orientation", m_imageOrientation); > > We're losing information in the dump now. I think you should add dumping for the source to make up for this. Done. A new function ImageSource::dump() is added and it is called form BitmapImage::dump(). >> Source/WebCore/platform/graphics/ImageSource.cpp:65 >> + m_decoder = nullptr; > > No need for the if () The if() is removed. >> Source/WebCore/platform/graphics/ImageSource.h:126 >> + IntSize frameSizeAtIndex(size_t, SubsamplingLevel = 0, RespectImageOrientationEnum = DoNotRespectImageOrientation) const; > > Please preserve the comments. Comments were added back. >> Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:164 >> + : m_nativeDecoder(CGImageSourceCreateIncremental(0)) > > nullptr Done. >> Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:170 >> + CFRelease(m_nativeDecoder); > > Why isn't this a RetainPtr<>? Done. >> Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:218 >> +} > > Is this supposed to return the original size, or the subsampled size? It returns the original size. A comment is added above the function in the class definition. >> Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:570 >> +} > > Original size, or subsampled size? It returns the original size. A comment is added above the function in the class definition. >> Source/WebCore/platform/image-decoders/ImageDecoder.cpp:107 >> + return std::unique_ptr<ImageDecoder>(new GIFImageDecoder(alphaOption, gammaAndColorProfileOption)); > > These should all use std::make_unique<> Done.

Comment on attachment 275066 [details] Patch Clearing flags on attachment: 275066 Committed r198782: <http://trac.webkit.org/changeset/198782>

All reviewed patches have been landed. Closing bug.