Bug 155498 – Move caching the ImageFrame from BitmapImage to ImageSource

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

[Said Abou-Hallawa]

The goal is to have caching the ImageFrameData be managed by the ImageSource which can be delegated to the ImageDecoder in the case of asynchronous image decoding case.

[Said Abou-Hallawa]

Created attachment 274105 [details]
Patch

[Said Abou-Hallawa]

Created attachment 274108 [details]
Patch

[Said Abou-Hallawa]

Created attachment 274111 [details]
Patch

[Said Abou-Hallawa]

Created attachment 274115 [details]
Patch

[Said Abou-Hallawa]

Created attachment 274118 [details]
Patch

[Said Abou-Hallawa]

Created attachment 274124 [details]
Patch for review

[Brent Fulgham]

Comment on attachment 274124 [details]
Patch for review

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

I think this looks good, but I wonder if hiding more of the direct m_source access wouldn't be better.

> Source/WebCore/platform/graphics/BitmapImage.cpp:-57
> -    , m_frameCount(0)

As long as you're cleaning this up, could you change these other initializations to use the newer C++ style?

> Source/WebCore/platform/graphics/BitmapImage.cpp:92
> +    ImageFrameIndex clearBeforeFrame = destroyAll ? m_source.m_frames.size() : m_currentFrame;

It's a little weird that we expose the m_frames member like this, especially when you have other methods like "frameSizeAtIndex". Would it be better to do something like m_source.frameCount() or similar?

> Source/WebCore/platform/graphics/BitmapImage.cpp:109
> +    if (!data() && m_source.m_frames.size())

Ditto m_frames

> Source/WebCore/platform/graphics/BitmapImage.cpp:112
> +    if (m_source.m_frames.framesBytes() > largeAnimationCutoff)

m_source.bytesInAllFrames()?

> Source/WebCore/platform/graphics/ImageFrameData.cpp:79
> +        m_duration = (duration < 0.011f) ? 0.100f : duration;

This change isn't really explained in your ChangeLog. Why are you allowing these short durations now?

[Said Abou-Hallawa]

Created attachment 285774 [details]
Patch

[Build Bot]

Comment on attachment 285774 [details]
Patch

Attachment 285774 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1848238

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 285778 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 285774 [details]
Patch

Attachment 285774 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1848244

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 285780 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 285774 [details]
Patch

Attachment 285774 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1848264

New failing tests:
media/video-controls-drag.html
fast/events/drag-and-drop-autoscroll-inner-frame.html

[Build Bot]

Created attachment 285781 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Said Abou-Hallawa]

Created attachment 285789 [details]
Patch

[Said Abou-Hallawa]

Created attachment 285791 [details]
Patch

[Said Abou-Hallawa]

Created attachment 285796 [details]
Patch

[Said Abou-Hallawa]

Created attachment 285800 [details]
Patch

[Said Abou-Hallawa]

Created attachment 285802 [details]
Patch

[Said Abou-Hallawa]

Created attachment 285806 [details]
Patch

[Build Bot]

Comment on attachment 285806 [details]
Patch

Attachment 285806 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1849142

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 285808 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 285806 [details]
Patch

Attachment 285806 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1849143

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 285809 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 285806 [details]
Patch

Attachment 285806 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1849145

New failing tests:
media/video-controls-drag.html
fast/events/drag-and-drop-autoscroll-inner-frame.html

[Build Bot]

Created attachment 285810 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Said Abou-Hallawa]

Created attachment 285815 [details]
Patch

[Said Abou-Hallawa]

Created attachment 285816 [details]
Patch

[Build Bot]

Comment on attachment 285816 [details]
Patch

Attachment 285816 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1850290

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 285818 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 285816 [details]
Patch

Attachment 285816 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1850294

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 285819 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 285816 [details]
Patch

Attachment 285816 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1850323

New failing tests:
media/video-controls-drag.html
fast/events/drag-and-drop-autoscroll-inner-frame.html

[Build Bot]

Created attachment 285820 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Said Abou-Hallawa]

Created attachment 285829 [details]
Patch

[Build Bot]

Comment on attachment 285829 [details]
Patch

Attachment 285829 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1852560

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 285831 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 285829 [details]
Patch

