Bug 31321

Created attachment 43218 [details] Patch

Comment on attachment 43218 [details] Patch > +static CGImageRef imageWithColorSpace(CGImageRef originalImage, ColorSpace colorSpace) > +{ > + static CGColorSpaceRef deviceSpace = CGColorSpaceCreateDeviceRGB(); > + CGColorSpaceRef originalColorSpace = CGImageGetColorSpace(originalImage); Note that CGImageGetColorSpace can return null for PDF images. > + // If the image already has a (non-device) color space, we don't want to > + // override it, so return. > + if (originalColorSpace != deviceSpace) This needs to be a call to !CFEqual().

Comment on attachment 43218 [details] Patch Generally speaking, having a colorSpace argument that is used only when an image is untagged seems subtle. I think we need a name for this argument other than just "colorSpace", since it's only used for an image that has no intrinsic color space. This applies to the GraphicsContext member functions as well as the Image member functions. > if (mayFillWithSolidColor()) { > - fillWithSolidColor(ctxt, dstRect, solidColor(), op); > + fillWithSolidColor(ctxt, dstRect, solidColor(), colorSpace, op); > return; > } I suspect that mayFillWithSolidColor returns true even in cases where the image is tagged with a color space. We need to test these cases. I suspect that we will do the wrong thing with a single pixel image that has a color space in it. I don't think the bug is necessarily new to this patch, although there is a chance this has been made worse by introducing the sRGB feature. > - static void fillWithSolidColor(GraphicsContext* ctxt, const FloatRect& dstRect, const Color& color, CompositeOperator op); > + static void fillWithSolidColor(GraphicsContext* ctxt, const FloatRect& dstRect, const Color& color, ColorSpace, CompositeOperator op); As long as you are modifying this line, you should remove the unhelpful argument names, "ctxt", "color", and "op". > #if PLATFORM(WIN) > virtual void drawFrameMatchingSourceSize(GraphicsContext*, const FloatRect& dstRect, const IntSize& srcSize, CompositeOperator) { } > #endif Seems this needs a ColorSpace argument too. > - virtual void draw(GraphicsContext*, const FloatRect& dstRect, const FloatRect& srcRect, CompositeOperator) = 0; > - void drawTiled(GraphicsContext*, const FloatRect& dstRect, const FloatPoint& srcPoint, const FloatSize& tileSize, CompositeOperator); > - void drawTiled(GraphicsContext*, const FloatRect& dstRect, const FloatRect& srcRect, TileRule hRule, TileRule vRule, CompositeOperator); > + virtual void draw(GraphicsContext*, const FloatRect& dstRect, const FloatRect& srcRect, ColorSpace, CompositeOperator) = 0; > + void drawTiled(GraphicsContext*, const FloatRect& dstRect, const FloatPoint& srcPoint, const FloatSize& tileSize, ColorSpace, CompositeOperator); > + void drawTiled(GraphicsContext*, const FloatRect& dstRect, const FloatRect& srcRect, TileRule hRule, TileRule vRule, ColorSpace, CompositeOperator); To me, the purpose of the ColorSpace argument is not at all clear. The fact that it's a color space to use with untagged images is a tricky concept, I think, and needs a comment and perhaps an argument name as well. > virtual void drawPattern(GraphicsContext*, const FloatRect& srcRect, const TransformationMatrix& patternTransform, > - const FloatPoint& phase, CompositeOperator, const FloatRect& destRect); > + const FloatPoint& phase, ColorSpace, CompositeOperator, const FloatRect& destRect); Same here. > + static CGColorSpaceRef deviceSpace = CGColorSpaceCreateDeviceRGB(); Isn't there already a helper function that holds a global copy of the device color space? > + CGColorSpaceRef originalColorSpace = CGImageGetColorSpace(originalImage); > + > + // If the image already has a (non-device) color space, we don't want to > + // override it, so return. > + if (originalColorSpace != deviceSpace) > + return 0; Can you really detect an untagged image this way? What about an image with a color space of null (a "mask image")? Do untagged images really return something that is == the result of CGColorSpaceCreateDeviceRGB? Is this a guaranteed part of the CoreGraphics or ImageIO API? Do you need to compare with CFEqual instead? > + switch (colorSpace) { > + case DeviceColorSpace: > + return 0; > + case sRGBColorSpace: { > +#if PLATFORM(WIN) || defined(BUILDING_ON_TIGER) > + return 0; > +#else > + static CGColorSpaceRef sRGBSpace = CGColorSpaceCreateWithName(kCGColorSpaceSRGB); > + return CGImageCreateCopyWithColorSpace(originalImage, sRGBSpace); > +#endif This will leak a CGImageRef each time. The function needs to return a RetainPtr, or be renamed to have the word "create" in its name, and callers need to adopt or release the returned pointer. It also seems likely to be inefficient to create a new image every time. We probably need to cache the color-spaced image in a data member rather than generating it every time we draw. But I could be wrong about that. > + default: > + ASSERT_NOT_REACHED(); > + return 0; > + } It's better to put this code outside the switch statement instead of in a default. That way we'll get a warning if we leave anything out. Is there any way to make the regression tests so they would fail even without doing pixel tests? I'm not sure these tests are helpful enough if only the pixel test results indicate the failure. review- because of the storage leak

I am about to attach an updated patch, but I will address comments first. (In reply to comment #3) > (From update of attachment 43218 [details]) > Generally speaking, having a colorSpace argument that is used only when an > image is untagged seems subtle. I think we need a name for this argument other > than just "colorSpace", since it's only used for an image that has no intrinsic > color space. This applies to the GraphicsContext member functions as well as > the Image member functions. > I agree, and this was bothering me while I was working on the patch. I just wasn't sure how to deal with it. I like your idea of calling the variable something other than "colorSpace," so this new patch names it "styleColorSpace." I am open to other ideas if you have them. I also added a comment to Image.h, though perhaps I should add one to GraphicsContext.h as well? > > if (mayFillWithSolidColor()) { > > - fillWithSolidColor(ctxt, dstRect, solidColor(), op); > > + fillWithSolidColor(ctxt, dstRect, solidColor(), colorSpace, op); > > return; > > } > > I suspect that mayFillWithSolidColor returns true even in cases where the image > is tagged with a color space. We need to test these cases. I suspect that we > will do the wrong thing with a single pixel image that has a color space in it. > I don't think the bug is necessarily new to this patch, although there is a > chance this has been made worse by introducing the sRGB feature. > I don't understand this. > > - static void fillWithSolidColor(GraphicsContext* ctxt, const FloatRect& dstRect, const Color& color, CompositeOperator op); > > + static void fillWithSolidColor(GraphicsContext* ctxt, const FloatRect& dstRect, const Color& color, ColorSpace, CompositeOperator op); > > As long as you are modifying this line, you should remove the unhelpful > argument names, "ctxt", "color", and "op". > Fixed. > > #if PLATFORM(WIN) > > virtual void drawFrameMatchingSourceSize(GraphicsContext*, const FloatRect& dstRect, const IntSize& srcSize, CompositeOperator) { } > > #endif > > Seems this needs a ColorSpace argument too. Fixed. > > > - virtual void draw(GraphicsContext*, const FloatRect& dstRect, const FloatRect& srcRect, CompositeOperator) = 0; > > - void drawTiled(GraphicsContext*, const FloatRect& dstRect, const FloatPoint& srcPoint, const FloatSize& tileSize, CompositeOperator); > > - void drawTiled(GraphicsContext*, const FloatRect& dstRect, const FloatRect& srcRect, TileRule hRule, TileRule vRule, CompositeOperator); > > + virtual void draw(GraphicsContext*, const FloatRect& dstRect, const FloatRect& srcRect, ColorSpace, CompositeOperator) = 0; > > + void drawTiled(GraphicsContext*, const FloatRect& dstRect, const FloatPoint& srcPoint, const FloatSize& tileSize, ColorSpace, CompositeOperator); > > + void drawTiled(GraphicsContext*, const FloatRect& dstRect, const FloatRect& srcRect, TileRule hRule, TileRule vRule, ColorSpace, CompositeOperator); > > To me, the purpose of the ColorSpace argument is not at all clear. The fact > that it's a color space to use with untagged images is a tricky concept, I > think, and needs a comment and perhaps an argument name as well. > Fixed. > > virtual void drawPattern(GraphicsContext*, const FloatRect& srcRect, const TransformationMatrix& patternTransform, > > - const FloatPoint& phase, CompositeOperator, const FloatRect& destRect); > > + const FloatPoint& phase, ColorSpace, CompositeOperator, const FloatRect& destRect); > > Same here. Fixed. > > > + static CGColorSpaceRef deviceSpace = CGColorSpaceCreateDeviceRGB(); > > Isn't there already a helper function that holds a global copy of the device > color space? > I made that helper function in GraphicsContextCG.cpp. I couldn't think of a good way to share it…I could put it in GraphicsContext.h, but then I would either have to wrap it in a CG-only if-def or create a PlatformColorSpace ifdef or something like that. Because I had so many questions, I decided just to use the CG call again rather than my new helper since we use that call a bunch of places in the code already without a central helper. What do you think is best? I am open to adjusting this, but unsure how to proceed. > > + CGColorSpaceRef originalColorSpace = CGImageGetColorSpace(originalImage); > > + > > + // If the image already has a (non-device) color space, we don't want to > > + // override it, so return. > > + if (originalColorSpace != deviceSpace) > > + return 0; > > Can you really detect an untagged image this way? What about an image with a > color space of null (a "mask image")? Do untagged images really return > something that is == the result of CGColorSpaceCreateDeviceRGB? Is this a > guaranteed part of the CoreGraphics or ImageIO API? Do you need to compare with > CFEqual instead? You can really detect and untagged image this way. I have not had any test cases with a null color space. The == does really work. However, since I don't know whether or not this is guaranteed by the CoreGraphics API and since Simon made the same suggestion above, I switched to using CFEqual. It also works, but I assure you that == does as well. I have test cases, and stepped through it in the debugger just to be certain. > > > + switch (colorSpace) { > > + case DeviceColorSpace: > > + return 0; > > + case sRGBColorSpace: { > > +#if PLATFORM(WIN) || defined(BUILDING_ON_TIGER) > > + return 0; > > +#else > > + static CGColorSpaceRef sRGBSpace = CGColorSpaceCreateWithName(kCGColorSpaceSRGB); > > + return CGImageCreateCopyWithColorSpace(originalImage, sRGBSpace); > > +#endif > > This will leak a CGImageRef each time. The function needs to return a > RetainPtr, or be renamed to have the word "create" in its name, and callers > need to adopt or release the returned pointer. > > It also seems likely to be inefficient to create a new image every time. We > probably need to cache the color-spaced image in a data member rather than > generating it every time we draw. But I could be wrong about that. I don't think my patch had any leaks because I think I was correctly dealing with the new image at all of the call sites of this static helper function. (There are only two call sites.) Just to make sure, I did switch it over to a RetainPtr model. > > > + default: > > + ASSERT_NOT_REACHED(); > > + return 0; > > + } > > It's better to put this code outside the switch statement instead of in a > default. That way we'll get a warning if we leave anything out. > Fixed. > Is there any way to make the regression tests so they would fail even without > doing pixel tests? I'm not sure these tests are helpful enough if only the > pixel test results indicate the failure. > I wish there was a way to test this within layout tests, but I don't know of one. I was hoping our color information would be equivalent to Digicolorometer information, but it's not. Not sure how to improve this.

Created attachment 43321 [details] New patch New patch! See comments above for details on how I addressed Darin's and Simon's comments.

(In reply to comment #4) > I like your idea of calling the variable > something other than "colorSpace," so this new patch names it > "styleColorSpace." Sounds better. > I am open to other ideas if you have them. I also added a > comment to Image.h, though perhaps I should add one to GraphicsContext.h as > well? Probably, yes. > > > if (mayFillWithSolidColor()) { > > > - fillWithSolidColor(ctxt, dstRect, solidColor(), op); > > > + fillWithSolidColor(ctxt, dstRect, solidColor(), colorSpace, op); > > > return; > > > } > > > > I suspect that mayFillWithSolidColor returns true even in cases where the image > > is tagged with a color space. We need to test these cases. I suspect that we > > will do the wrong thing with a single pixel image that has a color space in it. > > I don't think the bug is necessarily new to this patch, although there is a > > chance this has been made worse by introducing the sRGB feature. > > I don't understand this. Lets talk about it in person. > > > + static CGColorSpaceRef deviceSpace = CGColorSpaceCreateDeviceRGB(); > > > > Isn't there already a helper function that holds a global copy of the device > > color space? > > > > I made that helper function in GraphicsContextCG.cpp. I couldn't think of a > good way to share it…I could put it in GraphicsContext.h, but then I would > either have to wrap it in a CG-only if-def or create a PlatformColorSpace ifdef > or something like that. Because I had so many questions, I decided just to use > the CG call again rather than my new helper since we use that call a bunch of > places in the code already without a central helper. What do you think is best? > I am open to adjusting this, but unsure how to proceed. Maybe GraphicsContextPlatformPrivateCG.h is a possible place to put this. I also think we could consider renaming that to just GraphicsContextCG.h. Or make a new header file in the platform/graphics/cg subdirectory with a CG.h suffix in its name. > > > + CGColorSpaceRef originalColorSpace = CGImageGetColorSpace(originalImage); > > > + > > > + // If the image already has a (non-device) color space, we don't want to > > > + // override it, so return. > > > + if (originalColorSpace != deviceSpace) > > > + return 0; > > > > Can you really detect an untagged image this way? What about an image with a > > color space of null (a "mask image")? Do untagged images really return > > something that is == the result of CGColorSpaceCreateDeviceRGB? Is this a > > guaranteed part of the CoreGraphics or ImageIO API? Do you need to compare with > > CFEqual instead? > > You can really detect and untagged image this way. I have not had any test > cases with a null color space. The == does really work. However, since I don't > know whether or not this is guaranteed by the CoreGraphics API and since Simon > made the same suggestion above, I switched to using CFEqual. It also works, but > I assure you that == does as well. I have test cases, and stepped through it in > the debugger just to be certain. Switching to CFEqual has two downsides. 1) Slower. 2) This will now crash if CGImageGetColorSpace returns 0. But if it’s guaranteed to never return 0 then maybe we’re OK on this. > I don't think my patch had any leaks because I think I was correctly dealing > with the new image at all of the call sites of this static helper function. > (There are only two call sites.) Just to make sure, I did switch it over to a > RetainPtr model. Another option would have been to use the word "create" or "copy" in the function name and follow the create/copy rule from Core Foundation. > > Is there any way to make the regression tests so they would fail even without > > doing pixel tests? I'm not sure these tests are helpful enough if only the > > pixel test results indicate the failure. > > I wish there was a way to test this within layout tests, but I don't know of > one. I was hoping our color information would be equivalent to Digicolorometer > information, but it's not. Not sure how to improve this. Maybe we can add something to the layout test controller to let you read pixels out of the window. Or something that copies the contents of the window into a canvas element.

> > > > + static CGColorSpaceRef deviceSpace = CGColorSpaceCreateDeviceRGB(); > > > > > > Isn't there already a helper function that holds a global copy of the device > > > color space? > > > > > > > I made that helper function in GraphicsContextCG.cpp. I couldn't think of a > > good way to share it…I could put it in GraphicsContext.h, but then I would > > either have to wrap it in a CG-only if-def or create a PlatformColorSpace ifdef > > or something like that. Because I had so many questions, I decided just to use > > the CG call again rather than my new helper since we use that call a bunch of > > places in the code already without a central helper. What do you think is best? > > I am open to adjusting this, but unsure how to proceed. > > Maybe GraphicsContextPlatformPrivateCG.h is a possible place to put this. I > also think we could consider renaming that to just GraphicsContextCG.h. Or make > a new header file in the platform/graphics/cg subdirectory with a CG.h suffix > in its name. I moved these static helpers to GraphicsContextPlatformPrivateCG.h, and that is working well.

Comment on attachment 43321 [details] New patch I'd really like to see the color space be a default argument in the GraphicsContext functions. I'd love to see that change for say fillRect and any others you may already have done also. If you look at most of your calls, you're almost always passing in DeviceColorSpace... if you put that argument at the end of the functions and made it default, then it could be omitted, and you wouldn't have to patch a bunch of call sites.

Although if you think people need to be thinking about color spaces all the time, I'd accept that as an argument for why you wouldn't want to make it a default argument...

Comment on attachment 43321 [details] New patch > + // Adjust the color space. > + if (RetainPtr<CGImageRef> colorSpaceImage = imageWithColorSpace(image, styleColorSpace)) > + image = colorSpaceImage.releaseRef(); In at least some cases this will lead to a leak. For example, if shouldUseSubimage is true, then we will leak the subimage because we will overwrite "image" with a pointer to it. If shouldUseSubimage is false, then we will leak the result of imageWithColorSpace. I think we need to use RetainPtr to get this right, although it will mean extra retains in the no subimage, no color correction case. > + // Adjust the color space. > + if (RetainPtr<CGImageRef> colorSpaceImage = imageWithColorSpace(subImage.get(), styleColorSpace)) > + subImage = colorSpaceImage.releaseRef(); Probably the same problem here.

Comment on attachment 43321 [details] New patch review- because of the leaks

Created attachment 43521 [details] Newer Patch Here's a new patch that fixes the leak Darin found. Dave, about making defaults for the ColorSpace parameters: a lot of the call sites that use DeviceColorSpace right now represent bugs. For example, all of the spots in canvas code that send DeviceColorSpace need to be reconsidered, as do the Windows call sites, and the call sites in other ports for that matter. Right now, while support for this property is still under development, I think it's important for callers to think of ColorSpace whenever they call the functions. Once we feel convinced we have caught and fixed all of the bugs, maybe then we could enable defaults for the callers that really don't need to think about color space. What do you think about that?

Yeah that sounds fine. I suspected after I posted the comment that we probably want to keep it explicit so people have to think about it.

Comment on attachment 43521 [details] Newer Patch > +inline static CGColorSpaceRef deviceRGBColorSpaceRef() > +{ > + static CGColorSpaceRef deviceSpace = CGColorSpaceCreateDeviceRGB(); > + return deviceSpace; > +} > + > +inline static CGColorSpaceRef sRGBColorSpaceRef() > +{ > + // FIXME: Windows should be able to use kCGColorSpaceSRGB, this is tracked by http://webkit.org/b/31363. > +#if PLATFORM(WIN) || defined(BUILDING_ON_TIGER) > + return deviceRGBColorSpaceRef(); > +#else > + static CGColorSpaceRef sRGBSpace = CGColorSpaceCreateWithName(kCGColorSpaceSRGB); > + return sRGBSpace; > +#endif > +} These should not be marked static, since they are in header files. If marked static, there will be multiple copies of the global variables. Since not marked static, there will not be. Probably should mark these as FIXME, since ideally they would go in GraphicsContextCG.h if it existed. > +static RetainPtr<CGImageRef> imageWithColorSpace(CGImageRef originalImage, ColorSpace colorSpace) > +{ > + CGColorSpaceRef originalColorSpace = CGImageGetColorSpace(originalImage); > + > + // If the image already has a (non-device) color space, we don't want to > + // override it, so return. > + if (!originalColorSpace || !CFEqual(originalColorSpace, deviceRGBColorSpaceRef())) > + return 0; If you changed this to return originalImage instead of 0 whenever color change is not needed, then the callers would all be simplified. > + // Adjust the color space. > + if (RetainPtr<CGImageRef> colorSpaceImage = imageWithColorSpace(image.get(), styleColorSpace)) > + image = colorSpaceImage; Here's an example. You could eliminate the local variable and just say: image = imageWithColorSpace(image.get(), styleColorSpace); > - if (shouldUseSubimage) I suspect we no longer need shouldUseSubimage to be in a local variable, but it might still help clarity to keep it so I'm not sure it should be removed. r=me I really want better regression tests that don't rely on pixel results.

Thanks Darin! I addressed your comments before checking in. Fixed with revision 51212.