Bug 58851 – Add incremental decoding to WebP decoder

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Pascal Massimino]

add incremental decoding to WebP decoder

[Pascal Massimino]

Created attachment 90115 [details]
Patch

[Adam Barth]

Comment on attachment 90115 [details]
Patch

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

Looks great.  Some minor comments below.

> Source/WebCore/ChangeLog:8
> +        No new tests needed.

Normally we like a little more song and dance here about now incremental decoding is good for the soul and difficult to test.

> Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:87
>      const uint8_t* dataBytes =
>          reinterpret_cast<const uint8_t*>(m_data->data());

This should be on one line.  There isn't an 80 col limit.

> Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:100
> +    const bool dataComplete = isAllDataReceived();
> +    const int stride = width * bytesPerPixel;

WebKit isn't super agressive about using the const qualifier, but these are probably fine.

> Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:109
> +        buffer.setHasAlpha(false);

I'd add a FIXME about changing this once WebP supports alpha.

> Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:141
> +         buffer.setStatus(ImageFrame::FrameComplete);

Should we assert some relation to dataComplete here?

> Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:145
> +    } else {
> +         return false; // I/O suspension.
>      }

No { } on these lines.

> Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.h:36
> +typedef struct WebPIDecoder WebPIDecoder;

Should this typedef happen inside the WebCore namespace to avoid pollution?

> Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.h:51
> +    // incremental decoder, in case only partial data is available.

Please make this comment a complete sentence that starts with an initial capital letter.

[Pascal Massimino]

Created attachment 90120 [details]
Patch

[WebKit Review Bot]

Attachment 90120 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 4 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Comment on attachment 90120 [details]
Patch

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

Ideally pkasting would take a look at well.  Thanks!

> Source/WebCore/ChangeLog:12
> +	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.

Looks like you have tabs here.  We use spaces instead of tabs.

> Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:108
> +        // FIXME: must be updated when WebP supports alpha-channel.

We generally prefer complete sentences for comments.  I would write something like:

// FIXME: We currently hard code false below because libwebp doesn't support alpha yet.

[Pascal Massimino]

(In reply to comment #2)
> (From update of attachment 90115 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90115&action=review
> 
> Looks great.  Some minor comments below.
> 
> > Source/WebCore/ChangeLog:8
> > +        No new tests needed.
> 
> Normally we like a little more song and dance here about now incremental decoding is good for the soul and difficult to test.

added a more descriptive paragraph.

> 
> > Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:87
> >      const uint8_t* dataBytes =
> >          reinterpret_cast<const uint8_t*>(m_data->data());
> 
> This should be on one line.  There isn't an 80 col limit.

done

> 
> > Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:100
> > +    const bool dataComplete = isAllDataReceived();
> > +    const int stride = width * bytesPerPixel;
> 
> WebKit isn't super agressive about using the const qualifier, but these are probably fine.

removed the const. Not really critical.

> 
> > Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:109
> > +        buffer.setHasAlpha(false);
> 
> I'd add a FIXME about changing this once WebP supports alpha.

done

> 
> > Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:141
> > +         buffer.setStatus(ImageFrame::FrameComplete);
> 
> Should we assert some relation to dataComplete here?

no: WebP files can have comment sections at the end, for instance. And hence, you can fully decode the picture even when the data is incomplete and missing the trailing comments section.

> 
> > Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:145
> > +    } else {
> > +         return false; // I/O suspension.
> >      }
> 
> No { } on these lines.

done

> 
> > Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.h:36
> > +typedef struct WebPIDecoder WebPIDecoder;
> 
> Should this typedef happen inside the WebCore namespace to avoid pollution?
> 

it's a forward-declaration of libwebp's struct. Added a comment.

> > Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.h:51
> > +    // incremental decoder, in case only partial data is available.
> 
> Please make this comment a complete sentence that starts with an initial capital letter.

done.

[Pascal Massimino]

Created attachment 90121 [details]
Patch

[Peter Kasting]

Comment on attachment 90121 [details]
Patch

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.

[Pascal Massimino]

Created attachment 90124 [details]
Patch

[Pascal Massimino]

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

[Adam Barth]

Comment on attachment 90124 [details]
Patch

... and that's why I wish Peter reviewed every patch.

[WebKit Commit Bot]

Comment on attachment 90124 [details]
Patch

Clearing flags on attachment: 90124

Committed r84219: <http://trac.webkit.org/changeset/84219>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[noel gordon]

I don't see where the allocated m_decoder is ever deleted, filed bug 73756 about that.

[noel gordon]

Noticed WebPGetInfo() is continuously called even after we know the decoded image width/height.  Filed bug 73796 about that.

[noel gordon]

If allDataReceived happens during a progressive decode, the decoder performs full image decode.  Filed bug 74041 about that.

[noel gordon]

Progressive decodes sometimes fail to decode valid images.  Filed bug 74062 about that.

[noel gordon]

m_rgbBuffer is not cleared when decoding ends (that wastes memory) and the copy from it to the pixel backing store is slow. Filed bug 74263.