Attachment 285829 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1852592

New failing tests:
media/video-controls-drag.html
fast/events/drag-and-drop-autoscroll-inner-frame.html

[Build Bot]

Created attachment 285832 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 285829 [details]
Patch

Attachment 285829 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1852682

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 285834 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Said Abou-Hallawa]

Created attachment 285837 [details]
Patch

[Build Bot]

Comment on attachment 285837 [details]
Patch

Attachment 285837 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1853286

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 285846 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Said Abou-Hallawa]

Created attachment 285862 [details]
Patch

[Build Bot]

Comment on attachment 285862 [details]
Patch

Attachment 285862 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1854176

New failing tests:
http/tests/misc/slow-loading-animated-image.html

[Build Bot]

Created attachment 285867 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Said Abou-Hallawa]

Created attachment 285869 [details]
Patch

[Said Abou-Hallawa]

Created attachment 285879 [details]
Patch

[Said Abou-Hallawa]

Created attachment 285917 [details]
Patch

[Said Abou-Hallawa]

Created attachment 285923 [details]
Patch for review

[Said Abou-Hallawa]

Created attachment 289492 [details]
Patch

[Said Abou-Hallawa]

Created attachment 289494 [details]
Patch

[Said Abou-Hallawa]

Created attachment 289497 [details]
Patch

[Said Abou-Hallawa]

Created attachment 289498 [details]
Patch

[Build Bot]

Comment on attachment 289498 [details]
Patch

Attachment 289498 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2121947

New failing tests:
fast/replaced/outline-replaced-elements-offset.html
fast/images/favicon-as-image.html
http/tests/multipart/invalid-image-data-standalone.html
http/tests/misc/favicon-as-image.html
fast/events/standalone-image-drag-to-editable.html
http/tests/misc/acid3.html

[Build Bot]

Created attachment 289503 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289498 [details]
Patch

Attachment 289498 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2122074

New failing tests:
fast/replaced/outline-replaced-elements-offset.html
fast/images/favicon-as-image.html
http/tests/multipart/invalid-image-data-standalone.html
http/tests/misc/favicon-as-image.html
fast/events/standalone-image-drag-to-editable.html
http/tests/misc/acid3.html

[Build Bot]

Created attachment 289504 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289498 [details]
Patch

Attachment 289498 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2122060

New failing tests:
fast/replaced/outline-replaced-elements-offset.html
http/tests/multipart/invalid-image-data-standalone.html
http/tests/misc/favicon-as-image.html
http/tests/misc/acid3.html
fast/images/favicon-as-image.html

[Build Bot]

Created attachment 289505 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289498 [details]
Patch

Attachment 289498 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2122033

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 289507 [details]
Archive of layout-test-results from ews121 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Said Abou-Hallawa]

Created attachment 289566 [details]
Patch

[Said Abou-Hallawa]

Created attachment 289568 [details]
Patch

[Build Bot]

Comment on attachment 289568 [details]
Patch

Attachment 289568 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2126519

New failing tests:
fast/replaced/outline-replaced-elements-offset.html
fast/images/favicon-as-image.html
http/tests/multipart/invalid-image-data-standalone.html
http/tests/misc/favicon-as-image.html
fast/events/standalone-image-drag-to-editable.html
http/tests/misc/acid3.html

[Build Bot]

Created attachment 289571 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289568 [details]
Patch

Attachment 289568 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2126606

New failing tests:
fast/replaced/outline-replaced-elements-offset.html
fast/images/favicon-as-image.html
http/tests/multipart/invalid-image-data-standalone.html
http/tests/misc/favicon-as-image.html
fast/events/standalone-image-drag-to-editable.html
http/tests/misc/acid3.html

[Build Bot]

Created attachment 289572 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289568 [details]
Patch

Attachment 289568 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2126604

Number of test failures exceeded the failure limit.

[Build Bot]

Comment on attachment 289568 [details]
Patch

Attachment 289568 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2126607

New failing tests:
fast/replaced/outline-replaced-elements-offset.html
http/tests/multipart/invalid-image-data-standalone.html
http/tests/misc/favicon-as-image.html
http/tests/misc/acid3.html
fast/images/favicon-as-image.html

[Build Bot]

