[Webkit-unassigned] [Bug 58851] add incremental decoding to WebP decoder

[bugzilla-daemon at webkit.org]

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





--- Comment #10 from Pascal Massimino <pascal.massimino at gmail.com>  2011-04-18 17:50:57 PST ---
(In reply to comment #8)
> (From update of attachment 90121 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90121&action=review
> 
> r- for the concern about byte ordering; the rest are all nits.
> 
> > Source/WebCore/ChangeLog:5
> > +        add incremental decoding to WebP decoder
> 
> Nit: add -> Add

done

> 
> > Source/WebCore/ChangeLog:12
> > +        No new tests.
> > +        Testing buffered connection is flaky: one need a very big picture
> > +        to test with and an emulated clogged-connection. And even then,
> > +        there's no guaranty the test will eventually chose the
> > +        incremental-decoding code path.
> 
> Nit: Lots of grammar/mechanics errors.  How about this simpler version:
> 
> No new tests, as it's not possible for the layout test framework to force the decoders to decode incrementally.

done

> 
> > Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:86
> > +    const uint8_t* dataBytes = reinterpret_cast<const uint8_t*>(m_data->data());
> 
> Nit: Move this below the conditional, into the next section.

done

> 
> > Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:98
> > +    bool dataComplete = isAllDataReceived();
> 
> Nit: allDataReceived would be a more parallel-structured name.

done

> 
> > Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:116
> > +        if (!dataComplete) {
> > +            m_decoder = WebPINewRGB(MODE_RGB, m_rgbOutput.data(), m_rgbOutput.size(), stride);
> > +            if (!m_decoder)
> > +                return setFailed();
> > +        }
> 
> Nit: I think it would be slightly clearer if you moved this block into the else block below that actually uses the decoder, and changed the conditional to be "if (!m_decoder)".  Then all access to the decoder occurs within a single outer conditional arm.

indeed! moved.

> 
> > Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:117
> > +        m_lastVisibleRow = 0;
> 
> Nit: We don't need to zero this here, it was zeroed in the constructor.

done

> 
> > Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:119
> > +    int visiblePart = 0; // Last completed row.
> 
> Nit: newLastVisibleRow would be a better name, and would need no comment.

done

> 
> > Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:137
> > +            buffer.setRGBA(x, y, src[bytesPerPixel * x + 0], src[bytesPerPixel * x + 1], src[bytesPerPixel * x + 2], 0xff);
> 
> I'm confused.  This reverses the byte order for both incremental and non-incremental decoding.  I don't see any flag changes in the non-incremental case or other evidence that this is the right thing to do there (I have no idea on the incremental case as it's new).  Why is this happening?

oh! that's hidden in the noise: we were previously calling DecodeBGR() and passing the values in reversed order. It's more logical to now call DecodeRGB() and pass r,g and b.

> 
> > Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:144
> > +    if (m_lastVisibleRow == height) {
> > +         buffer.setStatus(ImageFrame::FrameComplete);
> > +         return true;
> > +    } else
> > +         return false; // I/O suspension.
> 
> Nit: Shorter:
> 
>     if (m_lastVisibleRow == height)
>         buffer.setStatus(ImageFrame::FrameComplete);
>     return m_lastVisibleRow == height;

done

> 
> > Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.h:52
> > +    // These fields store the decoder's state during incremental decoding, when only partial data is available.
> 
> Nit: Blank line before this.  Also, this sentence is technically false, since only the first of the three fields is incremental-only.  I'd instead do:
> 
>     WebPIDecoder* m_decoder; // This is only used when we want to decode() but not all data is available yet.
> 
> ...and leave the rest uncommented.

done


thanks! patch uploaded.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.