Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
26389006:
PDF: support perspective in simple shaders. (this version does not work well with tilling) (Closed)
|
Created:
7 years, 2 months ago by edisonn Modified:
7 years, 1 month ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionPDF: support perspective in simple shaders. (this version does not work well with tilling)
R=vandebo@chromium.org
Committed: https://code.google.com/p/skia/source/detail?r=11937
Patch Set 1 #
Total comments: 31
Patch Set 2 : code updates after code review #Patch Set 3 : previous upload failed #
Total comments: 6
Patch Set 4 : fix typo #Patch Set 5 : cmputre total matrix before extracting gradient #Patch Set 6 : revert gm change #Patch Set 7 : update comments #Patch Set 8 : revert white space changes in gm\ #
Total comments: 4
Patch Set 9 : code review updates + return from ContentEntry if perspective is used in matrix, in adittion to rep… #Messages
Total messages: 7 (0 generated)
Approach looks good. Question about inverse vs not perspective matrix and nits. It seems that tilling should work fine because the the perspective is applied before t is calculated, so the perspective should work out - if it's not the inverse/non-inverse issue then maybe the perspective needs to be applied in a different order? https://codereview.chromium.org/26389006/diff/1/src/pdf/SkPDFShader.cpp File src/pdf/SkPDFShader.cpp (right): https://codereview.chromium.org/26389006/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:212: Add a comment and a better name. apply_perspective_to_coordinates ? https://codereview.chromium.org/26389006/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:213: static SkString transformCoordonates(const SkMatrix& xy2xy) { nit: transform_coordinates (hacker style and spelling) https://codereview.chromium.org/26389006/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:228: // y = y / (p2 + p0 x + p1 y) SkMatrix.cpp:1201 suggests that this should be *, not / - maybe why you ended up with the inverse perspective matrix? https://codereview.chromium.org/26389006/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:236: code.append(" mul "); // x y y*p1 Consider the following formatting to be consistent with the rest of the file: code.append("dup "); // x y y code.appendScalar(p1); // x y y p1 code.append(" mul " // x y y*p1 " 2 index "); // x y y*p1 x code.appendScalar(p0); // x y y p1 x p0 code.append(" mul "); // x y y*p1 x*p0 code.appendScalar(p2); // x y y p1 x*p0 p2 code.append("add " // x y y*p1 x*p0+p2 "add " // x y y*p1+x*p0+p2 "1 index " // x y y*p1+x*p0+p2 y "1 index " // x y y*p1+x*p0+p2 y y*p1+x*p0+p2 "div " // x y y*p1+x*p0+p2 y/(y*p1+x*p0+p2) "4 1 roll " // y/(y*p1+x*p0+p2) x y y*p1+x*p0+p2 "exch " // y/(y*p1+x*p0+p2) x y*p1+x*p0+p2 y "pop " // y/(y*p1+x*p0+p2) x y*p1+x*p0+p2 "div " // y/(y*p1+x*p0+p2) x/(y*p1+x*p0+p2) "exch\n"); // x/(y*p1+x*p0+p2) y/(y*p1+x*p0+p2) https://codereview.chromium.org/26389006/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:242: code.append(" add "); // x y y*p1 x*p0+p2 nit: extra space between adds (several more below) https://codereview.chromium.org/26389006/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:244: code.appendS32(1); // x y y*p1+x*p0+p2 1 If you infact want mul and not div, consider this sequence, it's a couple instructions shorter: code.append("add " // x y y*p1 x*p0+p2 "add " // x y z "3 1 roll " // z x y "2 index " // z x y z "mul " // z x y*z "3 1 roll " // y*z z x "mul " // y*z z*x "exch\n"); // x*z y*z https://codereview.chromium.org/26389006/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:796: static bool splitPerspective(const SkMatrix in, SkMatrix* simple, nit: split_perspective https://codereview.chromium.org/26389006/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:796: static bool splitPerspective(const SkMatrix in, SkMatrix* simple, nit: simple -> affine (also update comment) https://codereview.chromium.org/26389006/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:804: SkScalar zero = SkIntToScalar(0); nit: make these all const ? https://codereview.chromium.org/26389006/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:825: simple->setAll(sx - p0 * tx / p2, kx - p1 * tx / p2, tx / p2, Isn't this simple->SetConcat(in, *perspectiveInverse) ? https://codereview.chromium.org/26389006/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:896: // 3 numbers (p0, p1, p2)so the shader will handle it eficiently. This comment isn't consistent with the code (perspective vs perspective inverse) https://codereview.chromium.org/26389006/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:934: functionCode = codeFunction(twoPointRadialInfo, perspectiveInverseOnly); I often get it wrong, so please correct me. Don't we want to pass the perspective matrix down (not the inverse)? The shader matrix doesn't contain any perspective, so it still needs to be applied...
https://codereview.chromium.org/26389006/diff/1/src/pdf/SkPDFShader.cpp File src/pdf/SkPDFShader.cpp (right): https://codereview.chromium.org/26389006/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:212: On 2013/10/17 18:19:36, vandebo wrote: > Add a comment and a better name. apply_perspective_to_coordinates ? Done. https://codereview.chromium.org/26389006/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:213: static SkString transformCoordonates(const SkMatrix& xy2xy) { On 2013/10/17 18:19:36, vandebo wrote: > nit: transform_coordinates (hacker style and spelling) Done. https://codereview.chromium.org/26389006/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:213: static SkString transformCoordonates(const SkMatrix& xy2xy) { On 2013/10/17 18:19:36, vandebo wrote: > nit: transform_coordinates (hacker style and spelling) Done. https://codereview.chromium.org/26389006/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:228: // y = y / (p2 + p0 x + p1 y) On 2013/10/17 18:19:36, vandebo wrote: > SkMatrix.cpp:1201 suggests that this should be *, not / - maybe why you ended up > with the inverse perspective matrix? It is the inverse perspective matrix. I updated the name. https://codereview.chromium.org/26389006/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:228: // y = y / (p2 + p0 x + p1 y) On 2013/10/17 18:19:36, vandebo wrote: > SkMatrix.cpp:1201 suggests that this should be *, not / - maybe why you ended up > with the inverse perspective matrix? it is the inverse perspective matrix https://codereview.chromium.org/26389006/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:236: code.append(" mul "); // x y y*p1 On 2013/10/17 18:19:36, vandebo wrote: > Consider the following formatting to be consistent with the rest of the file: > code.append("dup "); // x y y > code.appendScalar(p1); // x y y p1 > code.append(" mul " // x y y*p1 > " 2 index "); // x y y*p1 x > code.appendScalar(p0); // x y y p1 x p0 > code.append(" mul "); // x y y*p1 x*p0 > code.appendScalar(p2); // x y y p1 x*p0 p2 > code.append("add " // x y y*p1 x*p0+p2 > "add " // x y y*p1+x*p0+p2 > "1 index " // x y y*p1+x*p0+p2 y > "1 index " // x y y*p1+x*p0+p2 y y*p1+x*p0+p2 > "div " // x y y*p1+x*p0+p2 y/(y*p1+x*p0+p2) > "4 1 roll " // y/(y*p1+x*p0+p2) x y y*p1+x*p0+p2 > "exch " // y/(y*p1+x*p0+p2) x y*p1+x*p0+p2 y > "pop " // y/(y*p1+x*p0+p2) x y*p1+x*p0+p2 > "div " // y/(y*p1+x*p0+p2) x/(y*p1+x*p0+p2) > "exch\n"); // x/(y*p1+x*p0+p2) y/(y*p1+x*p0+p2) Done. https://codereview.chromium.org/26389006/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:242: code.append(" add "); // x y y*p1 x*p0+p2 On 2013/10/17 18:19:36, vandebo wrote: > nit: extra space between adds (several more below) Done. https://codereview.chromium.org/26389006/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:244: code.appendS32(1); // x y y*p1+x*p0+p2 1 On 2013/10/17 18:19:36, vandebo wrote: > If you infact want mul and not div, consider this sequence, it's a couple > instructions shorter: > > code.append("add " // x y y*p1 x*p0+p2 > "add " // x y z > "3 1 roll " // z x y > "2 index " // z x y z > "mul " // z x y*z > "3 1 roll " // y*z z x > "mul " // y*z z*x > "exch\n"); // x*z y*z I need div https://codereview.chromium.org/26389006/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:796: static bool splitPerspective(const SkMatrix in, SkMatrix* simple, On 2013/10/17 18:19:36, vandebo wrote: > nit: split_perspective Done. https://codereview.chromium.org/26389006/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:796: static bool splitPerspective(const SkMatrix in, SkMatrix* simple, On 2013/10/17 18:19:36, vandebo wrote: > nit: simple -> affine (also update comment) Done. https://codereview.chromium.org/26389006/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:804: SkScalar zero = SkIntToScalar(0); On 2013/10/17 18:19:36, vandebo wrote: > nit: make these all const ? Done. https://codereview.chromium.org/26389006/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:825: simple->setAll(sx - p0 * tx / p2, kx - p1 * tx / p2, tx / p2, On 2013/10/17 18:19:36, vandebo wrote: > Isn't this simple->SetConcat(in, *perspectiveInverse) ? I would prefer not to add up errors in the matrix. I would rather use the exact formulas. https://codereview.chromium.org/26389006/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:896: // 3 numbers (p0, p1, p2)so the shader will handle it eficiently. On 2013/10/17 18:19:36, vandebo wrote: > This comment isn't consistent with the code (perspective vs perspective inverse) Done. https://codereview.chromium.org/26389006/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:934: functionCode = codeFunction(twoPointRadialInfo, perspectiveInverseOnly); On 2013/10/17 18:19:36, vandebo wrote: > I often get it wrong, so please correct me. Don't we want to pass the > perspective matrix down (not the inverse)? The shader matrix doesn't contain > any perspective, so it still needs to be applied... First, the two point radial gm looks fine to me. Second, I think not. I want the inverse of the perspective, because when I apply it the point is in perspective space, so with the inverse perspective I move the point in the space without perspective where our shader code works fine.
I tried the gradients gm with this change and noticed that the second, third and fourth rows render as linear gradients in the chrome plugin. poppler seems to do the right thing on the fourth row so it may be a bug there. Can you take a look at the output to see if it's most likely a bug in the chrome plugin? Also, the direction of the gradient in the first row is flipped in the chrome plugin. https://codereview.chromium.org/26389006/diff/1/src/pdf/SkPDFShader.cpp File src/pdf/SkPDFShader.cpp (right): https://codereview.chromium.org/26389006/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:228: // y = y / (p2 + p0 x + p1 y) On 2013/10/18 16:10:13, edisonn wrote: > On 2013/10/17 18:19:36, vandebo wrote: > > SkMatrix.cpp:1201 suggests that this should be *, not / - maybe why you ended > up > > with the inverse perspective matrix? > > it is the inverse perspective matrix Regardless of it being the inverse matrix or not, SkMatrix.cpp:1201 says that the way to apply a matrix containing perspective is with multiplication, not division. Please help me understand why division is the right thing here. https://codereview.chromium.org/26389006/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:934: functionCode = codeFunction(twoPointRadialInfo, perspectiveInverseOnly); On 2013/10/18 16:10:13, edisonn wrote: > On 2013/10/17 18:19:36, vandebo wrote: > > I often get it wrong, so please correct me. Don't we want to pass the > > perspective matrix down (not the inverse)? The shader matrix doesn't contain > > any perspective, so it still needs to be applied... > > First, the two point radial gm looks fine to me. > Second, I think not. I want the inverse of the perspective, because when I apply > it the point is in perspective space, so with the inverse perspective I move the > point in the space without perspective where our shader code works fine. Using the forward perspective matrix here and switching to multiply in apply_perspective_to_coordinates() seems to give the same results as what you have. In the non-perspective case, the shader coordinates (those going through the ps function) are in a unit gradient space. The shader matrix transforms them to the canvas space. The shader matrix first goes from the unit gradient space to the specified gradient space, then through the shader transform, and finally uses the canvas matrix to get to the canvas coordinate space. It's similar in the perspective space. The ps function operates in the unit shader coordinate space, you then manually apply the perspective transform and finally finish the transform to the canvas space. https://codereview.chromium.org/26389006/diff/87001/src/pdf/SkPDFShader.cpp File src/pdf/SkPDFShader.cpp (right): https://codereview.chromium.org/26389006/diff/87001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:214: * Returns PS function code that would apply perspective to a x, y point. nit: would apply -> applies https://codereview.chromium.org/26389006/diff/87001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:217: * After ececuting this code on a PS stack, the last 2 elements are updated nit: ececuting -> executing https://codereview.chromium.org/26389006/diff/87001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:219: * xy2xy is the inverse perspective matrix. nit: update or remove
fixed the code. The issue was that perspective should have been extracted after mapper matrix was applied to final matrix to see results, please run the gm with out/Debug/gm -w FOLDER --useDocumentInsteadOfDevice --forcePerspectiveMatrix --match ~scaled_tilemodes ~convexpaths ~clipped-bitmap (a few gms are crashing in non pdf code, and needs to be skipped - the is a bug associated) https://codereview.chromium.org/26389006/diff/1/src/pdf/SkPDFShader.cpp File src/pdf/SkPDFShader.cpp (right): https://codereview.chromium.org/26389006/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:228: // y = y / (p2 + p0 x + p1 y) On 2013/10/21 19:23:18, vandebo wrote: > On 2013/10/18 16:10:13, edisonn wrote: > > On 2013/10/17 18:19:36, vandebo wrote: > > > SkMatrix.cpp:1201 suggests that this should be *, not / - maybe why you > ended > > up > > > with the inverse perspective matrix? > > > > it is the inverse perspective matrix > > Regardless of it being the inverse matrix or not, SkMatrix.cpp:1201 says that > the way to apply a matrix containing perspective is with multiplication, not > division. Please help me understand why division is the right thing here. It is the inverse perspective, it moves the point from a a space with perspective, in a space without the perspective https://codereview.chromium.org/26389006/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:934: functionCode = codeFunction(twoPointRadialInfo, perspectiveInverseOnly); On 2013/10/21 19:23:18, vandebo wrote: > On 2013/10/18 16:10:13, edisonn wrote: > > On 2013/10/17 18:19:36, vandebo wrote: > > > I often get it wrong, so please correct me. Don't we want to pass the > > > perspective matrix down (not the inverse)? The shader matrix doesn't > contain > > > any perspective, so it still needs to be applied... > > > > First, the two point radial gm looks fine to me. > > Second, I think not. I want the inverse of the perspective, because when I > apply > > it the point is in perspective space, so with the inverse perspective I move > the > > point in the space without perspective where our shader code works fine. > > Using the forward perspective matrix here and switching to multiply in > apply_perspective_to_coordinates() seems to give the same results as what you > have. > > In the non-perspective case, the shader coordinates (those going through the ps > function) are in a unit gradient space. The shader > matrix transforms them to the canvas space. The shader matrix first goes from > the unit gradient space to the specified gradient space, then through the shader > transform, and finally uses the canvas matrix to get to the canvas coordinate > space. > > It's similar in the perspective space. The ps function operates in the unit > shader coordinate space, you then manually apply the perspective transform and > finally finish the transform to the canvas space. updated the code, the perspective is removed from the final matrix after applying the mapper matrix https://codereview.chromium.org/26389006/diff/87001/src/pdf/SkPDFShader.cpp File src/pdf/SkPDFShader.cpp (right): https://codereview.chromium.org/26389006/diff/87001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:214: * Returns PS function code that would apply perspective to a x, y point. On 2013/10/21 19:23:19, vandebo wrote: > nit: would apply -> applies Done. https://codereview.chromium.org/26389006/diff/87001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:217: * After ececuting this code on a PS stack, the last 2 elements are updated On 2013/10/21 19:23:19, vandebo wrote: > nit: ececuting -> executing Done. https://codereview.chromium.org/26389006/diff/87001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:219: * xy2xy is the inverse perspective matrix. On 2013/10/21 19:23:19, vandebo wrote: > nit: update or remove Done.
LGTM https://codereview.chromium.org/26389006/diff/1/src/pdf/SkPDFShader.cpp File src/pdf/SkPDFShader.cpp (right): https://codereview.chromium.org/26389006/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:934: functionCode = codeFunction(twoPointRadialInfo, perspectiveInverseOnly); On 2013/10/23 19:45:17, edisonn wrote: > On 2013/10/21 19:23:18, vandebo wrote: > > On 2013/10/18 16:10:13, edisonn wrote: > > > On 2013/10/17 18:19:36, vandebo wrote: > > > > I often get it wrong, so please correct me. Don't we want to pass the > > > > perspective matrix down (not the inverse)? The shader matrix doesn't > > contain > > > > any perspective, so it still needs to be applied... > > > > > > First, the two point radial gm looks fine to me. > > > Second, I think not. I want the inverse of the perspective, because when I > > apply > > > it the point is in perspective space, so with the inverse perspective I move > > the > > > point in the space without perspective where our shader code works fine. > > > > Using the forward perspective matrix here and switching to multiply in > > apply_perspective_to_coordinates() seems to give the same results as what you > > have. > > > > In the non-perspective case, the shader coordinates (those going through the > ps > > function) are in a unit gradient space. The shader > > matrix transforms them to the canvas space. The shader matrix first goes > from > > the unit gradient space to the specified gradient space, then through the > shader > > transform, and finally uses the canvas matrix to get to the canvas coordinate > > space. > > > > It's similar in the perspective space. The ps function operates in the unit > > shader coordinate space, you then manually apply the perspective transform and > > finally finish the transform to the canvas space. > > updated the code, the perspective is removed from the final matrix after > applying the mapper matrix I don't understand why my argument is wrong, but the code seems to work, so I guess I am. https://codereview.chromium.org/26389006/diff/247001/src/pdf/SkPDFShader.cpp File src/pdf/SkPDFShader.cpp (right): https://codereview.chromium.org/26389006/diff/247001/src/pdf/SkPDFShader.cpp#... src/pdf/SkPDFShader.cpp:244: code.append("dup "); // x y y nit: add space here instead of line 241 https://codereview.chromium.org/26389006/diff/247001/src/pdf/SkPDFShader.cpp#... src/pdf/SkPDFShader.cpp:251: code.append(" add " // x y y*p1 x*p0+p2 nit: this is one op shorter, code.append(" add " // x y y*p1 x*p0+p2 "add " // x y z "3 1 roll " // z x y "2 index " // z x y z "div " // z x y/z "3 1 roll " // y/z z x "exch " // y/z x z "div " // y/z x/z "exch\n" // x/z y/z https://codereview.chromium.org/26389006/diff/247001/src/pdf/SkPDFShader.cpp#... src/pdf/SkPDFShader.cpp:893: SkMatrix perspectiveInverseOnly = SkMatrix::I(); nit: move to line 900 https://codereview.chromium.org/26389006/diff/247001/src/pdf/SkPDFShader.cpp#... src/pdf/SkPDFShader.cpp:897: nit: remove blank line
Message was sent while issue was closed.
Committed patchset #9 manually as r11937 (presubmit successful). |
