[Webkit-unassigned] [Bug 33268] make JPEG image dcoder read segmented SharedBuffer

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=33268





--- Comment #5 from Yong Li <yong.li.webkit at gmail.com>  2010-01-06 14:46:52 PST ---
(In reply to comment #3)
> (From update of attachment 45993 [details])
> Other than the style nits the bot pointed out, my comments:
> 
> > +unsigned SharedBuffer::extract(char* buffer, unsigned bufferLength, unsigned from)
> 
> "from" (and to some degree "buffer") are a little vague.  How about:
> 
> unsigned SharedBuffer::copyToContiguousBuffer(char* destBuffer, size_t
> bufferLength, size_t readOffset)

"readOffset" is good. was thinking "offset" is confusing.

> 
> > +class SharedBufferDataWalker {
> 
> Not totally obvious what this class does, maybe you can write a brief comment
> above it?
sure.
> 
> > +        unsigned lastOffset = -1;
> > +        Vector<unsigned char, 8192> buffer;
> 
> Is it a potential perf issue that we can decode at most 8K at a time?

A segment in SharedBuffer is 4k. in the case JPEG reader needs 2 segments to
continue moving, 8k is a good number. The vector can grow when 8k is not
enough. but I think 8k is better than 0 for performance (no need to call malloc
in most cases)

> 
> > +        SharedBufferDataWalker segment;
> 
> "segment" is kind of odd as a name... why not "dataWalker" or something?
ok.
> 
> > +        while(ture) {
> 
> Won't compile (typo)
found this too. just replaced for(;;) with it before making the patch
> 
> > +            if (readOffset == lastOffset && m_bufferLength == data.size())
> > +                break;
> > +
> > +            unsigned segmentLength = 0;
> > +            if (readOffset == lastOffset) {
> 
> Could just move the first conditional here inside the second conditional.
yes.
> 
> > +                // Need more data to proceed
> > +                if (buffer.isEmpty()) {
> > +                    segment.set(data, readOffset);
> > +                    segmentLength = segment.length();
> > +                    buffer.append(segment.data(), segmentLength);
> > +                }
> > +                segment.set(data, readOffset + buffer.size());
> > +                segmentLength = segment.length();
> > +                buffer.append(segment.data(), segmentLength);
> > +                m_info.src->bytes_in_buffer += segmentLength;
> > +                m_info.src->next_input_byte = (JOCTET*)buffer.data();
> > +                m_bufferLength = readOffset + buffer.size();
> > +            } else {
> > +                if (!buffer.isEmpty())
> > +                    buffer.clear();
> > +
> > +                lastOffset = readOffset;
> > +
> > +                segment.set(data, readOffset);
> > +                segmentLength = segment.length();
> > +
> > +                unsigned totalSize = readOffset + segmentLength;
> > +                m_info.src->bytes_in_buffer += totalSize - m_bufferLength;
> > +                m_info.src->next_input_byte = (JOCTET*)segment.data();
> > +
> > +                // If we still have bytes to skip, try to skip those now.
> > +                if (m_bytesToSkip)
> > +                    skipBytes(m_bytesToSkip);
> > +
> > +                m_bufferLength = totalSize;
> > +            }
> 
> I confess, I can't totally follow all this even after peering through it a few
> times.  There are too many different buffers and lengths.  Maybe some block
> comment somewhere explaining the algorithm, I dunno.

sometimes the low level decoding doesn't move on unless you feed it more data
in consecutive memory space. "buffer" is there for this purpose.

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