Spotted a huge performance improvement for image drawing

General JUCE discussion
1

Hi Jules, everybody,

I spotted a ~33% overall performance improvement in my plug-ins when optimizing some drawings (mac/coregraphics). Here are my finding:

When drawing my plug-ins’ background images, I tried and clipped it to the given Context’s bounds. This did a great performance boost, which I couldn’t event obtain with setBufferedToImage(true);.

I then thought that if it was the case for this image, it could be the case for any image drawing, so I transfered my clipping part to the Graphics::drawImageAt() method:

[code]void Graphics::drawImageAt (const Image& imageToDraw,
const int topLeftX, const int topLeftY,
const bool fillAlphaChannelWithCurrentBrush) const
{
Rectangle clipBounds = context.getClipBounds();
Rectangle imageBounds (topLeftX, topLeftY,
imageToDraw.getWidth(), imageToDraw.getHeight());

if (clipBounds.intersects(imageBounds))
{
    Rectangle<int> intersection = clipBounds.getIntersection(imageBounds);
    
    Image clippedImage = imageToDraw.getClippedImage (intersection
                                                      .translated (-topLeftX, -topLeftY));
    
    jassert(intersection.getWidth() == clippedImage.getWidth());
    jassert(intersection.getHeight() == clippedImage.getHeight());
    
    int imageW = clippedImage.getWidth();
    int imageH = clippedImage.getHeight();
    
    drawImage (clippedImage,
               intersection.getTopLeft().x, intersection.getTopLeft().y, imageW, imageH,
               0, 0, imageW, imageH,
               fillAlphaChannelWithCurrentBrush);
}

}[/code]

That worked as well. But I could make it even more general in the Graphics::drawImage method instead, when the transform is just a translation (width and height conserved):

[code]void Graphics::drawImage (const Image& imageToDraw,
int dx, int dy, int dw, int dh,
int sx, int sy, int sw, int sh,
const bool fillAlphaChannelWithCurrentBrush) const
{
// passing in a silly number can cause maths problems in rendering!
jassert (areCoordsSensibleNumbers (dx, dy, dw, dh));
jassert (areCoordsSensibleNumbers (sx, sy, sw, sh));

Rectangle<int> clipBounds = context.getClipBounds();
Rectangle<int> destRegion (dx, dy, dw, dh);

if (imageToDraw.isValid() && clipBounds.intersects (destRegion))
{
    if (dw == sw && dh == sh)
    {
        Rectangle<int> intersection = clipBounds.getIntersection (destRegion);
        Image clippedImage = imageToDraw.getClippedImage(intersection
                                                         .translated (sx - dx, sy - dy));
        
        jassert(intersection.getWidth() == clippedImage.getWidth());
        jassert(intersection.getHeight() == clippedImage.getHeight());
        
        drawImageTransformed(clippedImage,
                             AffineTransform::identity
                                             .translated (intersection.getTopLeft().x,
                                                          intersection.getTopLeft().y),
                             fillAlphaChannelWithCurrentBrush);
    }
    else
    {
        drawImageTransformed (imageToDraw.getClippedImage (Rectangle<int> (sx, sy, sw, sh)),
                              AffineTransform::scale (dw / (float) sw, dh / (float) sh)
                                              .translated ((float) dx, (float) dy),
                              fillAlphaChannelWithCurrentBrush);
    }
}

}[/code]

And that has also worked well; it also covers the drawImageAt optimization case (so, forget the first snippet). The previous drawImage implementation was not as efficient because the clipping was to the source dimensions instead of the clipped dimensions. Source dimensions are often the whole image. Having more than the clipped image is indeed required when the transform is not only a translation (hence, requiring some inter-pixel computations a the clipped borders), however in many cases the transform is only a translation, notably in the drawImageAt call.

I don’t know if this optimizes very specifically in the CoreGraphicsContext case or if it is more general; however the last code snippet’s overhead is insignificant, so I would greatly recommend including it in Juce.

All the best

2

Thanks. Good detective work, but I think you’re trying to solve it in the wrong place. There’s no reason to think that anything other than CoreGraphics would benefit from this, so it’d be counter-productive to make changes to the Graphics class.

I guess that what you’re seeing is because in CoreGraphicsContext::drawImage, it calls CoreGraphicsImage::createImage for the entire image, and you’re seeing improvements when you pass a clipped image because that only uses a subset of the image data. That’s a surprise, because CoreGraphicsImage::createImage isn’t supposed to actually copy any data, so it shouldn’t make any difference how big the image is… but if that’s what you’re seeing, then presumably on some systems a copy is involved.

So… I haven’t time to have a go myself, but performing the same clipping trick in CoreGraphicsContext::drawImage might be a smart move.

3

Probably related: http://www.rawmaterialsoftware.com/viewtopic.php?f=4&t=6701

4

Uh oh, my assertions revealed issues with the Rectangle class:

Rectangle::intersect implementation issue

bool intersects (const Rectangle& other) const noexcept { return pos.x + w > other.pos.x && pos.y + h > other.pos.y && pos.x < other.pos.x + other.w && pos.y < other.pos.y + other.h && w > ValueType() && h > ValueType(); }
misses && other.w > ValueType() && other.h > ValueType()

Rectangle::getIntersection’s test, in order to be consistent, should probably be it (nw > ValueType() && nh > ValueType()) instead of if (nw >= ValueType() && nh >= ValueType())

I also observed that my new assertions are triggered when the image we want to draw does not cover the whole (sx, sy, dx, dy) rectangle; but this case is not an issue, so I would recommend removing the new assertions I added on width and height between intersection to be drawn and the clipped image.

Also, I didn’t have time to try and port this code or fix the CoreGraphicsContext.

All the best

5

I think that as long as at least one of the rectangles in non-empty, then the other comparisons will fail, so adding an extra line to check whether the other rectangle is empty is actually redundant… But trying to prove that is making my brain hurt!

In the other case, it uses >= because even if the resulting rectangle is empty, you still want it to have the correct x and y coords, as some things might rely on that.

6

[code] Rectangle r1(0, 0, 12, 174);
Rectangle r2(4, 157, 4, 0);

std::cout << (r1.intersects(r2) ? "intersects" : "separated") << std::endl;
std::cout << (r2.intersects(r1) ? "intersects" : "separated") << std::endl;[/code]

[quote]intersects
separated[/quote]

7

Following this logic the comparisons in Rectangle::intersect should have been “>=” too because the rectangles intersect on a segment or on a point. This might be useful for example for vector graphics; however we need an additional version of this method with “>” which only answers true when the area is not empty, for example for image drawing code.

8

Rectangles should only be considered intersecting if there is at least one pixel enclosed by both. Pixels land in between the coordinates, so these rectangles should NOT intersect:

(0, 0, 1, 1) and (1, 0, 1, 1)

Rectangles are specified as (x, y, width, height)

9

So there is currently a bug:

Rectangle<int> r1(0, 0, 10, 10); Rectangle<int> r2(5, 1, 5, 0); std::cout << (r1.intersects(r2) ? "intersect" : "separated") << std::endl;

Should return “separated” instead of “intersect”.

10

r2 has zero height so it should never intersect with anything.

11

So, Jules, could you please fix this intersects() bug? Thanks

All the best

12

Yes, will do.

13

I’ve just seen the fix upstream, thanks!