Bug 155422 – Create a CG ImageDecoder class instead of defining it as CGImageSourceRef

• Component: Images
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

This is work towards CG asynchronous image decoding: https://bugs.webkit.org/show_bug.cgi?id=155322. The new class is a CG platform class. Most of its code will be extracted from ImageSourceCG.cpp.

[Said Abou-Hallawa]

Created attachment 273927 [details]
Patch

[Said Abou-Hallawa]

Created attachment 273928 [details]
Patch

[Said Abou-Hallawa]

Created attachment 273932 [details]
Patch

[Said Abou-Hallawa]

Created attachment 273935 [details]
Patch

[Said Abou-Hallawa]

Created attachment 273936 [details]
Patch

[Said Abou-Hallawa]

Created attachment 273940 [details]
patch for review

[Simon Fraser (smfr)]

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!

[Said Abou-Hallawa]

Created attachment 274969 [details]
Patch

[Said Abou-Hallawa]

Created attachment 274970 [details]
Patch

[Said Abou-Hallawa]

Created attachment 274994 [details]
Patch

[Said Abou-Hallawa]

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.

[Simon Fraser (smfr)]

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<>

[Said Abou-Hallawa]

Created attachment 275066 [details]
Patch

[Said Abou-Hallawa]

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.

[WebKit Commit Bot]

Comment on attachment 275066 [details]
Patch

Clearing flags on attachment: 275066

Committed r198782: <http://trac.webkit.org/changeset/198782>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.