[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 #6 from Pascal Massimino <pascal.massimino at gmail.com>  2011-04-18 17:01:27 PST ---
(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.

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