Bug 33213 - Make png image decoder work with segmented SharedBuffer
Summary: Make png image decoder work with segmented SharedBuffer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-05 10:18 PST by Yong Li
Modified: 2010-01-06 10:57 PST (History)
2 users (show)

See Also:


Attachments
the patch (7.75 KB, patch)
2010-01-05 10:18 PST, Yong Li
no flags Details | Formatted Diff | Diff
the patch (7.75 KB, patch)
2010-01-05 10:55 PST, Yong Li
darin: review+
Details | Formatted Diff | Diff
fixed some issues (8.20 KB, patch)
2010-01-05 13:22 PST, Yong Li
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yong Li 2010-01-05 10:18:50 PST
Created attachment 45903 [details]
the patch

See bug 33178
Comment 1 Yong Li 2010-01-05 10:19:39 PST
Comment on attachment 45903 [details]
the patch

oops. forgot to fix a bug
Comment 2 Yong Li 2010-01-05 10:55:53 PST
Created attachment 45911 [details]
the patch
Comment 3 WebKit Review Bot 2010-01-05 10:59:25 PST
style-queue ran check-webkit-style on attachment 45911 [details] without any errors.
Comment 4 Darin Adler 2010-01-05 12:08:26 PST
Comment on attachment 45911 [details]
the patch

> +    const char* contents;
> +    unsigned length = data.getSomeData(contents, 0);
> +
>      // We need at least 4 bytes to figure out what kind of image we're dealing with.
> -    int length = data.size();
>      if (length < 4)
>          return 0;

This seems a bit subtle and non-obvious. I guess the class guarantees that the first four bytes will all be in a single segment even if they come in one byte at a time, but nothing in the SharedBuffer header comments makes that clear.

> +        virtual void setData(SharedBuffer* data, bool allDataReceived)
> +        {
> +            m_data = data;
> +            m_isAllDataReceived = allDataReceived;
> +        }

It's not optimal style to have a virtual function defined in a header. The old code had that, but it would be better not to do it that way.

> +        , m_jobComplete(false)

For booleans data members and function members it's best to name them so they work in a sentence "image decoder <xxx>". So hasAlpha is a pretty good name but jobComplete is not as good. Maybe hasFinishedDecoding or hasReceivedAllData?

> +            if (!segmentLength) {
> +                if (!m_jobComplete) {
> +                    if (decoder->isAllDataReceived())
> +                        decoder->pngComplete();
> +                }
> +                break;
> +            }

I suggest putting the other code outside the loop. It seems we should just break when done and then finish the ob after the loop.

