Bug 90758

Created attachment 151202 [details] Patch

Created attachment 151426 [details] Patch

(In reply to comment #2) > Created an attachment (id=151426) [details] > Patch Add "m_frames[0].m_isComplete = true" in BitmapImage(NativeImagePtr, ImageObserver* = 0). BitmapImage(NativeImagePtr, ImageObserver* = 0) is implemented by each platform, and most of them does not clarify "m_frames[0].m_isComplete = true" except for VG port. BitmapImage(NativeImagePtr, ImageObserver* = 0) means to create new image with a complete platform image, so "m_frames[0].m_isComplete = true" is proper.

(In reply to comment #3) > (In reply to comment #2) > > Created an attachment (id=151426) [details] [details] > > Patch > > Add "m_frames[0].m_isComplete = true" in BitmapImage(NativeImagePtr, ImageObserver* = 0). > BitmapImage(NativeImagePtr, ImageObserver* = 0) is implemented by each platform, and most of them does not clarify "m_frames[0].m_isComplete = true" except for VG port. > BitmapImage(NativeImagePtr, ImageObserver* = 0) means to create new image with a complete platform image, so "m_frames[0].m_isComplete = true" is proper. I am not sure if this change should belong to this patch because m_isComplete is not a metadata by our definition. We also need to explain more about the consequences of this change. e.g., How does this change impact parallel image decoders?

Created attachment 151620 [details] Patch

(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > Created an attachment (id=151426) [details] [details] [details] > > > Patch > > > > Add "m_frames[0].m_isComplete = true" in BitmapImage(NativeImagePtr, ImageObserver* = 0). > > BitmapImage(NativeImagePtr, ImageObserver* = 0) is implemented by each platform, and most of them does not clarify "m_frames[0].m_isComplete = true" except for VG port. > > BitmapImage(NativeImagePtr, ImageObserver* = 0) means to create new image with a complete platform image, so "m_frames[0].m_isComplete = true" is proper. > > I am not sure if this change should belong to this patch because m_isComplete is not a metadata by our definition. We also need to explain more about the consequences of this change. e.g., How does this change impact parallel image decoders? Ok. I explain more detail about m_isComplete in ChangeLog. 3. Set m_isComplete to true in BitmapImage(NativeImagePtr, ImageObserver*). This constructor creates a BitmapImage from a platform specific NativeImagePtr. It sets m_frame of the first frame to the given NativeImagePtr. The BitmapImage created does not need decoding because decoding is already done. So, it is proper to set m_isComplete to true. Currently, only OpenVG port sets m_isComplete to true. The third change is needed for parallel image decoders. Currently, BitmapImage checks only m_frame in order to determine whether decoding is needed or not. But after parallel image decoders are landed, BitmapImage also needs to check m_isComplete. If m_isComplete is false, BitmapImage will try to decode again, and it can't actually decode because the BitmapImage created from a NativeImagePtr does not have encoded data.

Comment on attachment 151620 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151620&action=review > Source/WebCore/platform/graphics/BitmapImage.h:108 > NativeImagePtr m_frame; Doesn't make sense to use the a ImageFrameData object here as a member instead?

(In reply to comment #7) > (From update of attachment 151620 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=151620&action=review > > > Source/WebCore/platform/graphics/BitmapImage.h:108 > > NativeImagePtr m_frame; > > Doesn't make sense to use the a ImageFrameData object here as a member instead? I mean ImageFrameData from the patch of the bug #90443.

(In reply to comment #7) > (From update of attachment 151620 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=151620&action=review > > > Source/WebCore/platform/graphics/BitmapImage.h:108 > > NativeImagePtr m_frame; > > Doesn't make sense to use the a ImageFrameData object here as a member instead? Yes, we can use ImageFrameData as a member instead. But then member references become too verbose. For example, m_frames[i].m_isComplete becomes m_frames[i].m_imageFrameData.m_isComplete. Also because these two classes are used for different purposes, we didn't want to embed ImageFrameData in Bitmap's FrameData. So we chose to duplicate members here.

(In reply to comment #9) > Yes, we can use ImageFrameData as a member instead. But then member references become too verbose. For example, m_frames[i].m_isComplete becomes m_frames[i].m_imageFrameData.m_isComplete. We can avoid the verbosity by adding member access functions to the class, let's listen other's opinions. > Also because these two classes are used for different purposes, we didn't want to embed ImageFrameData in Bitmap's FrameData. > > So we chose to duplicate members here. Oh, I see. Hmm we have 3 slightly different instances of this member group: FrameData, ImageFrameData, ParallelFrameData. Did you consider to merge them somehow? Or would it increase the complexity too much and mix things that attend to different purposes?

(In reply to comment #10) > (In reply to comment #9) > > Yes, we can use ImageFrameData as a member instead. But then member references become too verbose. For example, m_frames[i].m_isComplete becomes m_frames[i].m_imageFrameData.m_isComplete. > > We can avoid the verbosity by adding member access functions to the class, let's listen other's opinions. If we can remove the verbosity by adding member functions, then I think it is okay to use ImageFrameData as a member in FrameData. > > > Also because these two classes are used for different purposes, we didn't want to embed ImageFrameData in Bitmap's FrameData. > > > > So we chose to duplicate members here. > > Oh, I see. Hmm we have 3 slightly different instances of this member group: FrameData, ImageFrameData, ParallelFrameData. Did you consider to merge them somehow? Or would it increase the complexity too much and mix things that attend to different purposes? ImageFrameData is used to pass the decoded image between ImageSoure and its clients while FrameData is used by BitmapImage to cache the decoded image. We originally used one class for both FrameData and ImageFrameData, but it increased the complexity too much because the life cycles of these two classes were different. So we chose to use two separate classes. However, it is okay to use ImageFrameData as a member in FrameData because FrameData is usually initialized by FrameData::setFromImageFrameData(const ImageFrameData&) and it is natural to keep a copy of ImageFrameData in FrameData. ParallelFrameData is used to pass a segment of decoded data from the decoder thread to the main thread. Because it is used internally only in ParallelImageDecoder and its members are different (the decoded segment instead of a native image pointer), we think it is better to leave it as is.

BTW, I admit that the class names such FrameData, ImageFrameData, ParallelFrameData and ImageFrame are quite confusing because they serve for slightly different purposes with almost the same name. We need to come up with better names here!

Created attachment 152894 [details] Patch

(In reply to comment #13) > Created an attachment (id=152894) [details] > Patch Rebased to the latest revision.

We are rewriting the preparation patches for async image decoding, so this bug is of no use.