Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
27236007:
Implement perspective for bitmaps in pdf. (Closed)
|
Created:
7 years, 2 months ago by edisonn Modified:
7 years, 2 months ago CC:
skia-review_googlegroups.com, bungeman-skia, caryclark Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionImplement perspective for bitmaps in pdf.
R=vandebo@chromium.org
Committed: https://code.google.com/p/skia/source/detail?r=11822
Patch Set 1 #Patch Set 2 : code cleanup #
Total comments: 10
Patch Set 3 : fix typo and better name vars #Patch Set 4 : reupload, some error happened at last upload #
Total comments: 19
Patch Set 5 : fixes required by the reviewers #
Total comments: 10
Patch Set 6 : nits and properly use SkScalarCeil #Patch Set 7 : nits and properly use SkScalarCeil #Messages
Total messages: 8 (0 generated)
DPI settings is coming in another CL
https://codereview.chromium.org/27236007/diff/3001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/27236007/diff/3001/src/pdf/SkPDFDevice.cpp#ne... src/pdf/SkPDFDevice.cpp:2068: SkRect bounds = path.getBounds(); Should this be further constrained by the current clip? https://codereview.chromium.org/27236007/diff/3001/src/pdf/SkPDFDevice.cpp#ne... src/pdf/SkPDFDevice.cpp:2070: // TODO(edisonn): add DPI settings. Currently 1 pixel/point, which does not look great, nit: 80 cols https://codereview.chromium.org/27236007/diff/3001/src/pdf/SkPDFDevice.cpp#ne... src/pdf/SkPDFDevice.cpp:2080: SkPoint zeroZero = SkPoint::Make(SkIntToScalar(0), SkIntToScalar(0)); zeroZero isn't used. https://codereview.chromium.org/27236007/diff/3001/src/pdf/SkPDFDevice.cpp#ne... src/pdf/SkPDFDevice.cpp:2094: // Make sure the final bits ar in the bitmap. ar -> are https://codereview.chromium.org/27236007/diff/20001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/27236007/diff/20001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:2044: SkRegion all; nit: all -> perspectiveBounds ? https://codereview.chromium.org/27236007/diff/20001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:2046: SkBitmap subsetBitmap = origBitmap; nit: Pull inside the hasPerspective if on 2051 and make it a pointer to avoid an unneeded copy. https://codereview.chromium.org/27236007/diff/20001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:2047: SkBitmap bitmap = origBitmap; For both the perspective and non-perspective case this makes an unneeded copy. Maybe make bitmap a pointer and point at origBitmap or a local perspectiveBitmap? https://codereview.chromium.org/27236007/diff/20001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:2048: const SkIRect* srcRect = origSrcRect; not needed, just reset origSrcRect to NULL if you apply it. https://codereview.chromium.org/27236007/diff/20001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:2051: if (matrix.hasPerspective()) { nit: use origMatrix in this code block (except where you set matrix at the end). It'll make it more clear that you're dealing with origMatrix and not something you've computed and put into matrix. https://codereview.chromium.org/27236007/diff/20001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:2062: SkPath path; nit: path -> perspectiveOutline ? https://codereview.chromium.org/27236007/diff/20001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:2070: // TODO(edisonn): add DPI settings. Currently 1 pixel/point, which does not look great, Nothing to be done, but this isn't necessarily 72 DPI, it depends on matrix. https://codereview.chromium.org/27236007/diff/20001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:2074: setup_transparent_bitmap(&bitmap, bounds.width(), bounds.height()); This approach looks fine and could be committed this way. I think a better approach would be to use a bitmap shader (in clamp mode) and draw a rect over the entire bounding box. Then intersect path from 2062 to the clip. That will avoid introducing alpha to the image while still giving good behavior at the edge of the image. Avoiding alpha will reduce the pdf size and generation CPU time some. https://codereview.chromium.org/27236007/diff/20001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:2076: SkAutoTUnref<SkBitmapDevice> device(SkNEW_ARGS(SkBitmapDevice, (bitmap))); Why not stack allocate this? https://codereview.chromium.org/27236007/diff/20001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:2086: SkMatrix matrix2 = matrix; nit: matrix2 -> offsetMatrix ?
https://codereview.chromium.org/27236007/diff/3001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/27236007/diff/3001/src/pdf/SkPDFDevice.cpp#ne... src/pdf/SkPDFDevice.cpp:2068: SkRect bounds = path.getBounds(); On 2013/10/15 21:41:32, vandebo wrote: > Should this be further constrained by the current clip? This is an optimization. I will add a todo. in a non-perspective world all the bitmap would have been saved. I would not make the perspective more efficient than the non-perspective case. https://codereview.chromium.org/27236007/diff/3001/src/pdf/SkPDFDevice.cpp#ne... src/pdf/SkPDFDevice.cpp:2070: // TODO(edisonn): add DPI settings. Currently 1 pixel/point, which does not look great, On 2013/10/15 21:41:32, vandebo wrote: > nit: 80 cols this file is already 100c https://codereview.chromium.org/27236007/diff/3001/src/pdf/SkPDFDevice.cpp#ne... src/pdf/SkPDFDevice.cpp:2080: SkPoint zeroZero = SkPoint::Make(SkIntToScalar(0), SkIntToScalar(0)); On 2013/10/15 21:41:32, vandebo wrote: > zeroZero isn't used. Done. https://codereview.chromium.org/27236007/diff/3001/src/pdf/SkPDFDevice.cpp#ne... src/pdf/SkPDFDevice.cpp:2094: // Make sure the final bits ar in the bitmap. On 2013/10/15 21:41:32, vandebo wrote: > ar -> are Done. https://codereview.chromium.org/27236007/diff/20001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/27236007/diff/20001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:2044: SkRegion all; On 2013/10/15 21:41:32, vandebo wrote: > nit: all -> perspectiveBounds ? Done. https://codereview.chromium.org/27236007/diff/20001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:2046: SkBitmap subsetBitmap = origBitmap; On 2013/10/15 21:41:32, vandebo wrote: > nit: Pull inside the hasPerspective if on 2051 and make it a pointer to avoid an > unneeded copy. Done. https://codereview.chromium.org/27236007/diff/20001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:2047: SkBitmap bitmap = origBitmap; On 2013/10/15 21:41:32, vandebo wrote: > For both the perspective and non-perspective case this makes an unneeded copy. > Maybe make bitmap a pointer and point at origBitmap or a local > perspectiveBitmap? Done. https://codereview.chromium.org/27236007/diff/20001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:2048: const SkIRect* srcRect = origSrcRect; On 2013/10/15 21:41:32, vandebo wrote: > not needed, just reset origSrcRect to NULL if you apply it. Done. https://codereview.chromium.org/27236007/diff/20001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:2051: if (matrix.hasPerspective()) { On 2013/10/15 21:41:32, vandebo wrote: > nit: use origMatrix in this code block (except where you set matrix at the end). > It'll make it more clear that you're dealing with origMatrix and not something > you've computed and put into matrix. Done. https://codereview.chromium.org/27236007/diff/20001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:2062: SkPath path; On 2013/10/15 21:41:32, vandebo wrote: > nit: path -> perspectiveOutline ? Done. https://codereview.chromium.org/27236007/diff/20001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:2074: setup_transparent_bitmap(&bitmap, bounds.width(), bounds.height()); Added a todo, will keep the code as is On 2013/10/15 21:41:32, vandebo wrote: > This approach looks fine and could be committed this way. I think a better > approach would be to use a bitmap shader (in clamp mode) and draw a rect over > the entire bounding box. Then intersect path from 2062 to the clip. That will > avoid introducing alpha to the image while still giving good behavior at the > edge of the image. Avoiding alpha will reduce the pdf size and generation CPU > time some. https://codereview.chromium.org/27236007/diff/20001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:2076: SkAutoTUnref<SkBitmapDevice> device(SkNEW_ARGS(SkBitmapDevice, (bitmap))); On 2013/10/15 21:41:32, vandebo wrote: > Why not stack allocate this? Done. https://codereview.chromium.org/27236007/diff/20001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:2086: SkMatrix matrix2 = matrix; On 2013/10/15 21:41:32, vandebo wrote: > nit: matrix2 -> offsetMatrix ? Done.
LGTM with suggestions. https://codereview.chromium.org/27236007/diff/3001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/27236007/diff/3001/src/pdf/SkPDFDevice.cpp#ne... src/pdf/SkPDFDevice.cpp:2070: // TODO(edisonn): add DPI settings. Currently 1 pixel/point, which does not look great, On 2013/10/16 15:08:03, edisonn wrote: > On 2013/10/15 21:41:32, vandebo wrote: > > nit: 80 cols > this file is already 100c Hardly, 31/2073 lines and most of those are over by one or two characters. https://codereview.chromium.org/27236007/diff/25001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/27236007/diff/25001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:2031: static void setup_transparent_bitmap(SkBitmap* bitmap, int width, int height) { nit: it seems unlikely that this will get reused. You could inline it, but I leave the call to you. https://codereview.chromium.org/27236007/diff/25001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:2050: // Raster the bitmap using perspective in a new bitmap. nit: Raster -> Rasterize/Render https://codereview.chromium.org/27236007/diff/25001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:2052: // TODO(edisonn): testability - add a flag to force this codepath. nit: Remove - you could pass a matrix with perspective, or you could have a test implementation of SkMatrix that returns true for hasPerspective() always. No need for a flag. https://codereview.chromium.org/27236007/diff/25001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:2058: subsetBitmap = origBitmap; nit: this makes a copy. You could move "SkBitmap subsetBitmap;" to line 2052 and set "bitmap" on 2056 and 2058. https://codereview.chromium.org/27236007/diff/25001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:2079: // Create transparent bitmap. nit: comment is unnecessary
https://codereview.chromium.org/27236007/diff/3001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/27236007/diff/3001/src/pdf/SkPDFDevice.cpp#ne... src/pdf/SkPDFDevice.cpp:2070: // TODO(edisonn): add DPI settings. Currently 1 pixel/point, which does not look great, On 2013/10/16 16:46:41, vandebo wrote: > On 2013/10/16 15:08:03, edisonn wrote: > > On 2013/10/15 21:41:32, vandebo wrote: > > > nit: 80 cols > > this file is already 100c > > Hardly, 31/2073 lines and most of those are over by one or two characters. Done. https://codereview.chromium.org/27236007/diff/25001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/27236007/diff/25001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:2031: static void setup_transparent_bitmap(SkBitmap* bitmap, int width, int height) { On 2013/10/16 16:46:41, vandebo wrote: > nit: it seems unlikely that this will get reused. You could inline it, but I > leave the call to you. Done. https://codereview.chromium.org/27236007/diff/25001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:2050: // Raster the bitmap using perspective in a new bitmap. On 2013/10/16 16:46:41, vandebo wrote: > nit: Raster -> Rasterize/Render Done. https://codereview.chromium.org/27236007/diff/25001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:2052: // TODO(edisonn): testability - add a flag to force this codepath. On 2013/10/16 16:46:41, vandebo wrote: > nit: Remove - you could pass a matrix with perspective, or you could have a test > implementation of SkMatrix that returns true for hasPerspective() always. No > need for a flag. Done. https://codereview.chromium.org/27236007/diff/25001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:2058: subsetBitmap = origBitmap; On 2013/10/16 16:46:41, vandebo wrote: > nit: this makes a copy. You could move "SkBitmap subsetBitmap;" to line 2052 > and set "bitmap" on 2056 and 2058. Done. https://codereview.chromium.org/27236007/diff/25001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:2079: // Create transparent bitmap. On 2013/10/16 16:46:41, vandebo wrote: > nit: comment is unnecessary Done.
LGTM
Message was sent while issue was closed.
Committed patchset #7 manually as r11822 (presubmit successful). |
