Implement printing via Skia PDF for macOS

RESOLVED FIXED in mozilla54

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

(Blocks 4 bugs)

Trunk
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 wontfix)

Details

(Whiteboard: [behind pref print.print_via_pdf_encoder=skia-pdf])

User Story

To use this functionality you need to build with:

  ac_add_options --enable-skia-pdf

and set the pref:

  print.print_via_pdf_encoder=skia-pdf

Attachments

(8 attachments, 8 obsolete attachments)

2.12 KB, patch
mchang
: review+
Details | Diff | Splinter Review
5.70 KB, patch
lsalzman
: review+
Details | Diff | Splinter Review
6.14 KB, patch
lsalzman
: review+
Details | Diff | Splinter Review
7.24 KB, patch
lsalzman
: review+
Details | Diff | Splinter Review
14.29 KB, patch
lsalzman
: review+
Details | Diff | Splinter Review
7.15 KB, patch
lsalzman
: review+
Details | Diff | Splinter Review
9.36 KB, patch
lsalzman
: review+
Details | Diff | Splinter Review
10.21 KB, patch
lsalzman
: review+
Details | Diff | Splinter Review
Use SkPDF to print whenever PDF is supported.
Component: Graphics → Printing: Output
Blocks: 1278259
Before we do this we need to improve the state of our printing tests.
Depends on: 1299848
We should definitely do that before shipping any substantial changes like this. I'd like to get some code landed behind a pref for testing though.
Lee, this fixes up the generated files directly, but doesn't include any of the changes needed in the generator scripts. Would you be able to fix those scripts when you land your Skia 55 update?
Attachment #8804771 - Flags: review?(lsalzman)
Attachment #8804775 - Flags: review?(lsalzman)
Comment on attachment 8804775 [details] [diff] [review]
part 3 - Implement PrintTargetSkPDF

Review of attachment 8804775 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/PrintTargetSkPDF.cpp
@@ +14,5 @@
> +PrintTargetSkPDF::PrintTargetSkPDF(const IntSize& aSize,
> +                                   SkDocument* aPDFDoc,
> +                                   UniquePtr<SkWStream> aStream)
> +  : PrintTarget(/* not using cairo_surface_t */ nullptr, aSize)
> +  , mPDFDoc(aPDFDoc)

Beware: sk_sp does not ref in its explicit constructor, so you want to use sk_ref_sp(aPDFDoc) here.

