[Webkit-unassigned] [Bug 178080] [GTK][WPE] Fix review comments on WEBPImageDecoder

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=178080

--- Comment #3 from Said Abou-Hallawa <sabouhallawa at apple.com> ---
(In reply to Miguel Gomez from comment #2)
> (In reply to Miguel Gomez from comment #1)
> > Said, I was fixing your comments and I found something weird when replacing
> > the usage of DecodingStatus::Partial. My idea was to set the buffer status
> > to DecodingStatus::Decoding at the end of initFrameBuffer(), and then, set
> > it to DecodingStatus::Complete if everything goes fine, or set it to
> > DecodingStatus::Partial if there isn't enough data to decode the frame, but
> > I found this inside ImageFrame.cpp:
> > 
> > void ImageFrame::setDecodingStatus(DecodingStatus decodingStatus)
> > {
> >     ASSERT(decodingStatus != DecodingStatus::Decoding);
> >     m_decodingStatus = decodingStatus;
> > }
> > 
> > DecodingStatus ImageFrame::decodingStatus() const
> > {
> >     ASSERT(m_decodingStatus != DecodingStatus::Decoding);
> >     return m_decodingStatus;
> > }
> > 
> > The asserts seem to imply that DecodingStatus::Decoding should not be used
> > for some reason, but I don't get why. Is there any reason for this? Or are
> > those asserts legacy code that could be removed? That code seems to be only
> > used for the image decoders in Source/WebCore/platform/image-decoders/.
> 
> Said, please, could you give a look to this? Thanks in advance!

Miguel, sorry for the delay. 

The ImageFrame which is cached by the ImageFrameCache should have its m_decodingStatus be one of the following values: { Invalid, Partial, Complete } depending on the how much data was decoded inside this ImageFrame::m_nativeImage.
-- If nothing is decoded, then m_decodingStatus should be Invalid.
-- If there is not enough data to decode the frame, then m_decodingStatus is Partial.
-- If all the data is available to decode the frame then m_decodingStatus is Complete.

The DecodingStatus::Decoding is used outside the ImageFrame and it is always used for the status of the current frame: BitmapImage::m_currentFrameDecodingStatus whose value is one of the following { Invalid, Partial, Complete, Decoding }

The reason for this distinction is we cache the ImageFrame in the ImageFrameCache::m_frames only when the decoding finishes. So there is no case where the ImageFrameCache caches an ImageFrame with ImageFrame:: m_decodingStatus equals to DecodingStatus::Decoding.

If you want to still use the DecodingStatus::Decoding when the GTK decoder creates its internal ImageFrames, you can can change the above assertions to be like this:

ASSERT_IMPLIES(!hasBackingStore(), decodingStatus != DecodingStatus::Decoding);

And you can expose ImageFrame::hasBackingStore() for all platforms where it is going to return false for CG platforms always:

#if !USE(CG)
    ImageBackingStore* backingStore() const { return m_backingStore ? m_backingStore.get() : nullptr; }
    bool hasBackingStore() const { return backingStore(); }
else
    bool hasBackingStore() const { return false; }
#endif


Thanks,
Said

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20171016/080ef6e3/attachment.html>