[webkit-reviews] review denied: [Bug 58851] add incremental decoding to WebP decoder : [Attachment 90121] Patch

[bugzilla-daemon at webkit.org]

Peter Kasting <pkasting at google.com> has denied	review:
Bug 58851: add incremental decoding to WebP decoder
https://bugs.webkit.org/show_bug.cgi?id=58851

Attachment 90121: Patch
https://bugs.webkit.org/attachment.cgi?id=90121&action=review

------- Additional Comments from Peter Kasting <pkasting at google.com>
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.