@@ +77,5 @@
> +
> +nsresult
> +PrintTargetSkPDF::BeginPage()
> +{
> +  mPageCanvas = mPDFDoc->beginPage(mSize.width, mSize.height);

You would want to use sk_ref_sp(...) here as well.

::: gfx/thebes/PrintTargetSkPDF.h
@@ +50,5 @@
> +  // The stream that the SkDocument outputs to.
> +  UniquePtr<SkWStream> mOStream;
> +
> +  // The SkCanvas and its wrapping DrawTarget for the current page:
> +  SkCanvas* mPageCanvas;

This should use sk_sp<SkCanvas>, otherwise this will end up leaking.
Comment on attachment 8804773 [details] [diff] [review]
part 2 - Support the creation of a DrawTargetSkia for an SkCanvas

Review of attachment 8804773 [details] [diff] [review]:
-----------------------------------------------------------------

For this to work I will need to add another patch to this bug that allows DrawTargetSkia to work with just an SkCanvas without an accompanying SkSurface (a change that came along with the Skia update). Otherwise, this should be okay.

::: gfx/2d/DrawTargetSkia.cpp
@@ +1682,5 @@
> +  bool isBackedByPixels = imageInfo.colorType() != kUnknown_SkColorType;
> +  if (isBackedByPixels) {
> +    // Note for PDF backed SkCanvas |alphaType == kUnknown_SkAlphaType|.
> +    SkAlphaType alphaType = imageInfo.alphaType();
> +    SkColor clearColor = (alphaType == kOpaque_SkAlphaType) ? SK_ColorBLACK : SK_ColorTRANSPARENT;

It would be much easier here to just do:
imageInfo.isOpaque() ? ... : ...
This is required for SkPDF to work, since it only gives us an SkCanvas without a wrapping SkSurface.
Attachment #8804825 - Flags: review?(mchang)
Attachment #8804825 - Flags: review?(mchang) → review+
Attachment #8804771 - Attachment is obsolete: true
Attachment #8804771 - Flags: review?(lsalzman)
Attachment #8805359 - Flags: review?(lsalzman)
Attachment #8804773 - Attachment is obsolete: true
Attachment #8804773 - Flags: review?(lsalzman)
Attachment #8805360 - Flags: review?(lsalzman)
Attachment #8805360 - Attachment is obsolete: true
Attachment #8805360 - Flags: review?(lsalzman)
Attachment #8805363 - Flags: review?(lsalzman)
Comment on attachment 8805363 [details] [diff] [review]
part 2 - Support the creation of a DrawTargetSkia for an SkCanvas

Review of attachment 8805363 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/Factory.cpp
@@ +704,5 @@
>  
>  #endif // USE_SKIA_GPU
>  
> +already_AddRefed<DrawTarget>
> +Factory::CreateDrawTargetWithSkCanvas(SkCanvas* aCanvas)

Needs to be guarded by #ifdef USE_SKIA, otherwise it won't compile with Skia not build in (a config we still support).
Attachment #8805359 - Attachment is obsolete: true
Attachment #8805359 - Flags: review?(lsalzman)
Attachment #8805399 - Flags: review?(lsalzman)
Attachment #8805363 - Attachment is obsolete: true
Attachment #8805363 - Flags: review?(lsalzman)
Attachment #8805400 - Flags: review?(lsalzman)
Attachment #8805400 - Flags: review?(lsalzman) → review+
Attachment #8805399 - Attachment is obsolete: true
Attachment #8805399 - Flags: review?(lsalzman)
Attachment #8805521 - Flags: review?(lsalzman)
Attachment #8805521 - Flags: review?(lsalzman) → review+
Keywords: leave-open
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/89304c262266
part 1 - Add an --enable-skia-pdf configuration option. r=lsalzman
https://hg.mozilla.org/integration/mozilla-inbound/rev/c974afea821b
part 2 - Support the creation of a DrawTargetSkia for an SkCanvas. r=lsalzman
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee15183d0572
part 3 - Allow DrawTargetSkia to work without an SkSurface. r=mchang
Depends on: 1316299
Comment on attachment 8804775 [details] [diff] [review]
part 3 - Implement PrintTargetSkPDF

Just closing this out as r- for now since there are still the above review comments that need addressing.
Attachment #8804775 - Flags: review?(lsalzman) → review-
Attachment #8804825 - Attachment description: part 4 - allow DrawTargetSkia to work without an SkSurface → part 3 - allow DrawTargetSkia to work without an SkSurface
Attachment #8804775 - Attachment is obsolete: true
Attachment #8814359 - Flags: review?(lsalzman)
Right now nsDeviceContext::BeginPage etc. have hacks to replace the PrintTargetCG for each page that is printed, because the CGGraphicsContext returned by PMSessionGetCGGraphicsContext is only valid for a single page. We should really be encapsulating that in the PrintTargetCG::BeginPage/EndPage methods. Doing that and removing the hacks means that I don't need to add hacks to the hacks in order to avoid replacing the PrintTarget for each page when we use PrintTargetSkPDF (the use of which will depends on a configure option, a pref and the exact printing action of the user.
Attachment #8814362 - Flags: review?(lsalzman)
Factoring this out from the next patch in the series to make that patch a bit cleaner.
Attachment #8814364 - Flags: review?(lsalzman)
This moves logic to where it really belongs, and makes the code that touches nsDeviceContextSpecX in the next patch cleaner by requiring fewer ifdefs.
Attachment #8814366 - Flags: review?(lsalzman)
To use this functionality you need to build with:

  ac_add_options --enable-skia-pdf

and set the pref:

  print.print_via_pdf_encoder=skia-pdf
Attachment #8814367 - Flags: review?(lsalzman)
Summary: Use SkPDF (Skia PDF) to print whenever PDF is supported → Implement printing via Skia PDF for macOS
User Story: (updated)
Attachment #8814367 - Attachment is obsolete: true
Attachment #8814367 - Flags: review?(lsalzman)
Attachment #8814390 - Flags: review?(lsalzman)
Attachment #8814359 - Flags: review?(lsalzman) → review+
Attachment #8814364 - Flags: review?(lsalzman) → review+
Attachment #8814362 - Flags: review?(lsalzman) → review+
Attachment #8814366 - Flags: review?(lsalzman) → review+
Attachment #8814390 - Flags: review?(lsalzman) → review+
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/125e6fb37319
part 4 - Implement a PrintTarget sub-class for printing via Skia PDF. r=lsalzman
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9f6271115de
part 5 - Rework the macOS printing code to get rid of the hacks that create a new PrintTarget for each page. r=lsalzman
https://hg.mozilla.org/integration/mozilla-inbound/rev/31fdab31ab51
part 6 - Add page start/end arguments to the PrintTarget::BeginPrinting virtual method and overloads. r=lsalzman
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2046ba27486
part 7 - Restructure the PMSessionBeginCGDocumentNoDialog related code to live in PrintTargetCG. r=lsalzman
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ecb0db80f15
part 8 - Implement printing via Skia PDF for macOS (behind pref print.print_via_pdf_encoder=skia-pdf). r=lsalzman
On the push after this landed, osx static builds started failing like https://treeherder.mozilla.org/logviewer.html#?job_id=39956059&repo=mozilla-inbound and haven't stopped failing since.

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/bc39d8cde9ab7f7615a0098f17396691c7354e86
Flags: needinfo?(jwatt)
If it matters, the static build on the push after yours was a clobber build.
Apparently you're supposed to clobber whenever you rename a file that creates an object file with same name as the previous file (as I did in part 5, renaming PrintTargetCG.cpp to PrintTargetCG.mm). I'll reland touching CLOBBER.
Flags: needinfo?(jwatt)
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/44360d87a266
part 4 - Implement a PrintTarget sub-class for printing via Skia PDF. r=lsalzman
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5c8bd440978
part 5 - Rework the macOS printing code to get rid of the hacks that create a new PrintTarget for each page. r=lsalzman
Were there other things still needing to be re-landed here? I backed out patches 4-8 but you've only relanded 4-5.
Flags: needinfo?(jwatt)
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ed482c5658d
part 6 - Add page start/end arguments to the PrintTarget::BeginPrinting virtual method and overloads. r=lsalzman
https://hg.mozilla.org/integration/mozilla-inbound/rev/39c61d16e30f
part 7 - Restructure the PMSessionBeginCGDocumentNoDialog related code to live in PrintTargetCG. r=lsalzman
https://hg.mozilla.org/integration/mozilla-inbound/rev/749e21617b58
part 8 - Implement printing via Skia PDF for macOS (behind pref print.print_via_pdf_encoder=skia-pdf). r=lsalzman
(In reply to Wes Kocher (:KWierso) from comment #32)
> Were there other things still needing to be re-landed here? I backed out
> patches 4-8 but you've only relanded 4-5.

I wanted to make sure that the file rename in part 5 did indeed stick with a clobber before landing the subsequent patches.
Comment on attachment 8814390 [details] [diff] [review]
part 8 - Implement printing via Skia PDF for macOS

Markus, I forgot, I also wanted your review of this.
Flags: needinfo?(jwatt)
Attachment #8814390 - Flags: review?(mstange)
Blocks: 1321689
No longer blocks: 1278259
Blocks: 1295109
Blocks: 1322653
No longer blocks: 1264551
Too late for firefox 52, mass-wontfix.
Blocks: 1359329
Comment on attachment 8814390 [details] [diff] [review]
part 8 - Implement printing via Skia PDF for macOS

All of the patches here have landed so it's a bit misleading to leave it open. I opened bug 1359329 to cover this needinfo and I will close this bug.
Attachment #8814390 - Flags: review?(mstange)
Status: NEW → RESOLVED
Closed: 2 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Depends on: 1360350
Blocks: 1372506
Whiteboard: [behind pref print.print_via_pdf_encoder=skia-pdf]
No longer blocks: 1295109
No longer depends on: 1299848
You need to log in before you can comment on or make changes to this bug.