Bug 55259 – [chromium] Abstract "pixels with a graphics context" into its own class

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

[Adrienne Walker]

[chromium] Abstract "pixels with a graphics context" into its own class

[Adrienne Walker]

Created attachment 83900 [details]
Patch

[Adrienne Walker]

This refactoring patch is working towards tiling layers.  Along with the obvious abstraction of the Skia/Cg logic, it splits out the update/draw logic from the LayerTilerChromium.cpp into separate functions.  Having PlatformCanvas and PlatformImage be its own class will make it much easier to merge the common logic of LayerTilerChromium, ContentLayerChromium, and ImageLayerChromium.

[James Robinson]

Comment on attachment 83900 [details]
Patch

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

Looks awesome!  Usual sort of collection of nitpicks

> Source/WebCore/platform/graphics/chromium/PlatformCanvas.cpp:2
> + * Copyright (C) 2010 Google Inc. All rights reserved.

times they are a'changing. 2011 :)

> Source/WebCore/platform/graphics/chromium/PlatformCanvas.h:32
> +#if USE(SKIA)
> +namespace skia { class PlatformCanvas; }
> +class SkBitmap;
> +#endif

nit: move this after the #includes

> Source/WebCore/platform/graphics/chromium/PlatformCanvas.h:64
> +        AutoLocker(PlatformCanvas*);

nit: explicit

> Source/WebCore/platform/graphics/chromium/PlatformCanvas.h:80
> +        Painter(PlatformCanvas*);

nit: explicit

[Adrienne Walker]

Committed r79877: <http://trac.webkit.org/changeset/79877>

[Adam Roben (:aroben)]

(In reply to comment #0)
> [chromium] Abstract "pixels with a graphics context" into its own class

Isn't that what ImageBuffer is?

[anton muhin]

Sample broken build log: http://chromesshgw.corp.google.com/i/chromium.webkit/builders/Webkit%20Win%20Builder/builds/4114/steps/compile/logs/stdio

Grep for PlatformCanvas.h

[James Robinson]

Windows has no idea what uint8_t is, apparently.

[Adrienne Walker]

(In reply to comment #6)
> Sample broken build log: http://chromesshgw.corp.google.com/i/chromium.webkit/builders/Webkit%20Win%20Builder/builds/4114/steps/compile/logs/stdio
> 
> Grep for PlatformCanvas.h

Thanks for rolling that out for me.  It's unfortunate that the win build check above didn't catch that.

(In reply to comment #5)
> (In reply to comment #0)
> > [chromium] Abstract "pixels with a graphics context" into its own class
> 
> Isn't that what ImageBuffer is?

Thanks for pointing this out.  I need to look into it more closely at it to see if it solves the same needs.  At the very least, using ImageBuffer as-is would involve an extra pixel-by-pixel copy operation for every texture upload, so it will need some additional APIs first.

[James Robinson]

(In reply to comment #8)
> (In reply to comment #6)
> > Sample broken build log: http://chromesshgw.corp.google.com/i/chromium.webkit/builders/Webkit%20Win%20Builder/builds/4114/steps/compile/logs/stdio
> > 
> > Grep for PlatformCanvas.h
> 
> Thanks for rolling that out for me.  It's unfortunate that the win build check above didn't catch that.

We have a win EWS bot but no chromium-win EWS :'(

> 
> (In reply to comment #5)
> > (In reply to comment #0)
> > > [chromium] Abstract "pixels with a graphics context" into its own class
> > 
> > Isn't that what ImageBuffer is?
> 
> Thanks for pointing this out.  I need to look into it more closely at it to see if it solves the same needs.  At the very least, using ImageBuffer as-is would involve an extra pixel-by-pixel copy operation for every texture upload, so it will need some additional APIs first.

Not necessarily, we can probably grab the bits out of the GraphicsContext in a platform-specific way in platform-specific code as needed.

[Adrienne Walker]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #6)
> > > Sample broken build log: http://chromesshgw.corp.google.com/i/chromium.webkit/builders/Webkit%20Win%20Builder/builds/4114/steps/compile/logs/stdio
> > > 
> > > Grep for PlatformCanvas.h
> > 
> > Thanks for rolling that out for me.  It's unfortunate that the win build check above didn't catch that.
> 
> We have a win EWS bot but no chromium-win EWS :'(

I still am not sure how stdint.h is magically included in one but not the other.

> > 
> > (In reply to comment #5)
> > > (In reply to comment #0)
> > > > [chromium] Abstract "pixels with a graphics context" into its own class
> > > 
> > > Isn't that what ImageBuffer is?
> > 
> > Thanks for pointing this out.  I need to look into it more closely at it to see if it solves the same needs.  At the very least, using ImageBuffer as-is would involve an extra pixel-by-pixel copy operation for every texture upload, so it will need some additional APIs first.
> 
> Not necessarily, we can probably grab the bits out of the GraphicsContext in a platform-specific way in platform-specific code as needed.

Looking at ImageBuffer, the code I would add to to it is pretty much the contents of this patch, except I would also (likely) need to add an implementation of these APIs to all the other ImageBufferData implementations (other than Skia and Cg).  I'm feeling inclined to commit this as-is because it significantly cleans up the compositor code and is just a refactoring of what already exists.

It's possible that there's no real performance win from the existing code and thus no benefit to having an extra path or extra classes.  However, at this point we don't really have good performance measurements of the compositor speed (and its performance characteristics will change drastically in the coming months as it becomes multithreaded).  In my mind, further cleanup or potential merging with ImageBuffer should really be left for a future patch when we can evaluate it with better data.

[Adrienne Walker]

Committed r80081: <http://trac.webkit.org/changeset/80081>