10
\$\begingroup\$

Lately, I've been part of a team working on a utility library for iOS called Thundercats (on Github). We're about to start working on some changes and upgrades in preparation for a 2.0 release, so we figured it might be a good time to get some of our code publicly reviewed.

The library is written in Objective-C and is set up to work with Cocoapods, but we want to make sure we the library works well from iOS 6.0 and up and with both Swift and Objective-C.

This code is from the UIImage extension. The method is used for blurring an image. It is exceptionally long. At 161 lines, it may be the longest method in the library. It's fairly complex and relatively lacking in comments, but that doesn't inherently make it unreadable.

It's important that the code is efficient, and I know that refactoring into other methods can sometimes decrease execution speed in Objective-C. Perhaps some of this could be refactored into C-style functions?

UIImage+TCAdditions.m

- (UIImage *)tc_imageWithBlurUsingRadius:(CGFloat)blurRadius
                               tintColor:(UIColor *)tintColor
                   saturationDeltaFactor:(CGFloat)saturationDeltaFactor
                               maskImage:(UIImage *)maskImage
{
    // Check pre-conditions.
    if (self.size.width < 1 || self.size.height < 1)
    {
        NSLog (@"*** error: invalid size: (%.2f x %.2f). Both dimensions must be >= 1: %@", self.size.width, self.size.height, self);
        return nil;
    }
    if (!self.CGImage)
    {
        NSLog (@"*** error: image must be backed by a CGImage: %@", self);
        return nil;
    }
    if (maskImage && !maskImage.CGImage)
    {
        NSLog (@"*** error: maskImage must be backed by a CGImage: %@", maskImage);
        return nil;
    }

    CGRect imageRect = { CGPointZero, self.size };
    UIImage *effectImage = self;

    BOOL hasBlur = blurRadius > __FLT_EPSILON__;
    BOOL hasSaturationChange = fabs(saturationDeltaFactor - 1.) > __FLT_EPSILON__;

    if (hasBlur || hasSaturationChange)
    {
        UIGraphicsBeginImageContextWithOptions(self.size, NO, [[UIScreen mainScreen] scale]);
        CGContextRef effectInContext = UIGraphicsGetCurrentContext();
        CGContextScaleCTM(effectInContext, 1.0, -1.0);
        CGContextTranslateCTM(effectInContext, 0, -self.size.height);
        CGContextDrawImage(effectInContext, imageRect, self.CGImage);

        vImage_Buffer effectInBuffer;
        effectInBuffer.data     = CGBitmapContextGetData(effectInContext);
        effectInBuffer.width    = CGBitmapContextGetWidth(effectInContext);
        effectInBuffer.height   = CGBitmapContextGetHeight(effectInContext);
        effectInBuffer.rowBytes = CGBitmapContextGetBytesPerRow(effectInContext);

        UIGraphicsBeginImageContextWithOptions(self.size, NO, [[UIScreen mainScreen] scale]);
        CGContextRef effectOutContext = UIGraphicsGetCurrentContext();
        vImage_Buffer effectOutBuffer;
        effectOutBuffer.data     = CGBitmapContextGetData(effectOutContext);
        effectOutBuffer.width    = CGBitmapContextGetWidth(effectOutContext);
        effectOutBuffer.height   = CGBitmapContextGetHeight(effectOutContext);
        effectOutBuffer.rowBytes = CGBitmapContextGetBytesPerRow(effectOutContext);

        if (hasBlur)
        {
            // A description of how to compute the box kernel width from the Gaussian
            // radius (aka standard deviation) appears in the SVG spec:
            // http://www.w3.org/TR/SVG/filters.html#feGaussianBlurElement
            //
            // For larger values of 's' (s >= 2.0), an approximation can be used: Three
            // successive box-blurs build a piece-wise quadratic convolution kernel, which
            // approximates the Gaussian kernel to within roughly 3%.
            //
            // let d = floor(s * 3*sqrt(2*pi)/4 + 0.5)
            //
            // ... if d is odd, use three box-blurs of size 'd', centered on the output pixel.
            //
            CGFloat inputRadius = blurRadius * [[UIScreen mainScreen] scale];
            uint32_t radius = floor(inputRadius * 3. * sqrt(2 * M_PI) / 4 + 0.5);

            if (radius % 2 != 1)
            {
                radius += 1; // force radius to be odd so that the three box-blur methodology works.
            }

            vImageBoxConvolve_ARGB8888(&effectInBuffer, &effectOutBuffer, NULL, 0, 0, radius, radius, 0, kvImageEdgeExtend);
            vImageBoxConvolve_ARGB8888(&effectOutBuffer, &effectInBuffer, NULL, 0, 0, radius, radius, 0, kvImageEdgeExtend);
            vImageBoxConvolve_ARGB8888(&effectInBuffer, &effectOutBuffer, NULL, 0, 0, radius, radius, 0, kvImageEdgeExtend);
        }

        BOOL effectImageBuffersAreSwapped = NO;

        if (hasSaturationChange)
        {
            CGFloat s = saturationDeltaFactor;
            CGFloat floatingPointSaturationMatrix[] = {
                0.0722 + 0.9278 * s,  0.0722 - 0.0722 * s,  0.0722 - 0.0722 * s,  0,
                0.7152 - 0.7152 * s,  0.7152 + 0.2848 * s,  0.7152 - 0.7152 * s,  0,
                0.2126 - 0.2126 * s,  0.2126 - 0.2126 * s,  0.2126 + 0.7873 * s,  0,
                0,                    0,                    0,  1,
            };

            const int32_t divisor = 256;
            NSUInteger matrixSize = sizeof(floatingPointSaturationMatrix)/sizeof(floatingPointSaturationMatrix[0]);
            int16_t saturationMatrix[matrixSize];

            for (NSUInteger i = 0; i < matrixSize; ++i)
            {
                saturationMatrix[i] = (int16_t)roundf(floatingPointSaturationMatrix[i] * divisor);
            }

            if (hasBlur)
            {
                vImageMatrixMultiply_ARGB8888(&effectOutBuffer, &effectInBuffer, saturationMatrix, divisor, NULL, NULL, kvImageNoFlags);
                effectImageBuffersAreSwapped = YES;
            }
            else
            {
                vImageMatrixMultiply_ARGB8888(&effectInBuffer, &effectOutBuffer, saturationMatrix, divisor, NULL, NULL, kvImageNoFlags);
            }
        }

        if (!effectImageBuffersAreSwapped)
        {
            effectImage = UIGraphicsGetImageFromCurrentImageContext();
        }

        UIGraphicsEndImageContext();

        if (effectImageBuffersAreSwapped)
        {
            effectImage = UIGraphicsGetImageFromCurrentImageContext();
        }

        UIGraphicsEndImageContext();
    }

    // Set up output context.
    UIGraphicsBeginImageContextWithOptions(self.size, NO, [[UIScreen mainScreen] scale]);
    CGContextRef outputContext = UIGraphicsGetCurrentContext();
    CGContextScaleCTM(outputContext, 1.0, -1.0);
    CGContextTranslateCTM(outputContext, 0, -self.size.height);

    // Draw base image.
    CGContextDrawImage(outputContext, imageRect, self.CGImage);

    // Draw effect image.
    if (hasBlur)
    {
        CGContextSaveGState(outputContext);

        if (maskImage)
        {
            CGContextClipToMask(outputContext, imageRect, maskImage.CGImage);
        }

        CGContextDrawImage(outputContext, imageRect, effectImage.CGImage);
        CGContextRestoreGState(outputContext);
    }

    // Add in color tint.
    if (tintColor)
    {
        CGContextSaveGState(outputContext);
        CGContextSetFillColorWithColor(outputContext, tintColor.CGColor);
        CGContextFillRect(outputContext, imageRect);
        CGContextRestoreGState(outputContext);
    }

    // Output image is ready.
    UIImage *outputImage = UIGraphicsGetImageFromCurrentImageContext();
    UIGraphicsEndImageContext();

    return outputImage;
}

