[webkit-reviews] review denied: [Bug 104174] Retry snapshots if they are too empty : [Attachment 178036] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Jon Lee
<jonlee at apple.com>'s request for review:
Bug 104174: Retry snapshots if they are too empty
https://bugs.webkit.org/show_bug.cgi?id=104174

Attachment 178036: Patch
https://bugs.webkit.org/attachment.cgi?id=178036&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=178036&action=review


> Source/WebCore/platform/graphics/mac/ImageMac.mm:180
> +bool BitmapImage::isAlmostSolidColor()
> +{
> +    ASSERT(frameCount() == 1);
> +
> +    CGImageRef image = frameAtIndex(0);
> +    ASSERT(CGImageGetBitsPerComponent(image) == 8);
> +
> +    CGBitmapInfo imageInfo = CGImageGetBitmapInfo(image);
> +    if (!(imageInfo & kCGBitmapByteOrder32Little) || (imageInfo &
kCGBitmapAlphaInfoMask) != kCGImageAlphaPremultipliedFirst) {
> +	   // FIXME: Consider being able to handle other pixel formats.
> +	   ASSERT_NOT_REACHED();
> +	   return false;
> +    }
> +
> +    size_t width = CGImageGetWidth(image);
> +    size_t height = CGImageGetHeight(image);
> +    size_t bytesPerRow = CGImageGetBytesPerRow(image);
> +
> +    RetainPtr<CFDataRef> provider =
adoptCF(CGDataProviderCopyData(CGImageGetDataProvider(image)));
> +    const UInt8* data = CFDataGetBytePtr(provider.get());
> +
> +    // Overlay a grid of sampling dots on top of a grayscale version of the
image.
> +    // For the interior points, calculate the difference in luminance among
the sample point
> +    // and its surrounds points, scaled by transparency.
> +    const unsigned sampleRows = 7;
> +    const unsigned sampleCols = 7;
> +    // FIXME: Refine the proper number of samples, and accommodate different
aspect ratios.
> +    if (width < sampleCols || height < sampleRows)
> +	   return false;
> +
> +    // Ensure that the last row/column land on the image perimeter.
> +    const float strideWidth = static_cast<float>(width - 1) / (sampleCols -
1);
> +    const float strideHeight = static_cast<float>(height - 1) / (sampleRows
- 1);
> +    float samples[sampleRows][sampleCols];
> +
> +    // Find the luminance of the sample points.
> +    float y = 0;
> +    const UInt8* row = data;
> +    for (unsigned i = 0; i < sampleRows; ++i) {
> +	   float x = 0;
> +	   for (unsigned j = 0; j < sampleCols; ++j) {
> +	       const UInt8* p0 = row + (static_cast<int>(x + .5)) * 4;
> +	       // R G B A
> +	       samples[i][j] = (0.2125 * *p0 + 0.7154 * *(p0+1) + 0.0721 *
*(p0+2)) * *(p0+3) / 255;
> +	       x += strideWidth;
> +	   }
> +	   y += strideHeight;
> +	   row = data + (static_cast<int>(y + .5)) * bytesPerRow;
> +    }
> +
> +    // Determine the image score.
> +    float accumScore = 0;
> +    for (unsigned i = 1; i < sampleRows - 1; ++i) {
> +	   for (unsigned j = 1; j < sampleCols - 1; ++j) {
> +	       float diff = samples[i - 1][j] + samples[i + 1][j] +
samples[i][j - 1] + samples[i][j + 1] - 4 * samples[i][j];
> +	       accumScore += diff * diff;
> +	   }
> +    }
> +
> +    // The score for a given sample can be within the range of 0 and 255^2.
> +    return accumScore < 2500 * (sampleRows - 2) * (sampleCols - 2);
> +}

I'm not sure that a heuristic like this belongs in BitmapImage. I think it
should be out in plugin code (or even platform code).