Bug 158684

Summary: Move the BitmapImage platform dependent code to a new file named NativeImage[CG|Cairo].cpp
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: ImagesAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, dbates, dino, sam, simon.fraser, thorton
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 155322, 155444    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Said Abou-Hallawa 2016-06-12 20:22:28 PDT
This addresses one of the review comments of https://bugs.webkit.org/show_bug.cgi?id=155444.
Comment 1 Said Abou-Hallawa 2016-06-12 20:25:08 PDT
Created attachment 281150 [details]
Patch
Comment 2 Said Abou-Hallawa 2016-06-12 21:52:59 PDT
Created attachment 281155 [details]
Patch
Comment 3 Said Abou-Hallawa 2016-06-12 22:18:14 PDT
Created attachment 281156 [details]
Patch
Comment 4 Said Abou-Hallawa 2016-06-12 22:35:14 PDT
Created attachment 281157 [details]
Patch
Comment 5 Said Abou-Hallawa 2016-06-12 22:49:39 PDT
Created attachment 281158 [details]
Patch
Comment 6 Said Abou-Hallawa 2016-06-13 00:06:54 PDT
Created attachment 281160 [details]
Patch
Comment 7 Said Abou-Hallawa 2016-09-08 11:03:00 PDT
Created attachment 288280 [details]
Patch
Comment 8 Said Abou-Hallawa 2016-09-08 11:08:37 PDT
Created attachment 288283 [details]
Patch
Comment 9 Said Abou-Hallawa 2016-09-08 11:40:54 PDT
Created attachment 288291 [details]
Patch
Comment 10 Simon Fraser (smfr) 2016-09-08 13:49:59 PDT
Comment on attachment 288291 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288291&action=review

> Source/WebCore/platform/graphics/BitmapImage.cpp:402
> +NativeImagePtr BitmapImage::nativeImageOfSize(const IntSize& size)
> +{
> +    size_t count = frameCount();
> +    
> +    for (size_t i = 0; i < count; ++i) {
> +        auto image = frameImageAtIndex(i);
> +        if (image && sizeOfImage(image) == size)
> +            return image;
> +    }
> +    
> +    // Fallback to the first frame image if we can't find the right size
> +    return frameImageAtIndex(0);
> +}

This was only used for CG for icons. I don't think you should generalize it for all platforms.

> Source/WebCore/platform/graphics/BitmapImage.cpp:420
> +Vector<NativeImagePtr> BitmapImage::framesNativeImages()
> +{
> +    Vector<NativeImagePtr> images;
> +    size_t count = frameCount();
> +    
> +    for (size_t i = 0; i < count; ++i) {
> +        if (auto image = frameImageAtIndex(i))
> +            images.append(image);
> +    }
> +    
> +    return images;
> +}

Ditto.

