Bug 178080 – [GTK][WPE] Fix review comments on WEBPImageDecoder

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Miguel Gomez]

This bug is to address review comments made by Said to the WEBPImageDecoder implementation, in https://bugs.webkit.org/show_bug.cgi?id=113124#c24

[Miguel Gomez]

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

[Miguel Gomez]

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

[Said Abou-Hallawa]

(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

[Miguel Gomez]

> 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 for the explanation Said!!
Ok, for this patch I'm going to use only Invalid, Partial and Complete with the meanings you mention. If we decide to use Decoding in the end, I'll create a new bug to change that to all the decoders and not only the WebP one.

[Miguel Gomez]

Created attachment 324005 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 324005 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=324005&action=review

> Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:38
> +    return WebPDemuxGetFrame(demuxer, index + 1, webpFrame);

Maybe it is worthy adding a comment that this wrapper was added because WebPDemuxGetFrame is 1-based function.

> Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:123
>          }

I think this loop is a little bit confusing. The problem is you have two different things you want to check in the same loop. I would suggest splitting the complete test from the alpha and the disposal method test:

size_t firstIncompleteFrame = frameIndex;
for (; firstIncompleteFrame; --firstIncompleteFrame) {
    if (m_frameBufferCache[firstIncompleteFrame - 1].isComplete())
        break;
}

for (size_t firstIndependentFrame = frameIndex; firstIndependentFrame > firstIncompleteFrame ; --firstIndependentFrame) {
    WebPIterator webpFrame;
    if (!webpFrameAtIndex(demuxer, firstIndependentFrame, &webpFrame))
        continue;

    IntRect frameRect(webpFrame.x_offset, webpFrame.y_offset, webpFrame.width, webpFrame.height);
    if (!frameRect.contains({ { }, size() }))
        continue;

    if (!webpFrame.has_alpha)
        return firstIndependentFrame;

    if (firstIndependentFrame < frameIndex && m_frameBufferCache[firstIndependentFrame].disposalMethod() == ImageFrame::DisposalMethod::RestoreToBackground)
        return firstIndependentFrame + 1;  
}

return firstIncompleteFrame;

[Miguel Gomez]

(In reply to Said Abou-Hallawa from comment #6)
> Comment on attachment 324005 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=324005&action=review
> 
> > Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:38
> > +    return WebPDemuxGetFrame(demuxer, index + 1, webpFrame);
> 
> Maybe it is worthy adding a comment that this wrapper was added because
> WebPDemuxGetFrame is 1-based function.

Sure!

> > Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:123
> >          }
> 
> I think this loop is a little bit confusing. The problem is you have two
> different things you want to check in the same loop. I would suggest
> splitting the complete test from the alpha and the disposal method test:
> 
> size_t firstIncompleteFrame = frameIndex;
> for (; firstIncompleteFrame; --firstIncompleteFrame) {
>     if (m_frameBufferCache[firstIncompleteFrame - 1].isComplete())
>         break;
> }
> 
> for (size_t firstIndependentFrame = frameIndex; firstIndependentFrame >
> firstIncompleteFrame ; --firstIndependentFrame) {
>     WebPIterator webpFrame;
>     if (!webpFrameAtIndex(demuxer, firstIndependentFrame, &webpFrame))
>         continue;
> 
>     IntRect frameRect(webpFrame.x_offset, webpFrame.y_offset,
> webpFrame.width, webpFrame.height);
>     if (!frameRect.contains({ { }, size() }))
>         continue;
> 
>     if (!webpFrame.has_alpha)
>         return firstIndependentFrame;
> 
>     if (firstIndependentFrame < frameIndex &&
> m_frameBufferCache[firstIndependentFrame].disposalMethod() ==
> ImageFrame::DisposalMethod::RestoreToBackground)
>         return firstIndependentFrame + 1;  
> }
> 
> return firstIncompleteFrame;

Yes, the loop is a bit complex cause I'm trying to find the first frame that fulfills one of the three conditions. But your approach looks good. I'll change it.
Thanks for the feedback Said! :)

[Miguel Gomez]

Created attachment 324378 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 324378 [details]
Patch

Clearing flags on attachment: 324378

Committed r223754: <https://trac.webkit.org/changeset/223754>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.