Created attachment 289573 [details]
Archive of layout-test-results from ews123 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Created attachment 289574 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Said Abou-Hallawa]

Created attachment 289597 [details]
Patch

[Said Abou-Hallawa]

Created attachment 289605 [details]
Patch

[Build Bot]

Comment on attachment 289605 [details]
Patch

Attachment 289605 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2128020

New failing tests:
http/tests/cache/memory-cache-pruning.html

[Build Bot]

Created attachment 289615 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289605 [details]
Patch

Attachment 289605 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2128025

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 289617 [details]
Archive of layout-test-results from ews124 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 289605 [details]
Patch

Attachment 289605 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2128163

New failing tests:
http/tests/cache/memory-cache-pruning.html

[Build Bot]

Created attachment 289623 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289605 [details]
Patch

Attachment 289605 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2128167

New failing tests:
http/tests/cache/memory-cache-pruning.html

[Build Bot]

Created attachment 289625 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Said Abou-Hallawa]

Created attachment 289633 [details]
Patch

[Said Abou-Hallawa]

Created attachment 289863 [details]
Patch

[Said Abou-Hallawa]

Created attachment 289871 [details]
Patch

[Said Abou-Hallawa]

Created attachment 289882 [details]
Patch

[Said Abou-Hallawa]

Created attachment 289889 [details]
Patch

[Tim Horton]

Comment on attachment 289889 [details]
Patch

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

> Source/WebCore/platform/graphics/BitmapImage.cpp:398
> +    const_cast<ImageSource&>(m_source).dump(ts);

Why the const_cast? Is something wrong somewhere else? Not usually a good sign.

> Source/WebCore/platform/graphics/BitmapImage.h:76
> +    String filenameExtension() const override { return const_cast<ImageSource&>(m_source).filenameExtension(); }
> +    Optional<IntPoint> hotSpot() const override { return const_cast<ImageSource&>(m_source).hotSpot(); }

Ditto on the const_casts. Lots around here. What's up with that?

> Source/WebCore/platform/graphics/ImageFrameCache.h:53
> +    // Image metadata which is calculated by the decoder or can deduced by the case of the memory NativeImage.

I can't parse the second half of this comment.

> Source/WebCore/platform/graphics/ImageFrameCache.h:107
> +    ImageDecoder *m_decoder { nullptr };

Star's on the wrong side.

> Source/WebCore/platform/graphics/ImageSource.cpp:91
> +    // If we have decoded frames but there is no encoded data, we shouldn't destroy
> +    // the decoded image since we won't be able to reconstruct it later.

How does that happen?

> Source/WebCore/platform/graphics/ImageSource.h:116
> +    // FIXME: We should expose a setting to enable/disable progressive loading remove the PLATFORM(IOS)-guard.

Grammar is not quite right here ("so that we can remove", maybe?)

[Said Abou-Hallawa]

Created attachment 289899 [details]
Patch

[Said Abou-Hallawa]

Created attachment 289957 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 289889 [details]
Patch

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

>> Source/WebCore/platform/graphics/BitmapImage.cpp:398
>> +    const_cast<ImageSource&>(m_source).dump(ts);
> 
> Why the const_cast? Is something wrong somewhere else? Not usually a good sign.

Done. ImageSource::dump() is now a const function. All it does it calls ImageFrameCache::dump() which is also a const function. ImageFrameCache::dump() dumps the image metadata. It does not try to cache any of them. It dumps whatever is already cached.

The reason for having ImageSource::dump() not const from the beginning was, it was considered logically const. The only side effect, it had, was lazily caching some of the image metadata. But since this does not seem right, we can dump the existing values without having to mix const calls and non-const calls.

>> Source/WebCore/platform/graphics/BitmapImage.h:76
>> +    Optional<IntPoint> hotSpot() const override { return const_cast<ImageSource&>(m_source).hotSpot(); }
> 
> Ditto on the const_casts. Lots around here. What's up with that?

