Sdwebimage: A method named 'decodedAndScaledDownImageWithImage' in Class "SDImageCoderHelper' maybe has an error.

[yyhinbeijing]

Error method

+ (UIImage *)decodedAndScaledDownImageWithImage:(UIImage *)image limitBytes:(NSUInteger)bytes {
    if (![self shouldDecodeImage:image]) {
        return image;
    }

    if (![self shouldScaleDownImage:image limitBytes:bytes]) {
        return [self decodedImageWithImage:image];
    }

    CGFloat destTotalPixels;
    CGFloat tileTotalPixels;
    if (bytes == 0) {
        bytes = kDestImageLimitBytes;
    }
    destTotalPixels = bytes / kBytesPerPixel;
    tileTotalPixels = destTotalPixels / 3;
    CGContextRef destContext;

    // autorelease the bitmap context and all vars to help system to free memory when there are memory warning.
    // on iOS7, do not forget to call [[SDImageCache sharedImageCache] clearMemory];
    @autoreleasepool {
        CGImageRef sourceImageRef = image.CGImage;

        CGSize sourceResolution = CGSizeZero;
        sourceResolution.width = CGImageGetWidth(sourceImageRef);
        sourceResolution.height = CGImageGetHeight(sourceImageRef);
        CGFloat sourceTotalPixels = sourceResolution.width * sourceResolution.height;
        // Determine the scale ratio to apply to the input image
        // that results in an output image of the defined size.
        // see kDestImageSizeMB, and how it relates to destTotalPixels.
        CGFloat imageScale = sqrt(destTotalPixels / sourceTotalPixels);
        CGSize destResolution = CGSizeZero;
        destResolution.width = (int)(sourceResolution.width * imageScale);
        destResolution.height = (int)(sourceResolution.height * imageScale);

        // device color space
        CGColorSpaceRef colorspaceRef = [self colorSpaceGetDeviceRGB];
        BOOL hasAlpha = [self CGImageContainsAlpha:sourceImageRef];
        // iOS display alpha info (BGRA8888/BGRX8888)
        CGBitmapInfo bitmapInfo = kCGBitmapByteOrder32Host;
        bitmapInfo |= hasAlpha ? kCGImageAlphaPremultipliedFirst : kCGImageAlphaNoneSkipFirst;

        // kCGImageAlphaNone is not supported in CGBitmapContextCreate.
        // Since the original image here has no alpha info, use kCGImageAlphaNoneSkipFirst
        // to create bitmap graphics contexts without alpha info.
        destContext = CGBitmapContextCreate(NULL,
                                            destResolution.width,
                                            destResolution.height,
                                            kBitsPerComponent,
                                            0,
                                            colorspaceRef,
                                            bitmapInfo);

        if (destContext == NULL) {
            return image;
        }
        CGContextSetInterpolationQuality(destContext, kCGInterpolationHigh);

        // Now define the size of the rectangle to be used for the
        // incremental bits from the input image to the output image.
        // we use a source tile width equal to the width of the source
        // image due to the way that iOS retrieves image data from disk.
        // iOS must decode an image from disk in full width 'bands', even
        // if current graphics context is clipped to a subrect within that
        // band. Therefore we fully utilize all of the pixel data that results
        // from a decoding operation by anchoring our tile size to the full
        // width of the input image.
        CGRect sourceTile = CGRectZero;
        sourceTile.size.width = sourceResolution.width;
        // The source tile height is dynamic. Since we specified the size
        // of the source tile in MB, see how many rows of pixels high it
        // can be given the input image width.
        sourceTile.size.height = (int)(tileTotalPixels / sourceTile.size.width );
        sourceTile.origin.x = 0.0f;
        // The output tile is the same proportions as the input tile, but
        // scaled to image scale.
        CGRect destTile;
        destTile.size.width = destResolution.width;
        destTile.size.height = sourceTile.size.height * imageScale;
        destTile.origin.x = 0.0f;
        // The source seem overlap is proportionate to the destination seem overlap.
        // this is the amount of pixels to overlap each tile as we assemble the output image.
        float sourceSeemOverlap = (int)((kDestSeemOverlap/destResolution.height)*sourceResolution.height);
        CGImageRef sourceTileImageRef;
        // calculate the number of read/write operations required to assemble the
        // output image.
        int iterations = (int)( sourceResolution.height / sourceTile.size.height );
        // If tile height doesn't divide the image height evenly, add another iteration
        // to account for the remaining pixels.
        int remainder = (int)sourceResolution.height % (int)sourceTile.size.height;
        if(remainder) {
            iterations++;
        }
        // Add seem overlaps to the tiles, but save the original tile height for y coordinate calculations.
        float sourceTileHeightMinusOverlap = sourceTile.size.height;
        sourceTile.size.height += sourceSeemOverlap;
        destTile.size.height += kDestSeemOverlap;
        for( int y = 0; y < iterations; ++y ) {
            @autoreleasepool {
                sourceTile.origin.y = y * sourceTileHeightMinusOverlap + sourceSeemOverlap;
                destTile.origin.y = destResolution.height - (( y + 1 ) * sourceTileHeightMinusOverlap * imageScale + kDestSeemOverlap);
                sourceTileImageRef = CGImageCreateWithImageInRect( sourceImageRef, sourceTile );
                if( y == iterations - 1 && remainder ) {
                    float dify = destTile.size.height;
                    destTile.size.height = CGImageGetHeight( sourceTileImageRef ) * imageScale;
                    dify -= destTile.size.height;
                    destTile.origin.y += dify;
                }
                CGContextDrawImage( destContext, destTile, sourceTileImageRef );
                CGImageRelease( sourceTileImageRef );
            }
        }

        CGImageRef destImageRef = CGBitmapContextCreateImage(destContext);
        CGContextRelease(destContext);
        if (destImageRef == NULL) {
            return image;
        }
#if SD_MAC
        UIImage *destImage = [[UIImage alloc] initWithCGImage:destImageRef scale:image.scale orientation:kCGImagePropertyOrientationUp];
#else
        UIImage *destImage = [[UIImage alloc] initWithCGImage:destImageRef scale:image.scale orientation:image.imageOrientation];
#endif
        CGImageRelease(destImageRef);
        if (destImage == nil) {
            return image;
        }
        SDImageCopyAssociatedObject(image, destImage);
        destImage.sd_isDecoded = YES;
        return destImage;
    }
}

