Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
54913004:
Implement DPI for perspective bitmaps in PDF - we save the bitmap at the resolution requested. (Closed)
|
Created:
7 years, 1 month ago by edisonn Modified:
7 years, 1 month ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionImplement DPI for perspective bitmaps in PDF - we save the bitmap at the resolution requested.
R=reed@google.com, vandebo@chromium.org
Committed: https://code.google.com/p/skia/source/detail?r=12230
Patch Set 1 #Patch Set 2 : re-upload (last one failed) #Patch Set 3 : 80c/l, remove commentd code #
Total comments: 7
Patch Set 4 : remove sqrt #
Total comments: 8
Patch Set 5 : use SkScalar instead of float #
Total comments: 8
Patch Set 6 : fix scaling #Patch Set 7 : remove redundants scale ops #Patch Set 8 : take 2, matrix handling #
Total comments: 11
Patch Set 9 : nits #
Messages
Total messages: 20 (0 generated)
The scaling wasn't obvious to me, please explain. If appropriate, add a comment. https://codereview.chromium.org/54913004/diff/90001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/54913004/diff/90001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:2070: float scaleX = sqrt(fRasterDpi / (float)DPI_FOR_RASTER_SCALE_ONE); I don't understand, why sqrt ? https://codereview.chromium.org/54913004/diff/90001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:2088: SkScalarCeilToInt(scaleX * bounds.width()), This makes the image bigger https://codereview.chromium.org/54913004/diff/90001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:2101: offsetMatrix.postScale(scaleX, scaleY); Then this draws it bigger. Should this be 1/scaleX ?
https://codereview.chromium.org/54913004/diff/90001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/54913004/diff/90001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:2070: float scaleX = sqrt(fRasterDpi / (float)DPI_FOR_RASTER_SCALE_ONE); On 2013/10/31 19:26:43, vandebo wrote: > I don't understand, why sqrt ? For some reason I was thinking DPI is "dots per square inch". But you are right, it is "dots per inch" so I removed the sqrt. https://codereview.chromium.org/54913004/diff/90001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:2088: SkScalarCeilToInt(scaleX * bounds.width()), On 2013/10/31 19:26:43, vandebo wrote: > This makes the image bigger correct, when bitmap is put in perspective mode, it tries to preserve as much signal as it can it can, up to the DPI requested. Pixels that are farther away, will became smaller, and the larger image will accommodate them. Please run the gm to see yourself. out/Debug/gm -w ~/gm_force_persp_1000/ --useDocumentInsteadOfDevice --forcePerspectiveMatrix --match ~scaled_tilemodes ~convexpaths ~clipped-bitmap ~xfermodes --pdfRasterDpi 10 vs out/Debug/gm -w ~/gm_force_persp_1000/ --useDocumentInsteadOfDevice --forcePerspectiveMatrix --match ~scaled_tilemodes ~convexpaths ~clipped-bitmap ~xfermodes --pdfRasterDpi 100 https://codereview.chromium.org/54913004/diff/90001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:2101: offsetMatrix.postScale(scaleX, scaleY); On 2013/10/31 19:26:43, vandebo wrote: > Then this draws it bigger. Should this be 1/scaleX ? The gms just work, I guess the code is correct.
https://codereview.chromium.org/54913004/diff/90001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/54913004/diff/90001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:2101: offsetMatrix.postScale(scaleX, scaleY); On 2013/10/31 19:49:31, edisonn wrote: > On 2013/10/31 19:26:43, vandebo wrote: > > Then this draws it bigger. Should this be 1/scaleX ? > > The gms just work, I guess the code is correct. After much effort, I understand. Above makes the canvas big, this says to draw big. https://codereview.chromium.org/54913004/diff/130001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/54913004/diff/130001/src/pdf/SkPDFDevice.cpp#... src/pdf/SkPDFDevice.cpp:2070: float scaleX = fRasterDpi / (float)DPI_FOR_RASTER_SCALE_ONE; This should probably use SkScalar instead of float. https://codereview.chromium.org/54913004/diff/130001/src/pdf/SkPDFDevice.cpp#... src/pdf/SkPDFDevice.cpp:2076: scaleX = fabs(scaleX); SkScalarAbs https://codereview.chromium.org/54913004/diff/130001/src/pdf/SkPDFDevice.cpp#... src/pdf/SkPDFDevice.cpp:2077: scaleY = fabs(scaleY); Actually, I don't think you want to do this. When *bitmap is drawn on line 2150, fInitialTransform is in effect. I suspect that the values you're getting in the gm are all 1 or -1 and thus your fabs() is correcting for that. I think you need to unapply the intialtransform (after you calculate the resolution for the desired DPI) on line 2113. You could prove this by using an initial transform in the gm (with a manual perspective).
https://codereview.chromium.org/54913004/diff/130001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/54913004/diff/130001/src/pdf/SkPDFDevice.cpp#... src/pdf/SkPDFDevice.cpp:2070: float scaleX = fRasterDpi / (float)DPI_FOR_RASTER_SCALE_ONE; On 2013/10/31 23:20:36, vandebo wrote: > This should probably use SkScalar instead of float. Done. https://codereview.chromium.org/54913004/diff/130001/src/pdf/SkPDFDevice.cpp#... src/pdf/SkPDFDevice.cpp:2076: scaleX = fabs(scaleX); On 2013/10/31 23:20:36, vandebo wrote: > SkScalarAbs Done. https://codereview.chromium.org/54913004/diff/130001/src/pdf/SkPDFDevice.cpp#... src/pdf/SkPDFDevice.cpp:2077: scaleY = fabs(scaleY); On 2013/10/31 23:20:36, vandebo wrote: > Actually, I don't think you want to do this. When *bitmap is drawn on line > 2150, fInitialTransform is in effect. I suspect that the values you're getting > in the gm are all 1 or -1 and thus your fabs() is correcting for that. Yes, but getScaleX()/Y() can can return anything, as initial transform can have anything (except perspective) I think > you need to unapply the intialtransform (after you calculate the resolution for > the desired DPI) on line 2113. This seems to work fast and correct. I am not sure why I would choose something that might work slower, and has a more complex logic, unless it is clearly providing better results You could prove this by using an initial > transform in the gm (with a manual perspective). Initial transform does not support perspective
https://codereview.chromium.org/54913004/diff/130001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/54913004/diff/130001/src/pdf/SkPDFDevice.cpp#... src/pdf/SkPDFDevice.cpp:2077: scaleY = fabs(scaleY); On 2013/11/01 15:00:08, edisonn wrote: > On 2013/10/31 23:20:36, vandebo wrote: > > Actually, I don't think you want to do this. When *bitmap is drawn on line > > 2150, fInitialTransform is in effect. I suspect that the values you're getting > > in the gm are all 1 or -1 and thus your fabs() is correcting for that. > > Yes, but getScaleX()/Y() can can return anything, as initial transform can have > anything (except perspective) True, you do need to use Abs for the bitmap.setConfig() > > I think > > you need to unapply the intialtransform (after you calculate the resolution > for > > the desired DPI) on line 2113. > > This seems to work fast and correct. I am not sure why I would choose something > that might work slower, and has a more complex logic, unless it is clearly > providing better results Because it will give the wrong result if the initial transform has a scale other than 1 or -1. > You could prove this by using an initial > > transform in the gm (with a manual perspective). > Initial transform does not support perspective Initial transform doesn't need perspective - just a non unit scale factor. With gm, you've use the force-perspective flag with an initial transform, but you could use a gm that uses perspective to test.
https://codereview.chromium.org/54913004/diff/130001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/54913004/diff/130001/src/pdf/SkPDFDevice.cpp#... src/pdf/SkPDFDevice.cpp:2077: scaleY = fabs(scaleY); On 2013/11/01 22:13:42, vandebo wrote: > On 2013/11/01 15:00:08, edisonn wrote: > > On 2013/10/31 23:20:36, vandebo wrote: > > > Actually, I don't think you want to do this. When *bitmap is drawn on line > > > 2150, fInitialTransform is in effect. I suspect that the values you're > getting > > > in the gm are all 1 or -1 and thus your fabs() is correcting for that. > > > > Yes, but getScaleX()/Y() can can return anything, as initial transform can > have > > anything (except perspective) > > True, you do need to use Abs for the bitmap.setConfig() > > > > I think > > > you need to unapply the intialtransform (after you calculate the resolution > > for > > > the desired DPI) on line 2113. > > > > This seems to work fast and correct. I am not sure why I would choose > something > > that might work slower, and has a more complex logic, unless it is clearly > > providing better results > > Because it will give the wrong result if the initial transform has a scale other > than 1 or -1. > > > You could prove this by using an initial > > > transform in the gm (with a manual perspective). > > Initial transform does not support perspective > > Initial transform doesn't need perspective - just a non unit scale factor. With > gm, you've use the force-perspective flag with an initial transform, Incorrect, the initial transform is not affected by perspective. And actually we assert if initial transform has perspective. The perspective is in the canvas's matrix but you > could use a gm that uses perspective to test. > Algorithm: 1) take the rect of the bitmap, apply perspective on it (origMatrix), the resulting shape needs now only initial matrix transformation 2) compute scale for given DPI and initial transform 3) correct scale to be positive now the scale takes in account DPI, the initial transform, and the orogMatrix(cariing perspective) and the bitmap size
Let me know if you'd like more clarification. https://codereview.chromium.org/54913004/diff/180001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/54913004/diff/180001/src/pdf/SkPDFDevice.cpp#... src/pdf/SkPDFDevice.cpp:2197: scaleX *= fInitialTransform.getScaleX(); The initial transform is already applied to the canvas, so we do need to account for the initial transform when calculating the scale factor for the DPI... see comment on line 2237 https://codereview.chromium.org/54913004/diff/180001/src/pdf/SkPDFDevice.cpp#... src/pdf/SkPDFDevice.cpp:2212: SkScalarCeilToInt(scaleX * bounds.width()), Apply Abs here instead of on 2200 so you don't have to do it again on 2237. ... SkScalarAbs(scaleX) .... https://codereview.chromium.org/54913004/diff/180001/src/pdf/SkPDFDevice.cpp#... src/pdf/SkPDFDevice.cpp:2225: offsetMatrix.postScale(scaleX, scaleY); I'm not sure if you need to unapply the initial transform scaling before or after this line. https://codereview.chromium.org/54913004/diff/180001/src/pdf/SkPDFDevice.cpp#... src/pdf/SkPDFDevice.cpp:2237: // and scaled to reflect DPI. But we need to unapply it here so that it doesn't get applied twice (since it's already on the canvas): scaleX /= fInitialTransform.getScaleX(); scaleY /= fInitialTransform.getScaleY();
https://codereview.chromium.org/54913004/diff/180001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/54913004/diff/180001/src/pdf/SkPDFDevice.cpp#... src/pdf/SkPDFDevice.cpp:2197: scaleX *= fInitialTransform.getScaleX(); On 2013/11/04 19:55:10, vandebo wrote: > The initial transform is already applied to the canvas, so we do need to account > for the initial transform when calculating the scale factor for the DPI... see > comment on line 2237 Done. https://codereview.chromium.org/54913004/diff/180001/src/pdf/SkPDFDevice.cpp#... src/pdf/SkPDFDevice.cpp:2212: SkScalarCeilToInt(scaleX * bounds.width()), On 2013/11/04 19:55:10, vandebo wrote: > Apply Abs here instead of on 2200 so you don't have to do it again on 2237. > > ... SkScalarAbs(scaleX) .... Done. https://codereview.chromium.org/54913004/diff/180001/src/pdf/SkPDFDevice.cpp#... src/pdf/SkPDFDevice.cpp:2225: offsetMatrix.postScale(scaleX, scaleY); On 2013/11/04 19:55:10, vandebo wrote: > I'm not sure if you need to unapply the initial transform scaling before or > after this line. Done. https://codereview.chromium.org/54913004/diff/180001/src/pdf/SkPDFDevice.cpp#... src/pdf/SkPDFDevice.cpp:2237: // and scaled to reflect DPI. On 2013/11/04 19:55:10, vandebo wrote: > But we need to unapply it here so that it doesn't get applied twice (since it's > already on the canvas): > > scaleX /= fInitialTransform.getScaleX(); > scaleY /= fInitialTransform.getScaleY(); Done.
This is what I think it should look like. I haven't tested it. https://codereview.chromium.org/60863003
On 2013/11/05 21:27:15, vandebo wrote: > This is what I think it should look like. I haven't tested it. > https://codereview.chromium.org/60863003 It does not work like that ... I hit asserts out/Debug/gm -w ~/gm_force_persp --useDocumentInsteadOfDevice --forcePerspectiveMatrix --match ~scaled_tilemodes ~convexpaths ~clipped-bitmap --pdfRasterDpi 72 GM: drawing... xfermodes [1590 640] ../src/pdf/SkPDFDevice.cpp:2005: failed assertion "!contentEntries || !contentEntries->fNext.get()" /usr/local/google/home/edisonn/scripts/persp.sh: line 4: 562 Segmentation fault (core dumped) out/Debug/gm -w ~/gm_force_persp --useDocumentInsteadOfDevice --forcePerspectiveMatrix --match ~scaled_tilemodes ~convexpaths ~clipped-bitmap --pdfRasterDpi 72
On 2013/11/05 21:40:57, edisonn wrote: > On 2013/11/05 21:27:15, vandebo wrote: > > This is what I think it should look like. I haven't tested it. > > https://codereview.chromium.org/60863003 > > It does not work like that ... I hit asserts > > out/Debug/gm -w ~/gm_force_persp --useDocumentInsteadOfDevice > --forcePerspectiveMatrix --match ~scaled_tilemodes ~convexpaths ~clipped-bitmap > --pdfRasterDpi 72 > > GM: drawing... xfermodes [1590 640] > ../src/pdf/SkPDFDevice.cpp:2005: failed assertion "!contentEntries || > !contentEntries->fNext.get()" > /usr/local/google/home/edisonn/scripts/persp.sh: line 4: 562 Segmentation > fault (core dumped) out/Debug/gm -w ~/gm_force_persp > --useDocumentInsteadOfDevice --forcePerspectiveMatrix --match ~scaled_tilemodes > ~convexpaths ~clipped-bitmap --pdfRasterDpi 72 I'm not going to code through you or debug for you. I pointed out a real issue and the idea of how to address it.
On 2013/11/06 18:42:00, vandebo wrote: > On 2013/11/05 21:40:57, edisonn wrote: > > On 2013/11/05 21:27:15, vandebo wrote: > > > This is what I think it should look like. I haven't tested it. > > > https://codereview.chromium.org/60863003 > > > > It does not work like that ... I hit asserts > > > > out/Debug/gm -w ~/gm_force_persp --useDocumentInsteadOfDevice > > --forcePerspectiveMatrix --match ~scaled_tilemodes ~convexpaths > ~clipped-bitmap > > --pdfRasterDpi 72 > > > > GM: drawing... xfermodes [1590 640] > > ../src/pdf/SkPDFDevice.cpp:2005: failed assertion "!contentEntries || > > !contentEntries->fNext.get()" > > /usr/local/google/home/edisonn/scripts/persp.sh: line 4: 562 Segmentation > > fault (core dumped) out/Debug/gm -w ~/gm_force_persp > > --useDocumentInsteadOfDevice --forcePerspectiveMatrix --match > ~scaled_tilemodes > > ~convexpaths ~clipped-bitmap --pdfRasterDpi 72 > > I'm not going to code through you or debug for you. I pointed out a real issue > and the idea of how to address it. Ok, it crashes because of the sign of the scale. When getBounds is called, is basically fixes reorders the points, so I have to fix manually the scale, like in my original solution With the solution you suggested, and the fix of the sign, actually it does not work, it basically ignores the DPI. I am running now tests on my original cl, which works.
This solution should be better.
This seems right. A few nits... https://codereview.chromium.org/54913004/diff/440001/gm/gmmain.cpp File gm/gmmain.cpp (right): https://codereview.chromium.org/54913004/diff/440001/gm/gmmain.cpp#newcode1480 gm/gmmain.cpp:1480: DEFINE_int32(pdfRasterDpi, 72, "Scale at which at which the non suported features " nit: wrap at 80col, so you're already wrapping close to it. https://codereview.chromium.org/54913004/diff/440001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/54913004/diff/440001/src/pdf/SkPDFDevice.cpp#... src/pdf/SkPDFDevice.cpp:2198: total.postConcat(fInitialTransform); Should this be preConcat? fInitialTransform is applied first. https://codereview.chromium.org/54913004/diff/440001/src/pdf/SkPDFDevice.cpp#... src/pdf/SkPDFDevice.cpp:2204: total.postConcat(scaleDpi); nit: scaleDPI isn't needed, just do postScale here. https://codereview.chromium.org/54913004/diff/440001/src/pdf/SkPDFDevice.cpp#... src/pdf/SkPDFDevice.cpp:2225: SkScalarCeilToInt(scaleX * bounds.width()), Just use physicalPerspectiveOutline.getBounds().width(), it's the same value with out a division and multiplication. https://codereview.chromium.org/54913004/diff/440001/src/pdf/SkPDFDevice.cpp#... src/pdf/SkPDFDevice.cpp:2252: matrix.postTranslate(deltaX, deltaY); Not sure, should this be preTranslate ?
https://codereview.chromium.org/54913004/diff/440001/gm/gmmain.cpp File gm/gmmain.cpp (right): https://codereview.chromium.org/54913004/diff/440001/gm/gmmain.cpp#newcode1480 gm/gmmain.cpp:1480: DEFINE_int32(pdfRasterDpi, 72, "Scale at which at which the non suported features " On 2013/11/07 22:44:00, vandebo wrote: > nit: wrap at 80col, so you're already wrapping close to it. Done. https://codereview.chromium.org/54913004/diff/440001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/54913004/diff/440001/src/pdf/SkPDFDevice.cpp#... src/pdf/SkPDFDevice.cpp:2198: total.postConcat(fInitialTransform); On 2013/11/07 22:44:00, vandebo wrote: > Should this be preConcat? fInitialTransform is applied first. I don't think so, gms fail and they don't output at the right resolution https://codereview.chromium.org/54913004/diff/440001/src/pdf/SkPDFDevice.cpp#... src/pdf/SkPDFDevice.cpp:2204: total.postConcat(scaleDpi); On 2013/11/07 22:44:00, vandebo wrote: > nit: scaleDPI isn't needed, just do postScale here. Done. https://codereview.chromium.org/54913004/diff/440001/src/pdf/SkPDFDevice.cpp#... src/pdf/SkPDFDevice.cpp:2225: SkScalarCeilToInt(scaleX * bounds.width()), On 2013/11/07 22:44:00, vandebo wrote: > Just use physicalPerspectiveOutline.getBounds().width(), it's the same value > with out a division and multiplication. Done. https://codereview.chromium.org/54913004/diff/440001/src/pdf/SkPDFDevice.cpp#... src/pdf/SkPDFDevice.cpp:2252: matrix.postTranslate(deltaX, deltaY); On 2013/11/07 22:44:00, vandebo wrote: > Not sure, should this be preTranslate ? gms fail badly if I use preTranslate
LGTM https://codereview.chromium.org/54913004/diff/440001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/54913004/diff/440001/src/pdf/SkPDFDevice.cpp#... src/pdf/SkPDFDevice.cpp:2198: total.postConcat(fInitialTransform); On 2013/11/11 17:26:56, edisonn wrote: > On 2013/11/07 22:44:00, vandebo wrote: > > Should this be preConcat? fInitialTransform is applied first. > > I don't think so, gms fail and they don't output at the right resolution Oops, I got Pre/Post mixed up in what they do.
Mike/Brian, could you LGTM? made a protected method a public one. Since the CL is editing public API, you must have an LGTM from one of: ('reed@chromium.org', 'reed@google.com', 'bsalomon@chromium.org', 'bsalomon@google.com')
lgtm
Message was sent while issue was closed.
Committed patchset #9 manually as r12230 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
