1777209 - Printing a PDF as Save to PDF produces rasterized output depending on the page format

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Marco Castelluccio [:marco]]

With the PDF from https://github.com/mozilla/pdf.js/issues/14277, printing to PDF with A4 page format gets rasterized output, while US Legal gets non-rasterized output.

With the PDF from bug 1772225, it's exactly the opposite.

Comment 1

[Jonathan Kew [:jfkthame]]

This happens when _cairo_pdf_surface_analyze_operation here decides that the pattern surface needs padding to fill the destination box. In the "bad" cases, I'm seeing a small discrepancy where the destination is a pixel or two larger in one dimension than the pattern.

AFAICT, this problem is arising due to an accumulation of floating/fixed-point errors in computations involving bounds and transforms, though I haven't pinned down exactly where things go astray. There are a bunch of things that might be contributing to this: we have a mix of double- and single-precision floating point in Gecko; we pass (single-precision) floats to cairo although its cairo_matrix_t holds doubles; but then cairo also uses 24.8 fixed-point internally (see cairo_point_t etc).

I'm concerned it may be difficult to entirely avoid this sort of discrepancy, given the mix of types that are involved in the different layers of software. Another possible way forward, though, would be if we could avoid the use of CAIRO_EXTEND_PAD on the surface pattern. If it instead had CAIRO_EXTEND_NONE, the presence of a pixel or two mismatch in extents wouldn't cause a problem here.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1777209 - Use DrawSurface rather than FillRect to paint <canvas>, and don't use EXTEND_PAD when writing to PDF. r=jrmuizel

Comment 3

[Jonathan Kew [:jfkthame]]

So here's a possible approach that seems to work for me in local testing. I can't think of any reason we'd actually want the CLAMP-type behavior here, and using NONE instead means we're immune to the small size discrepancy that's causing the problem within cairo-pdf-surface.

Jeff, WDYT -- is this reasonable?

Comment 4

[Jeff Muizelaar [:jrmuizel]]

We want CLAMP because we want the edges of the canvas to be sharp. e.g. if we have a solid red canvas we don't want the edge to fade out to 0/NONE we want it to have a sharp edge between red and the background.

So this isn't really an approach I want to take.

Comment 5

[Jonathan Kew [:jfkthame]]

Hmm. How about if we do this only when the canvas we're rendering is actually a pdf.js page, rather than an arbitrary <canvas> element within an HTML page?

Comment 6

[Jeff Muizelaar [:jrmuizel]]

If we moved canvas painting from Fill to DrawSurface, it would be easy for DrawTargetCairo to use NONE instead of PAD when drawing to a pdf-surface. I'd prefer that vs exposing NONE on all the backends and special casing the canvas painting.

Comment 7

[bszekely]

Backed out for causing multiple failures

Failure line 1: TEST-UNEXPECTED-FAIL | layout/generic/test/test_bug449653.html | No red should be shown
Failure log 1

[task 2022-07-01T14:58:23.704Z] 14:58:23     INFO - TEST-START | layout/generic/test/test_bug449653.html
[task 2022-07-01T14:58:23.705Z] 14:58:23     INFO - TEST-INFO | started process screencapture
[task 2022-07-01T14:58:23.772Z] 14:58:23     INFO - TEST-INFO | screencapture: exit 0
[task 2022-07-01T14:58:23.773Z] 14:58:23     INFO - TEST-UNEXPECTED-FAIL | layout/generic/test/test_bug449653.html | No red should be shown 
[task 2022-07-01T14:58:23.773Z] 14:58:23     INFO -     SimpleTest.ok@SimpleTest/SimpleTest.js:417:16
[task 2022-07-01T14:58:23.773Z] 14:58:23     INFO -     doTest@layout/generic/test/test_bug449653.html:35:5
[task 2022-07-01T14:58:23.773Z] 14:58:23     INFO -     onload@layout/generic/test/test_bug449653.html:1:1
[task 2022-07-01T14:58:23.773Z] 14:58:23     INFO - GECKO(2609) | MEMORY STAT | vsize 6723MB | residentFast 124MB | heapAllocated 15MB
[task 2022-07-01T14:58:23.774Z] 14:58:23     INFO - TEST-OK | layout/generic/test/test_bug449653.html | took 109ms
[task 2022-07-01T14:58:23.775Z] 14:58:23     INFO - TEST-START | layout/generic/test/test_bug460532.html

Failure line 2: REFTEST TEST-UNEXPECTED-FAIL | layout/reftests/bugs/602200-1.html == layout/reftests/bugs/602200-1-ref.html | image comparison, max difference: 111, number of differing pixels: 400
Failure log 2
Reftest analyzer

Test image is slightly thicker than the reference image

Push with failures

Backout link

Comment 8

[Jeff Muizelaar [:jrmuizel]]

It looks like we're not snapping when previously we would.

Comment 9

[Jonathan Kew [:jfkthame]]

It looks like just doing destGFXRect.Round() before we pass it to DrawSurface will fix the failures here. Previously we were relying on modifying the transform to put the canvas rendering in the right place/size, and we used SnapTransform in that codepath, but that's gone now.

Comment 10

[Natalia Csoregi [:nataliaCs]]

Backed out for causing reftest failures e.g. 602200-4.html

