Bug 155422 - Create a CG ImageDecoder class instead of defining it as CGImageSourceRef
Summary: Create a CG ImageDecoder class instead of defining it as CGImageSourceRef
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords:
Depends on: 155412
Blocks: 155322 155444
  Show dependency treegraph
 
Reported: 2016-03-13 23:09 PDT by Said Abou-Hallawa
Modified: 2016-03-29 09:18 PDT (History)
8 users (show)

See Also:


Attachments
Patch (88.71 KB, patch)
2016-03-13 23:14 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (90.62 KB, patch)
2016-03-13 23:31 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (89.64 KB, patch)
2016-03-14 00:30 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (90.38 KB, patch)
2016-03-14 00:41 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (90.36 KB, patch)
2016-03-14 01:08 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
patch for review (62.39 KB, patch)
2016-03-14 01:44 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (55.71 KB, patch)
2016-03-25 20:38 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (57.67 KB, patch)
2016-03-25 21:03 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (57.82 KB, patch)
2016-03-26 19:06 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (63.89 KB, patch)
2016-03-28 16:51 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.