RESOLVED FIXED 159819
Rename FrameData to ImageFrame, move it to a separate file and use it for all ports
https://bugs.webkit.org/show_bug.cgi?id=159819
Summary Rename FrameData to ImageFrame, move it to a separate file and use it for all...
Said Abou-Hallawa
Reported 2016-07-15 11:05:14 PDT
Currently ImageFrame is a subset of ImageFrameData so the later can replace the former.
Attachments
Patch (226.61 KB, patch)
2016-07-15 11:06 PDT, Said Abou-Hallawa
no flags
Patch (206.13 KB, patch)
2016-07-15 12:55 PDT, Said Abou-Hallawa
no flags
Patch (206.13 KB, patch)
2016-07-15 15:05 PDT, Said Abou-Hallawa
no flags
Patch (206.13 KB, patch)
2016-07-15 15:12 PDT, Said Abou-Hallawa
no flags
Patch for review (34.22 KB, patch)
2016-07-15 17:33 PDT, Said Abou-Hallawa
no flags
Patch (97.85 KB, patch)
2016-09-14 11:13 PDT, Said Abou-Hallawa
no flags
Patch (97.97 KB, patch)
2016-09-14 11:44 PDT, Said Abou-Hallawa
no flags
Patch (100.12 KB, patch)
2016-09-14 12:02 PDT, Said Abou-Hallawa
no flags
Patch (100.11 KB, patch)
2016-09-14 12:06 PDT, Said Abou-Hallawa
no flags
Archive of layout-test-results from ews103 for mac-yosemite (1.01 MB, application/zip)
2016-09-14 13:17 PDT, Build Bot
no flags
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (1.21 MB, application/zip)
2016-09-14 13:21 PDT, Build Bot
no flags
Archive of layout-test-results from ews116 for mac-yosemite (1.52 MB, application/zip)
2016-09-14 13:22 PDT, Build Bot
no flags
Patch (107.98 KB, patch)
2016-09-14 13:44 PDT, Said Abou-Hallawa
no flags
Archive of layout-test-results from ews102 for mac-yosemite (977.36 KB, application/zip)
2016-09-14 14:36 PDT, Build Bot
no flags
Archive of layout-test-results from ews113 for mac-yosemite (1.63 MB, application/zip)
2016-09-14 14:49 PDT, Build Bot
no flags
Patch (112.20 KB, patch)
2016-09-14 15:38 PDT, Said Abou-Hallawa
no flags
Archive of layout-test-results from ews101 for mac-yosemite (1.11 MB, application/zip)
2016-09-14 16:47 PDT, Build Bot
no flags
Archive of layout-test-results from ews112 for mac-yosemite (1.51 MB, application/zip)
2016-09-14 16:47 PDT, Build Bot
no flags
Patch (112.33 KB, patch)
2016-09-14 17:32 PDT, Said Abou-Hallawa
no flags
Patch (114.64 KB, patch)
2016-09-14 18:39 PDT, Said Abou-Hallawa
no flags
Archive of layout-test-results from ews103 for mac-yosemite (1011.74 KB, application/zip)
2016-09-14 19:40 PDT, Build Bot
no flags
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (975.54 KB, application/zip)
2016-09-14 19:44 PDT, Build Bot
no flags
Archive of layout-test-results from ews116 for mac-yosemite (1.55 MB, application/zip)
2016-09-14 19:48 PDT, Build Bot
no flags
Patch (115.33 KB, patch)
2016-09-15 09:44 PDT, Said Abou-Hallawa
no flags
Archive of layout-test-results from ews103 for mac-yosemite (1.03 MB, application/zip)
2016-09-15 10:37 PDT, Build Bot
no flags
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (1.21 MB, application/zip)
2016-09-15 10:41 PDT, Build Bot
no flags
Archive of layout-test-results from ews115 for mac-yosemite (1.58 MB, application/zip)
2016-09-15 10:54 PDT, Build Bot
no flags
Patch (115.17 KB, patch)
2016-09-15 11:05 PDT, Said Abou-Hallawa
no flags
Patch (123.80 KB, patch)
2016-09-15 14:02 PDT, Said Abou-Hallawa
no flags
Patch (123.95 KB, patch)
2016-09-16 11:23 PDT, Said Abou-Hallawa
no flags
Patch (123.93 KB, patch)
2016-09-20 09:57 PDT, Said Abou-Hallawa
no flags
Said Abou-Hallawa
Comment 1 2016-07-15 11:06:29 PDT
Said Abou-Hallawa
Comment 2 2016-07-15 12:55:56 PDT
Said Abou-Hallawa
Comment 3 2016-07-15 15:05:31 PDT
Said Abou-Hallawa
Comment 4 2016-07-15 15:12:27 PDT
Said Abou-Hallawa
Comment 5 2016-07-15 17:33:18 PDT
Created attachment 283827 [details] Patch for review
Darin Adler
Comment 6 2016-07-16 09:08:18 PDT
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.
Said Abou-Hallawa
Comment 7 2016-09-14 11:13:29 PDT
Said Abou-Hallawa
Comment 8 2016-09-14 11:44:19 PDT
Simon Fraser (smfr)
Comment 9 2016-09-14 11:50:24 PDT
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?
Said Abou-Hallawa
Comment 10 2016-09-14 12:02:32 PDT
Said Abou-Hallawa
Comment 11 2016-09-14 12:06:50 PDT
Build Bot
Comment 12 2016-09-14 13:17:00 PDT
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
Build Bot
Comment 13 2016-09-14 13:17:03 PDT
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
Build Bot
Comment 14 2016-09-14 13:21:35 PDT
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
Build Bot
Comment 15 2016-09-14 13:21:38 PDT
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
Build Bot
Comment 16 2016-09-14 13:22:09 PDT
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
Build Bot
Comment 17 2016-09-14 13:22:12 PDT
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
Said Abou-Hallawa
Comment 18 2016-09-14 13:44:34 PDT
Build Bot
Comment 19 2016-09-14 14:36:29 PDT
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
Build Bot
Comment 20 2016-09-14 14:36:33 PDT
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
Build Bot
Comment 21 2016-09-14 14:49:18 PDT
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
Build Bot
Comment 22 2016-09-14 14:49:22 PDT
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
Said Abou-Hallawa
Comment 23 2016-09-14 15:38:51 PDT
Build Bot
Comment 24 2016-09-14 16:47:05 PDT
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
Build Bot
Comment 25 2016-09-14 16:47:08 PDT
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
Build Bot
Comment 26 2016-09-14 16:47:36 PDT
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
Build Bot
Comment 27 2016-09-14 16:47:39 PDT
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
Said Abou-Hallawa
Comment 28 2016-09-14 17:32:41 PDT
Said Abou-Hallawa
Comment 29 2016-09-14 18:39:28 PDT
Build Bot
Comment 30 2016-09-14 19:40:14 PDT
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
Build Bot
Comment 31 2016-09-14 19:40:19 PDT
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
Build Bot
Comment 32 2016-09-14 19:44:25 PDT
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
Build Bot
Comment 33 2016-09-14 19:44:29 PDT
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
Build Bot
Comment 34 2016-09-14 19:47:59 PDT
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
Build Bot
Comment 35 2016-09-14 19:48:02 PDT
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
Said Abou-Hallawa
Comment 36 2016-09-15 09:44:26 PDT
Build Bot
Comment 37 2016-09-15 10:37:43 PDT
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
Build Bot
Comment 38 2016-09-15 10:37:46 PDT
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
Build Bot
Comment 39 2016-09-15 10:41:06 PDT
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
Build Bot
Comment 40 2016-09-15 10:41:09 PDT
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
Build Bot
Comment 41 2016-09-15 10:54:27 PDT
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
Build Bot
Comment 42 2016-09-15 10:54:31 PDT
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
Said Abou-Hallawa
Comment 43 2016-09-15 11:05:06 PDT
Said Abou-Hallawa
Comment 44 2016-09-15 14:02:06 PDT
Said Abou-Hallawa
Comment 45 2016-09-15 14:28:31 PDT
*** Bug 159795 has been marked as a duplicate of this bug. ***
Said Abou-Hallawa
Comment 46 2016-09-15 14:29:19 PDT
*** Bug 155444 has been marked as a duplicate of this bug. ***
Said Abou-Hallawa
Comment 47 2016-09-15 14:49:42 PDT
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.
Tim Horton
Comment 48 2016-09-15 19:04:35 PDT
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?
Tim Horton
Comment 49 2016-09-15 19:07:03 PDT
(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
Said Abou-Hallawa
Comment 50 2016-09-16 11:06:07 PDT
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.
Said Abou-Hallawa
Comment 51 2016-09-16 11:23:59 PDT
Said Abou-Hallawa
Comment 52 2016-09-16 11:26:24 PDT
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().
Said Abou-Hallawa
Comment 53 2016-09-16 12:55:02 PDT
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.
Simon Fraser (smfr)
Comment 54 2016-09-19 14:52:42 PDT
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.
Said Abou-Hallawa
Comment 55 2016-09-20 09:57:01 PDT
Said Abou-Hallawa
Comment 56 2016-09-20 10:03:01 PDT
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.
WebKit Commit Bot
Comment 57 2016-09-20 11:26:55 PDT
Comment on attachment 289365 [details] Patch Clearing flags on attachment: 289365 Committed r206156: <http://trac.webkit.org/changeset/206156>
WebKit Commit Bot
Comment 58 2016-09-20 11:27:03 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.