The same file also includes some convenient ways of calling these methods. I don't think there's much to review in these. I'm primarily adding these for some context:

- (UIImage *)tc_imageWithLightEffect
{
    UIColor *tintColor = [UIColor colorWithWhite:1.0 alpha:0.3];
    return [self tc_imageWithBlurUsingRadius:30 tintColor:tintColor saturationDeltaFactor:1.8 maskImage:nil];
}


- (UIImage *)tc_imageWithExtraLightEffect
{
    UIColor *tintColor = [UIColor colorWithWhite:0.97 alpha:0.82];
    return [self tc_imageWithBlurUsingRadius:20 tintColor:tintColor saturationDeltaFactor:1.8 maskImage:nil];
}


- (UIImage *)tc_imageWithDarkEffect
{
    UIColor *tintColor = [UIColor colorWithWhite:0.11 alpha:0.73];
    return [self tc_imageWithBlurUsingRadius:20 tintColor:tintColor saturationDeltaFactor:1.8 maskImage:nil];
}


- (UIImage *)tc_imageWithTintEffectUsingColor:(UIColor *)tintColor
{
    const CGFloat EffectColorAlpha = 0.6;
    UIColor *effectColor = tintColor;
    unsigned long componentCount = CGColorGetNumberOfComponents(tintColor.CGColor);

    if (componentCount == 2)
    {
        CGFloat b;
        if ([tintColor getWhite:&b alpha:NULL])
        {
            effectColor = [UIColor colorWithWhite:b alpha:EffectColorAlpha];
        }
    }
    else
    {
        CGFloat r, g, b;
        if ([tintColor getRed:&r green:&g blue:&b alpha:NULL])
        {
            effectColor = [UIColor colorWithRed:r green:g blue:b alpha:EffectColorAlpha];
        }
    }

    return [self tc_imageWithBlurUsingRadius:10 tintColor:effectColor saturationDeltaFactor:-1.0 maskImage:nil];
}
\$\endgroup\$
4
\$\begingroup\$

