This change makes the API of ImageSource support asynchronous image decoding. This is a preparation for asynchronous image decoding. After ImageSource compelets image decoding, ImageSource sends NativeImagePtr to BitmapImage via ImageSourceObserver::didDecodeFrameAtIndex(). This patch renames createFrameAtIndex to requestFrameAtIndex, to imply this method can be asynchrous. If ImageSource has ImageSourceObserver, this method sends the result via didDecodeFrameAtIndex() either synchronously or asynchronously. Otherwise, this method returns the result synchronously. e.g. GraphicsContext3D calls requestFrameAtIndex without ImageSourceObserver.
Created attachment 157190 [details] Patch
High level comments first. I wonder if it is better to make ImageSource a full asynchronous interface. Which is: class ImageSource { public: ImageSource(ImageSourceObserver* observer, ...); void requestFrameAtIndex(size_t index, bool useAsynchronousDecodingIfPossible = false); }; class ImageSourceObserver { public: virtual void didDecodeFrameAtIndex(size_t, NativeImagePtr); }; And then create a wrapper of SynchronousImageSource: class SynchronousImageSource : public ImageSourceObserver { public: SynchronousImageSource(...) : m_imageSource(this, ...) { } virtual void didDecodeFrameAtIndex(size_t index, NativaImagePtr frame) { m_frames[index] = frame; } NativeImagePtr createFrameAtIndex(size_t index) { m_imageSource.requestFrameAtIndex(index); return m_frames[index]; } private: Vector<NativeImagePtr> m_frames; ImageSource m_imageSource; }; This way we make ImageSource fully asynchronous and separate a synchronous interface. We'll also need to move image decoding in frameDurationAtIndex() and orientationAtIndex() to ImageDecoder such that individual decoder can decide how to implement it. The latter can be done separately though.
(In reply to comment #2) > This way we make ImageSource fully asynchronous and separate a synchronous interface. We'll also need to move image decoding in frameDurationAtIndex() and orientationAtIndex() to ImageDecoder such that individual decoder can decide how to implement it. The latter can be done separately though. That's good idea. GraphicsContext3D and others will use SynchronousImageSource instead of ImageSource. I'll update it.
Created attachment 157406 [details] Patch
(In reply to comment #4) > Created an attachment (id=157406) [details] > Patch This patch introduced SynchronousImageSource, which Alpha suggested. SynchronousImageSource makes us avoid making a mistake. So, the changelog was also changed. "This change makes the API of ImageSource support asynchronous image decoding. This is a preparation for asynchronous image decoding. After ImageSource completes image decoding, ImageSource sends NativeImagePtr to BitmapImage via ImageSourceObserver::didDecodeFrameAtIndex(). This patch renames createFrameAtIndex to requestFrameAtIndex, to imply this method can be asynchronous. This patch introduced SynchronousImageSource, which decodes image synchronously. The main reason to add this class is to prevent clients from misusing ImageSource. ImageSource will be able to perform sync and async image decoding, so it will be hard to use ImageSource correctly. So, after this patch, only BitmapImage and SynchronousImageSource can create and use ImageSource, and other clients which need to use ImageSource must use SynchronousImageSource. Currently, GraphicsContext3D and WebImageSkia use SynchronousImageSource."
Comment on attachment 157406 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157406&action=review > Source/WebCore/ChangeLog:24 > + No new tests - existing image tests cover these cases. I believe image layout tests will verify the correctness of this change, but it's best to list the tests here, e.g. compositing/images/content-image-change.html fast/images/jpeg-with-color-profile.html > Source/WebCore/platform/graphics/BitmapImage.cpp:-132 > - ASSERT(m_decodedSize == 0 || numFrames > 1); I think we don't need to change this line in this patch. > Source/WebCore/platform/graphics/ImageSource.cpp:147 > + ASSERT(m_observer); I don't think this is necessary since constructor always have this assigned. If this is 0 it will crash in the following line anyway. > Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:307 > + ASSERT(m_observer); I don't think ASSERT is needed here.
Comment on attachment 157406 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157406&action=review > Source/WebCore/platform/graphics/BitmapImage.h:185 > + virtual void didDecodeFrameAtIndex(size_t, NativeImagePtr); please add OVERRIDE annotation when you're overriding a virtual. It goes after the signature but before the ; > Source/WebCore/platform/graphics/skia/GraphicsContext3DSkia.cpp:60 > + SynchronousImageSource decoder(ImageSource::AlphaNotPremultiplied, > ignoreGammaAndColorProfile ? ImageSource::GammaAndColorProfileIgnored : ImageSource::GammaAndColorProfileApplied); please make the parameter line up with opening ( as it did before the type name change
I was just checking for style, I don't have opinions about the code itself.
Dongsung: I have looked at the code and looks good to me. James helped to check the coding style. Please fix those nits and I think it'll be ready for review. smfr will probably be the best person to review this. Please add him to cc list after you upload a new patch. Thanks.
Created attachment 157681 [details] Patch
(In reply to comment #6) Thanks for review > I believe image layout tests will verify the correctness of this change, but it's best to list the tests here, e.g. Ok. I add the paragraph in the changelog Test: compositing/images/* fast/images/* fast/dom/HTMLImageElement/* http/tests/images/* css3/images/* > > - ASSERT(m_decodedSize == 0 || numFrames > 1); > I think we don't need to change this line in this patch. Ok. > > + ASSERT(m_observer); > I don't think this is necessary since constructor always have this assigned. If this is 0 it will crash in the following line anyway. > I don't think ASSERT is needed here. Ditto.
(In reply to comment #7) Thanks for review. > please add OVERRIDE annotation when you're overriding a virtual. It goes after the signature but before the ; Fixed. > please make the parameter line up with opening ( as it did before the type name change Ditto.
(In reply to comment #9) > I have looked at the code and looks good to me. James helped to check the coding style. Please fix those nits and I think it'll be ready for review. smfr will probably be the best person to review this. Please add him to cc list after you upload a new patch. Thanks. Thanks for your instruction. I added him. :)
(In reply to comment #10) > Created an attachment (id=157681) [details] > Patch The third patch is changed a lot. I described the details in the change log. ImageSource::frameHasAlphaAtIndex can trigger image decoding again. BitmapImage always queries alpha without triggering image decoding, while SychronousImageSource always triggers image decoding to get alpha. Clients can not use ImageSource directly. Clients can decode an image with BitmapImage or SychronousImageSource. We could change three methods of ImageSource this way after adding SychronousImageSource.
Comment on attachment 157681 [details] Patch Attachment 157681 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13478050
(In reply to comment #15) > (From update of attachment 157681 [details]) > Attachment 157681 [details] did not pass mac-ews (mac): > Output: http://queues.webkit.org/results/13478050 Oops, mac is broken.
(In reply to comment #16) > (In reply to comment #15) > > (From update of attachment 157681 [details] [details]) > > Attachment 157681 [details] [details] did not pass mac-ews (mac): > > Output: http://queues.webkit.org/results/13478050 > > Oops, mac is broken. Changing the mac build system by hand is too hard. I'll fix it.
Created attachment 157706 [details] Patch
(In reply to comment #18) > Created an attachment (id=157706) [details] > Patch Make mac build well.
Comment on attachment 157706 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157706&action=review > Source/WebCore/platform/graphics/BitmapImage.cpp:-133 > - nit: Don't remove this empty line. > Source/WebCore/platform/graphics/BitmapImage.cpp:150 > + m_frames[index].m_isComplete = m_source.frameIsCompleteAtIndex(index, ImageSource::NonRequestDecoding); I can understand why you add this flag here: you want to prevent this call from doing a full image decoding internally. I suggest we don't worry about this yet. It becomes confusing with details at this level because we want to just complete asynchronous ImageSource for this change. > Source/WebCore/platform/graphics/ImageSource.cpp:178 > ImageFrame* buffer = m_decoder->frameBufferAtIndex(index); I understand this function causes a full decode when didDecodeFrameAtIndex() is called and you want to avoid that with RequestDecodingMode. I think a better way to do this is to delegate the call to ImageDecoder. And then only GIFImageDecoder will do a full frame decode, which is not in the scope of parallel image decoding and keep us out of trouble. I suggest we don't change this implementation, we should narrow our focus to just making ImageSource asynchronous. > Source/WebCore/platform/graphics/ImageSource.cpp:203 > + Same here we should let ImageDecoder answer this call. > Source/WebCore/platform/graphics/ImageSource.cpp:218 > + if (mode == RequestDecoding) Same here we don't need to do this. We should have a separate change to move decoding in this call into ImageDecoder. > Source/WebCore/platform/graphics/ImageSource.h:151 > + float frameDurationAtIndex(size_t, RequestDecodingMode = RequestDecoding); I don't think we need this enum. See below in SynchronousImageSource. > Source/WebCore/platform/graphics/SynchronousImageSource.cpp:106 > + return m_imageSource.frameHasAlphaAtIndex(index); We can write this function as: m_imageSource.requestFrameAtIndex(index); return m_imageSource.frameHasAlphaAtIndex(index); If we're worried about future changes we can write a unit test to make sure behavior is correct. Also give a comment to state calling requestFrameAtIndex() is needed. > Source/WebCore/platform/graphics/SynchronousImageSource.cpp:111 > + return m_imageSource.frameIsCompleteAtIndex(index); Same here: m_imageSource.requestFrameAtIndex(index); return m_imageSource.frameIsCompleteAtIndex(index);
Please limit the scope of this change to just refactoring ImageSource to be asynchronous, i.e. limit the scope to just ImageSource::requestFrameAtIndex(), ImageSourceObserver, SynchronousImageSource and switch usage of ImageSource to SynchronousImageSource. We should do the refactoring of ImageSource, ImageDecoder::decodeAtIndex in a separate patch and discussed in a different bug. Having it included in this change is confusing for other reviewers. I think your second patch with a few nits would be good for review.
Comment on attachment 157706 [details] Patch Attachment 157706 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13463811
Created attachment 158682 [details] Patch
(In reply to comment #21) > Please limit the scope of this change to just refactoring ImageSource to be asynchronous, i.e. limit the scope to just ImageSource::requestFrameAtIndex(), ImageSourceObserver, SynchronousImageSource and switch usage of ImageSource to SynchronousImageSource. > > We should do the refactoring of ImageSource, ImageDecoder::decodeAtIndex in a separate patch and discussed in a different bug. Having it included in this change is confusing for other reviewers. I think your second patch with a few nits would be good for review. Thank you for your review. I agree with you. I'll file another bug about ImageDecoder.
Created attachment 158695 [details] Patch
(In reply to comment #20) > m_imageSource.requestFrameAtIndex(index); > return m_imageSource.frameHasAlphaAtIndex(index); We need the code above because ImageSource::frameHasAlphaAtIndex does not decode the given frame. > > m_imageSource.requestFrameAtIndex(index); > return m_imageSource.frameIsCompleteAtIndex(index); However, the code above is not yet needed because ImageSource::frameIsCompleteAtIndex decodes the given frame. A follow-up patch will change frameIsCompleteAtIndex() not to decode anymore, so the code above will be needed. I'll file another bug.
Comment on attachment 158695 [details] Patch Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.