[Webkit-unassigned] [Bug 31806] Make GIF decoder support down-sampling

[bugzilla-daemon at webkit.org]

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





--- Comment #4 from Peter Kasting <pkasting at google.com>  2009-11-30 11:58:10 PST ---
(In reply to comment #3)
> (In reply to comment #2)
> > I didn't look too closely at the patch, but I wonder if it could be simplified
> > even further by making scaledRect() be available with or without this define
> > (and have it simply map to rect() when there's no scaling).
> 
> I guess the point is trying not to affect the existing code. "scaledRect()"
> looks a little weird when down-sampling is not enabled. If a reviewer says he
> will r+ with that, I would definitely like to do this change.

If you were to do this you could probably name the accessor something clearer
like "possiblyScaledRect()" or "outputRect()" or something, which would
hopefully take care of any confusion.

> > Similarly, a
> > couple of the places where we run the same loop body over a different set of
> > coordinates, maybe we could simply set a temp to different values in the two
> > cases, instead of #ifdefing the loop body?
> 
> Actually that's what I have tried. But I really don't like checking the same
> value inside a loop, although the compiler may be able to optimize it. Also the
> code in that way is not neat, I have to put #if at both the beginning and the
> end of the loop body. It looks worse than current patch.

I tried refactoring it myself and was able to come up with something pretty
clean for the relevant code in haveDecodedRow():

***

    // Write one row's worth of data into the frame.  There is no guarantee
that
    // (rowEnd - rowBuffer) == (size().width() - m_reader->frameXOffset()), so
    // we must ensure we don't run off the end of either the source data or the
    // row's X-coordinates.
    int destXEnd = std::min(x + (rowEnd - rowBuffer), size().width());
    int destYEnd = std::min(y + static_cast<int>(repeatCount),
size().height());

    if (m_scaled) {
        x = upperBoundScaledX(x);
        if (x < 0)
            return;
        destXEnd = lowerBoundScaledX(destXEnd - 1, x + 1) + 1;
        if (destXEnd <= x)
            return;

        y = upperBoundScaledY(y);
        if (y < 0)
            return;
        destYEnd = lowerBoundScaledY(destYEnd - 1, y + 1) + 1;
        if (destYEnd <= y)
            return;
    }

    for (int destX = x; destX < destXEnd; ++destX) {
        unsigned char* sourceAddr = rowBuffer + (m_scaled ?
(m_scaledColumns[destX] - m_reader->frameXOffset()) : destX);
        copyOnePixel(m_reader, sourceAddr, colorMap, colorMapSize,
writeTransparentPixels, buffer, destX , y, m_currentBufferSawAlpha);
    }

    // Tell the frame to copy the row data if need be.
    if (repeatCount > 1)
        buffer.copyRowNTimes(x, destXEnd, y, destYEnd);
}

***

Several points to note:
* This made it clear that |bufferSize| was an unused temp and could be removed.
* The above code assumes that you make |m_scaled| exist unconditionally (and
it's just always false if we're not downsampling), which I think is probably a
good change since it should help readability in general.
* This exposed a bug in your existing patch, where in the #ifdefed path you
failed to clamp the Y endpoint to size().height().

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