Error code location and my analysis

   sourceTile.size.height = (int)(tileTotalPixels / sourceTile.size.width );
   int iterations = (int)( sourceResolution.height / sourceTile.size.height );

• if tileTotalPixels < sourceTile.size.width
• and get sourceTile.size.heigh == 0
• then iterations must be a negative infinity number
• we get a black image.
• this is wrong logic.

Advice

add codes as follow after code "sourceTile.size.height = (int)(tileTotalPixels / sourceTile.size.width )";

  if (tileTotalPixels < sourceTile.size.width) {
            sourceTile.size.height = 1;
   }

[dreampiggy]

Why this sourceTile.size can be 0 ? You means, you pass a CGImage has 0 widht or height ?

[dreampiggy]

I guess you want to limit a image to the size which is unpossible to calculate the tile. Like you say:

tileTotalPixels (Which is calculated based on limitBytes) < sourceResolution.width

[dreampiggy]

@yyhinbeijing It's OK to add a protect here. Also, add a earily return if CGImage.width or CGImage.height == 0. Could you help to create a MR ?

[yyhinbeijing]

Why this sourceTile.size can be 0 ? You means, you pass a CGImage has 0 width or height ?

NO , what i described two day ago is problematic.I update and describe more particularly.
if tileTotalPixels < sourceTile.size.width && tileTotalPixels> 0 && sourceTile.size.width > 0
sourceTile.size.height = (int)(tileTotalPixels / sourceTile.size.width ); (this code from that method)
sourceTile.size.height type is int ,so sourceTile.size.height must be 0.
My reference(calculation) is correct?

[yyhinbeijing]

@yyhinbeijing It's OK to add a protect here. Also, add a earily return if CGImage.width or CGImage.height == 0. Could you help to create a MR ?
We should consider this in such case as you said,but this condition is different from what I said. I try to create a MR.Thanks for your feedback.

[dreampiggy]

I check this again.

if tileTotalPixels < sourceTile.size.width

In theory, this can be true, however, which means, your original full image pixel width, shoud be at least 87382. Because we already have a check that limitBytes arg should be at least 1MB (1024 * 1024 Bytes), see code: https://github.com/SDWebImage/SDWebImage/blob/5.8.4/SDWebImage/Core/SDImageCoderHelper.m#L602

This is not possible in real world usage.

Calculation:

1. The tile width is equal to image pixel width;
2. The tile height set to 1;
3. The tile slice use 3;
4. Bytes per pixel use 4 (RGBA8888)
5. 1024 * 1024 / 4 / 3 / 1 = 87381.333333333333333

So, what's actually your problem is ? Can you provide a demo ?

[yyhinbeijing]

I check this again.

if tileTotalPixels < sourceTile.size.width

In theory, this can be true, however, which means, your original full image pixel width, shoud be at least 87382. Because we already have a check that limitBytes arg should be at least 1MB (1024 * 1024 Bytes), see code: https://github.com/SDWebImage/SDWebImage/blob/5.8.4/SDWebImage/Core/SDImageCoderHelper.m#L602

