Bug 155456 - Merge CG ImageSource and non CG ImageSource implementation in one file
Summary: Merge CG ImageSource and non CG ImageSource implementation in one file
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: 155444 156465
Blocks: 155322 155498
  Show dependency treegraph
 
Reported: 2016-03-14 13:31 PDT by Said Abou-Hallawa
Modified: 2016-04-11 17:09 PDT (History)
13 users (show)

See Also:


Attachments
Patch (139.79 KB, patch)
2016-03-14 13:37 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (139.44 KB, patch)
2016-03-14 14:01 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (139.79 KB, patch)
2016-03-14 15:18 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch for review (23.09 KB, patch)
2016-03-14 15:36 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (34.40 KB, patch)
2016-03-30 16:44 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (35.40 KB, patch)
2016-03-30 18:14 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (36.28 KB, patch)
2016-03-31 10:33 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (39.10 KB, patch)
2016-03-31 15:57 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (47.42 KB, patch)
2016-04-08 22:43 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (47.08 KB, patch)
2016-04-08 22:55 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (61.78 KB, patch)
2016-04-09 01:16 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (55.26 KB, patch)
2016-04-09 02:55 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (55.26 KB, patch)
2016-04-09 03:59 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (56.06 KB, patch)
2016-04-09 04:22 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (56.32 KB, patch)
2016-04-10 00:23 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (63.91 KB, patch)
2016-04-10 20:20 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (64.09 KB, patch)
2016-04-10 20:27 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (63.52 KB, patch)
2016-04-10 20:42 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (65.11 KB, patch)
2016-04-10 21:47 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (65.19 KB, patch)
2016-04-11 12:28 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (65.15 KB, patch)
2016-04-11 12:48 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-14 13:31:54 PDT
The goal is to make it easy to cache the ImageFrameData in the ImageSource instead of having it cached with the BitmapImage.
Comment 1 Said Abou-Hallawa 2016-03-14 13:37:14 PDT
Created attachment 274014 [details]
Patch
Comment 2 Simon Fraser (smfr) 2016-03-14 13:46:31 PDT
Is that the right patch? It looks like the full patch.
Comment 3 Said Abou-Hallawa 2016-03-14 13:55:49 PDT
(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.
Comment 4 Said Abou-Hallawa 2016-03-14 14:01:11 PDT
Created attachment 274023 [details]
Patch
Comment 5 Said Abou-Hallawa 2016-03-14 15:18:14 PDT
Created attachment 274035 [details]
Patch
Comment 6 Said Abou-Hallawa 2016-03-14 15:36:30 PDT
Created attachment 274038 [details]
Patch for review
Comment 7 Simon Fraser (smfr) 2016-03-16 11:28:12 PDT
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?
Comment 8 Said Abou-Hallawa 2016-03-30 16:44:16 PDT
Created attachment 275237 [details]
Patch
Comment 9 Said Abou-Hallawa 2016-03-30 18:14:52 PDT
Created attachment 275252 [details]
Patch
Comment 10 Said Abou-Hallawa 2016-03-31 10:33:04 PDT
Created attachment 275300 [details]
Patch
Comment 11 Said Abou-Hallawa 2016-03-31 15:38:32 PDT
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.
Comment 12 Said Abou-Hallawa 2016-03-31 15:57:47 PDT
Created attachment 275346 [details]
Patch
Comment 13 Darin Adler 2016-04-02 21:51:21 PDT
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?
Comment 14 Said Abou-Hallawa 2016-04-08 22:43:08 PDT
Created attachment 276080 [details]
Patch
Comment 15 Said Abou-Hallawa 2016-04-08 22:55:48 PDT
Created attachment 276081 [details]
Patch
Comment 16 Said Abou-Hallawa 2016-04-09 01:16:14 PDT
Created attachment 276082 [details]
Patch
Comment 17 Said Abou-Hallawa 2016-04-09 02:55:11 PDT
Created attachment 276083 [details]
Patch
Comment 18 Said Abou-Hallawa 2016-04-09 03:59:54 PDT
Created attachment 276084 [details]
Patch
Comment 19 Said Abou-Hallawa 2016-04-09 04:22:37 PDT
Created attachment 276085 [details]
Patch
Comment 20 Said Abou-Hallawa 2016-04-10 00:23:29 PDT
Created attachment 276101 [details]
Patch
Comment 21 Said Abou-Hallawa 2016-04-10 00:27:18 PDT
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.
Comment 22 Darin Adler 2016-04-10 16:46:09 PDT
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.
Comment 23 Said Abou-Hallawa 2016-04-10 20:20:19 PDT
Created attachment 276125 [details]
Patch
Comment 24 Said Abou-Hallawa 2016-04-10 20:27:41 PDT
Created attachment 276126 [details]
Patch
Comment 25 Said Abou-Hallawa 2016-04-10 20:42:38 PDT
Created attachment 276127 [details]
Patch
Comment 26 Said Abou-Hallawa 2016-04-10 21:47:09 PDT
Created attachment 276128 [details]
Patch
Comment 27 Said Abou-Hallawa 2016-04-10 23:38:26 PDT
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.
Comment 28 WebKit Commit Bot 2016-04-11 00:29:30 PDT
Comment on attachment 276128 [details]
Patch

Clearing flags on attachment: 276128

Committed r199290: <http://trac.webkit.org/changeset/199290>
Comment 29 WebKit Commit Bot 2016-04-11 00:29:37 PDT
All reviewed patches have been landed.  Closing bug.
Comment 30 Carlos Alberto Lopez Perez 2016-04-11 03:01:09 PDT
(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?
Comment 31 Carlos Alberto Lopez Perez 2016-04-11 03:33:30 PDT
(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
Comment 32 Michael Catanzaro 2016-04-11 05:40:02 PDT
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
Comment 33 WebKit Commit Bot 2016-04-11 05:42:35 PDT
Re-opened since this is blocked by bug 156465
Comment 34 Michael Catanzaro 2016-04-11 07:28:58 PDT
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).
Comment 35 Said Abou-Hallawa 2016-04-11 12:28:23 PDT
Created attachment 276165 [details]
Patch
Comment 36 Said Abou-Hallawa 2016-04-11 12:36:55 PDT
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.
Comment 37 Said Abou-Hallawa 2016-04-11 12:48:06 PDT
Created attachment 276166 [details]
Patch
Comment 38 WebKit Commit Bot 2016-04-11 14:33:13 PDT
Comment on attachment 276166 [details]
Patch

Clearing flags on attachment: 276166

Committed r199312: <http://trac.webkit.org/changeset/199312>
Comment 39 WebKit Commit Bot 2016-04-11 14:33:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 40 Michael Catanzaro 2016-04-11 17:09:08 PDT
GTK bot is happy :)