Bug 155422

Summary: Create a CG ImageDecoder class instead of defining it as CGImageSourceRef
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: ImagesAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, dino, japhet, jonlee, sam, simon.fraser, thorton
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 155412    
Bug Blocks: 155322, 155444    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
patch for review
none
Patch
none
Patch
none
Patch
none
Patch none

Description Said Abou-Hallawa 2016-03-13 23:09:42 PDT 
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.
Comment 1 Said Abou-Hallawa 2016-03-13 23:14:02 PDT 
Created attachment 273927 [details]
Patch
Comment 2 Said Abou-Hallawa 2016-03-13 23:31:15 PDT 
Created attachment 273928 [details]
Patch
Comment 3 Said Abou-Hallawa 2016-03-14 00:30:03 PDT 
Created attachment 273932 [details]
Patch
Comment 4 Said Abou-Hallawa 2016-03-14 00:41:26 PDT 
Created attachment 273935 [details]
Patch
Comment 5 Said Abou-Hallawa 2016-03-14 01:08:22 PDT 
Created attachment 273936 [details]
Patch
Comment 6 Said Abou-Hallawa 2016-03-14 01:44:45 PDT 
Created attachment 273940 [details]
patch for review
Comment 7 Simon Fraser (smfr) 2016-03-16 11:39:51 PDT 
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!
Comment 8 Said Abou-Hallawa 2016-03-25 20:38:28 PDT 
Created attachment 274969 [details]
Patch
Comment 9 Said Abou-Hallawa 2016-03-25 21:03:44 PDT 
Created attachment 274970 [details]
Patch
Comment 10 Said Abou-Hallawa 2016-03-26 19:06:21 PDT 
Created attachment 274994 [details]
Patch
Comment 11 Said Abou-Hallawa 2016-03-26 23:42:17 PDT 
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 12 Simon Fraser (smfr) 2016-03-28 10:32:00 PDT 
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<>
Comment 13 Said Abou-Hallawa 2016-03-28 16:51:28 PDT 
Created attachment 275066 [details]
Patch
Comment 14 Said Abou-Hallawa 2016-03-28 17:34:33 PDT 
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 15 WebKit Commit Bot 2016-03-29 09:18:24 PDT 
Comment on attachment 275066 [details]
Patch

Clearing flags on attachment: 275066

Committed r198782: <http://trac.webkit.org/changeset/198782>
Comment 16 WebKit Commit Bot 2016-03-29 09:18:29 PDT 
All reviewed patches have been landed.  Closing bug.