Bug 176118

Summary: Virtualize ImageDecoder
Product: WebKit Reporter: Jer Noble <jer.noble>
Component: ImagesAssignee: Jer Noble <jer.noble>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, clopez, commit-queue, eric.carlson, jonlee, magomez, mcatanzaro, rniwa, sabouhallawa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=176089
Bug Depends on:    
Bug Blocks: 176825    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews101 for mac-elcapitan
none
Archive of layout-test-results from ews117 for mac-elcapitan
none
Archive of layout-test-results from ews126 for ios-simulator-wk2
none
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews105 for mac-elcapitan-wk2
none
Patch for landing
jer.noble: commit-queue-
Patch for landing
jer.noble: commit-queue-
Patch for landing
jer.noble: commit-queue-
Patch for landing
jer.noble: commit-queue-
Patch for landing
jer.noble: commit-queue-
Patch for landing
jer.noble: commit-queue-
Patch for landing
jer.noble: commit-queue-
Patch for landing
jer.noble: commit-queue-
Patch for landing
none
Patch for landing
jer.noble: commit-queue-
Patch for landing
jer.noble: commit-queue-
Patch for landing
none
Patch for landing none

Jer Noble
Reported 2017-08-30 12:35:08 PDT
Virtualize ImageDecoder
Attachments
Patch (24.07 KB, patch)
2017-09-08 12:28 PDT, Jer Noble 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews101 for mac-elcapitan (1.54 MB, application/zip)
2017-09-08 13:42 PDT, Build Bot 		no flags
Details
Archive of layout-test-results from ews117 for mac-elcapitan (2.29 MB, application/zip)
2017-09-08 13:55 PDT, Build Bot 		no flags
Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (1.65 MB, application/zip)
2017-09-08 14:19 PDT, Build Bot 		no flags
Details
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (2.30 MB, application/zip)
2017-09-08 14:31 PDT, Build Bot 		no flags
Details
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (2.37 MB, application/zip)
2017-09-08 16:32 PDT, Build Bot 		no flags
Details
Patch for landing (193.07 KB, patch)
2017-09-12 22:47 PDT, Jer Noble 		jer.noble: commit-queue-
DetailsFormatted DiffDiff
Patch for landing (53.33 KB, patch)
2017-09-13 12:20 PDT, Jer Noble 		jer.noble: commit-queue-
DetailsFormatted DiffDiff
Patch for landing (53.92 KB, patch)
2017-09-13 12:40 PDT, Jer Noble 		jer.noble: commit-queue-
DetailsFormatted DiffDiff
Patch for landing (54.74 KB, patch)
2017-09-13 13:30 PDT, Jer Noble 		jer.noble: commit-queue-
DetailsFormatted DiffDiff
Patch for landing (29.81 KB, patch)
2017-09-13 13:45 PDT, Jer Noble 		jer.noble: commit-queue-
DetailsFormatted DiffDiff
Patch for landing (78.08 KB, patch)
2017-09-13 14:29 PDT, Jer Noble 		jer.noble: commit-queue-
DetailsFormatted DiffDiff
Patch for landing (117.29 KB, patch)
2017-09-13 14:41 PDT, Jer Noble 		jer.noble: commit-queue-
DetailsFormatted DiffDiff
Patch for landing (117.20 KB, patch)
2017-09-13 15:05 PDT, Jer Noble 		jer.noble: commit-queue-
DetailsFormatted DiffDiff
Patch for landing (117.29 KB, patch)
2017-09-13 15:36 PDT, Jer Noble 		no flags
DetailsFormatted DiffDiff
Patch for landing (118.04 KB, patch)
2017-09-13 16:28 PDT, Jer Noble 		jer.noble: commit-queue-
DetailsFormatted DiffDiff
Patch for landing (119.00 KB, patch)
2017-09-14 22:33 PDT, Jer Noble 		jer.noble: commit-queue-
DetailsFormatted DiffDiff
Patch for landing (119.00 KB, patch)
2017-09-15 22:41 PDT, Jer Noble 		no flags
DetailsFormatted DiffDiff
Patch for landing (119.01 KB, patch)
2017-09-18 08:41 PDT, Jer Noble 		no flags
DetailsFormatted DiffDiff
Show Obsolete (18) View All Add attachment proposed patch, testcase, etc.
Jer Noble
Comment 1 2017-09-08 12:28:11 PDT
Created attachment 320287 [details] Patch
Build Bot
Comment 2 2017-09-08 13:42:25 PDT Comment hidden (obsolete)
Build Bot
Comment 3 2017-09-08 13:42:26 PDT Comment hidden (obsolete)
Build Bot
Comment 4 2017-09-08 13:55:34 PDT Comment hidden (obsolete)
Build Bot
Comment 5 2017-09-08 13:55:35 PDT Comment hidden (obsolete)
Build Bot
Comment 6 2017-09-08 14:19:22 PDT Comment hidden (obsolete)
Build Bot
Comment 7 2017-09-08 14:19:24 PDT Comment hidden (obsolete)
Build Bot
Comment 8 2017-09-08 14:31:54 PDT Comment hidden (obsolete)
Build Bot
Comment 9 2017-09-08 14:31:55 PDT Comment hidden (obsolete)
Build Bot
Comment 10 2017-09-08 16:32:25 PDT Comment hidden (obsolete)
Build Bot
Comment 11 2017-09-08 16:32:27 PDT Comment hidden (obsolete)
Miguel Gomez
Comment 12 2017-09-12 00:27:05 PDT
Maybe it would be interesting to fix https://bugs.webkit.org/show_bug.cgi?id=172773 before fixing this. It will define a single ImageDecoder.h for all the ports, and then this change can be performed easier.
Jer Noble
Comment 13 2017-09-12 22:43:33 PDT
(In reply to Miguel Gomez from comment #12) > Maybe it would be interesting to fix > https://bugs.webkit.org/show_bug.cgi?id=172773 before fixing this. It will > define a single ImageDecoder.h for all the ports, and then this change can > be performed easier. WRT that patch, I'm not enthusiastic about how many platform details are exposed the virtual base class. I think that it would be much cleaner to merge Said's patch into this one than vice versa.
Jer Noble
Comment 14 2017-09-12 22:47:06 PDT
Created attachment 320620 [details] Patch for landing
Miguel Gomez
Comment 15 2017-09-13 00:43:24 PDT
Both GTK and WPE ports already have an ImageDecoder superclass in Source/WebCore/platform/image-decoders. I guess that adding a new ImageDecoder definition in Source/WebCore/platform is what is causing WPE and GTK bots to fail.
Jer Noble
Comment 16 2017-09-13 08:30:39 PDT
(In reply to Miguel Gomez from comment #15) > Both GTK and WPE ports already have an ImageDecoder superclass in > Source/WebCore/platform/image-decoders. I guess that adding a new > ImageDecoder definition in Source/WebCore/platform is what is causing WPE > and GTK bots to fail. Yep. I may have to integrate the two before landing.
Said Abou-Hallawa
Comment 17 2017-09-13 10:20:45 PDT
(In reply to Jer Noble from comment #16) > (In reply to Miguel Gomez from comment #15) > > Both GTK and WPE ports already have an ImageDecoder superclass in > > Source/WebCore/platform/image-decoders. I guess that adding a new > > ImageDecoder definition in Source/WebCore/platform is what is causing WPE > > and GTK bots to fail. > > Yep. I may have to integrate the two before landing. Another way to write this patch can be the following structure: Source/WebCore/platform/graphics/BaseImageDecoder.h class BaseImageDecoder : public ThreadSafeRefCounted<BaseImageDecoder> { // Add the virtual functions here. }; Source/WebCore/platform/graphics/cg/ImageDecoderCG.h class ImageDecoder : public BaseImageDecoder { // Override the virtual functions here. };
Jer Noble
Comment 18 2017-09-13 10:56:43 PDT
(In reply to Said Abou-Hallawa from comment #17) > (In reply to Jer Noble from comment #16) > > (In reply to Miguel Gomez from comment #15) > > > Both GTK and WPE ports already have an ImageDecoder superclass in > > > Source/WebCore/platform/image-decoders. I guess that adding a new > > > ImageDecoder definition in Source/WebCore/platform is what is causing WPE > > > and GTK bots to fail. > > > > Yep. I may have to integrate the two before landing. > > Another way to write this patch can be the following structure: > > Source/WebCore/platform/graphics/BaseImageDecoder.h > > class BaseImageDecoder : public ThreadSafeRefCounted<BaseImageDecoder> { > // Add the virtual functions here. > }; > > > Source/WebCore/platform/graphics/cg/ImageDecoderCG.h > > class ImageDecoder : public BaseImageDecoder { > // Override the virtual functions here. > }; True, but that would probably require changing all the references in ImageSource, ImageFrameCache, etc. from ImageDecoder to BaseImageDecoder.
Jer Noble
Comment 19 2017-09-13 12:20:05 PDT
Created attachment 320674 [details] Patch for landing
Jer Noble
Comment 20 2017-09-13 12:40:51 PDT
Created attachment 320676 [details] Patch for landing
Jer Noble
Comment 21 2017-09-13 13:30:42 PDT
Created attachment 320682 [details] Patch for landing
Jer Noble
Comment 22 2017-09-13 13:45:24 PDT
Created attachment 320686 [details] Patch for landing
Build Bot
Comment 23 2017-09-13 13:48:44 PDT Comment hidden (obsolete)
Jer Noble
Comment 24 2017-09-13 14:29:17 PDT
Created attachment 320693 [details] Patch for landing
Build Bot
Comment 25 2017-09-13 14:30:52 PDT Comment hidden (obsolete)
Jer Noble
Comment 26 2017-09-13 14:41:39 PDT
Created attachment 320694 [details] Patch for landing
Build Bot
Comment 27 2017-09-13 14:44:10 PDT Comment hidden (obsolete)
Jer Noble
Comment 28 2017-09-13 14:54:40 PDT
(In reply to Build Bot from comment #27) > Attachment 320694 [details] did not pass style-queue: > (snip). Ugh.
Jer Noble
Comment 29 2017-09-13 15:05:38 PDT
Created attachment 320695 [details] Patch for landing
Build Bot
Comment 30 2017-09-13 15:07:36 PDT Comment hidden (obsolete)
Jer Noble
Comment 31 2017-09-13 15:36:39 PDT
Created attachment 320699 [details] Patch for landing
Build Bot
Comment 32 2017-09-13 15:42:35 PDT Comment hidden (obsolete)
Jer Noble
Comment 33 2017-09-13 16:28:32 PDT
Created attachment 320707 [details] Patch for landing
Build Bot
Comment 34 2017-09-13 16:31:44 PDT Comment hidden (obsolete)
Jer Noble
Comment 35 2017-09-13 16:37:29 PDT
Okay, the GTK+ and WPE bots are happy; now to work on the Win bot.
Jer Noble
Comment 36 2017-09-13 16:39:59 PDT
(In reply to Jer Noble from comment #35) > Okay, the GTK+ and WPE bots are happy; now to work on the Win bot. Actually, the Win bot might be happy too, judging by the immediately prior patch for landing.
Jer Noble
Comment 37 2017-09-13 17:13:17 PDT
Okay, looks like the GTK+, WPE, and Win bots are all happy. Said, Miguel, would you mind looking this over and giving a LGTM (or not)? I'm specifically interested in what you think of: - the rename of the image-decoders base class from ImageDecoder -> ScalableImageDecoder - the changes to ScalableImageDecoder::frameIsCompleteAtIndex(...), ScalableImageDecoder::frameDurationAtIndex(...) to make them const-correct.
Said Abou-Hallawa
Comment 38 2017-09-14 15:33:16 PDT
Comment on attachment 320707 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=320707&action=review > Source/WebCore/platform/graphics/ImageDecoder.cpp:47 > +#if USE(CG) > + return ImageDecoderCG::create(data, alphaOption, gammaAndColorProfileOption); > +#elif USE(DIRECT2D) > + return ImageDecoderDirect2D::create(data, alphaOption, gammaAndColorProfileOption); > +#else > + return ScalableImageDecoder::create(data, alphaOption, gammaAndColorProfileOption); > +#endif I do not like using different names for different platforms. I would suggest using the same name for all platforms, e.g. PlatformImageDecoder. In this case ImageDecoderCG.h and ImageDecoderCG.cpp will have the declaration and the implementation of PlatformImageDecoder for cg. ImageSource and ImageFrameCache should change ImageDecoder to PlatformImageDecoder. And no need for this file. ImageSource::ensureDecoderAvailable() will call PlatformImageDecoder::create() which will be implemented by all platforms. > Source/WebCore/platform/graphics/ImageDecoder.h:54 > + virtual String uti() const = 0; No need for this function in the base class. It is only implemented by the CG ImageDecoder and it is called form CG code only. > Source/WebCore/platform/graphics/cg/ImageDecoderCG.h:32 > +class ImageDecoderCG : public ImageDecoder { Should we add final here? class ImageDecoderCG final : public ImageDecoder > Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.cpp:33 > +#include "ImageSource.h" ImageDecoder should not know anything about ImageSource. I do not think there is a need for this header file here. > Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.h:40 > +class ImageDecoderDirect2D : public ImageDecoder { final? > Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.h:52 > String filenameExtension() const; I was surprised that the Windows port built fine while this function does not have the keyword "override" or "final". Then I realized that this file does not compile on any port. So please add final to all the virtual functions in this header file. > Source/WebCore/platform/image-decoders/ScalableImageDecoder.cppSource/WebCore/platform/image-decoders/ImageDecoder.cpp:173 > +bool ScalableImageDecoder::frameIsCompleteAtIndex(size_t index) const I do not think this function can be const easily. This function has to call frameBufferAtIndex() which is not const and it will be very difficult to make this one const. You may end up marking all the functions in all the classes to be const and all the members to be mutable. > Source/WebCore/platform/image-decoders/ScalableImageDecoder.cppSource/WebCore/platform/image-decoders/ImageDecoder.cpp:177 > - ImageFrame* buffer = frameBufferAtIndex(index); > - return buffer && buffer->isComplete(); > + if (m_frameBufferCache.size() <= index) > + return true; > + return m_frameBufferCache[index].isComplete(); I do not understand this change. Why do we return true if index >= m_frameBufferCache.size()? Animated images may change the frameCount as more is received. So if you call frameIsCompleteAtIndex() for frame say 100 while m_frameBufferCache.size() is 90, we should return false. The reason is as more data is received, the frameCount can change to say 120 frames. Once the frameCount changes, m_frameBufferCache will be grown to 120. And in this case the frame will be incomplete till its data is received. Also why did you replace calling frameBufferAtIndex() to directly accessing m_frameBufferCache? frameBufferAtIndex() does lazy decoding and caching for the requested frame. So if you call frameBufferAtIndex(), more frames will be decoded and cached. If you access m_frameBufferCache, you will not get up-to-date decoded frames. Look at BMPImageDecoder::frameBufferAtIndex() and you will see that it calls decode() if the frame is not complete. > Source/WebCore/platform/image-decoders/ScalableImageDecoder.cppSource/WebCore/platform/image-decoders/ImageDecoder.cpp:-199 > - ImageFrame* buffer = frameBufferAtIndex(index); > - if (!buffer || buffer->isInvalid()) Ditto. > Source/WebCore/platform/image-decoders/ScalableImageDecoder.cppSource/WebCore/platform/image-decoders/ImageDecoder.cpp:206 > - const float duration = buffer->duration() / 1000.0f; > + const float duration = m_frameBufferCache[index].duration(); Why do we return milli-seconds instead of seconds? > Source/WebCore/platform/image-decoders/ScalableImageDecoder.hSource/WebCore/platform/image-decoders/ImageDecoder.h:65 > + String filenameExtension() const override = 0; I do not think you need to override a pure virtual function by another pure virtual function. > Source/WebCore/platform/image-decoders/ScalableImageDecoder.hSource/WebCore/platform/image-decoders/ImageDecoder.h:66 > + String uti() const override { return emptyString(); } No need for this function if you remove it from the base class. > Source/WebCore/platform/image-decoders/ScalableImageDecoder.hSource/WebCore/platform/image-decoders/ImageDecoder.h:169 > + size_t bytesDecodedToDetermineProperties() const override { return 0; } Should be final. > Source/WebCore/platform/image-decoders/bmp/BMPImageDecoder.h:52 > + String filenameExtension() const override { return ASCIILiteral("bmp"); } > + void setData(SharedBuffer&, bool allDataReceived) override; > + ImageFrame* frameBufferAtIndex(size_t index) override; > + // CAUTION: setFailed() deletes |m_reader|. Be careful to avoid > + // accessing deleted memory, especially when calling this from inside > + // BMPImageReader! > + bool setFailed() override; I think our coding style states that as long as the class is marked as final, all the virtual functions should be marked final as well. > Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:31 > #ifndef BMPImageReader_h > #define BMPImageReader_h Can we use pragma once? > Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.h:58 > + String filenameExtension() const override { return ASCIILiteral("gif"); } > + void setData(SharedBuffer& data, bool allDataReceived) override; > + bool setSize(const IntSize&) override; > + size_t frameCount() const override; > + RepetitionCount repetitionCount() const override; > + ImageFrame* frameBufferAtIndex(size_t index) override; > + // CAUTION: setFailed() deletes |m_reader|. Be careful to avoid > + // accessing deleted memory, especially when calling this from inside > + // GIFImageReader! > + bool setFailed() override; > + void clearFrameBufferCache(size_t clearBeforeFrame) override; Ditto. override -> final. > Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:59 > + std::optional<IntPoint> hotSpot() const override; Ditto. override -> final.
Said Abou-Hallawa
Comment 39 2017-09-14 15:42:53 PDT
Comment on attachment 320707 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=320707&action=review >> Source/WebCore/platform/graphics/ImageDecoder.cpp:47 >> +#endif > > I do not like using different names for different platforms. I would suggest using the same name for all platforms, e.g. PlatformImageDecoder. In this case ImageDecoderCG.h and ImageDecoderCG.cpp will have the declaration and the implementation of PlatformImageDecoder for cg. ImageSource and ImageFrameCache should change ImageDecoder to PlatformImageDecoder. And no need for this file. ImageSource::ensureDecoderAvailable() will call PlatformImageDecoder::create() which will be implemented by all platforms. I think I am wrong regarding the changes in ImageSource and ImageFrameCache. I think we should keep ImageSource and ImageFrameCache reference ImageDecoder only if we want to allow more subclassing.
Jer Noble
Comment 40 2017-09-14 21:31:39 PDT
(In reply to Said Abou-Hallawa from comment #38) > Comment on attachment 320707 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=320707&action=review > > > Source/WebCore/platform/graphics/ImageDecoder.cpp:47 > > +#if USE(CG) > > + return ImageDecoderCG::create(data, alphaOption, gammaAndColorProfileOption); > > +#elif USE(DIRECT2D) > > + return ImageDecoderDirect2D::create(data, alphaOption, gammaAndColorProfileOption); > > +#else > > + return ScalableImageDecoder::create(data, alphaOption, gammaAndColorProfileOption); > > +#endif > > I do not like using different names for different platforms. I would suggest > using the same name for all platforms, e.g. PlatformImageDecoder. In this > case ImageDecoderCG.h and ImageDecoderCG.cpp will have the declaration and > the implementation of PlatformImageDecoder for cg. ImageSource and > ImageFrameCache should change ImageDecoder to PlatformImageDecoder. And no > need for this file. ImageSource::ensureDecoderAvailable() will call > PlatformImageDecoder::create() which will be implemented by all platforms. That only makes sense if every platform will have a single ImageDecoder, which isn't the case for GTK+, and soon won't be the case for Cocoa. Also, I don't like that the name of the class contained within the file will be different than the name of the file itself. It would also mean that no port could ever use both the CG and the BMP decoders simultaneously (for example). > > Source/WebCore/platform/graphics/ImageDecoder.h:54 > > + virtual String uti() const = 0; > > No need for this function in the base class. It is only implemented by the > CG ImageDecoder and it is called form CG code only. Then it can have a default return value, rather than be a pure virtual. > > Source/WebCore/platform/graphics/cg/ImageDecoderCG.h:32 > > +class ImageDecoderCG : public ImageDecoder { > > Should we add final here? class ImageDecoderCG final : public ImageDecoder Sure. > > Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.cpp:33 > > +#include "ImageSource.h" > > ImageDecoder should not know anything about ImageSource. I do not think > there is a need for this header file here. Ok. > > Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.h:40 > > +class ImageDecoderDirect2D : public ImageDecoder { > > final? Sure. > > Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.h:52 > > String filenameExtension() const; > > I was surprised that the Windows port built fine while this function does > not have the keyword "override" or "final". Then I realized that this file > does not compile on any port. So please add final to all the virtual > functions in this header file. Ok. > > Source/WebCore/platform/image-decoders/ScalableImageDecoder.cppSource/WebCore/platform/image-decoders/ImageDecoder.cpp:173 > > +bool ScalableImageDecoder::frameIsCompleteAtIndex(size_t index) const > > I do not think this function can be const easily. Conceptually, this function should be const. In no way should asking whether a frame is complete mutate the underlying class. If this function is expected to mutate, it should be named "ensureFrameIsCompleteAtIndex(...)". > This function has to call > frameBufferAtIndex() which is not const and it will be very difficult to > make this one const. In this patch, the ScalableImageDecoder no longer calls frameBufferAtIndex(), it asks the m_frameBufferCache whether it the frame at that index is complete. That may be incorrect. But I think we need a better explanation about why asking whether a frame is complete will force the decoder to attempt to decode a frame. > You may end up marking all the functions in all the > classes to be const and all the members to be mutable. > > > Source/WebCore/platform/image-decoders/ScalableImageDecoder.cppSource/WebCore/platform/image-decoders/ImageDecoder.cpp:177 > > - ImageFrame* buffer = frameBufferAtIndex(index); > > - return buffer && buffer->isComplete(); > > + if (m_frameBufferCache.size() <= index) > > + return true; > > + return m_frameBufferCache[index].isComplete(); > > I do not understand this change. Why do we return true if index >= > m_frameBufferCache.size()? Whoops, that should be "return false". > Animated images may change the frameCount as more > is received. So if you call frameIsCompleteAtIndex() for frame say 100 while > m_frameBufferCache.size() is 90, we should return false. The reason is as > more data is received, the frameCount can change to say 120 frames. Once the > frameCount changes, m_frameBufferCache will be grown to 120. And in this > case the frame will be incomplete till its data is received. > > Also why did you replace calling frameBufferAtIndex() to directly accessing > m_frameBufferCache? frameBufferAtIndex() does lazy decoding and caching for > the requested frame. So if you call frameBufferAtIndex(), more frames will > be decoded and cached. If you access m_frameBufferCache, you will not get > up-to-date decoded frames. Look at BMPImageDecoder::frameBufferAtIndex() and > you will see that it calls decode() if the frame is not complete. That is an idiosyncrasy of the decoders in image-decoders/ and is not the behavior of either of the other decoders in the tree. If asking whether a frame is complete is supposed to kick off a background decode, then the function is named incorrectly. > > Source/WebCore/platform/image-decoders/ScalableImageDecoder.cppSource/WebCore/platform/image-decoders/ImageDecoder.cpp:-199 > > - ImageFrame* buffer = frameBufferAtIndex(index); > > - if (!buffer || buffer->isInvalid()) > > Ditto. The answer is the same here. Asking for a frames' duration should not kick off a background decode of the frame. If it should, that should be the API, and not a side-effect of asking. > > Source/WebCore/platform/image-decoders/ScalableImageDecoder.cppSource/WebCore/platform/image-decoders/ImageDecoder.cpp:206 > > - const float duration = buffer->duration() / 1000.0f; > > + const float duration = m_frameBufferCache[index].duration(); > > Why do we return milli-seconds instead of seconds? Why is the ImageFrame have different time units than ImageDecoder? > > Source/WebCore/platform/image-decoders/ScalableImageDecoder.hSource/WebCore/platform/image-decoders/ImageDecoder.h:65 > > + String filenameExtension() const override = 0; > > I do not think you need to override a pure virtual function by another pure > virtual function. True, this is an oversight. Removed. > > Source/WebCore/platform/image-decoders/ScalableImageDecoder.hSource/WebCore/platform/image-decoders/ImageDecoder.h:66 > > + String uti() const override { return emptyString(); } > > No need for this function if you remove it from the base class. Or move the default there. > > Source/WebCore/platform/image-decoders/ScalableImageDecoder.hSource/WebCore/platform/image-decoders/ImageDecoder.h:169 > > + size_t bytesDecodedToDetermineProperties() const override { return 0; } > > Should be final. > > > Source/WebCore/platform/image-decoders/bmp/BMPImageDecoder.h:52 > > + String filenameExtension() const override { return ASCIILiteral("bmp"); } > > + void setData(SharedBuffer&, bool allDataReceived) override; > > + ImageFrame* frameBufferAtIndex(size_t index) override; > > + // CAUTION: setFailed() deletes |m_reader|. Be careful to avoid > > + // accessing deleted memory, especially when calling this from inside > > + // BMPImageReader! > > + bool setFailed() override; > > I think our coding style states that as long as the class is marked as > final, all the virtual functions should be marked final as well. Ok. > > Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:31 > > #ifndef BMPImageReader_h > > #define BMPImageReader_h > > Can we use pragma once? Yes. All these files violate our current coding guidelines in one way or another. > > Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.h:58 > > + String filenameExtension() const override { return ASCIILiteral("gif"); } > > + void setData(SharedBuffer& data, bool allDataReceived) override; > > + bool setSize(const IntSize&) override; > > + size_t frameCount() const override; > > + RepetitionCount repetitionCount() const override; > > + ImageFrame* frameBufferAtIndex(size_t index) override; > > + // CAUTION: setFailed() deletes |m_reader|. Be careful to avoid > > + // accessing deleted memory, especially when calling this from inside > > + // GIFImageReader! > > + bool setFailed() override; > > + void clearFrameBufferCache(size_t clearBeforeFrame) override; > > Ditto. override -> final. Ok. > > Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:59 > > + std::optional<IntPoint> hotSpot() const override; > > Ditto. override -> final. Ok.
Jer Noble
Comment 41 2017-09-14 21:35:18 PDT
(In reply to Jer Noble from comment #40) > (In reply to Said Abou-Hallawa from comment #38) > > Comment on attachment 320707 [details] > > > Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.cpp:33 > > > +#include "ImageSource.h" > > > > ImageDecoder should not know anything about ImageSource. I do not think > > there is a need for this header file here. > > Ok. Wait, if no port uses this file, why is it still in the tree?
Jer Noble
Comment 42 2017-09-14 21:37:19 PDT
(In reply to Jer Noble from comment #41) > Wait, if no port uses this file, why is it still in the tree? Actually, this was in reference to the comment: > > I was surprised that the Windows port built fine while this function does > > not have the keyword "override" or "final". Then I realized that this file > > does not compile on any port. So please add final to all the virtual > > functions in this header file. If no port uses this file, we should just remove it, rather than incur the ongoing maintenance burden of changing it whenever the interface in ImageDecoder changes.
Jer Noble
Comment 43 2017-09-14 22:33:13 PDT
Created attachment 320869 [details] Patch for landing
Build Bot
Comment 44 2017-09-14 22:34:55 PDT
Attachment 320869 [details] did not pass style-queue: ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:84: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:85: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:86: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:88: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:89: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:90: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:92: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:93: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/graphics/ImageDecoder.h:35: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:64: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:65: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:69: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:70: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] Total errors found: 13 in 26 files If any of these errors are false positives, please file a bug against check-webkit-style.
Jer Noble
Comment 45 2017-09-15 09:22:00 PDT
(In reply to Jer Noble from comment #40) > (In reply to Said Abou-Hallawa from comment #38) > > Comment on attachment 320707 [details] > > Patch for landing > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=320707&action=review > > > > > > Also why did you replace calling frameBufferAtIndex() to directly accessing > > m_frameBufferCache? frameBufferAtIndex() does lazy decoding and caching for > > the requested frame. So if you call frameBufferAtIndex(), more frames will > > be decoded and cached. If you access m_frameBufferCache, you will not get > > up-to-date decoded frames. Look at BMPImageDecoder::frameBufferAtIndex() and > > you will see that it calls decode() if the frame is not complete. > > That is an idiosyncrasy of the decoders in image-decoders/ and is not the > behavior of either of the other decoders in the tree. If asking whether a > frame is complete is supposed to kick off a background decode, then the > function is named incorrectly. Looking into this more, I'm pretty sure that your interpretation is incorrect. ImageFrameCache will do it's own synchronous or asynchronous decoding of frames as needed, either in ImageFrameCache::frameAtIndexCacheIfNeeded(...) or ImageFrameCache::requestFrameAsyncDecodingAtIndex(...). If calling ScalableImageDecoder::frameIsCompleteAtIndex(...) will do a lazy, synchronous decode of an image, that would entirely violate the intent of the ImageFrameCache::requestFrameAsyncDecodingAtIndex(...) method, which calls frameIsCompleteAtIndex(...) before enqueueing a decode. Now, this may have been broken this entire time. But as far as I can see, the old behavior was incorrect, and the new behavior is correct.
Said Abou-Hallawa
Comment 46 2017-09-15 09:55:13 PDT
Comment on attachment 320869 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=320869&action=review > Source/WebCore/platform/image-decoders/ScalableImageDecoder.cppSource/WebCore/platform/image-decoders/ImageDecoder.cpp:177 > - ImageFrame* buffer = frameBufferAtIndex(index); > - return buffer && buffer->isComplete(); > + if (m_frameBufferCache.size() <= index) > + return false; > + return m_frameBufferCache[index].isComplete(); Maybe the function name is inappropriate but the expectation is this class does a lazy decoding. This means when it receives new encoded data, it basically stores it but it does nothing with it. Only when it needs to answer a question whose answer may be in the already received data, it started the decoding process. So I think this change is incorrect. I would expect this change will break all the image layout tests on GTK. > Source/WebCore/platform/image-decoders/ScalableImageDecoder.cppSource/WebCore/platform/image-decoders/ImageDecoder.cpp:206 > - const float duration = buffer->duration() / 1000.0f; > + const float duration = m_frameBufferCache[index].duration(); Please notice that ImageFrame is used in two places for two different reasons. -- ScalableImageDecoder builds its own cache of ImageFrames to help answering the frame query metadata questions and to decode the image frames quickly. -- ImageFrameCache builds its own cache of ImageFrames to guarantee the availability of decode frames for the visible image. Notice also: -- We used to have different but similar structures one was named FrameData and which used to live in BitmapImage.h and the other one was called ImageFrame and was living in Source/WebCore/platform/image-decoders/ImageDecoder.h. I managed to merge the two structures after many re-factoring patches. My plan was to eliminate this double caching and have the GTK decoders have access to the ImageFrameCache which is also created by the ImageSource. We have one conceptual difference between the two uses of ImageFrame: -- If the ImageFrame is in ImageFrameCache, the duration is in seconds. -- If the ImageFrame is in ScalableImageDecoder, the duration is in milli seconds. I know it is confusing and I caused much of it, but if I have enough time I will fix this issue. So I think this change is incorrect. If you want to unify the units of the duration in the ImageFrame we can do the following: -- Change the type of ImageFrame::m_duration to be Seconds. -- Ensure all the GTK decoders sets the milliseconds of m_duration, while the GC decoder sets the seconds of m_duration.
Jer Noble
Comment 47 2017-09-15 10:21:11 PDT
(In reply to Said Abou-Hallawa from comment #46) > Comment on attachment 320869 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=320869&action=review > > > Source/WebCore/platform/image-decoders/ScalableImageDecoder.cppSource/WebCore/platform/image-decoders/ImageDecoder.cpp:177 > > - ImageFrame* buffer = frameBufferAtIndex(index); > > - return buffer && buffer->isComplete(); > > + if (m_frameBufferCache.size() <= index) > > + return false; > > + return m_frameBufferCache[index].isComplete(); > > Maybe the function name is inappropriate but the expectation is this class > does a lazy decoding. This means when it receives new encoded data, it > basically stores it but it does nothing with it. Only when it needs to > answer a question whose answer may be in the already received data, it > started the decoding process. > > So I think this change is incorrect. I would expect this change will break > all the image layout tests on GTK. As I pointed out above, this can't be true, as it would mean that async decoding was broken. The only call sites for frameIsCompleteAtIndex(...) are in ImageFrameCache, and it definitely doesn't expect decoding to occur, and it will explicitly start decoding, explicitly, itself. > > Source/WebCore/platform/image-decoders/ScalableImageDecoder.cppSource/WebCore/platform/image-decoders/ImageDecoder.cpp:206 > > - const float duration = buffer->duration() / 1000.0f; > > + const float duration = m_frameBufferCache[index].duration(); > > Please notice that ImageFrame is used in two places for two different > reasons. > > -- ScalableImageDecoder builds its own cache of ImageFrames to help > answering the frame query metadata questions and to decode the image frames > quickly. > -- ImageFrameCache builds its own cache of ImageFrames to guarantee the > availability of decode frames for the visible image. Ok. > Notice also: > > -- We used to have different but similar structures one was named FrameData > and which used to live in BitmapImage.h and the other one was called > ImageFrame and was living in > Source/WebCore/platform/image-decoders/ImageDecoder.h. I managed to merge > the two structures after many re-factoring patches. My plan was to eliminate > this double caching and have the GTK decoders have access to the > ImageFrameCache which is also created by the ImageSource. > > We have one conceptual difference between the two uses of ImageFrame: > -- If the ImageFrame is in ImageFrameCache, the duration is in seconds. > -- If the ImageFrame is in ScalableImageDecoder, the duration is in milli > seconds. > > I know it is confusing and I caused much of it, but if I have enough time I > will fix this issue. > > So I think this change is incorrect. If you want to unify the units of the > duration in the ImageFrame we can do the following: Okay, for now, we can just change the return type of ImageDecoder::frameDurationAtIndex(...) to be Seconds, and each decoder can handle converting to seconds correctly. In a future patch, ImageFrame can be cleaned up. I don't want to get into rewriting the duration logic of each GTK+ decoder. > -- Change the type of ImageFrame::m_duration to be Seconds. > -- Ensure all the GTK decoders sets the milliseconds of m_duration, while > the GC decoder sets the seconds of m_duration.
Jer Noble
Comment 48 2017-09-15 10:29:36 PDT
(In reply to Jer Noble from comment #47) > (In reply to Said Abou-Hallawa from comment #46) > > Comment on attachment 320869 [details] > > Patch for landing > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=320869&action=review > > > > So I think this change is incorrect. I would expect this change will break > > all the image layout tests on GTK. > > As I pointed out above, this can't be true, as it would mean that async > decoding was broken. The only call sites for frameIsCompleteAtIndex(...) > are in ImageFrameCache, and it definitely doesn't expect decoding to occur, > and it will explicitly start decoding, explicitly, itself. Anyway, this should be easy to test; I'll install a GTK+ VM and see.
Jer Noble
Comment 49 2017-09-15 21:55:29 PDT
(In reply to Jer Noble from comment #48) > (In reply to Jer Noble from comment #47) > > (In reply to Said Abou-Hallawa from comment #46) > > > Comment on attachment 320869 [details] > > > Patch for landing > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=320869&action=review > > > > > > So I think this change is incorrect. I would expect this change will break > > > all the image layout tests on GTK. > > > > As I pointed out above, this can't be true, as it would mean that async > > decoding was broken. The only call sites for frameIsCompleteAtIndex(...) > > are in ImageFrameCache, and it definitely doesn't expect decoding to occur, > > and it will explicitly start decoding, explicitly, itself. > > Anyway, this should be easy to test; I'll install a GTK+ VM and see. So I finally got a GTK+ build working in a VM, and Said is indeed correct that this change causes animating images to stop working with non-CG decoders. After digging around, I think I understand why. The underlying problem is the disconnect in what the BitmapImage means by "frameIsCompleteAtIndex()", and what the non-CG decoders mean by "frameIsCompleteAtIndex()". The former wants to know if the decoder has been fed enough data to allow decoding of the frame to succeed. The latter wants to answer whether the frame has been decoded yet. ImageDecoderCG::frameIsCompleteAtIndex() agrees with BitmapImage. So in BitmapImage::internalStartAnimation(), the image doesn't want to start a decode operation at an index for which the decoder doesn't have enough data, so it bails early if frameIsCompleteAtIndex() returns false, and never enqueues a decode operation. Meanwhile, the image-decoders/ decoders will all synchronously decode the image for index when asked frameIsCompleteAtIndex(), making any async decode operations entirely pointless. So, what do we do? As far as I can see, the image-decoders/ decoders need to be entirely rewritten to /parse/ the image data when that data is appended. Note /parse/ and not decode. They need to be able to answer the question of whether decoding can proceed without actually decoding first. Because right now, for non-CG ports, async decoding of GIFs (for example) is entirely broken and is most likely happening entirely on the main thread. But, for the purposes of this patch, that's way to big a re-write for me to take on. I'll back out my change to ScalableImageDecoder::frameIsCompleteAtIndex() (without making it non-const; the bad design in the image-decoders/ decoders shouldn't force design decisions on the other decoders in the tree.)
Jer Noble
Comment 50 2017-09-15 22:41:11 PDT
Created attachment 320995 [details] Patch for landing
Build Bot
Comment 51 2017-09-15 22:43:54 PDT
Attachment 320995 [details] did not pass style-queue: ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:84: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:85: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:86: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:88: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:89: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:90: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:92: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:93: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/graphics/ImageDecoder.h:35: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:64: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:65: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:69: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:70: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] Total errors found: 13 in 26 files If any of these errors are false positives, please file a bug against check-webkit-style.
Michael Catanzaro
Comment 52 2017-09-16 07:55:00 PDT
(In reply to Jer Noble from comment #49) > Because right now, for > non-CG ports, async decoding of GIFs (for example) is entirely broken and is > most likely happening entirely on the main thread. GIFs have been totally broken for over a year now, see bug #176089. (I only reported it recently, but they broke sometime last summer or fall IIRC.) Yes, it is pretty embarrassing that we have not fixed it in all that time.
Jer Noble
Comment 53 2017-09-18 08:36:44 PDT
(In reply to Michael Catanzaro from comment #52) > (In reply to Jer Noble from comment #49) > > Because right now, for > > non-CG ports, async decoding of GIFs (for example) is entirely broken and is > > most likely happening entirely on the main thread. > > GIFs have been totally broken for over a year now, see bug #176089. (I only > reported it recently, but they broke sometime last summer or fall IIRC.) > > Yes, it is pretty embarrassing that we have not fixed it in all that time. Thanks for the extra context Michael. I'll update the FIXMEs in this patch to bug #176089.
Jer Noble
Comment 54 2017-09-18 08:41:23 PDT
Created attachment 321093 [details] Patch for landing
Build Bot
Comment 55 2017-09-18 08:43:48 PDT
Attachment 321093 [details] did not pass style-queue: ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:84: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:85: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:86: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:88: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:89: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:90: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:92: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:93: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/graphics/ImageDecoder.h:35: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:64: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:65: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:69: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:70: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] Total errors found: 13 in 26 files If any of these errors are false positives, please file a bug against check-webkit-style.
WebKit Commit Bot
Comment 56 2017-09-18 09:25:03 PDT
Comment on attachment 321093 [details] Patch for landing Clearing flags on attachment: 321093 Committed r222151: <http://trac.webkit.org/changeset/222151>
WebKit Commit Bot
Comment 57 2017-09-18 09:25:06 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 58 2017-09-27 12:52:37 PDT
<rdar://problem/34694191>
Note You need to log in before you can comment on or make changes to this bug.