ImageSource::hotSpot() and ImageSource::filenameExtension() were not const because they may need to cache their values reading from the ImageDecoder. I could have done of one of two things to remove these const_cast:
1. Make ImageSource::hotSpot() and ImageSource::filenameExtension() const which would require ImageFrameCache::hotSpot() and ImageFrameCache::filenameExtension() to be const also. I tried that but I end up marking almost all of the functions in ImageFrameCache const and all the data members to be mutable. This did not seem right when marking functions like ImageFrameCache ::setFrameMetadata() or ImageFrameCache::decodedSizeChanged() be const. 
2. Remove the const qualifiers from the base class functions Image::hotSpot() and Image::filenameExtension(). Although this does not very elegant but I think this is the best we can do to fix this issue.

>> Source/WebCore/platform/graphics/ImageFrameCache.h:53
>> +    // Image metadata which is calculated by the decoder or can deduced by the case of the memory NativeImage.
> 
> I can't parse the second half of this comment.

This comment is now read as:

    // Image metadata which is calculated either by the ImageDecoder or directly
    // from the NativeImage if this class was created for a memory image.

>> Source/WebCore/platform/graphics/ImageFrameCache.h:107
>> +    ImageDecoder *m_decoder { nullptr };
> 
> Star's on the wrong side.

Fixed.

>> Source/WebCore/platform/graphics/ImageSource.cpp:91
>> +    // the decoded image since we won't be able to reconstruct it later.
> 
> How does that happen?

I think this can happen if the BitmapImage is created for a memory image in which case no ImageDecoder is needed and no encoded data is available. All we have is the NativeImagePtr and in this case we should not delete this frame since there is no way to get it back.

>> Source/WebCore/platform/graphics/ImageSource.h:116
>> +    // FIXME: We should expose a setting to enable/disable progressive loading remove the PLATFORM(IOS)-guard.
> 
> Grammar is not quite right here ("so that we can remove", maybe?)

Done.

[Said Abou-Hallawa]

Created attachment 289981 [details]
Patch

[Said Abou-Hallawa]

Created attachment 289985 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 289985 [details]
Patch

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

> Source/WebCore/ChangeLog:19
> +        To remove this burden from the BitmapImage the member 'm_frames' is removed
> +        from BitmapImage. A new member named 'm_frameCache' of type ImageFrameCache
> +        is added to ImageSource. This class handles caching and recaching the image
> +        metadata and the ImageFrames if the image needs decoding. When the BitmapImage
> +        is initialized with a memory image, the ImageFrameCache initializes its
> +        metadata and ImageFrames directly from the NativeImagePtr.

I'm worried this is a bit too much splitting. Could ImageFrameCache be internal to ImageSource? Or can you more strongly justify splitting out ImageFrameCache because of future async decoding changes?

> Source/WebCore/ChangeLog:21
> +        When the BitmapImage replying ImageFrame queries, it will ask the ImageSource

is replying to?

> Source/WebCore/platform/graphics/BitmapImage.cpp:156
> +    float scale = subsamplingScale(context, destRect, srcRect);

If scale is only used when LOG() is enabled (debug), then won't this generate a warning in release builds?

> Source/WebCore/platform/graphics/BitmapImage.h:154
> +    bool isAnimated() const override { return const_cast<BitmapImage*>(this)->frameCount() > 1; }

This is a bit gross.

> Source/WebCore/platform/graphics/ImageFrame.h:121
> +    void setDuration(float duration) { m_duration = duration; }

Times are normally doubles. What units is this in?

> Source/WebCore/platform/graphics/ImageFrameCache.cpp:56
> +    // Since we don't have a decoder, we can't figure out the image orientation.
> +    // Set m_sizeRespectingOrientation to be the same as m_size so it's not 0x0.

Can't we query CG for image metadata?

> Source/WebCore/platform/graphics/ImageFrameCache.cpp:68
> +    unsigned decodedSize = 0;
> +    for (size_t i = 0; i <  count; ++i)
> +        decodedSize += m_frames[i].clearImage();

It's odd that we always clear data from the first frame. Don't we want to do so taking into account which frame is currently displayed?

> Source/WebCore/platform/graphics/ImageFrameCache.cpp:155
> +void ImageFrameCache::ensureFramesSize()

This function has a confusing name, since "size" in most of the file refers to image size.

> Source/WebCore/platform/graphics/ImageFrameCache.cpp:197
> +    frame.m_subsamplingLevel = subsamplingLevel;

It's weird that we consider subsamplingLevel as part of metadata, since it depends on the size at which the image is going to be rendered.

