Currently ImageFrame is a subset of ImageFrameData so the later can replace the former.
Created attachment 283771 [details] Patch
Created attachment 283784 [details] Patch
Created attachment 283797 [details] Patch
Created attachment 283799 [details] Patch
Created attachment 283827 [details] Patch for review
Comment on attachment 283827 [details] Patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=283827&action=review Refactoring looks OK. > Source/WebCore/platform/graphics/ImageFrame.cpp:37 > +#include "Color.h" > + > +#if USE(CG) > +#include "ImageDecoderCG.h" > +#else > +#include "ImageDecoder.h" > +#endif > + > +#include "PlatformImage.h" Normally we put all unconditional includes first, and then includes afterward. So PlatformImage.h should be paragraphed with Color.h. > Source/WebCore/platform/graphics/ImageFrame.cpp:50 > + m_size = PlatformImage(m_image).size(); > + m_hasAlpha = PlatformImage(m_image).hasAlpha(); I know this code is just being moved, but I noticed this: Is the construction of a PlatformImage guaranteed to be inexpensive? If not, then I suggest we put it into a local variable so we don’t have to do it twice. > Source/WebCore/platform/graphics/ImageFrame.cpp:97 > +void ImageFrame::clearSubImages() I know this code is just being moved, but "subimages" is a single coined word, not two words "sub images", so it should be spelled Subimages, not SubImages.
Created attachment 288833 [details] Patch
Created attachment 288840 [details] Patch
Comment on attachment 288840 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288840&action=review > Source/WebCore/platform/graphics/ImageFrame.h:40 > +using SubsamplingLevel = int; Normally we typedef. > Source/WebCore/platform/graphics/ImageFrame.h:42 > +enum : SubsamplingLevel { enum class? > Source/WebCore/platform/graphics/ImageFrame.h:49 > +// The animation cycles infinitely if repetitionCount() is AnimationLoopInfinite. No need for this comment; the values are self-explanatory. > Source/WebCore/platform/graphics/ImageFrame.h:50 > +using AnimationLoop = int; typedef? > Source/WebCore/platform/graphics/ImageFrame.h:52 > +enum : AnimationLoop { enum class?
Created attachment 288843 [details] Patch
Created attachment 288844 [details] Patch
Comment on attachment 288844 [details] Patch Attachment 288844 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2073837 New failing tests: http/tests/misc/slow-loading-animated-image.html
Created attachment 288847 [details] Archive of layout-test-results from ews103 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 288844 [details] Patch Attachment 288844 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2073846 New failing tests: http/tests/misc/slow-loading-animated-image.html
Created attachment 288849 [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 288844 [details] Patch Attachment 288844 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2073829 New failing tests: http/tests/misc/slow-loading-animated-image.html
Created attachment 288850 [details] Archive of layout-test-results from ews116 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 288853 [details] Patch
Comment on attachment 288853 [details] Patch Attachment 288853 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2074197 New failing tests: http/tests/misc/slow-loading-animated-image.html
Created attachment 288860 [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
Comment on attachment 288853 [details] Patch Attachment 288853 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2074198 New failing tests: http/tests/misc/slow-loading-animated-image.html
Created attachment 288865 [details] Archive of layout-test-results from ews113 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 288874 [details] Patch
Comment on attachment 288874 [details] Patch Attachment 288874 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2075045 New failing tests: http/tests/misc/slow-loading-animated-image.html
Created attachment 288894 [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
Comment on attachment 288874 [details] Patch Attachment 288874 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2075007 New failing tests: http/tests/misc/slow-loading-animated-image.html
Created attachment 288896 [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 288903 [details] Patch
Created attachment 288909 [details] Patch
Comment on attachment 288909 [details] Patch Attachment 288909 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2076243 New failing tests: http/tests/misc/slow-loading-animated-image.html
Created attachment 288915 [details] Archive of layout-test-results from ews103 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 288909 [details] Patch Attachment 288909 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2076249 New failing tests: http/tests/misc/slow-loading-animated-image.html
Created attachment 288916 [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 288909 [details] Patch Attachment 288909 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2076250 New failing tests: http/tests/misc/slow-loading-animated-image.html
Created attachment 288918 [details] Archive of layout-test-results from ews116 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 288958 [details] Patch
Comment on attachment 288958 [details] Patch Attachment 288958 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2080400 New failing tests: http/tests/misc/slow-loading-animated-image.html
Created attachment 288969 [details] Archive of layout-test-results from ews103 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 288958 [details] Patch Attachment 288958 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2080405 New failing tests: http/tests/misc/slow-loading-animated-image.html
Created attachment 288971 [details] Archive of layout-test-results from ews106 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 288958 [details] Patch Attachment 288958 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2080439 New failing tests: http/tests/misc/slow-loading-animated-image.html
Created attachment 288972 [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 288975 [details] Patch
Created attachment 288997 [details] Patch
*** Bug 159795 has been marked as a duplicate of this bug. ***
*** Bug 155444 has been marked as a duplicate of this bug. ***
Comment on attachment 288840 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288840&action=review >> Source/WebCore/platform/graphics/ImageFrame.h:42 >> +enum : SubsamplingLevel { > > enum class? Done. And the using statement is removed since SubsamplingLevel can be strongly typed. >> Source/WebCore/platform/graphics/ImageFrame.h:49 >> +// The animation cycles infinitely if repetitionCount() is AnimationLoopInfinite. > > No need for this comment; the values are self-explanatory. Removed. >> Source/WebCore/platform/graphics/ImageFrame.h:50 >> +using AnimationLoop = int; > > typedef? Done. >> Source/WebCore/platform/graphics/ImageFrame.h:52 >> +enum : AnimationLoop { > > enum class? No. Because AnimationLoop is not strongly typed. In addition to the values below, it can be any other unsigned value.
Comment on attachment 288997 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288997&action=review > Source/WebCore/platform/graphics/ImageFrame.h:132 > + void fillMetaData(ImageDecoder&, size_t, SubsamplingLevel, bool animating); Please pick one of Metadata or MetaData (probably Metadata), but not both. > Source/WebCore/platform/graphics/ImageSource.h:-54 > -// Right now GIFs are the only recognized image format that supports animation. > -// The animation system and the constants below are designed with this in mind. > -// GIFs have an optional 16-bit unsigned loop count that describes how an > -// animated GIF should be cycled. If the loop count is absent, the animation > -// cycles once; if it is 0, the animation cycles infinitely; otherwise the > -// animation plays n + 1 cycles (where n is the specified loop count). If the > -// GIF decoder defaults to cAnimationLoopOnce in the absence of any loop count > -// and translates an explicit "0" loop count to cAnimationLoopInfinite, then we > -// get a couple of nice side effects: Are you sure you got all of these behaviors right? Do we have tests? Can we? Can you at least manually test?
(In reply to comment #47) > Comment on attachment 288840 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=288840&action=review > > >> Source/WebCore/platform/graphics/ImageFrame.h:42 > >> +enum : SubsamplingLevel { > > > > enum class? > > Done. And the using statement is removed since SubsamplingLevel can be > strongly typed. > > >> Source/WebCore/platform/graphics/ImageFrame.h:49 > >> +// The animation cycles infinitely if repetitionCount() is AnimationLoopInfinite. > > > > No need for this comment; the values are self-explanatory. > > Removed. > > >> Source/WebCore/platform/graphics/ImageFrame.h:50 > >> +using AnimationLoop = int; > > > > typedef? > > Done. > > >> Source/WebCore/platform/graphics/ImageFrame.h:52 > >> +enum : AnimationLoop { > > > > enum class? > > No. Because AnimationLoop is not strongly typed. In addition to the values > below, it can be any other unsigned value. I think it should be called AnimationLoopCount or just LoopCount
Comment on attachment 288997 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288997&action=review >> Source/WebCore/platform/graphics/ImageFrame.h:132 >> + void fillMetaData(ImageDecoder&, size_t, SubsamplingLevel, bool animating); > > Please pick one of Metadata or MetaData (probably Metadata), but not both. Done. >> Source/WebCore/platform/graphics/ImageSource.h:-54 >> -// get a couple of nice side effects: > > Are you sure you got all of these behaviors right? Do we have tests? Can we? Can you at least manually test? This comment is not correct. Here is the calculation of the repetition count according to ImageDecoder::repetitionCount() in ImageDecoderCG.cpp: Not a gif or a png image repetitionCount = 0 (AnimationLoopNone) LoopCount property is missing repetitionCount = 1 (AnimationLoopOnce) LoopCount property == 0 repetitionCount = Infinite (AnimationLoopInfinite) LoopCount property == n (including 1) repetitionCount = n (including AnimationLoopOnce) I do not think also the values of the cAnimation* have any nice side effects at least in the current state of the code. Yes they do not conflict with real loop count values. But there is no need for that except for AnimationLoopInfinite which should be a special case. cAnimationNone was chosen to be -2. I changed that to be 0 so I could have the following initializations in BitmapImage.h AnimationLoop m_repetitionCount { AnimationLoopNone }; AnimationLoop m_repetitionsComplete { AnimationLoopNone }; Instead of int m_repetitionCount { cAnimationNone }; int m_repetitionsComplete { 0 }; m_repetitionCount represents how many loops the image can animate. And m_repetitionsComplete represents how many animation loops have finished. The first one is initialized with -1 while the second initialized with zero. I think it makes more sense to start both of them with 0 (or AnimationLoopNone). cAnimationOnce was chosen to be 0. I changed that to be 1 so I have can have the following condition in BitmapImage::startAnimation() bool BitmapImage::shouldAnimate() { return repetitionCount(false) && !m_animationFinished && imageObserver(); } Instead of bool BitmapImage::shouldAnimate() { return (repetitionCount(false) != AnimationLoopNone && !m_animationFinished && imageObserver()); } I reviewed all the use cases of these constants and I found they were special cases when checking the value of the repetitionCount. There is no math which depends on these values. So there is no harm in changing their values to reflect their their actual meanings.
Created attachment 289081 [details] Patch
Comment on attachment 288840 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288840&action=review >>>> Source/WebCore/platform/graphics/ImageFrame.h:42 >>>> +enum : SubsamplingLevel { >>> >>> enum class? >> >> Done. And the using statement is removed since SubsamplingLevel can be strongly typed. > > I think it should be called AnimationLoopCount or just LoopCount I changed it to be RepetitionCount because the function that calculates it is named repetitionCount().
Comment on attachment 288997 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288997&action=review >>> Source/WebCore/platform/graphics/ImageSource.h:-54 >>> -// get a couple of nice side effects: >> >> Are you sure you got all of these behaviors right? Do we have tests? Can we? Can you at least manually test? > > This comment is not correct. Here is the calculation of the repetition count according to ImageDecoder::repetitionCount() in ImageDecoderCG.cpp: > > Not a gif or a png image repetitionCount = 0 (AnimationLoopNone) > LoopCount property is missing repetitionCount = 1 (AnimationLoopOnce) > LoopCount property == 0 repetitionCount = Infinite (AnimationLoopInfinite) > LoopCount property == n (including 1) repetitionCount = n (including AnimationLoopOnce) > > I do not think also the values of the cAnimation* have any nice side effects at least in the current state of the code. Yes they do not conflict with real loop count values. But there is no need for that except for AnimationLoopInfinite which should be a special case. > > cAnimationNone was chosen to be -2. I changed that to be 0 so I could have the following initializations in BitmapImage.h > AnimationLoop m_repetitionCount { AnimationLoopNone }; > AnimationLoop m_repetitionsComplete { AnimationLoopNone }; > > Instead of > > int m_repetitionCount { cAnimationNone }; > int m_repetitionsComplete { 0 }; > > m_repetitionCount represents how many loops the image can animate. And m_repetitionsComplete represents how many animation loops have finished. The first one is initialized with -1 while the second initialized with zero. I think it makes more sense to start both of them with 0 (or AnimationLoopNone). > > cAnimationOnce was chosen to be 0. I changed that to be 1 so I have can have the following condition in BitmapImage::startAnimation() > > bool BitmapImage::shouldAnimate() > { > return repetitionCount(false) && !m_animationFinished && imageObserver(); > } > > Instead of > > bool BitmapImage::shouldAnimate() > { > return (repetitionCount(false) != AnimationLoopNone && !m_animationFinished && imageObserver()); > } > > I reviewed all the use cases of these constants and I found they were special cases when checking the value of the repetitionCount. There is no math which depends on these values. So there is no harm in changing their values to reflect their their actual meanings. This is already covered by many tests. For example fast/images/gif-loop-count.html has an image with a missing repetition key so ImageDecoder::repetitionCount() returns RepetitionCountOnce. And http/tests/misc/slow-loading-animated-image.html has as image with repetition key equals to zero so ImageDecoder::repetitionCount() returns RepetitionCountInfinite.
Comment on attachment 289081 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=289081&action=review > Source/WebCore/platform/graphics/BitmapImage.cpp:108 > + frameBytesCleared += m_frames[i].clear(ImageFrame::Caching::Metadata); It's confusing that clear(ImageFrame::Caching::Metadata) does NOT clear metadata. I would expect to read clear(KeepMetadata) or clear(FrameData) > Source/WebCore/platform/graphics/BitmapImage.cpp:168 > + NativeImagePtr image = m_source.createFrameImageAtIndex(index, subsamplingLevel); > + m_frames[index].initialize(WTFMove(image), *m_source.decoder(), index, subsamplingLevel, repetitionCount(false)); It seems a little odd here to get the image from the ImageSource, and then pass it to initialize(), but also pass the source's decoder. It's also sad that the native image type shows up in BitmapImage.cpp. > Source/WebCore/platform/graphics/BitmapImage.cpp:272 > - if (int bytes = m_frames[m_frames.size() - 1].clear(true)) { > + if (int bytes = m_frames[m_frames.size() - 1].clear(ImageFrame::Caching::Empty)) { I don't know what's being cleared here. Maybe using this enum for clear() is just confusing.
Created attachment 289365 [details] Patch
Comment on attachment 289081 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=289081&action=review >> Source/WebCore/platform/graphics/BitmapImage.cpp:108 >> + frameBytesCleared += m_frames[i].clear(ImageFrame::Caching::Metadata); > > It's confusing that clear(ImageFrame::Caching::Metadata) does NOT clear metadata. I would expect to read clear(KeepMetadata) or clear(FrameData) The caching argument was removed. Calling clear(ImageFrame::Caching::Metadata) means clearing the NativeImagePtr only but keeping the metadata. So I created another function and I named it ImageFrame::clearImage() which will be called here instead. >> Source/WebCore/platform/graphics/BitmapImage.cpp:168 >> + m_frames[index].initialize(WTFMove(image), *m_source.decoder(), index, subsamplingLevel, repetitionCount(false)); > > It seems a little odd here to get the image from the ImageSource, and then pass it to initialize(), but also pass the source's decoder. It's also sad that the native image type shows up in BitmapImage.cpp. This will be fixed in the fix of https://bugs.webkit.org/show_bug.cgi?id=155498 where caching the frames will be moved to the ImageSource and the ImageFrame will be hidden from BitmapImage. >> Source/WebCore/platform/graphics/BitmapImage.cpp:272 >> + if (int bytes = m_frames[m_frames.size() - 1].clear(ImageFrame::Caching::Empty)) { > > I don't know what's being cleared here. Maybe using this enum for clear() is just confusing. The caching argument was removed.
Comment on attachment 289365 [details] Patch Clearing flags on attachment: 289365 Committed r206156: <http://trac.webkit.org/changeset/206156>
All reviewed patches have been landed. Closing bug.