Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23545017:
Create a semi-stable API for capturing the state of an SkCanvas and reconstructing that state acros… (Closed)
|
Created:
7 years, 3 months ago by djsollen Modified:
7 years, 3 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionCreate a semi-stable API for capturing the state of an SkCanvas and reconstructing that state across different versions of Skia.
R=joth@chromium.org, mtklein@google.com, reed@google.com, scroggo@google.com
Committed: https://code.google.com/p/skia/source/detail?r=11010
Committed: https://code.google.com/p/skia/source/detail?r=11013
Patch Set 1 #
Total comments: 86
Patch Set 2 : addressing comments #
Total comments: 4
Patch Set 3 : fixing nits #
Total comments: 2
Patch Set 4 : rebase #
Total comments: 3
Messages
Total messages: 17 (0 generated)
https://codereview.chromium.org/23545017/diff/1/gm/canvasstate.cpp File gm/canvasstate.cpp (right): https://codereview.chromium.org/23545017/diff/1/gm/canvasstate.cpp#newcode14 gm/canvasstate.cpp:14: #include "SkCanvasUtils.h" need to move this inline with other includes. https://codereview.chromium.org/23545017/diff/1/gm/canvasstate.cpp#newcode161 gm/canvasstate.cpp:161: SkCanvas* newCanvas = canvas; need to add comment as to why we do this here.
lgtm, but please wait for others
can we put the new header in src/utils for now, so it ain't exactly public? Just a minor thought
https://codereview.chromium.org/23545017/diff/1/gm/canvasstate.cpp File gm/canvasstate.cpp (right): https://codereview.chromium.org/23545017/diff/1/gm/canvasstate.cpp#newcode168 gm/canvasstate.cpp:168: newCanvas->drawRect(fRect, fBluePaint); I know this is just a test, so if it is testing what we want it to that's fine. Shouldn't we report if CaptureCanvasState failed? https://codereview.chromium.org/23545017/diff/1/include/utils/SkCanvasUtils.h File include/utils/SkCanvasUtils.h (right): https://codereview.chromium.org/23545017/diff/1/include/utils/SkCanvasUtils.h... include/utils/SkCanvasUtils.h:15: class SK_API SkCanvasUtils { This seems to be a rather generic name, considering that all of the current functions are more specifically about recording/reusing a canvas state. If we ever have more canvas utils, Wouldn't we want them to be separate from these (namespace-wise)? Also, should this be a namespace instead of a class? It seems we rarely use named namespaces in Skia; is this our preferred way of doing things? https://codereview.chromium.org/23545017/diff/1/include/utils/SkCanvasUtils.h... include/utils/SkCanvasUtils.h:27: * is also recommended that the original canvas also to be used during this I'm not sure what this sentence means. https://codereview.chromium.org/23545017/diff/1/include/utils/SkCanvasUtils.h... include/utils/SkCanvasUtils.h:62: static void ReleaseCanvasState(SkCanvasState* state); I think it would be useful to be explicit in the comments about which side of the boundary each call is made on and when. https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp File src/utils/SkCanvasUtils.cpp (right): https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:19: kRGB_565_RasterConfig = 4, Maybe a comment explaining why these start with 4? https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:55: int32_t rowBytes; // Number of bytes from start of one line to next. nit: alignment of //'s Also, should rowBytes be size_t like in SkBitmap? https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:64: class SkCanvasState { Should this inherit from SkNoncopyable and hide the default constructor, or not worry about it since it's a private class? https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:80: sk_free(mcState.clipRects); Should we set this to NULL in the constructor? https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:84: int32_t version; // The CanvasState version this struct was built with. Is there a reason these don't follow our convention of starting with 'f' for fields? Same with the rest of the classes/structs in this file; although I think we don't always follow the convention for structs? https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:97: public: nit: spacing. We typically have no spaces before "public" https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:102: virtual void clipRect(const SkRect& rect, SkRegion::Op op, bool antialias) { SK_OVERRIDE? https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:106: virtual void clipPath(const SkPath&, SkRegion::Op, bool antialias) { SK_OVERRIDE? https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:110: private: Same spacing nit. https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:144: SkCanvasState* SkCanvasUtils::CaptureCanvasState(SkCanvas* canvas) { Should we check for a NULL canvas? https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:145: // Check the clip can be decomposed into simple rectangles. Comment seems inaccurate; the validator checks for antialiasing, but allows arbitrary paths. https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:154: SkTScopedPtr<SkCanvasState> canvasState(SkNEW_ARGS(SkCanvasState, (canvasSize))); Aren't we moving away from SkTScopedPtr? https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:170: // we currently only work for a bitmap backed devices nit: The word 'a' is not needed here https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:176: SkCanvasLayerState* layerState = (SkCanvasLayerState*) layerWriter.reserve(sizeof(SkCanvasLayerState)); 100 chars. https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:183: layerState->content.raster.config = nit: I find a switch statement much more readable than a cascading ? : (may just be a personal preference though) https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:264: SkCanvas* SkCanvasUtils::CreateFromCanvasState(SkCanvasState* state) { Should we check for a NULL state? https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:272: if (state->layerCount < 1) Braces. https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:273: return NULL; 4 space tabs
Might have doubled up on some of scroggo's comments. They count double! https://codereview.chromium.org/23545017/diff/1/gm/canvasstate.cpp File gm/canvasstate.cpp (right): https://codereview.chromium.org/23545017/diff/1/gm/canvasstate.cpp#newcode161 gm/canvasstate.cpp:161: SkCanvas* newCanvas = canvas; Yeah, I'm confused. :) The name newCanvas makes me thing we're allocating a new one. https://codereview.chromium.org/23545017/diff/1/include/utils/SkCanvasUtils.h File include/utils/SkCanvasUtils.h (right): https://codereview.chromium.org/23545017/diff/1/include/utils/SkCanvasUtils.h... include/utils/SkCanvasUtils.h:25: * It is required that the original SkCanvas remain valid until all canvases Can we have the SkCanvasState grab a ref on the SkCanvas to make this automatic? https://codereview.chromium.org/23545017/diff/1/include/utils/SkCanvasUtils.h... include/utils/SkCanvasUtils.h:31: * canvas are NOT currently captured. A unit test for this will make sure we don't get this doc out of date with reality. https://codereview.chromium.org/23545017/diff/1/include/utils/SkCanvasUtils.h... include/utils/SkCanvasUtils.h:38: static SkCanvasState* CaptureCanvasState(SkCanvas* canvas); Given that this is opaque, maybe we can use typedef to hide that it's a pointer too? Is there value in that? Or maybe negative value? Am I going too C here? https://codereview.chromium.org/23545017/diff/1/include/utils/SkCanvasUtils.h... include/utils/SkCanvasUtils.h:52: static SkCanvas* CreateFromCanvasState(SkCanvasState* state); Can this be a const* or const&? (Or if * is typedef'd away, just const?) If we always pass a const SkCanvasState over the line, we can't possibly get confused and release it there. https://codereview.chromium.org/23545017/diff/1/include/utils/SkCanvasUtils.h... include/utils/SkCanvasUtils.h:58: * of any SkCanvas objects that it was used to create. This is what needs update after what we all just talked about, right? Simpler to maintain stack order? https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp File src/utils/SkCanvasUtils.cpp (right): https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:13: #include "SkTScopedPtr.h" Ben will scream. I think the Ben-friendly version in SkAutoTDelete? https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:23: typedef int32_t RasterConfig; Can you plaster a few warnings around to remind the reader that we're trying to keep a stable ABI here, and that's why we do things like typedef int32_t RasterConfig instead of just using the enum? https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:39: int32_t* clipRects; // Clip area: 4 ints per rect in {x,y,w,h} format. Can't hurt to have a struct for this, right? Then we don't have to remember the 4x multiplier when we reconstruct the canvas. https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:40: stray newline https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:45: CanvasBackend type; // |pixel| format: a value from AwPixelConfig. Comment is confusing? https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:61: } content; You may find it more readable to remove the name "content", and just work with raster or gpu directly. Pretty sure you can do that right? https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:80: sk_free(mcState.clipRects); On 2013/08/28 17:22:01, scroggo wrote: > Should we set this to NULL in the constructor? More than that, what about initializing the structs generally? Or is the idea, there's no way to get one of these without filling it in from an SkCanvas, so delay initialization until then? I'd rather go whole-hog one way or the other... make this thing's constructor and destructor comprehensive, or just make SkCanvasState a struct and do both the init and cleanup externally. https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:84: int32_t version; // The CanvasState version this struct was built with. On 2013/08/28 17:22:01, scroggo wrote: > Is there a reason these don't follow our convention of starting with 'f' for > fields? Same with the rest of the classes/structs in this file; although I think > we don't always follow the convention for struc Given that they're public members, I personally like it this way. https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:84: int32_t version; // The CanvasState version this struct was built with. Add a reminder that we can never reorder around the version field? I that too obvious? https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:160: const int layerBufferSize = 3 * sizeof(SkCanvasLayerState); Remind us where 3 comes from? Just a guess about how many layers are likely? https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:164: for (SkCanvas::LayerIter layer(canvas, false); !layer.done(); layer.next()) { can you add an inline "false /*meaning*/" comment here? Or name it? Just helps clueless guys like me read new code. https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:171: const SkBitmap& bitmap = layer.device()->accessBitmap(true); Same for true? https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:185: bitmap.config() == SkBitmap::kARGB_4444_Config ? kARGB_4444_RasterConfig : Can't we just screw 4444 now? https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:186: bitmap.config() == SkBitmap::kRGB_565_Config ? kRGB_565_RasterConfig : -1; return NULL on the failure case? https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:190: if (layerState->content.raster.config < 0) { Oh. :) Maybe move this up with the rest of raster.config? https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:199: canvasState->layers = (SkCanvasLayerState*) sk_malloc_throw(layerWriter.size()); Slight paranoia... add an assert that layerWriter.size() % sizeof(SkCanvasLayerState) == 0? https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:227: SkRegion::kUnion_Op); dedent SkRegion::kUnion_Op to line up with SkIRect::... ? https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:241: SkBitmap::kNo_Config; Why the asymmetry for 4444? We let a 4444 canvas be passed across the line but not interacted with on the other side? https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:256: SkAutoTUnref<SkCanvas> canvas(SkNEW_ARGS(SkCanvas, (device.get()))); Given that you don't otherwise use the device, and we never don't detach canvas, might be clearer to write as SkCanvas* canvas = SkNEW_ARGS(SkCanvas, (SkNEW_ARGS(SkDevice, (bitmap)))); setup_canvas...(..., canvas); return canvas; ? https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:286: canvas->addCanvas(canvasLayer.get()); Would it be effectively the same to call canvasLayer.detach() here? If so, it'd make it clearer that we're really doing an exclusive ownership transfer.
https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp File src/utils/SkCanvasUtils.cpp (right): https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:256: SkAutoTUnref<SkCanvas> canvas(SkNEW_ARGS(SkCanvas, (device.get()))); On 2013/08/28 18:20:46, mtklein wrote: > Given that you don't otherwise use the device, and we never don't detach canvas, > might be clearer to write as > > SkCanvas* canvas = SkNEW_ARGS(SkCanvas, (SkNEW_ARGS(SkDevice, (bitmap)))); > setup_canvas...(..., canvas); > return canvas; > > ? The SkCanvas constructor will ref the device, so we still need to deref it. https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:286: canvas->addCanvas(canvasLayer.get()); On 2013/08/28 18:20:46, mtklein wrote: > Would it be effectively the same to call canvasLayer.detach() here? If so, it'd > make it clearer that we're really doing an exclusive ownership transfer. No, addCanvas refs it, so we want canvasLayer to get unref'ed
I just looked at the main API .h lgtm w.r.t. what I need. Thank you! https://codereview.chromium.org/23545017/diff/1/include/utils/SkCanvasUtils.h File include/utils/SkCanvasUtils.h (right): https://codereview.chromium.org/23545017/diff/1/include/utils/SkCanvasUtils.h... include/utils/SkCanvasUtils.h:27: * is also recommended that the original canvas also to be used during this On 2013/08/28 17:22:01, scroggo wrote: > I'm not sure what this sentence means. recommended that the original canvas NOT be used... ? https://codereview.chromium.org/23545017/diff/1/include/utils/SkCanvasUtils.h... include/utils/SkCanvasUtils.h:62: static void ReleaseCanvasState(SkCanvasState* state); On 2013/08/28 17:22:01, scroggo wrote: > I think it would be useful to be explicit in the comments about which side of > the boundary each call is made on and when. Yes. I'd group CaptureCanvasState & ReleaseCanvasState together, and sketch a quick example flow at top of the file:- // canvas provider code, compiled against skia version N. SkCanvasState* state = CaptureCanvasState(my_canvas); if (state) { consumer_render(state) ReleaseCanvasState(state) } // canvas consumer code, compiled against skia version >= N void consumer_render(SkCanvasState* state) { SkCanvas* canvas = CreateFromCanvasState(state); if (canvas) { canvas->draw...() delete canvas; // or unref or? } }
https://codereview.chromium.org/23545017/diff/1/gm/canvasstate.cpp File gm/canvasstate.cpp (right): https://codereview.chromium.org/23545017/diff/1/gm/canvasstate.cpp#newcode168 gm/canvasstate.cpp:168: newCanvas->drawRect(fRect, fBluePaint); On 2013/08/28 17:22:01, scroggo wrote: > I know this is just a test, so if it is testing what we want it to that's fine. > > Shouldn't we report if CaptureCanvasState failed? This was really just jammed in here and I'm convinced that GM isn't the right place for this as it wants us to draw with a few canvas types that we can't draw with. I'm going to create a test that has the behavior that I want. https://codereview.chromium.org/23545017/diff/1/include/utils/SkCanvasUtils.h File include/utils/SkCanvasUtils.h (right): https://codereview.chromium.org/23545017/diff/1/include/utils/SkCanvasUtils.h... include/utils/SkCanvasUtils.h:15: class SK_API SkCanvasUtils { On 2013/08/28 17:22:01, scroggo wrote: > This seems to be a rather generic name, considering that all of the current > functions are more specifically about recording/reusing a canvas state. If we > ever have more canvas utils, Wouldn't we want them to be separate from these > (namespace-wise)? > > Also, should this be a namespace instead of a class? It seems we rarely use > named namespaces in Skia; is this our preferred way of doing things? Done. https://codereview.chromium.org/23545017/diff/1/include/utils/SkCanvasUtils.h... include/utils/SkCanvasUtils.h:25: * It is required that the original SkCanvas remain valid until all canvases On 2013/08/28 18:20:46, mtklein wrote: > Can we have the SkCanvasState grab a ref on the SkCanvas to make this automatic? Done. https://codereview.chromium.org/23545017/diff/1/include/utils/SkCanvasUtils.h... include/utils/SkCanvasUtils.h:27: * is also recommended that the original canvas also to be used during this On 2013/08/28 17:22:01, scroggo wrote: > I'm not sure what this sentence means. Done. https://codereview.chromium.org/23545017/diff/1/include/utils/SkCanvasUtils.h... include/utils/SkCanvasUtils.h:31: * canvas are NOT currently captured. On 2013/08/28 18:20:46, mtklein wrote: > A unit test for this will make sure we don't get this doc out of date with > reality. Done. https://codereview.chromium.org/23545017/diff/1/include/utils/SkCanvasUtils.h... include/utils/SkCanvasUtils.h:58: * of any SkCanvas objects that it was used to create. On 2013/08/28 18:20:46, mtklein wrote: > This is what needs update after what we all just talked about, right? Simpler > to maintain stack order? Done. https://codereview.chromium.org/23545017/diff/1/include/utils/SkCanvasUtils.h... include/utils/SkCanvasUtils.h:62: static void ReleaseCanvasState(SkCanvasState* state); On 2013/08/28 17:22:01, scroggo wrote: > I think it would be useful to be explicit in the comments about which side of > the boundary each call is made on and when. Done. https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp File src/utils/SkCanvasUtils.cpp (right): https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:13: #include "SkTScopedPtr.h" On 2013/08/28 18:20:46, mtklein wrote: > Ben will scream. I think the Ben-friendly version in SkAutoTDelete? Done. https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:19: kRGB_565_RasterConfig = 4, On 2013/08/28 17:22:01, scroggo wrote: > Maybe a comment explaining why these start with 4? Done. https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:23: typedef int32_t RasterConfig; On 2013/08/28 18:20:46, mtklein wrote: > Can you plaster a few warnings around to remind the reader that we're trying to > keep a stable ABI here, and that's why we do things like typedef int32_t > RasterConfig instead of just using the enum? Done. https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:39: int32_t* clipRects; // Clip area: 4 ints per rect in {x,y,w,h} format. On 2013/08/28 18:20:46, mtklein wrote: > Can't hurt to have a struct for this, right? Then we don't have to remember the > 4x multiplier when we reconstruct the canvas. Done. https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:40: On 2013/08/28 18:20:46, mtklein wrote: > stray newline Done. https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:45: CanvasBackend type; // |pixel| format: a value from AwPixelConfig. On 2013/08/28 18:20:46, mtklein wrote: > Comment is confusing? Done. https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:55: int32_t rowBytes; // Number of bytes from start of one line to next. On 2013/08/28 17:22:01, scroggo wrote: > nit: alignment of //'s > > Also, should rowBytes be size_t like in SkBitmap? No size_t is not guaranteed to always be 32 bits. For example, it can be 64 bits on a 64 bit system. However we probably should make the a uint_32t since there can never be a negative number and it will more closely match the common size_t implementation. https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:61: } content; On 2013/08/28 18:20:46, mtklein wrote: > You may find it more readable to remove the name "content", and just work with > raster or gpu directly. Pretty sure you can do that right? Done. https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:64: class SkCanvasState { On 2013/08/28 17:22:01, scroggo wrote: > Should this inherit from SkNoncopyable and hide the default constructor, or not > worry about it since it's a private class? This needs to stay as close to a simple struct as possible. I don't want to inherit from anything to keep the ABI stable. In the event that those inherited classes were to add virtual methods or member variables compatibility would be broken. https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:97: public: On 2013/08/28 17:22:01, scroggo wrote: > nit: spacing. We typically have no spaces before "public" Done. https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:102: virtual void clipRect(const SkRect& rect, SkRegion::Op op, bool antialias) { On 2013/08/28 17:22:01, scroggo wrote: > SK_OVERRIDE? Done. https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:106: virtual void clipPath(const SkPath&, SkRegion::Op, bool antialias) { On 2013/08/28 17:22:01, scroggo wrote: > SK_OVERRIDE? Done. https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:110: private: On 2013/08/28 17:22:01, scroggo wrote: > Same spacing nit. Done. https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:144: SkCanvasState* SkCanvasUtils::CaptureCanvasState(SkCanvas* canvas) { On 2013/08/28 17:22:01, scroggo wrote: > Should we check for a NULL canvas? Done. https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:145: // Check the clip can be decomposed into simple rectangles. On 2013/08/28 17:22:01, scroggo wrote: > Comment seems inaccurate; the validator checks for antialiasing, but allows > arbitrary paths. non-antialiased arbitrary paths are OK as the get decomposed into a bunch of different rects. https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:154: SkTScopedPtr<SkCanvasState> canvasState(SkNEW_ARGS(SkCanvasState, (canvasSize))); On 2013/08/28 17:22:01, scroggo wrote: > Aren't we moving away from SkTScopedPtr? Done. https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:160: const int layerBufferSize = 3 * sizeof(SkCanvasLayerState); On 2013/08/28 18:20:46, mtklein wrote: > Remind us where 3 comes from? Just a guess about how many layers are likely? Done. https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:164: for (SkCanvas::LayerIter layer(canvas, false); !layer.done(); layer.next()) { On 2013/08/28 18:20:46, mtklein wrote: > can you add an inline "false /*meaning*/" comment here? Or name it? Just helps > clueless guys like me read new code. Done. https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:170: // we currently only work for a bitmap backed devices On 2013/08/28 17:22:01, scroggo wrote: > nit: The word 'a' is not needed here Done. https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:171: const SkBitmap& bitmap = layer.device()->accessBitmap(true); On 2013/08/28 18:20:46, mtklein wrote: > Same for true? Done. https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:186: bitmap.config() == SkBitmap::kRGB_565_Config ? kRGB_565_RasterConfig : -1; On 2013/08/28 18:20:46, mtklein wrote: > return NULL on the failure case? Done. https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:199: canvasState->layers = (SkCanvasLayerState*) sk_malloc_throw(layerWriter.size()); On 2013/08/28 18:20:46, mtklein wrote: > Slight paranoia... add an assert that layerWriter.size() % > sizeof(SkCanvasLayerState) == 0? Done. https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:227: SkRegion::kUnion_Op); On 2013/08/28 18:20:46, mtklein wrote: > dedent SkRegion::kUnion_Op to line up with SkIRect::... ? Done. https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:241: SkBitmap::kNo_Config; On 2013/08/28 18:20:46, mtklein wrote: > Why the asymmetry for 4444? We let a 4444 canvas be passed across the line but > not interacted with on the other side? Done. https://codereview.chromium.org/23545017/diff/1/src/utils/SkCanvasUtils.cpp#n... src/utils/SkCanvasUtils.cpp:286: canvas->addCanvas(canvasLayer.get()); On 2013/08/28 18:20:46, mtklein wrote: > Would it be effectively the same to call canvasLayer.detach() here? If so, it'd > make it clearer that we're really doing an exclusive ownership transfer. the detach would run first which would push the refCnt to zero before we could add it to the main canvas (which will increment the refCnt).
lgtm just some nits i noticed reading through again. https://codereview.chromium.org/23545017/diff/15001/src/utils/SkCanvasStateUt... File src/utils/SkCanvasStateUtils.cpp (right): https://codereview.chromium.org/23545017/diff/15001/src/utils/SkCanvasStateUt... src/utils/SkCanvasStateUtils.cpp:136: bool fFailed; 4 space https://codereview.chromium.org/23545017/diff/15001/src/utils/SkCanvasStateUt... src/utils/SkCanvasStateUtils.cpp:153: * and some more commont complex clips (e.g. a clipRect with a sub-rect commont -> common https://codereview.chromium.org/23545017/diff/15001/src/utils/SkCanvasStateUt... src/utils/SkCanvasStateUtils.cpp:185: SkDEBUGF(("CaptureCanvasState does not currently support canvases with antialiased clips.\n")); over 100 here?
Overall lgtm (I wish code review figured out you moved the new file and compared from revision 1 to 2, but it seems you addressed everything). reed@ suggested putting the header in src/utils. Do they need it in include? https://codereview.chromium.org/23545017/diff/15001/src/utils/SkCanvasStateUt... File src/utils/SkCanvasStateUtils.cpp (right): https://codereview.chromium.org/23545017/diff/15001/src/utils/SkCanvasStateUt... src/utils/SkCanvasStateUtils.cpp:192: setup_MC_state(&canvasState->mcState, canvas->getTotalMatrix(), canvas->getTotalClip()); getTotalClip() is deprecated, and it seems that the replacement function getClipDeviceBounds() doesn't do enough. Trust canvas not to remove getTotalClip() entirely? https://codereview.chromium.org/23545017/diff/23001/tests/CanvasStateTest.cpp File tests/CanvasStateTest.cpp (right): https://codereview.chromium.org/23545017/diff/23001/tests/CanvasStateTest.cpp... tests/CanvasStateTest.cpp:29: const int configCount = sizeof(configs) / sizeof(SkBitmap::Config); Won't this cause a warning somewhere (size_t to int)? https://codereview.chromium.org/23545017/diff/23001/tests/CanvasStateTest.cpp... tests/CanvasStateTest.cpp:37: const int layerCombinations = sizeof(layerAlpha) / sizeof(int); Warning?
Message was sent while issue was closed.
Committed patchset #3 manually as r11010 (presubmit successful).
reverted prior to the rebase with https://skia.googlecode.com/svn/trunk@11011
Message was sent while issue was closed.
Committed patchset #4 manually as r11013 (presubmit successful).
Message was sent while issue was closed.
One late comment.... (from playing with the patch rather than from reading it) https://codereview.chromium.org/23545017/diff/12001/src/utils/SkCanvasStateUt... File src/utils/SkCanvasStateUtils.cpp (right): https://codereview.chromium.org/23545017/diff/12001/src/utils/SkCanvasStateUt... src/utils/SkCanvasStateUtils.cpp:330: canvas->addCanvas(canvasLayer.get()); I think there's a bug here that you never look at the restored layer's x & y values. I believe this is the reason for the symptoms I'm seeing over in https://b/10152369#ISSUE_HistoryHeader64 although I can't exactly explain the link. I tried translating canvasLayer by -x & -y prior to adding it, but that seemed to have no effect what-so-ever. So I'm not quite sure. (Aside: as you have a clip and matrix exported per layer, maybe the x & y are redundant in the flattened state. Just need to work out how to pre-apply them into the other fields on the export side).
Message was sent while issue was closed.
https://codereview.chromium.org/23545017/diff/12001/src/utils/SkCanvasStateUt... File src/utils/SkCanvasStateUtils.cpp (right): https://codereview.chromium.org/23545017/diff/12001/src/utils/SkCanvasStateUt... src/utils/SkCanvasStateUtils.cpp:330: canvas->addCanvas(canvasLayer.get()); On 2013/08/30 06:45:57, joth wrote: > I think there's a bug here that you never look at the restored layer's x & y > values. I believe this is the reason for the symptoms I'm seeing over in > https://b/10152369#ISSUE_HistoryHeader64 although I can't exactly explain the > link. > > I tried translating canvasLayer by -x & -y prior to adding it, but that seemed > to have no effect what-so-ever. So I'm not quite sure. (Aside: as you have a > clip and matrix exported per layer, maybe the x & y are redundant in the > flattened state. Just need to work out how to pre-apply them into the other > fields on the export side). Hmm OK x & y maybe a red-herring. I looked a bit more at how this is implemented using SkNWayCanvas vs a stack of devices (as I had in https://codereview.chromium.org/22799011/diff/3001/android_webview/browser/in...) and I see SkNWayCanvas::setMatrix is just forwarded verbatim to each underlying canvas -- there's not way for each layer canvas to retain it's own "device" layer translation indenpendent of how the user uses the public API translate / matrix. the cc software_renderer makes pretty extensive use of setMatrix which as I understand it will blow away and translate and result in each layer having exact same matrix. This is why I see the top of the logical page appear in all the layers - even the one that is positioned at the bottom of the page.
Message was sent while issue was closed.
https://codereview.chromium.org/23545017/diff/12001/src/utils/SkCanvasStateUt... File src/utils/SkCanvasStateUtils.cpp (right): https://codereview.chromium.org/23545017/diff/12001/src/utils/SkCanvasStateUt... src/utils/SkCanvasStateUtils.cpp:330: canvas->addCanvas(canvasLayer.get()); On 2013/08/30 07:05:04, joth wrote: > On 2013/08/30 06:45:57, joth wrote: > > I think there's a bug here that you never look at the restored layer's x & y > > values. I believe this is the reason for the symptoms I'm seeing over in > > https://b/10152369#ISSUE_HistoryHeader64 although I can't exactly explain the > > link. > > > > I tried translating canvasLayer by -x & -y prior to adding it, but that seemed > > to have no effect what-so-ever. So I'm not quite sure. (Aside: as you have a > > clip and matrix exported per layer, maybe the x & y are redundant in the > > flattened state. Just need to work out how to pre-apply them into the other > > fields on the export side). > > Hmm OK x & y maybe a red-herring. > > I looked a bit more at how this is implemented using SkNWayCanvas vs a stack of > devices (as I had in > https://codereview.chromium.org/22799011/diff/3001/android_webview/browser/in...) > and I see SkNWayCanvas::setMatrix is just forwarded verbatim to each underlying > canvas -- there's not way for each layer canvas to retain it's own "device" > layer translation indenpendent of how the user uses the public API translate / > matrix. > > the cc software_renderer makes pretty extensive use of setMatrix which as I > understand it will blow away and translate and result in each layer having > exact same matrix. This is why I see the top of the logical page appear in all > the layers - even the one that is positioned at the bottom of the page. > Yes, the setMatrix will cause some havoc with this solution. I'll work on coding something up so that if someone overwrites the entire matrix like that we preserve the original x,y position for every canvas that we insert into the n-way canvas. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