I'll say r=me even though I normally don't review code in this file. Seems fine.
Comment 5 Yong Li 2010-01-05 12:17:37 PST
(In reply to comment #4)
> (From update of attachment 45911 [details])
> > +    const char* contents;
> > +    unsigned length = data.getSomeData(contents, 0);
> > +
> >      // We need at least 4 bytes to figure out what kind of image we're dealing with.
> > -    int length = data.size();
> >      if (length < 4)
> >          return 0;
> 
> This seems a bit subtle and non-obvious. I guess the class guarantees that the
> first four bytes will all be in a single segment even if they come in one byte
> at a time, but nothing in the SharedBuffer header comments makes that clear.
> 

You're right. To make it safe, I'll rewrite this code for the corner case, so that we won't have a hard-to-debug problem if the assumption becomes false in the future.

> > +        virtual void setData(SharedBuffer* data, bool allDataReceived)
> > +        {
> > +            m_data = data;
> > +            m_isAllDataReceived = allDataReceived;
> > +        }
> 
> It's not optimal style to have a virtual function defined in a header. The old
> code had that, but it would be better not to do it that way.

Not sure if any derived class has implemented different setData(). If not, I'll remove "virtual"
> 
> > +        , m_jobComplete(false)
> 
> For booleans data members and function members it's best to name them so they
> work in a sentence "image decoder <xxx>". So hasAlpha is a pretty good name but
> jobComplete is not as good. Maybe hasFinishedDecoding or hasReceivedAllData?

will use hasFinishedDecoding

> 
> > +            if (!segmentLength) {
> > +                if (!m_jobComplete) {
> > +                    if (decoder->isAllDataReceived())
> > +                        decoder->pngComplete();
> > +                }
> > +                break;
> > +            }
> 
> I suggest putting the other code outside the loop. It seems we should just
> break when done and then finish the ob after the loop.

Agree, and actually the 2 "if" can be merged.

> 
> I'll say r=me even though I normally don't review code in this file. Seems
> fine.

thanks a lot. but I'll post a new patch regarding to above issues.
Comment 6 Yong Li 2010-01-05 13:22:21 PST
Created attachment 45927 [details]
fixed some issues
Comment 7 WebKit Review Bot 2010-01-05 13:28:51 PST
style-queue ran check-webkit-style on attachment 45927 [details] without any errors.
Comment 8 Darin Adler 2010-01-05 14:03:18 PST
Comment on attachment 45927 [details]
fixed some issues

> +    // XBMs require 8 bytes of info.
> +    static const unsigned maxMarkerLength = 8;
> +
> +    char contents[maxMarkerLength];
> +    unsigned length = 0;
> +    for (;;) {
> +        const char* moreContents;
> +        unsigned moreContentsLength = data.getSomeData(moreContents, 0);
> +        if (!moreContentsLength)
> +            break;
> +        unsigned bytesToCopy = min(maxMarkerLength - length, moreContentsLength);
> +        memcpy(contents + length, moreContents, bytesToCopy);
> +        length += bytesToCopy;
> +        if (length == maxMarkerLength)
> +            break;
> +    }

This cries out for a helper function. SharedBuffer could provide a function to that has this logic in it rather than requiring callers to repeatedly call getSomeData. Or at least put this in a separate function in this file.

Seems OK. r=me
Comment 9 Yong Li 2010-01-05 14:57:04 PST
(In reply to comment #8)
> (From update of attachment 45927 [details])
> > +    // XBMs require 8 bytes of info.
> > +    static const unsigned maxMarkerLength = 8;
> > +
> > +    char contents[maxMarkerLength];
> > +    unsigned length = 0;
> > +    for (;;) {
> > +        const char* moreContents;
> > +        unsigned moreContentsLength = data.getSomeData(moreContents, 0);
> > +        if (!moreContentsLength)
> > +            break;
> > +        unsigned bytesToCopy = min(maxMarkerLength - length, moreContentsLength);
> > +        memcpy(contents + length, moreContents, bytesToCopy);
> > +        length += bytesToCopy;
> > +        if (length == maxMarkerLength)
> > +            break;
> > +    }
> 
> This cries out for a helper function. SharedBuffer could provide a function to
> that has this logic in it rather than requiring callers to repeatedly call
> getSomeData. Or at least put this in a separate function in this file.
> 
> Seems OK. r=me

I'm putting it into a new function with a bug fixed too: getSomeData() should be called with increasing offset but not 0.

Would you like to review the new patch? Or I can commit it now?
Comment 10 Yong Li 2010-01-05 15:10:33 PST
Comment on attachment 45927 [details]
fixed some issues

landed @ 52831
Comment 11 Yong Li 2010-01-05 15:10:55 PST
bug closed
Comment 12 Yong Li 2010-01-06 10:48:27 PST
Darin, there's a bug in yesterday's commit for png decoder:

I move "if (!m_hasFinishedDecoding && decoder->isAllDataReceived())" out of the loop, but it hurts another "break".

        while (unsigned segmentLength = data.getSomeData(segment, m_readOffset)) {
            m_readOffset += segmentLength;
            m_currentBufferSize = m_readOffset;
            png_process_data(m_png, m_info, reinterpret_cast<png_bytep>(const_cast<char*>(segment)), segmentLength);
            if ((sizeOnly && decoder->isSizeAvailable()) || m_hasFinishedDecoding)
                break;
        }
        if (!m_hasFinishedDecoding && decoder->isAllDataReceived())
            decoder->pngComplete();

When "sizeonly" is true, decoder->pngComplete() shouldn't be called.

Should I post a patch in this bug, or raise a new bug?
Comment 13 Darin Adler 2010-01-06 10:53:12 PST
Patch in this bug is fine. r=me in advance on adding sizeOnly to the if condition, so you don't need to post a patch at all if that's what you do.
Comment 14 Yong Li 2010-01-06 10:57:05 PST
Darin, there's a bug in yesterday's commit for png decoder:

I move "if (!m_hasFinishedDecoding && decoder->isAllDataReceived())" out of the loop, but it hurts another "break".

        while (unsigned segmentLength = data.getSomeData(segment, m_readOffset)) {
            m_readOffset += segmentLength;
            m_currentBufferSize = m_readOffset;
            png_process_data(m_png, m_info, reinterpret_cast<png_bytep>(const_cast<char*>(segment)), segmentLength);
            if ((sizeOnly && decoder->isSizeAvailable()) || m_hasFinishedDecoding)
                break;
        }
        if (!m_hasFinishedDecoding && decoder->isAllDataReceived())
            decoder->pngComplete();

When "sizeonly" is true, decoder->pngComplete() shouldn't be called.

Should I post a patch in this bug, or raise a new bug?(In reply to comment #13)
> Patch in this bug is fine. r=me in advance on adding sizeOnly to the if
> condition, so you don't need to post a patch at all if that's what you do.

Peter Kasting already raised a bug for it. bug 33258