• backout: https://hg.mozilla.org/integration/autoland/rev/024ee66c8b815cb95c4e47a4bcfd4a017a753bca
• push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&revision=1b43d3f389468361fdead22a9e90d13aa6c87bab
• failure log: https://treeherder.mozilla.org/logviewer?job_id=383301171&repo=autoland&lineNumber=11135

[task 2022-07-04T15:30:43.704Z] 15:30:43     INFO - REFTEST TEST-START | layout/reftests/bugs/602200-4.html == layout/reftests/bugs/602200-4-ref.html
[task 2022-07-04T15:30:43.706Z] 15:30:43     INFO - REFTEST TEST-LOAD | file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/bugs/602200-4.html | 1527 / 2068 (73%)
[task 2022-07-04T15:30:43.828Z] 15:30:43     INFO - REFTEST TEST-LOAD | file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/bugs/602200-4-ref.html | 1527 / 2068 (73%)
[task 2022-07-04T15:30:44.297Z] 15:30:44     INFO - REFTEST TEST-UNEXPECTED-FAIL | layout/reftests/bugs/602200-4.html == layout/reftests/bugs/602200-4-ref.html | image comparison, max difference: 52, number of differing pixels: 20

Comment 11

[Jeff Muizelaar [:jrmuizel]]

I think this failure is caused by not taking the context's transform into account when snapping.

Comment 12

[Jeff Muizelaar [:jrmuizel]]

I think you might be able to use MaybeSnapToDevicePixels()

Comment 13

[Jonathan Kew [:jfkthame]]

Yeah, I'll give that a try. A bit weird that it only failed on a reftest-snapshot job, "normal" reftests seem fine. But I'm not too clear what the differences are.

Anyhow, I'll try the snap method.

Comment 14

[Jeff Muizelaar [:jrmuizel]]

reftest-snapshot doesn't use WebRender and instead uses nsDisplayItem::Paint. "normal" reftests won't hit this code path very often because they go through nsDisplayItem::CreateWebRenderCommands

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7d9376649d6d
Use DrawSurface rather than FillRect to paint <canvas>, and don't use EXTEND_PAD when writing to PDF. r=jrmuizel

Comment 16

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/7d9376649d6d

Comment 17

[Marco Castelluccio [:marco]]

(In reply to Marco Castelluccio [:marco] from comment #0)

With the PDF from https://github.com/mozilla/pdf.js/issues/14277, printing to PDF with A4 page format gets rasterized output, while US Legal gets non-rasterized output.

With the PDF from bug 1772225, it's exactly the opposite.

I can confirm this is no longer happening. Thanks Jonathan and Jeff!

Comment 18

[Marco Castelluccio [:marco]]

Should we uplift to 103 and ESR 102?

Comment 19

[Jonathan Kew [:jfkthame]]

Given that the patch here is a significant change to the code-path being used, and not well covered by extensive automated tests, I'd like to give it a bit of time on Nightly before considering uplift.

Not sure how I feel about 103 -- it's not like this is a recent regression or a crippling failure, so maybe it should just ride the 104 train? I do think we should uplift to ESR once we're more confident we haven't regressed anything, but maybe that should happen after this has hit beta.

(If you want to argue for a more rapid process, feel free to make the case.... I'm not necessarily committed to a specific timeline, just felt like we should move a bit cautiously.)

Comment 20

[Marco Castelluccio [:marco]]

(In reply to Jonathan Kew (:jfkthame) from comment #19)

Given that the patch here is a significant change to the code-path being used, and not well covered by extensive automated tests, I'd like to give it a bit of time on Nightly before considering uplift.

Yeah, we should really address the lack of automated tests there... We are planning to add some in pdf.js: https://github.com/mozilla/pdf.js/issues/13853.

Not sure how I feel about 103 -- it's not like this is a recent regression or a crippling failure, so maybe it should just ride the 104 train? I do think we should uplift to ESR once we're more confident we haven't regressed anything, but maybe that should happen after this has hit beta.

(If you want to argue for a more rapid process, feel free to make the case.... I'm not necessarily committed to a specific timeline, just felt like we should move a bit cautiously.)

Riding the trains sounds fine to me, we have had this bug for a long time anyway!

Comment 21

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:jfkthame, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox103 to wontfix.

For more information, please visit auto_nag documentation.

Comment 22

[Marco Castelluccio [:marco]]

Given what we just discussed, wontfix for 103.

Comment 23

[Marco Castelluccio [:marco]]

[Tracking Requested - why for this release]: Important for enterprise use cases.

Comment 24

[Jonathan Kew [:jfkthame]]

Comment on attachment 9283440 [details]
Bug 1777209 - Use DrawSurface rather than FillRect to paint <canvas>, and don't use EXTEND_PAD when writing to PDF. r=jrmuizel

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This patch avoids spuriously rasterizing all the page content in some cases when writing to a PDF. For users who often save PDF versions of pages/documents, this is a significant improvement in their experience.
• User impact if declined: Some Save-to- PDF output files may be rasterized, making the resulting file much larger, lower quality, and less useful -- no search, no copy/paste of text, etc.
• Fix Landed on Version: 104
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): This significantly changes the code path used when "printing" to a PDF, so feels like it does carry some risk, but the change landed a month ago now and no regressions have been noted in Nightly/Beta.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9283440 [details]
Bug 1777209 - Use DrawSurface rather than FillRect to paint <canvas>, and don't use EXTEND_PAD when writing to PDF. r=jrmuizel

Approved for 102.3esr.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/7c1c700c6f36