> Source/WebCore/platform/graphics/BitmapImage.cpp:475
> +    if (!haveFrameImageAtIndex(0) && m_source.frameSizeAtIndex(0, 0) != IntSize(1, 1)) {

Isn't that second 0 a subsampling level?

> Source/WebCore/platform/graphics/BitmapImage.cpp:481
> +    if (!ensureFrameIsCached(0))

What does this 0 mean? Would be nicer if the function name had "at index" in it.

> Source/WebCore/platform/graphics/BitmapImage.cpp:552
> +    if (!m_cachedImage) {

Blank line above this please.

> Source/WebCore/platform/graphics/BitmapImage.cpp:594
> +    m_frameTimer = std::make_unique<Timer>(*this, &BitmapImage::advanceAnimation);

This is odd. Normally we store Timers by value.

> Source/WebCore/platform/graphics/BitmapImage.h:62
> -        , m_hasAlpha(true)
> +        , m_hasAlpha(false)

Why? m_hasAlpha(false) is the "safe" value (if we get it wrong, we risk drawing garbage pixels).

> Source/WebCore/platform/graphics/BitmapImage.h:72
>      // Returns whether there was cached image data to clear.

Comment is no longer accurate.

> Source/WebCore/platform/graphics/BitmapImage.h:80
> +        m_subsamplingLevel = 0;

Don't we have enums for this?

> Source/WebCore/platform/graphics/NativeImage.h:59
> +IntSize sizeOfImage(const NativeImagePtr&);
> +bool hasAlphaOfImage(const NativeImagePtr&);
> +Color singlePixelSolidColorOfImage(const NativeImagePtr&);

Why the "OfImage"? Old names seem fine.

> Source/WebCore/platform/graphics/NativeImage.h:64
> +void drawImage(const NativeImagePtr&, GraphicsContext&, const FloatRect&, const FloatRect&, const IntSize&, CompositeOperator, BlendMode, const ImageOrientation&);
> +void clearImageSubImages(const NativeImagePtr&);

Maybe drawNativeImage, clearNativeImageSubImages

> Source/WebCore/platform/graphics/cg/NativeImageCG.cpp:52
> +    // FIXME: Answer correctly the question: does the CGImageRef have alpha channnel?

ImageDiff implements this as:
    CGImageAlphaInfo info = CGImageGetAlphaInfo(image);    
    return (info >= kCGImageAlphaPremultipliedLast) && (info <= kCGImageAlphaFirst);
Comment 11 Said Abou-Hallawa 2016-09-08 15:07:22 PDT
Created attachment 288336 [details]
Patch
Comment 12 Said Abou-Hallawa 2016-09-08 16:52:55 PDT
Created attachment 288361 [details]
Patch
Comment 13 WebKit Commit Bot 2016-09-08 18:08:49 PDT
Comment on attachment 288361 [details]
Patch

Clearing flags on attachment: 288361

Committed r205682: <http://trac.webkit.org/changeset/205682>
Comment 14 WebKit Commit Bot 2016-09-08 18:08:54 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Said Abou-Hallawa 2016-09-12 14:12:10 PDT
Comment on attachment 288291 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288291&action=review

>> Source/WebCore/platform/graphics/BitmapImage.cpp:402
>> +}
> 
> This was only used for CG for icons. I don't think you should generalize it for all platforms.

Done. nativeImageOfSize() and framesNativeImages() are now declared and implemented within if USE(CG) ... #endif.

>> Source/WebCore/platform/graphics/BitmapImage.cpp:475
>> +    if (!haveFrameImageAtIndex(0) && m_source.frameSizeAtIndex(0, 0) != IntSize(1, 1)) {
> 
> Isn't that second 0 a subsampling level?

Done. the SubsamplingLevel enum is defined in ImageSource.h

>> Source/WebCore/platform/graphics/BitmapImage.cpp:481
>> +    if (!ensureFrameIsCached(0))
> 
> What does this 0 mean? Would be nicer if the function name had "at index" in it.

Done. This function is now named ensureFrameAtIndexIsCached().

>> Source/WebCore/platform/graphics/BitmapImage.cpp:552
>> +    if (!m_cachedImage) {
> 
> Blank line above this please.

Done. A new line is added.

>> Source/WebCore/platform/graphics/BitmapImage.cpp:594
>> +    m_frameTimer = std::make_unique<Timer>(*this, &BitmapImage::advanceAnimation);
> 
> This is odd. Normally we store Timers by value.

This is how it was implemented. I will clean it up in a follow-up patch.

>> Source/WebCore/platform/graphics/BitmapImage.h:62
>> +        , m_hasAlpha(false)
> 
> Why? m_hasAlpha(false) is the "safe" value (if we get it wrong, we risk drawing garbage pixels).

m_hasAlpha can be initialized with false because it is accessed only in BitmapImage::frameHasAlphaAtIndex() and it is guarded by m_haveMetadata. Both m_haveMetadata and m_hasAlpha are set in BitmapImage constructor and in BitmapImage::cacheFrame().

>> Source/WebCore/platform/graphics/BitmapImage.h:72
>>      // Returns whether there was cached image data to clear.
> 
> Comment is no longer accurate.

The comment was removed.

>> Source/WebCore/platform/graphics/BitmapImage.h:80
>> +        m_subsamplingLevel = 0;
> 
> Don't we have enums for this?

Fixed.

>> Source/WebCore/platform/graphics/NativeImage.h:59
>> +Color singlePixelSolidColorOfImage(const NativeImagePtr&);
> 
> Why the "OfImage"? Old names seem fine.

Fixed. Names were changed to:
nativeImageSize()
nativeImageHasAlpha()
nativeImageSinglePixelSolidColor()

>> Source/WebCore/platform/graphics/NativeImage.h:64
>> +void clearImageSubImages(const NativeImagePtr&);
> 
> Maybe drawNativeImage, clearNativeImageSubImages

Fixed. Names were changed as suggested.

>> Source/WebCore/platform/graphics/cg/NativeImageCG.cpp:52
>> +    // FIXME: Answer correctly the question: does the CGImageRef have alpha channnel?
> 
> ImageDiff implements this as:
>     CGImageAlphaInfo info = CGImageGetAlphaInfo(image);    
>     return (info >= kCGImageAlphaPremultipliedLast) && (info <= kCGImageAlphaFirst);

Forgot to add in the <http://trac.webkit.org/changeset/205682>. I will add it in a follow up patch.