I'd say that this code is pretty straightforward and I don't find this method to be so long I can't understand it. That said, I think you're right that it could be a little more readable if you broke it up.

Break Things Into Separate Functions

I'd be surprised if breaking this into some smaller methods caused much of a slowdown. The bulk of the time will be spent doing the box blurs, probably. You should profile it to be sure, of course.

So I'd break each separate filter into a separate method. Something like:

- (void)applyBlurWithRadius:(CGFloat)blurRadius
              toInputBuffer:(vImage_Buffer&)inputBuffer
           withOutputBuffer:(vImage_Buffer&)outputBuffer;

- (void)applySaturation:(CGFloat)saturationDelta
          toInputBuffer:(vImage_Buffer&)inputBuffer
       withOutputBuffer:(vImage_Buffer&)outputBuffer;

- (void)drawImage:(CGImageRef)image
         withMask:(CGImageRef)mask
        toContext:(CGContextRef)context;

- (void)applyTint:(UIColor*)tint
      toCGContext:(CGContextRef)context;

Don't Do Work You Don't Need To

I also notice that you're always drawing the image, then drawing over it. That's necessary when there's a mask, but if there's no mask and you're blurring the image, you're doing an extra draw. I'd add a check around the first call to CGContextDrawImage(), like this:

// Draw base image if there's a mask or we didn't blur it
if ((maskImage) || (!hasBlur))
{
    CGContextDrawImage(outputContext, imageRect, self.CGImage);
}

// Draw effect image.
if (hasBlur)
{
    CGContextSaveGState(outputContext);
    // ...etc.

Actually, does the current code work correctly if there's only a saturation change and no blur? It looks to me like it doesn't, but I might be misunderstanding something. (If that's the case that the (!hasBlur) should be ((!hasBlur) && (!hasSaturationChange)) and the next if should be if (hasBlur || hasSaturationChange).)

Why Use the CPU When You Can Use the GPU?

Is there a reason you're doing this work on the CPU? It seems like it would be more efficient to do it on the GPU using CoreImage. They have built-in filters for Gaussian blur, Color Matrix, compositing, etc.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.