Bug 55259

Summary: [chromium] Abstract "pixels with a graphics context" into its own class
Product: WebKit Reporter: Adrienne Walker <enne>
Component: WebCore Misc.Assignee: Adrienne Walker <enne>
Status: RESOLVED FIXED    
Severity: Normal CC: antonm, aroben, enne, jamesr, kbr, nduca, vangelis
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 55388    
Bug Blocks:    
Attachments:
Description Flags
Patch jamesr: review+

Adrienne Walker
Reported 2011-02-25 14:02:06 PST
[chromium] Abstract "pixels with a graphics context" into its own class
Attachments
Patch (38.30 KB, patch)
2011-02-25 16:34 PST, Adrienne Walker
jamesr: review+
Adrienne Walker
Comment 1 2011-02-25 16:34:30 PST
Adrienne Walker
Comment 2 2011-02-25 16:39:35 PST
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 3 2011-02-25 17:24:24 PST
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
Comment 4 2011-02-28 10:03:39 PST
Adam Roben (:aroben)
Comment 5 2011-02-28 10:04:48 PST
(In reply to comment #0) > [chromium] Abstract "pixels with a graphics context" into its own class Isn't that what ImageBuffer is?
anton muhin
Comment 6 2011-02-28 10:34:25 PST
James Robinson
Comment 7 2011-02-28 11:05:17 PST
Windows has no idea what uint8_t is, apparently.
Adrienne Walker
Comment 8 2011-02-28 13:53:51 PST
(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
Comment 9 2011-02-28 16:46:27 PST
(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
Comment 10 2011-03-01 12:31:59 PST
(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
Comment 11 2011-03-01 18:35:56 PST
Note You need to log in before you can comment on or make changes to this bug.