Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1925233003:
SkPDF: Use type 2/3 shading for gradient shaders (Closed)
DescriptionSkPDF: Use type 2/3 shading for gradient shaders
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1925233003
Committed: https://skia.googlesource.com/skia/+/e75cdcb85b73c4484a992bf5531394632e757870
Patch Set 1 #
Total comments: 2
Patch Set 2 : addressed hal's comment #
Total comments: 2
Patch Set 3 : fix for colorstops that are smaller than their predecessor #Patch Set 4 : add detection of coincident circles + detect perspective transform #Patch Set 5 : addressed hal's comment #Patch Set 6 : redesigned gradient stop logic #
Total comments: 1
Patch Set 7 : Added Adobe to the authors file #Patch Set 8 : removed whitespace #Messages
Total messages: 39 (12 generated)
Description was changed from ========== Moved to type 2/3 shading BUG=skia: ========== to ========== Moved to type 2/3 shading BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
halcanary@google.com changed reviewers: + halcanary@google.com
Thanks. I'm looking at it.
reed@google.com changed reviewers: + caryclark@google.com, fmalita@chromium.org, reed@google.com
I wonder if we can ever try a virtual on SkShader for PDF, so that these sorts of specializations could live in their associated specialty subclass, and not in PDF core.
On 2016/04/29 00:38:41, reed1 wrote: > I wonder if we can ever try a virtual on SkShader for PDF, so that these sorts > of specializations could live in their associated specialty subclass, and not in > PDF core. I think it would be just as messy but more spread out.
https://codereview.chromium.org/1925233003/diff/1/src/pdf/SkPDFShader.cpp File src/pdf/SkPDFShader.cpp (right): https://codereview.chromium.org/1925233003/diff/1/src/pdf/SkPDFShader.cpp#new... src/pdf/SkPDFShader.cpp:195: retval->insertObject("C0", sk_ref_sp(c0.get())); retval->insertObject("C0", c0); should work. sk_sp has a copy constructor for that. retval->insertObject("C0", std:move(c0)); is even better.
https://codereview.chromium.org/1925233003/diff/20001/src/pdf/SkPDFShader.cpp File src/pdf/SkPDFShader.cpp (right): https://codereview.chromium.org/1925233003/diff/20001/src/pdf/SkPDFShader.cpp... src/pdf/SkPDFShader.cpp:822: pdfShader->insertObject("Function", std::move(gradientStitchCode(*info))); don't need the std::move here, it is already a r-value.
Something seems to be wrong with the output, depending on the rasterizer. For example, with Pdfium, gradient_many_stops contains a purple patch where I expect red. With an older verision of Poppler, the output is completely wrong. Adobe Reader displays it correctly. I suppose I should file a Pdfium bug. By the way, is there a command-line command I can use to rasterize a PDF using Adobe Reader?
On 2016/05/04 13:57:27, Hal Canary wrote: > By the way, is there a command-line command I can use to rasterize a PDF using > Adobe Reader? What I want is a way to automate this task: "C:\Program Files (x86)\Adobe\Acrobat 11.0\Acrobat\Acrobat.exe" file.pdf "File" → "Save As Other..." → "Image" → "PNG" "Settings..." Set "Resolution" to "72 pixels/inch" "OK" "Save"
I think we want to push hard to write the most efficient PDF, assuming it works on "real" viewers. If that means we have to work on fixes for PDFium ... great!
I looked at the diffs on another rasteriser, MacOS's CoreGraphics.
The only GMs that produced concerning results are:
gradients
gradients_2pt_conical_edge
gradients_dup_color_stops
gradients_local_perspective
gradients_many
linear_gradient_tiny
On 2016/05/04 17:21:59, Hal Canary wrote: > I looked at the diffs on another rasteriser, MacOS's CoreGraphics. > > The only GMs that produced concerning results are: > > gradients > gradients_2pt_conical_edge > gradients_dup_color_stops > gradients_local_perspective > gradients_many > linear_gradient_tiny These same six GMs also have problems in Adobe Acrobat.
https://codereview.chromium.org/1925233003/diff/1/src/pdf/SkPDFShader.cpp File src/pdf/SkPDFShader.cpp (right): https://codereview.chromium.org/1925233003/diff/1/src/pdf/SkPDFShader.cpp#new... src/pdf/SkPDFShader.cpp:808: info->fTileMode == SkShader::kClamp_TileMode; It is unfortunate that this leaves the old way of doing things in cases other than clamp mode. We'd like to 'round trip' so that the PDF generated by Skia can be consumed by PDFium and generate efficient Skia pictures. The non-clamp path can't be turned back into Skia shaders.
I also think perspective is the other case we aren't handling correctly. https://codereview.chromium.org/1925233003/diff/20001/src/pdf/SkPDFShader.cpp File src/pdf/SkPDFShader.cpp (right): https://codereview.chromium.org/1925233003/diff/20001/src/pdf/SkPDFShader.cpp... src/pdf/SkPDFShader.cpp:246: } else if (offset == 0) {// if a colorstop has offset 0, remove previous stops. does this ever happen? offsets [0,0,1,1] with a clamp means that color[0] and color[3] are used outside the bounds of the gradient, and color[1] fades to color[2] are inside the gradient. I think this is the cause of several of our problems.
On 2016/05/04 14:56:12, Hal Canary wrote: > On 2016/05/04 13:57:27, Hal Canary wrote: > > By the way, is there a command-line command I can use to rasterize a PDF using > > Adobe Reader? > > What I want is a way to automate this task: > > "C:\Program Files (x86)\Adobe\Acrobat 11.0\Acrobat\Acrobat.exe" file.pdf > "File" → "Save As Other..." → "Image" → "PNG" > "Settings..." > Set "Resolution" to "72 pixels/inch" > "OK" > "Save" According to Leonard, you can't from the command line, but you can with an Action or a Preflight droplet
On 2016/05/04 17:42:59, Hal Canary wrote: > I also think perspective is the other case we aren't handling correctly. > > https://codereview.chromium.org/1925233003/diff/20001/src/pdf/SkPDFShader.cpp > File src/pdf/SkPDFShader.cpp (right): > > https://codereview.chromium.org/1925233003/diff/20001/src/pdf/SkPDFShader.cpp... > src/pdf/SkPDFShader.cpp:246: } else if (offset == 0) {// if a colorstop has > offset 0, remove previous stops. > does this ever happen? yes, there's a test file that exercises this which is why I made that change. It's up for debate what should happen but I made the PDF export match what the raster output does. > offsets [0,0,1,1] with a clamp means that color[0] and color[3] are used outside > the bounds of the gradient, and color[1] fades to color[2] are inside the > gradient. > > I think this is the cause of several of our problems.
On 2016/05/04 17:38:26, caryclark wrote: > https://codereview.chromium.org/1925233003/diff/1/src/pdf/SkPDFShader.cpp > File src/pdf/SkPDFShader.cpp (right): > > https://codereview.chromium.org/1925233003/diff/1/src/pdf/SkPDFShader.cpp#new... > src/pdf/SkPDFShader.cpp:808: info->fTileMode == SkShader::kClamp_TileMode; > It is unfortunate that this leaves the old way of doing things in cases other > than clamp mode. > > We'd like to 'round trip' so that the PDF generated by Skia can be consumed by > PDFium and generate efficient Skia pictures. The non-clamp path can't be turned > back into Skia shaders. I believe I can fix the case for all axial gradients using a tiling pattern. radial/conic gradients are harder because they draw differently. When we implemented xps -> pdf, we simply turned those gradients into images.
On 2016/05/04 17:21:59, Hal Canary wrote: > I looked at the diffs on another rasteriser, MacOS's CoreGraphics. > > The only GMs that produced concerning results are: > gradients > > > gradients_2pt_conical_edge > > > gradients_dup_color_stops > > > gradients_local_perspective > > > gradients_many > > > linear_gradient_tiny Can you check if patch #3 fixes those for you? I saw a problem with gradients where colorstops were out of order.
> Can you check if patch #3 fixes those for you? > I saw a problem with gradients where colorstops were out of order. I added special code to deal with edge cases where the inner circle meets the outer one. Raster seems to be wrong here for certain cases. I tried to apply the perspective to the endpoints of the gradient but it didn't resemble raster or the current pdf output. I will debug it some more but it's possible that I can't implement it with interpolation functions.
On 2016/05/10 03:05:25, Rik wrote: > > Can you check if patch #3 fixes those for you? > > I saw a problem with gradients where colorstops were out of order. > > I added special code to deal with edge cases where the inner circle meets the > outer one. Raster seems to be wrong here for certain cases. > > I tried to apply the perspective to the endpoints of the gradient but it didn't > resemble raster or the current pdf output. I will debug it some more but it's > possible that I can't implement it with interpolation functions. I experimented with rasterizing non-PDF conforming gradients and it's a pretty easy fix. I can add that in a new review, or update this one.
Description was changed from ========== Moved to type 2/3 shading BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Moved to type 2/3 shading BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Moved to type 2/3 shading BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== SkPDF: Use type 2/3 shading for gradient shaders BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1925233003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: skia_presubmit-Trybot on client.skia.fyi (JOB_FAILED, http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)
The CQ bit was checked by cabanier@adobe.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1925233003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, with 1 nit https://codereview.chromium.org/1925233003/diff/100001/src/pdf/SkPDFShader.cpp File src/pdf/SkPDFShader.cpp (right): https://codereview.chromium.org/1925233003/diff/100001/src/pdf/SkPDFShader.cp... src/pdf/SkPDFShader.cpp:190: nit: please remove trailing whitespace.
The CQ bit was checked by halcanary@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from halcanary@google.com Link to the patchset: https://codereview.chromium.org/1925233003/#ps140001 (title: "removed whitespace")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1925233003/140001
Message was sent while issue was closed.
Description was changed from ========== SkPDF: Use type 2/3 shading for gradient shaders BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== SkPDF: Use type 2/3 shading for gradient shaders BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/e75cdcb85b73c4484a992bf5531394632e757870 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://skia.googlesource.com/skia/+/e75cdcb85b73c4484a992bf5531394632e757870 |
