Bug 37621 - Chromium/mac: Image thumbnails don't work for indexed images
Summary: Chromium/mac: Image thumbnails don't work for indexed images
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nico Weber
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-14 16:13 PDT by Nico Weber
Modified: 2010-04-15 16:59 PDT (History)
2 users (show)

See Also:


Attachments
Patch (2.32 KB, patch)
2010-04-14 16:18 PDT, Nico Weber
no flags Details | Formatted Diff | Diff
Patch (2.53 KB, patch)
2010-04-15 13:01 PDT, Nico Weber
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nico Weber 2010-04-14 16:13:11 PDT
WebKit bug for http://crbug.com/41512
Comment 1 Nico Weber 2010-04-14 16:18:01 PDT
Created attachment 53382 [details]
Patch
Comment 2 Nico Weber 2010-04-14 16:19:29 PDT
The CoreGraphics docs say that CGCreateContext doesn't work with indexed color spaces. Always using RGB is probably better for CMYK images too.
Comment 3 Avi Drissman 2010-04-14 16:26:24 PDT
> Index: WebCore/platform/chromium/DragImageChromiumMac.cpp
> ===================================================================
> +    CGColorSpaceRef deviceRGB = CGColorSpaceCreateDeviceRGB();

RetainPtr?

> +    CGContextRef context = CGBitmapContextCreate(0, width, height, 8, width * 4, deviceRGB, kCGImageAlphaPremultipliedFirst | kCGBitmapByteOrder32Host);
> +    CGColorSpaceRelease(deviceRGB);
> +

BTW, the comment about CGCreateContext not working for indexed color spaces is IMHO comment-worthy in the code. Just me.
Comment 4 Avi Drissman 2010-04-15 07:50:08 PDT
> Index: WebCore/platform/chromium/DragImageChromiumMac.cpp
> ===================================================================
> +    CGColorSpaceRef deviceRGB = CGColorSpaceCreateDeviceRGB();

BTW, CGColorSpaceCreateDeviceRGB hasn't created a device RGB color space since 10.3, so the variable name, technically speaking, is inaccurate. Since CGColorSpaceCreateDeviceRGB() == CGColorSpaceCreateWithName(kCGColorSpaceGenericRGB), you should switch to the latter as it's clearer what's it's doing.
Comment 5 Nico Weber 2010-04-15 13:01:52 PDT
Created attachment 53464 [details]
Patch

Did the RetainPtr thing and even the CGColorSpaceCreateWithName(kCGColorSpaceGenericRGB) suggestion.

I didn't add a comment though, because just saying that createcontext doesn't work with indexed images misses the point. We really want RGB here, and not whatever weird color space the image might be in.

(Also, the WebKit way apparently is to do `svn annotate` and then look at revision log/bug afaiu).
Comment 6 Avi Drissman 2010-04-15 13:08:56 PDT
(In reply to comment #5)
> (Also, the WebKit way apparently is to do `svn annotate` and then look at
> revision log/bug afaiu).

Fine with me.

Code LGTM.
Comment 7 Dimitri Glazkov (Google) 2010-04-15 13:10:47 PDT
Comment on attachment 53464 [details]
Patch

motownavi can't be wrong. rs=me.
Comment 8 WebKit Commit Bot 2010-04-15 16:58:58 PDT
Comment on attachment 53464 [details]
Patch

Clearing flags on attachment: 53464

Committed r57687: <http://trac.webkit.org/changeset/57687>
Comment 9 WebKit Commit Bot 2010-04-15 16:59:02 PDT
All reviewed patches have been landed.  Closing bug.