Bug 104174 – Retry snapshots if they are too empty

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Jon Lee]

Evaluate the snapshot and try again if it's almost a blank image.

[Radar WebKit Bug Importer]

<rdar://problem/12820146>

[Jon Lee]

Created attachment 177859 [details]
Patch

[Jon Lee]

Comment on attachment 177859 [details]
Patch

Oops, wrong bug. :-/

[Jon Lee]

Created attachment 177876 [details]
Patch

[Early Warning System Bot]

Comment on attachment 177876 [details]
Patch

Attachment 177876 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15152616

[Jon Lee]

Created attachment 177886 [details]
Patch

Comment on attachment 177876 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177876&action=review

> Source/WebCore/platform/graphics/mac/ImageMac.mm:128
> +    size_t bpr = CGImageGetBytesPerRow(image);

Please use bytesPerRow.

> Source/WebCore/platform/graphics/mac/ImageMac.mm:131
> +    RetainPtr<CFDataRef> provider = adoptCF(CGDataProviderCopyData(CGImageGetDataProvider(image)));
> +    const UInt8* data = CFDataGetBytePtr(provider.get());

Does this really guarantee that we get planar RGBA data? If an image is created with a PNG data provider, won’t this return that PNG data?

> Source/WebCore/platform/graphics/mac/ImageMac.mm:136
> +    const int sampleRows = 7, sampleCols = 7;

Please split this into two statements. Could these be unsigned?

> Source/WebCore/platform/graphics/mac/ImageMac.mm:143
> +    const float strideW = (float)(width - 1) / (sampleCols - 1);
> +    const float strideH = (float)(height - 1) / (sampleRows - 1);

We normally prefer static_cast to C-style casts even for POD types. I’d spell out Width and Height here.

> Source/WebCore/platform/graphics/mac/ImageMac.mm:144
> +    float samples[sampleRows * sampleCols];

Why not make this a two-dimensional array?

[EFL EWS Bot]

Comment on attachment 177886 [details]
Patch

Attachment 177886 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15151786

[Jon Lee]

Comment on attachment 177876 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177876&action=review

>> Source/WebCore/platform/graphics/mac/ImageMac.mm:128
>> +    size_t bpr = CGImageGetBytesPerRow(image);
> 
> Please use bytesPerRow.

Done.

>> Source/WebCore/platform/graphics/mac/ImageMac.mm:131
>> +    const UInt8* data = CFDataGetBytePtr(provider.get());
> 
> Does this really guarantee that we get planar RGBA data? If an image is created with a PNG data provider, won’t this return that PNG data?

I thought it did, because CGImageRef is bitmap. The documentation seems to indicate that it's the decoded pixel data, but I could be wrong. Technical note QA1509 states "The pixel data returned by CGDataProviderCopyData has not been color matched and is in the format that the image is in, as described by the various CGImageGet functions", which does not include flags like the raw data. Does this sound right?

>> Source/WebCore/platform/graphics/mac/ImageMac.mm:136
>> +    const int sampleRows = 7, sampleCols = 7;
> 
> Please split this into two statements. Could these be unsigned?

Done and done.

>> Source/WebCore/platform/graphics/mac/ImageMac.mm:144
>> +    float samples[sampleRows * sampleCols];
> 
> Why not make this a two-dimensional array?

Done, and refactored.

[Jon Lee]

Created attachment 177966 [details]
Patch

(In reply to comment #9)
> I thought it did, because CGImageRef is bitmap. The documentation seems to indicate that it's the decoded pixel data, but I could be wrong. Technical note QA1509 states "The pixel data returned by CGDataProviderCopyData has not been color matched and is in the format that the image is in, as described by the various CGImageGet functions", which does not include flags like the raw data. Does this sound right?

Thanks for the reference. It’s clear that this will return pixel data. It’s not clear if, for example, the channels will be in RGBA order.

[Jon Lee]

Created attachment 178036 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 178036 [details]
Patch

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

[Jon Lee]

Created attachment 178086 [details]
Patch

[Jon Lee]

Committed r136905: <http://trac.webkit.org/changeset/136905>