Closed Bug 1777209 Opened 2 months ago Closed 1 month ago

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

Categories

(Core :: Graphics, defect)

defect

Tracking

()

RESOLVED FIXED
104 Branch
Tracking Status
firefox-esr91 --- wontfix
firefox-esr102 105+ affected
firefox102 --- wontfix
firefox103 --- wontfix
firefox104 --- fixed

People

(Reporter: marco, Assigned: jfkthame)

References

Details

Attachments

(1 file)

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.

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.

Assignee: nobody → jfkthame
Status: NEW → ASSIGNED

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?

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.

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?

Attachment #9283440 - Attachment description: Bug 1777209 - Define ExtendMode::NONE and use instead of CLAMP in nsDisplayCanvas. r=jrmuizel → Bug 1777209 - Define ExtendMode::NONE and use instead of CLAMP in nsDisplayCanvas when rendering a pdf.js page. r=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.

Attachment #9283440 - Attachment description: Bug 1777209 - Define ExtendMode::NONE and use instead of CLAMP in nsDisplayCanvas when rendering a pdf.js page. r=jrmuizel → Bug 1777209 - Use DrawSurface rather than FillRect to paint <canvas>, and don't use EXTEND_PAD when writing to PDF. r=jrmuizel

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

Flags: needinfo?(jfkthame)

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

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.

Flags: needinfo?(jfkthame)

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

[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
Flags: needinfo?(jfkthame)

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

I think you might be able to use MaybeSnapToDevicePixels()

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.

Flags: needinfo?(jfkthame)

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

Pushed by jkew@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 1 month ago
Resolution: --- → FIXED
Target Milestone: --- → 104 Branch

(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!

Should we uplift to 103 and ESR 102?

Flags: needinfo?(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.)

Flags: needinfo?(jfkthame)

(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!

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.

Flags: needinfo?(jfkthame)

Given what we just discussed, wontfix for 103.

Flags: needinfo?(jfkthame)

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

See Also: → 1386573
QA Whiteboard: [qa-104b-p2]

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.
Attachment #9283440 - Flags: approval-mozilla-esr102?
You need to log in before you can comment on or make changes to this bug.