Bug 58851

Summary: Add incremental decoding to WebP decoder
Product: WebKit Reporter: Pascal Massimino <pascal.massimino>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, commit-queue, noel.gordon, pkasting, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on: 73756, 73796, 74041, 74062, 74263    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Pascal Massimino 2011-04-18 16:29:24 PDT
add incremental decoding to WebP decoder
Comment 1 Pascal Massimino 2011-04-18 16:32:53 PDT
Created attachment 90115 [details]
Patch
Comment 2 Adam Barth 2011-04-18 16:38:24 PDT
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.
Comment 3 Pascal Massimino 2011-04-18 16:57:20 PDT
Created attachment 90120 [details]
Patch
Comment 4 WebKit Review Bot 2011-04-18 16:58:55 PDT
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.
Comment 5 Adam Barth 2011-04-18 17:00:43 PDT
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.
Comment 6 Pascal Massimino 2011-04-18 17:01:27 PDT
(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.
Comment 7 Pascal Massimino 2011-04-18 17:03:54 PDT
Created attachment 90121 [details]
Patch
Comment 8 Peter Kasting 2011-04-18 17:37:09 PDT
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.
Comment 9 Pascal Massimino 2011-04-18 17:48:30 PDT
Created attachment 90124 [details]
Patch
Comment 10 Pascal Massimino 2011-04-18 17:50:57 PDT
(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.
Comment 11 Adam Barth 2011-04-18 17:54:14 PDT
Comment on attachment 90124 [details]
Patch

... and that's why I wish Peter reviewed every patch.
Comment 12 WebKit Commit Bot 2011-04-18 20:43:39 PDT
Comment on attachment 90124 [details]
Patch

Clearing flags on attachment: 90124

Committed r84219: <http://trac.webkit.org/changeset/84219>
Comment 13 WebKit Commit Bot 2011-04-18 20:43:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 noel gordon 2011-12-03 17:22:04 PST
I don't see where the allocated m_decoder is ever deleted, filed bug 73756 about that.
Comment 15 noel gordon 2011-12-04 20:56:54 PST
Noticed WebPGetInfo() is continuously called even after we know the decoded image width/height.  Filed bug 73796 about that.
Comment 16 noel gordon 2011-12-07 17:55:59 PST
If allDataReceived happens during a progressive decode, the decoder performs full image decode.  Filed bug 74041 about that.
Comment 17 noel gordon 2011-12-08 13:53:28 PST
Progressive decodes sometimes fail to decode valid images.  Filed bug 74062 about that.
Comment 18 noel gordon 2011-12-11 22:04:09 PST
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.