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.
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?
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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?)
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.
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.
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.
2016-03-15 11:51 PDT, Said Abou-Hallawa
2016-03-15 12:08 PDT, Said Abou-Hallawa
2016-03-15 12:34 PDT, Said Abou-Hallawa
2016-03-15 12:55 PDT, Said Abou-Hallawa
2016-03-15 13:25 PDT, Said Abou-Hallawa
2016-03-15 13:56 PDT, Said Abou-Hallawa
2016-08-10 15:31 PDT, Said Abou-Hallawa
2016-08-10 16:17 PDT, Build Bot
2016-08-10 16:24 PDT, Build Bot
2016-08-10 16:34 PDT, Build Bot
2016-08-10 17:33 PDT, Said Abou-Hallawa
2016-08-10 17:42 PDT, Said Abou-Hallawa
2016-08-10 18:20 PDT, Said Abou-Hallawa
2016-08-10 18:56 PDT, Said Abou-Hallawa
2016-08-10 19:08 PDT, Said Abou-Hallawa
2016-08-10 19:26 PDT, Said Abou-Hallawa
2016-08-10 20:12 PDT, Build Bot
2016-08-10 20:18 PDT, Build Bot
2016-08-10 20:22 PDT, Build Bot
2016-08-10 23:22 PDT, Said Abou-Hallawa
2016-08-11 00:11 PDT, Said Abou-Hallawa
2016-08-11 01:02 PDT, Build Bot
2016-08-11 01:08 PDT, Build Bot
2016-08-11 01:18 PDT, Build Bot
2016-08-11 09:22 PDT, Said Abou-Hallawa
2016-08-11 10:09 PDT, Build Bot
2016-08-11 10:24 PDT, Build Bot
2016-08-11 10:43 PDT, Build Bot
2016-08-11 11:23 PDT, Said Abou-Hallawa
2016-08-11 13:21 PDT, Build Bot
2016-08-11 15:34 PDT, Said Abou-Hallawa
2016-08-11 16:47 PDT, Build Bot
2016-08-11 16:57 PDT, Said Abou-Hallawa
2016-08-11 18:33 PDT, Said Abou-Hallawa
2016-08-12 10:09 PDT, Said Abou-Hallawa
2016-08-12 11:36 PDT, Said Abou-Hallawa
2016-09-21 16:59 PDT, Said Abou-Hallawa
2016-09-21 17:30 PDT, Said Abou-Hallawa
2016-09-21 17:59 PDT, Said Abou-Hallawa
2016-09-21 18:16 PDT, Said Abou-Hallawa
2016-09-21 19:05 PDT, Build Bot
2016-09-21 19:16 PDT, Build Bot
2016-09-21 19:17 PDT, Build Bot
2016-09-21 19:22 PDT, Build Bot
2016-09-22 09:30 PDT, Said Abou-Hallawa
2016-09-22 09:46 PDT, Said Abou-Hallawa
2016-09-22 10:35 PDT, Build Bot
2016-09-22 10:46 PDT, Build Bot
2016-09-22 10:51 PDT, Build Bot
2016-09-22 10:51 PDT, Build Bot
2016-09-22 14:22 PDT, Said Abou-Hallawa
2016-09-22 14:49 PDT, Said Abou-Hallawa
2016-09-22 15:46 PDT, Build Bot
2016-09-22 15:49 PDT, Build Bot
2016-09-22 16:04 PDT, Build Bot
2016-09-22 16:09 PDT, Build Bot
2016-09-22 17:19 PDT, Said Abou-Hallawa
2016-09-26 13:40 PDT, Said Abou-Hallawa
2016-09-26 14:37 PDT, Said Abou-Hallawa
2016-09-26 15:55 PDT, Said Abou-Hallawa
2016-09-26 16:29 PDT, Said Abou-Hallawa
2016-09-26 19:06 PDT, Said Abou-Hallawa
2016-09-27 09:44 PDT, Said Abou-Hallawa
2016-09-27 10:57 PDT, Said Abou-Hallawa
2016-09-27 11:38 PDT, Said Abou-Hallawa
2016-09-27 16:06 PDT, Said Abou-Hallawa
2016-09-27 17:47 PDT, Said Abou-Hallawa