[Said Abou-Hallawa]

Comment on attachment 289985 [details]
Patch

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

>> Source/WebCore/ChangeLog:19
>> +        metadata and ImageFrames directly from the NativeImagePtr.
> 
> I'm worried this is a bit too much splitting. Could ImageFrameCache be internal to ImageSource? Or can you more strongly justify splitting out ImageFrameCache because of future async decoding changes?

I added the following statement to the ChangeLog:

        The plan for ImageFrameCache is to be extended for the asynchronous image
        decoding and also to be used by the non CG image decoders which cache
        other copies of the ImageFrames. This double caching should be removed.

>> Source/WebCore/ChangeLog:21
>> +        When the BitmapImage replying ImageFrame queries, it will ask the ImageSource
> 
> is replying to?

Fixed.

>> Source/WebCore/platform/graphics/BitmapImage.cpp:156
>> +    float scale = subsamplingScale(context, destRect, srcRect);
> 
> If scale is only used when LOG() is enabled (debug), then won't this generate a warning in release builds?

No. Because scale is used in this statement also:

    SubsamplingLevel subsamplingLevel = m_source.subsamplingLevelForScale(scale);

And subsamplingLevel is used in the LOG statement and in the following statement also:

    auto image = frameImageAtIndex(m_currentFrame, subsamplingLevel);

>> Source/WebCore/platform/graphics/BitmapImage.h:154
>> +    bool isAnimated() const override { return const_cast<BitmapImage*>(this)->frameCount() > 1; }
> 
> This is a bit gross.

I fixed all the const issues by making the BitmapImage::m_source member be mutable.

>> Source/WebCore/platform/graphics/ImageFrame.h:121
>> +    void setDuration(float duration) { m_duration = duration; }
> 
> Times are normally doubles. What units is this in?

We set ImageFrame::m_duration to the output of ImageDecoder::frameDurationAtIndex() which is float. It defined to be float for all ports.

>> Source/WebCore/platform/graphics/ImageFrameCache.cpp:56
>> +    // Set m_sizeRespectingOrientation to be the same as m_size so it's not 0x0.
> 
> Can't we query CG for image metadata?

I do not think we can query CG for the image orientation and I do not think we need to do. This is the case where a BitmapImage, an ImageSource and an ImageFrameCache are created for a memory image. In this case the assumption is the memory image will be displayed with the default orientation.

>> Source/WebCore/platform/graphics/ImageFrameCache.cpp:68
>> +        decodedSize += m_frames[i].clearImage();
> 
> It's odd that we always clear data from the first frame. Don't we want to do so taking into account which frame is currently displayed?

Yes we do. We always clear all the frames that comes before count = BitmapImage::m_currentFrame if destroyAll is false. I think this is the best choice since we are clearing the frames that have already been displayed and we are keeping the current one and any other one that comes after it.

>> Source/WebCore/platform/graphics/ImageFrameCache.cpp:155
>> +void ImageFrameCache::ensureFramesSize()
> 
> This function has a confusing name, since "size" in most of the file refers to image size.

I renamed it to growFrames().

>> Source/WebCore/platform/graphics/ImageFrameCache.cpp:197
>> +    frame.m_subsamplingLevel = subsamplingLevel;
> 
> It's weird that we consider subsamplingLevel as part of metadata, since it depends on the size at which the image is going to be rendered.

Yes this is true. But the ImageFrame::m_size and ImageFrame::m_nativeImage are different for different subsamplingLevel. We want to check always if an ImageFrame is valid for a given subsamplingLevel. If it is not, we need to recache it. If the ImageFrames of an image were cached for a certain subsamplingLevel and then the subsamplingLevel got changed, we are going to recache the ImageFrames when any of them is requested. So at some point the ImageFrames of this image will have different subsamplingLevel. But this is fine since we may fix all of them later when we need to render them.

[Said Abou-Hallawa]

Created attachment 290017 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 290017 [details]
Patch

Rejecting attachment 290017 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 290017, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/2158164

[Said Abou-Hallawa]

Created attachment 290033 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 290033 [details]
Patch

Clearing flags on attachment: 290033

Committed r206481: <http://trac.webkit.org/changeset/206481>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/32406251>