This is not possible in real world usage.

Calculation:

1. The tile width is equal to image pixel width;
2. The tile height set to 1;
3. The tile slice use 3;
4. Bytes per pixel use 4 (RGBA8888)
5. 1024 * 1024 / 4 / 3 / 1 = 87381.333333333333333

So, what's actually your problem is ? Can you provide a demo ?

You are right, thank you.
I use your this method by closing those codes

 if (![self shouldScaleDownImage:image limitBytes:bytes]) {
        return [self decodedImageWithImage:image];
    }

So any image can be handled.I reviewed this method "shouldScaleDownImage: limitBytes" several days ago.
I got the same conclusion from your codes ,that is:

if tileTotalPixels < sourceTile.size.width
tileTotalPixels must be 87381 which is very big
this is my blog: SDWebImage 图片压缩方法我的见解

I expect to get your reply, so I don't write this to your github issue.
You give me a same explanation, thank you.

[dreampiggy]

@yyhinbeijing Protect avaiable in #3067. If you really want.

Actually, maybe we can remove that limit to must supply at least 1MB bytes. So that this protect can have a better meaning.

[yyhinbeijing]

@yyhinbeijing Protect avaiable in #3067. If you really want.

Actually, maybe we can remove that limit to must supply at least 1MB bytes. So that this protect can have a better meaning.

I review #3067, but I don't understand who you say. What you mean is to remove the limit(at least 1MB bytes.) ,codes like this.

if (![self shouldScaleDownImage:image limitBytes:bytes]) {
        return [self decodedImageWithImage:image];
    }

and to add what I wrote? ，codes like this.

if (tileTotalPixels < sourceTile.size.width) {
            sourceTile.size.height = 1;
   }

If we do like this ,we can handle all images and make image bytes to the real limitBytes, avoiding return originImage and originBytes.
But many "limitBytes = 0" are in SDWebImage codes, so many issues will come up if we remove the limit.
So please consider ,thank you.

[dreampiggy]

If we do like this ,we can handle all images and make image bytes to the real limitBytes, avoiding return originImage and originBytes.

No.

This means, if I pass limitBytes to 1, you still at least allocate the image pixel width * 1 / 4 bytes in the memory. And final output image memory buffer bytes is larger than 1.

This not what limit means....Your memory allocation beyonds what user request.

The only difference between decodeImage and decodeAndScaleDownLargeImage, is whether to scale down pixel sizes...If this is important for user, we can have a refactory (behavior changes) in 5.9.0. The bugfix will apply for 5.8.x as well.

[yyhinbeijing]

This means, if I pass limitBytes to 1, you still at least allocate the image pixel width * 1 / 4 bytes in the memory. And final output image memory buffer bytes is larger than 1.

Wonderful ,thank you.

[dreampiggy]

If you want the accurate scale down, why not using UIImage+Transform ?
sd_resizedImageWithSize:scaleMode:

That does not have anything hack bahavior.

The decodeAndScaleDownLargeImage is from the real old history code. Actually.

[yyhinbeijing]

If you want the accurate scale down, why not using UIImage+Transform ?
sd_resizedImageWithSize:scaleMode:

That does not have anything hack bahavior.

The decodeAndScaleDownLargeImage is from the real old history code. Actually.

Ah,Method like sd_resizedImageWithSize:scaleMode: is used regularly ,which is more simple.
I am very curious about what method decodedAndScaledDownImageWithImage wrote, which is not frequently-used.
Maybe apple drawRect method core theory is like decodedAndScaledDownImageWithImage method contents, do you think so?
Thanks .

[dreampiggy]

I am very curious about what method decodedAndScaledDownImageWithImage wrote, which is not frequently-used.

In history. Someone copy paste the code from Apple's demo: https://developer.apple.com/library/archive/samplecode/LargeImageDownsizing/Introduction/Intro.html#//apple_ref/doc/uid/DTS40011173

Maybe at that time, they just want to scale it down. But however, if user pass you a 10000x10000 pixel UIImage, the simple logic (which create a CGBitmapContext with 10000x10000 and call CGContextDrawImage with full image, like current UIImage+Transform implementation) will cause OOM. Because it allocate double temp buffer.

The Apple demo code use Tile, to allocate only smaller bitmap buffer, and use foo-in loop to draw smaller tile line by line. This can help to avoid OOM.

I think, this decodeAndScaleDownLargeImage should do some refactory, to match its naming. Which should always scale down if your passed limitBytes is smaller than the current UIImage's bytes usage. Not something magic.