Bug 90758 – Refactoring metadata of BitmapImage.

• Status: RESOLVED
• Resolution: INVALID
• Component: Images

[Dongseong Hwang]

There is no precise definition of FrameData's metadata. So I assume the metadata as a combination of m_orientation, m_duration, and m_hasAlpha. This definition works fine because currently there is no method that uses m_haveMetadata on its purpose.

This patch contains two changes:
    
1. Remove FrameData::m_haveMetadata for readability.
Only BitmapImage::dataChanged uses m_haveMetadata in order to check whether the frame is cleared or not, and it is enough to check whether FrameData::m_frame is 0.
    
2. Don't decode again if metadata exist.
If the indexed frame was once decoded completely and cleared by destroyDecodedData(), We don't need to decode again in order to know metadata because destroyDecodedData() does not reset the metadata. See destroyDecodedData().

[Dongseong Hwang]

Created attachment 151202 [details]
Patch

[Dongseong Hwang]

Created attachment 151426 [details]
Patch

[Dongseong Hwang]

(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.

[Kwang Yul Seo]

(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?

[Dongseong Hwang]

Created attachment 151620 [details]
Patch

[Dongseong Hwang]

(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.

[Zoltan Horvath]

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?

[Zoltan Horvath]

(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.

[Kwang Yul Seo]

(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.

[Zoltan Horvath]

(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?

[Kwang Yul Seo]

(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.

[Kwang Yul Seo]

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!

[Dongseong Hwang]

Created attachment 152894 [details]
Patch

[Dongseong Hwang]

(In reply to comment #13)
> Created an attachment (id=152894) [details]
> Patch

Rebased to the latest revision.

[Dongseong Hwang]

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