Bug 155456 – Merge CG ImageSource and non CG ImageSource implementation in one file

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Said Abou-Hallawa]

The goal is to make it easy to cache the ImageFrameData in the ImageSource instead of having it cached with the BitmapImage.

[Said Abou-Hallawa]

Created attachment 274014 [details]
Patch

[Simon Fraser (smfr)]

Is that the right patch? It looks like the full patch.

[Said Abou-Hallawa]

(In reply to comment #2)
> Is that the right patch? It looks like the full patch.

I mentioned in the Changelog that this patch is not for review. It is for EWS only. Once the bots are happy, I am going to post the diff based on the patch submitted to the previous bugs in the chain which is https://bugs.webkit.org/show_bug.cgi?id=155422.

[Said Abou-Hallawa]

Created attachment 274023 [details]
Patch

[Said Abou-Hallawa]

Created attachment 274035 [details]
Patch

[Said Abou-Hallawa]

Created attachment 274038 [details]
Patch for review

[Simon Fraser (smfr)]

Comment on attachment 274038 [details]
Patch for review

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

This patch needs to be done in such a way that it applies to TOT.

> Source/WebCore/ChangeLog:11
> +        Moving the ImageFrameData caching to ImageSource is hard with two separate
> +        implementations for the same class. ImageSource.cpp and ImageSourceCG.cpp
> +        have a lot of duplicated code. So merging them in one file will remove the
> +        duplicated code and it is a step forward for the asynchronous image decoding.

This is contrary to our usual approach of putting platform-specific code in its own file. I think you need a strong justification for it.

Often we don't subclass per-platform, but do have platformFoo() methods that go into a separate file.

> Source/WebCore/platform/graphics/ImageSource.cpp:61
> +        // Recent versions of ImageIO discard previously decoded image frames if the client

A comment that says "recent versions of foo" quickly becomes useless, since the reader has no notion of what is "recent".

> Source/WebCore/platform/graphics/ImageSource.cpp:84
> +    m_decoder = new ImageDecoder();

We never call bare 'new'. This should be a unique_ptr.

> Source/WebCore/platform/graphics/ImageSource.cpp:145
> +    

Whitespace (lots of it here and below).

> Source/WebCore/platform/graphics/ImageSource.cpp:162
> +#if USE(CG)
> +        return cAnimationLoopOnce;
> +#else
> +        return cAnimationNone;
> +#endif

Why this seemingly arbitrary difference?

> Source/WebCore/platform/graphics/ImageSource.cpp:205
> +        

Whitespace.

> Source/WebCore/platform/graphics/ImageSource.h:37
> +#include <wtf/text/WTFString.h>

Why does the header need this?

[Said Abou-Hallawa]

Created attachment 275237 [details]
Patch

[Said Abou-Hallawa]

Created attachment 275252 [details]
Patch

[Said Abou-Hallawa]

Created attachment 275300 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 274038 [details]
Patch for review

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

>> Source/WebCore/ChangeLog:11
>> +        duplicated code and it is a step forward for the asynchronous image decoding.
> 
> This is contrary to our usual approach of putting platform-specific code in its own file. I think you need a strong justification for it.
> 
> Often we don't subclass per-platform, but do have platformFoo() methods that go into a separate file.

The goal is to move all the platform specific code to the ImageDecoder and to make ImageSource platform independent. The goal also is to have one ImageDecoder header file but two implementations.

>> Source/WebCore/platform/graphics/ImageSource.cpp:61
>> +        // Recent versions of ImageIO discard previously decoded image frames if the client
> 
> A comment that says "recent versions of foo" quickly becomes useless, since the reader has no notion of what is "recent".

The comment was changed.

>> Source/WebCore/platform/graphics/ImageSource.cpp:84
>> +    m_decoder = new ImageDecoder();
> 
> We never call bare 'new'. This should be a unique_ptr.

Done. This has been taken care of in http://trac.webkit.org/changeset/198782.

>> Source/WebCore/platform/graphics/ImageSource.cpp:145
>> +    
> 
> Whitespace (lots of it here and below).

Removed.

>> Source/WebCore/platform/graphics/ImageSource.cpp:162
>> +#endif
> 
> Why this seemingly arbitrary difference?

This difference was inherited from the old separated code. We now return cAnimationNone if the decoder is not initialized.

>> Source/WebCore/platform/graphics/ImageSource.cpp:205
>> +        
> 
> Whitespace.

Removed.

>> Source/WebCore/platform/graphics/ImageSource.h:37
>> +#include <wtf/text/WTFString.h>
> 
> Why does the header need this?

You right. This header file is not needed.

[Said Abou-Hallawa]

Created attachment 275346 [details]
Patch

[Darin Adler]

Comment on attachment 275346 [details]
Patch

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

> Source/WebCore/platform/graphics/ImageSource.cpp:68
> +    // ImageIO discards previously decoded image frames if the client application no longer holds
> +    // references to them, so there's no need to throw away the decoder unless we're explicitly asked
> +    // to destroy all of the frames.

This comment is not written correctly for a platform-independent function. This explains why the behavior is correct for Cocoa platforms but not why it’s correct for other platforms. I’m not even sure it is.

> Source/WebCore/platform/graphics/ImageSource.cpp:85
>      m_decoder = ImageDecoder::create(*data, m_alphaOption, m_gammaAndColorProfileOption);

Since this function always dereferences data, it should take a reference rather than a pointer.

> Source/WebCore/platform/graphics/ImageSource.cpp:109
> +    // Values chosen to be appropriate for iOS.

Not a complete enough comment for a platform independent function. Why are values that are appropriate for iOS also good for other platforms?

> Source/WebCore/platform/graphics/ImageSource.h:2
> + * Copyright (C) 2004, 2005, 2006, 2016 Apple Inc.  All rights reserved.

Set of years is probably wrong. Did we really leave this header untouched during 2007-2015?

[Said Abou-Hallawa]

Created attachment 276080 [details]
Patch

[Said Abou-Hallawa]

Created attachment 276081 [details]
Patch

[Said Abou-Hallawa]

Created attachment 276082 [details]
Patch

[Said Abou-Hallawa]

Created attachment 276083 [details]
Patch

[Said Abou-Hallawa]

Created attachment 276084 [details]
Patch

[Said Abou-Hallawa]

Created attachment 276085 [details]
Patch

[Said Abou-Hallawa]

Created attachment 276101 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 275346 [details]
Patch

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

>> Source/WebCore/platform/graphics/ImageSource.cpp:68
>> +    // to destroy all of the frames.
> 
> This comment is not written correctly for a platform-independent function. This explains why the behavior is correct for Cocoa platforms but not why it’s correct for other platforms. I’m not even sure it is.

Done. The comment was changed.

>> Source/WebCore/platform/graphics/ImageSource.cpp:85
>>      m_decoder = ImageDecoder::create(*data, m_alphaOption, m_gammaAndColorProfileOption);
> 
> Since this function always dereferences data, it should take a reference rather than a pointer.

Done. The image decoder create and setData are taking a reference instead of a pointer. But the image reader setData() is still taking a pointer.

>> Source/WebCore/platform/graphics/ImageSource.cpp:109
>> +    // Values chosen to be appropriate for iOS.
> 
> Not a complete enough comment for a platform independent function. Why are values that are appropriate for iOS also good for other platforms?

A FIXME comment is added.

>> Source/WebCore/platform/graphics/ImageSource.h:2
>> + * Copyright (C) 2004, 2005, 2006, 2016 Apple Inc.  All rights reserved.
> 
> Set of years is probably wrong. Did we really leave this header untouched during 2007-2015?

Done.

[Darin Adler]

Comment on attachment 276101 [details]
Patch

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

> Source/WebCore/platform/graphics/ImageSource.cpp:36
> +#if USE(CG)
> +#include "ImageDecoderCG.h"
> +#else
>  #include "ImageDecoder.h"
> +#endif

Long term we should clean this up, I think. There are better patterns for this than #if in the files using the header.

> Source/WebCore/platform/graphics/ImageSource.cpp:97
> +    

Stray whitespace here should be deleted.

> Source/WebCore/platform/graphics/ImageSource.cpp:106
> +    const int cMaximumImageAreaBeforeSubsampling = 5 * 1024 * 1024;

No need for the "c" prefix here.

> Source/WebCore/platform/graphics/ImageSource.cpp:109
> +    for (SubsamplingLevel subsamplingLevel = 0; subsamplingLevel < maxSubsamplingLevel; ++subsamplingLevel) {

I think the local variable could just be named "level".

> Source/WebCore/platform/graphics/ImageSource.cpp:110
> +        IntSize frameSize = frameSizeAtIndex(0, subsamplingLevel);

I think the code would read better without the local variable.

> Source/WebCore/platform/graphics/ImageSource.cpp:121
> +    if (scale <= 0 || scale > 1)
> +        return 0;

Probably not important here, but a good habit is to write this the other way so it will reject a NaN:

    if (!(scale > 0 && scale <= 1))
        return 0;

> Source/WebCore/platform/graphics/ImageSource.cpp:128
> +    SubsamplingLevel result = ceilf(log2f(1 / scale));

Better to use std::ceil and std::log2 rather than ceilf and log2f.

> Source/WebCore/platform/graphics/ImageSource.cpp:179
> +    return !initialized() || m_decoder->frameHasAlphaAtIndex(index);

A little peculiar to return true for all frames when not initialized. In future we might want to return and reverse the boolean sense of this and give it a new name, since it’s really a function that checks for "guaranteed not to have alpha" and returns false if that guarantee holds.

> Source/WebCore/platform/graphics/ImageSource.cpp:193
> +    ImageOrientation orientation = orientationAtIndex(index);

Putting this in a local variable causes us to evaluate it even when shouldRespectImageOrientation means we would not need to. I suggest just moving this into the expression below.

> Source/WebCore/platform/graphics/ImageSource.h:115
> +    bool isSizeAvailable();

Strange that this is non-const, but the size function itself is const.

Generally unclear which ImageSource functions are const.

> Source/WebCore/platform/graphics/ImageSource.h:123
> +    bool getHotSpot(IntPoint&) const;

In future could return and rename this to hotSpot and have it return Optional<IntPoint>.

> Source/WebCore/platform/graphics/ImageSource.h:151
> +    // The default value of m_allowSubsampling should be the same as defaultImageSubsamplingEnabled in Settings.cpp

Is there no better way to achieve this than with a comment?

> Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.cpp:167
>      RefPtr<SharedBuffer> pngData(SharedBuffer::create(&m_data->data()[dirEntry.m_imageOffset], m_data->size() - dirEntry.m_imageOffset));

This should be a Ref, not a RefPtr, I think.

[Said Abou-Hallawa]

Created attachment 276125 [details]
Patch

[Said Abou-Hallawa]

Created attachment 276126 [details]
Patch

[Said Abou-Hallawa]

Created attachment 276127 [details]
Patch

[Said Abou-Hallawa]

Created attachment 276128 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 276101 [details]
Patch

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

>> Source/WebCore/platform/graphics/ImageSource.cpp:36
>> +#endif
> 
> Long term we should clean this up, I think. There are better patterns for this than #if in the files using the header.

Yes ideally we should have one header file for ImageDecoder and different implementations.

>> Source/WebCore/platform/graphics/ImageSource.cpp:97
>> +    
> 
> Stray whitespace here should be deleted.

Whitespace was removed.

>> Source/WebCore/platform/graphics/ImageSource.cpp:106
>> +    const int cMaximumImageAreaBeforeSubsampling = 5 * 1024 * 1024;
> 
> No need for the "c" prefix here.

The prefix was removed.

>> Source/WebCore/platform/graphics/ImageSource.cpp:109
>> +    for (SubsamplingLevel subsamplingLevel = 0; subsamplingLevel < maxSubsamplingLevel; ++subsamplingLevel) {
> 
> I think the local variable could just be named "level".

The name of the local variable was changed.

>> Source/WebCore/platform/graphics/ImageSource.cpp:121
>> +        return 0;
> 
> Probably not important here, but a good habit is to write this the other way so it will reject a NaN:
> 
>     if (!(scale > 0 && scale <= 1))
>         return 0;

The if-statment was changed.

>> Source/WebCore/platform/graphics/ImageSource.cpp:128
>> +    SubsamplingLevel result = ceilf(log2f(1 / scale));
> 
> Better to use std::ceil and std::log2 rather than ceilf and log2f.

Done.

>> Source/WebCore/platform/graphics/ImageSource.cpp:193
>> +    ImageOrientation orientation = orientationAtIndex(index);
> 
> Putting this in a local variable causes us to evaluate it even when shouldRespectImageOrientation means we would not need to. I suggest just moving this into the expression below.

Done.

>> Source/WebCore/platform/graphics/ImageSource.h:115
>> +    bool isSizeAvailable();
> 
> Strange that this is non-const, but the size function itself is const.
> 
> Generally unclear which ImageSource functions are const.

The function prototype was changed to be const. The const-ness of this class will be  cleaned in another patch.

>> Source/WebCore/platform/graphics/ImageSource.h:123
>> +    bool getHotSpot(IntPoint&) const;
> 
> In future could return and rename this to hotSpot and have it return Optional<IntPoint>.

Done.

>> Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.cpp:167
>>      RefPtr<SharedBuffer> pngData(SharedBuffer::create(&m_data->data()[dirEntry.m_imageOffset], m_data->size() - dirEntry.m_imageOffset));
> 
> This should be a Ref, not a RefPtr, I think.

The only SharedBuffer::create() function which returns Ref requires the first parameter to be const unsigned char*. m_data->data() returns char*. This causes SharedBuffer::create() which returns PassRefPtr to be called.

[WebKit Commit Bot]

Comment on attachment 276128 [details]
Patch

Clearing flags on attachment: 276128

Committed r199290: <http://trac.webkit.org/changeset/199290>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Alberto Lopez Perez]

(In reply to comment #28)
> Comment on attachment 276128 [details]
> Patch
> 
> Clearing flags on attachment: 276128
> 
> Committed r199290: <http://trac.webkit.org/changeset/199290>

This has caused 300 new failures on the GTK+ layout tests:

- Tests for r199289 148 unexpected failures https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20%28Tests%29/builds/15017
- Tests for r199290 449 unexpected failures https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20%28Tests%29/builds/15018


And since I don't see that this patch does any rebaseline on the Mac port, I wonder....

Does it make sense that the GTK+ port needs 300 new rebaselines after this patch but not the Mac port?
Or perhaps this is indicating that something broke in the GTK+ port?

[Carlos Alberto Lopez Perez]

(In reply to comment #30)
> (In reply to comment #28)
> > Comment on attachment 276128 [details]
> > Patch
> > 
> > Clearing flags on attachment: 276128
> > 
> > Committed r199290: <http://trac.webkit.org/changeset/199290>
> 
> This has caused 300 new failures on the GTK+ layout tests:
> 
> - Tests for r199289 148 unexpected failures
> https://build.webkit.org/builders/GTK%20Linux%2064-
> bit%20Release%20%28Tests%29/builds/15017
> - Tests for r199290 449 unexpected failures
> https://build.webkit.org/builders/GTK%20Linux%2064-
> bit%20Release%20%28Tests%29/builds/15018
> 
> 
> And since I don't see that this patch does any rebaseline on the Mac port, I
> wonder....
> 
> Does it make sense that the GTK+ port needs 300 new rebaselines after this
> patch but not the Mac port?
> Or perhaps this is indicating that something broke in the GTK+ port?

Something similar seems to happens on the EFL port:

- Tests for r199289 123 unexpected failures https://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2/builds/27464
- Tests for r199290 308 unexpected failures https://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2/builds/27465

[Michael Catanzaro]

Looking at a couple of the tests, they do not need rebaselined, they're just broken. E.g. first one I checked:

--- /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/accessibility/aria-current-global-attribute-expected.txt
+++ /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/accessibility/aria-current-global-attribute-actual.txt
@@ -5,7 +5,7 @@
 
 
 PASS spanAccessible('text1') is false
-PASS spanAccessible('text2') is true
+FAIL spanAccessible('text2') should be true. Was false.
 PASS successfullyParsed is true
 
 TEST COMPLETE

[WebKit Commit Bot]

Re-opened since this is blocked by bug 156465

[Michael Catanzaro]

I rolled this out because it wasn't clear to me what went wrong; Said, any chance it's something easy to address?

Worth mentioning that this did unexpectedly fix approximately 10 broken tests on GTK (which are now broken again after the rollout).

[Said Abou-Hallawa]

Created attachment 276165 [details]
Patch

[Said Abou-Hallawa]

The mistake I did was to access ImageDecoder::m_frameBufferCache directly to get the ImageFrame. In the original code, ImageDecoder::frameBufferAtIndex() was used instead which has the side effect of doing the decoding if it was not done.

The fix is to use frameBufferAtIndex() to get the ImageFrame in ImageDecoder::frameIsCompleteAtIndex(), ImageDecoder::frameDurationAtIndex() and ImageDecoder::createFrameImageAtIndex(). This is the same as we used to do in ImageSource::frameIsCompleteAtIndex(), ImageSource::frameDurationAtIndex() and ImageSource::createFrameImageAtIndex() for non CG code path.

[Said Abou-Hallawa]

Created attachment 276166 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 276166 [details]
Patch

Clearing flags on attachment: 276166

Committed r199312: <http://trac.webkit.org/changeset/199312>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

GTK bot is happy :)