Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a thin wrapper around Skia's PDF backend #775

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

LoadingByte
Copy link
Contributor

Skia has a convenient PDF backend that turns draw calls into PDF pages. It would be amazing to have access to that backend in Skiko too.

This changeset adds a direct wrapper around Skia's PDF API. I left out the following features:

  • SkPDF::Metadata::fStructureElementTreeRoot together with SkPDF::SetNodeId, which make up an API for adding semantic annotations to elements in the PDF. I deemed this not useful enough to justify the cost of implementing it at this point.
  • SkPDF::Metadata::fExecutor, which is an experimental API for multithreading the creation of PDFs. As Skiko doesn't support the SkExecutor yet, adding this would have incurred high cost for possibly little reward.
  • SkPDF::Metadata::fSubsetter, which allows to choose between two font subsetters, one of which is already deprecated and will thus likely be removed by the Skia team in the near future.
  • SkDocument::abort, which just waits for all background threads to terminate and then returns. This seems non-useful and even confusing because we don't support PDF multithreading.

In contrast to my color space changeset, where I wrote POJOs to mirror the existing codebase, I now decided to employ data classes. If you for some reason want to avoid data classes in Skiko, just tell me, and I'll refactor them back to POJOs.

I'm looking forward to your comments.

@dima-avdeev-jb dima-avdeev-jb self-requested a review August 1, 2023 09:21
@dima-avdeev-jb
Copy link
Contributor

Thanks for PR! We will need some time to review it.

@dima-avdeev-jb dima-avdeev-jb removed their request for review August 1, 2023 13:41
@Phaestion
Copy link
Contributor

We would love to have this functionality in Skiko! Any update on when this can be merged?

@dima-avdeev-jb
Copy link
Contributor

We would love to have this functionality in Skiko! Any update on when this can be merged?

Sorry, not yet. For now we have higher priority tasks.

@sgopi93
Copy link

sgopi93 commented Feb 26, 2024

@LoadingByte can you include SkAnnotateRectWithURL with this PR, if possible

@LoadingByte
Copy link
Contributor Author

@sgopi93 I too believe that'd be a great addition, but seeing as this PR already lingers for seven months, I think the best bet to getting it merged some day is to keep it very basic and not inflate it even further for the time being. Once the Skiko team merged it, feel free to file a follow-up PR with support for the three functions in SkAnnotation.h :)

@sgopi93
Copy link

sgopi93 commented May 8, 2024

Hi @LoadingByte , Is it possible you to merge this branch with master. As this branch is far behind master?

@kmbisset89
Copy link

@LoadingByte would love to see this go in.

@LoadingByte LoadingByte force-pushed the pdf-backend branch 9 times, most recently from 49650f1 to b30d5a5 Compare October 21, 2024 00:02
@LoadingByte
Copy link
Contributor Author

I updated this patch to be in line with master.

@kmbisset89
Copy link

@LoadingByte thanks I will give it a try in the morning.

@kmbisset89
Copy link

@eymar Could you please see about helping this get code get in?

@jstarczewski
Copy link

Hello @eymar @dima-avdeev-jb Are there any plans on merging this PR soon?

@piotr-bar
Copy link

@eymar @dima-avdeev-jb when will it be possible to merge it?

@chrisjenx
Copy link

Would love to see this in Skiko too, was looking at generating PDF's from Compose Multiplatform, looks like this should enable that by drawing compose output to the canvas?

@Phaestion
Copy link
Contributor

Would love to see this in Skiko too, was looking at generating PDF's from Compose Multiplatform, looks like this should enable that by drawing compose output to the canvas?

Exactly @chrisjenx. It's similar to the SVG canvas that you can draw on.
I tested the PDF drawing mechanisms when this PR originally landed and it worked just like drawing to a SVG canvas.

@PiotrSwierczynski
Copy link

@dima-avdeev-jb My team is also looking forward to the merge this PR very much. Can you give any update regarding the timeline?

@pjBooms pjBooms requested review from eymar and igordmn February 5, 2025 10:18
@eymar
Copy link
Member

eymar commented Feb 10, 2025

Hi @LoadingByte ! Thank you for patience :) And thanks everyone for a ping.

To proceed with this PR, I think we need to ensure:

  1. The binary size is not affected much. (First, it's better to rebase the branch on the latest master. It contains newer Skia version, which might've broken the API).
  • Could you please let us know how the binary size changes in this PR?
  1. The new feature has tests. Since the API is in commonMain, it makes sense to have the common tests too (the currently added tests are in jvmTest).

@LoadingByte
Copy link
Contributor Author

I just rebased my PR onto master and updated it to Skia m132, which mainly consisted of adding the new PDFMetadata::lang property. Nothing else was broken.

I can't move the tests to commonTest because they require a WStream implementation, and Skiko offers only one right now, namely OutputWStream from jvmMain. I guess for the same reason, SVGCanvasTest is also in jvmTest even though SVGCanvas is in commonMain, just like in my case.

The binary size of skiko-awt-runtime-linux-x64.jar increases from 12.9MB to 13.6MB. I'm not entirely sure how your native build system works and how it automatically pulled in more C source files even though I only include another Skia header, so you might want to have a look whether I did everything correctly there.

@eymar
Copy link
Member

eymar commented Feb 11, 2025

I tried a locally built skiko with a trivial Compose app. The size changes for a release app:

MacOS Desktop ~2Mb, iOS ~2Mb, Web (wasm) ~0.1Mb.
(The size change is mostly in .dylib and .wasm binary).

Since app size mostly matters for distribution in AppStore and for Web browsers, I guess it makes sense to keep this API in jvmMain only. And probably we should review other APIs to reduce the size.

@igordmn What do you think about adding this new API to jvmMain only?
(Currently it can't be used anywhere except jvm anyway, since WStream is implemented only in jvmMain. Same for SVGCanvas)

@chrisjenx
Copy link

I'm ok with it being jvm only. iOS we have pdf libs same for Android. Web not sure.

But I think most people's use cases for this are to generate pdfs on the backend to send out to clients.

@igordmn
Copy link
Collaborator

igordmn commented Feb 13, 2025

The binary size of skiko-awt-runtime-linux-x64.jar increases from 12.9MB to 13.6MB

MacOS Desktop ~2Mb

This is quite big just for one feature that is needed only for some developers. We should find a way to compile it as a separate dynamic library packed into its own jar:

   implementation("org.jetbrains.skiko:skiko:...")
   implementation("org.jetbrains.skiko:skiko-awt-runtime-$platform:...")
   implementation("org.jetbrains.skiko:skiko-awt-runtime-pdf-$platform:...")

It will require some changes in Gradle files and in library loading code. A similar approach will be applied in #1017 (comment) later

What do you think about adding this new API to jvmMain only?

Okay to make it JVM only. Can be extended on other platforms when needed.

@LoadingByte
Copy link
Contributor Author

@igordmn Should I change this PR myself, and if yes, what exactly should I change? Or are you still discussing and/or will take care of applying the changes it yourself?

@igordmn
Copy link
Collaborator

igordmn commented Feb 19, 2025

I suggest to wait what we decide there soon, it is under review/discussion.

Though, we may not implement it at all, as there are more options with that PR, it may just require an already compiled external lib. Then we'll ask you to look at it. If in the end we can achieve this option, then there shouldn't be any blockers for merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.