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

[bugzilla-daemon at webkit.org]

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


Peter Kasting <pkasting at google.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #90121|review+, commit-queue?      |review-
               Flag|                            |




--- Comment #8 from Peter Kasting <pkasting at google.com>  2011-04-18 17:37:09 PST ---
(From update of attachment 90121)
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

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

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

> Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:98
> +    bool dataComplete = isAllDataReceived();

Nit: allDataReceived would be a more parallel-structured name.

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

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

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

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

> 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;

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

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