Bug 178080 - [GTK][WPE] Fix review comments on WEBPImageDecoder
Summary: [GTK][WPE] Fix review comments on WEBPImageDecoder
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Miguel Gomez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-10-09 05:20 PDT by Miguel Gomez
Modified: 2017-10-20 04:08 PDT (History)
3 users (show)

See Also:


Attachments
Patch (10.64 KB, patch)
2017-10-17 02:38 PDT, Miguel Gomez
no flags Details | Formatted Diff | Diff
Patch (11.02 KB, patch)
2017-10-20 02:08 PDT, Miguel Gomez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Miguel Gomez 2017-10-09 05:20:57 PDT
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
Comment 1 Miguel Gomez 2017-10-09 05:27:51 PDT
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/.
Comment 2 Miguel Gomez 2017-10-16 04:59:23 PDT
(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!
Comment 3 Said Abou-Hallawa 2017-10-16 11:34:24 PDT
(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
Comment 4 Miguel Gomez 2017-10-17 01:30:40 PDT
> 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.
Comment 5 Miguel Gomez 2017-10-17 02:38:39 PDT
Created attachment 324005 [details]
Patch
Comment 6 Said Abou-Hallawa 2017-10-19 12:08:23 PDT
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;
Comment 7 Miguel Gomez 2017-10-20 01:30:19 PDT
(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! :)
Comment 8 Miguel Gomez 2017-10-20 02:08:04 PDT
Created attachment 324378 [details]
Patch
Comment 9 WebKit Commit Bot 2017-10-20 04:08:15 PDT
Comment on attachment 324378 [details]
Patch

Clearing flags on attachment: 324378

Committed r223754: <https://trac.webkit.org/changeset/223754>
Comment 10 WebKit Commit Bot 2017-10-20 04:08:17 PDT
All reviewed patches have been landed.  Closing bug.