Bug 31806 – Make GIF decoder support down-sampling

• Component: Images
• Status: RESOLVED FIXED

[Yong Li]

ENABLE(IMAGE_DECODER_DOWN_SAMPLING) is currently supported by JPEG decoder and PNG decoder. But some websites can have huge (for example, more than 10M pixels) gif images. Making GIF decoder support on-the-fly down-sampling will solve this problem.

Patch is coming.

[Yong Li]

Created attachment 43716 [details]
The patch

[Peter Kasting]

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).    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?

[Yong Li]

(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.

> 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.

[Peter Kasting]

(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().

[Adam Barth]

Attachment 43716 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Done processing WebCore/platform/image-decoders/ImageDecoder.cpp
WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp:286:  One line control clauses should not use braces.  [whitespace/braces] [4]
Done processing WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp
Done processing WebCore/platform/image-decoders/ImageDecoder.h
Total errors found: 1

[Yong Li]

(In reply to comment #5)
> Attachment 43716 [details] did not pass style-queue:
> 
> Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
> Done processing WebCore/platform/image-decoders/ImageDecoder.cpp
> WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp:286:  One line control
> clauses should not use braces.  [whitespace/braces] [4]
> Done processing WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp
> Done processing WebCore/platform/image-decoders/ImageDecoder.h
> Total errors found: 1

Hi, Adam,

The code is here:

#if ENABLE(IMAGE_DECODER_DOWN_SAMPLING)
    if (m_scaled) {
        int left = upperBoundScaledX(frameRect.x());
        int right = lowerBoundScaledX(frameRect.right(), left);
        int top = upperBoundScaledY(frameRect.y());
        int bottom = lowerBoundScaledY(frameRect.bottom(), top);
        buffer->setScaledRect(IntRect(left, top, right - left, bottom - top));
    } else {
        buffer->setScaledRect(frameRect);
    }
#endif

I can remove the braces but I really think leaving them there in this case is better.

-Yong

[Peter Kasting]

(In reply to comment #6)
> The code is here:
> 
> #if ENABLE(IMAGE_DECODER_DOWN_SAMPLING)
>     if (m_scaled) {
>         int left = upperBoundScaledX(frameRect.x());
>         int right = lowerBoundScaledX(frameRect.right(), left);
>         int top = upperBoundScaledY(frameRect.y());
>         int bottom = lowerBoundScaledY(frameRect.bottom(), top);
>         buffer->setScaledRect(IntRect(left, top, right - left, bottom - top));
>     } else {
>         buffer->setScaledRect(frameRect);
>     }
> #endif
> 
> I can remove the braces but I really think leaving them there in this case is
> better.

Personally, I agree with you.  However, the WebKit style guide is explicit about this case, and says that there are to be no braces on the second arm.  We shouldn't violate the style guide simply because we don't like some of its rules, or there is no point to having a style guide.

[George Staikos]

(In reply to comment #7)

> > I can remove the braces but I really think leaving them there in this case is
> > better.
> 
> Personally, I agree with you.  However, the WebKit style guide is explicit
> about this case, and says that there are to be no braces on the second arm.  We
> shouldn't violate the style guide simply because we don't like some of its
> rules, or there is no point to having a style guide.

That's why they're "guidelines" and not hard rules.

[Mark Rowe (bdash)]

(In reply to comment #8)
> (In reply to comment #7)
> > We shouldn't violate the style guide simply because we don't like some of its
> > rules, or there is no point to having a style guide.
> 
> That's why they're "guidelines" and not hard rules.

The “Contributing Code” page at <http://webkit.org/coding/contributing.html> could hardly be clearer about this:

> Patches must comply with the code style guidelines.

It’s also worth noting that the reviewer policy explicitly states:

> Reviewers are expected to ensure that patches they review follow project policies.

Coding style is one such policy.  Project policy should be followed, not personal preference.

[George Staikos]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > We shouldn't violate the style guide simply because we don't like some of its
> > > rules, or there is no point to having a style guide.
> > 
> > That's why they're "guidelines" and not hard rules.
> 
> The “Contributing Code” page at <http://webkit.org/coding/contributing.html>
> could hardly be clearer about this:
> 
> > Patches must comply with the code style guidelines.
> 
> It’s also worth noting that the reviewer policy explicitly states:
> 
> > Reviewers are expected to ensure that patches they review follow project policies.
> 
> Coding style is one such policy.  Project policy should be followed, not
> personal preference.

The amount of time spent on this policy with no valid argument that I can see is awesome.  Just imagine how many bugs we didn't implement because we spent time debating where we put the braces in two perfectly reasonable and clean cases instead of writing more code.  Now we even write policing tools for it!  Extra awesome.  I propose that the next step is we optimize gcc to only accept code according to the webkit coding style.  Surely the parser will be faster, resulting in faster compile times of WebKit which in the end make this all worthwhile. A+

Now back to getting something real done.

[Mark Rowe (bdash)]

(In reply to comment #10)
> 
> The amount of time spent on this policy with no valid argument that I can see
> is awesome.  Just imagine how many bugs we didn't implement because we spent
> time debating where we put the braces in two perfectly reasonable and clean
> cases instead of writing more code.

The only reason it is being debated at all is that the style guidelines were ignored.  If they’re followed then there’s nothing to debate.

[Yong Li]

Created attachment 45438 [details]
updated

1. fix a bug: test destYEnd with size.height()
2. style fix: remove the braces from the one-line "else" block
3. remove an unused local variable: "bufferSize"

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45438 [details] without any errors.

[Yong Li]

Created attachment 45439 [details]
new patch

oops, missed ChangeLog in previous patch

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45439 [details] without any errors.

[Yong Li]

Created attachment 46378 [details]
the patch

keep up with ToT changes. haveDecodedRow returns a boolean now.

[Adam Treat]

Comment on attachment 46378 [details]
the patch

Passes all style and build bots and looks good to me.  I do understand and sympathize with the sentiment that it'd be nice to reduce the #if ENABLE(IMAGE_DECODER_DOWN_SAMPLING) checks, but not sure the proposed solutions make anything better.

[Yong Li]

landed @ r53148. bug closed