fix: jpeg precision metadata

diegomura / react-pdf

📄 Create PDF files using React

https://react-pdf.org

MIT License

14.24k stars 1.11k forks source link

fix: jpeg precision metadata #2689

Closed joelybahh closed 1 month ago

joelybahh commented 1 month ago

Fix https://github.com/diegomura/react-pdf/issues/2625

Implementing the improved fix off of the back of oogas pull request, and information from this comment thread.

By dynamically adapting to the actual bit depth, we avoid hardcoding 8 bits (which would limit quality for higher bit-depth images).
Avoids this.bits being undefined, which was problematic in more strict PDF viewers such as Adobe, Mac Preview and Safari.

I had the time and wanted to trial the open source flow anyway (Feel free to decline this if this is overstepping someone else's work I'm not trying to take credit or anything, I am simply looking to get this fix rolled out ASAP as we are using it in a company project).

Trying to be helpful in expediting is all.

changeset-bot[bot] commented 1 month ago

🦋 Changeset detected

Latest commit: 510baf24d8acb8c226f436894f559178f95e18d0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages

| Name | Type | | ----------------------- | ----- | | @react-pdf/pdfkit | Patch | | @react-pdf/layout | Patch | | @react-pdf/renderer | Patch | | @react-pdf/svgkit | Patch | | @react-pdf/examples | Patch | | @react-pdf/e2e-node-cjs | Patch | | @react-pdf/e2e-node-esm | Patch |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

diegomura commented 1 month ago

Thanks!

joelybahh commented 1 month ago

@diegomura I installed latest version of @reactpdf/renderer and noticed the lib for pdfkit isn't the same as the output I get locally for it. Is this simply because these changes aren't deployed yet?

If they are deployed, just a note that the build output from this change doesn't align to the expected one, the line change I added isn't present basically.

Sorry if its just because deployment is bundled with other changes, just saw a release yesterday so thought it might have been this.

knuula commented 1 month ago

I am wondering the same thing as @joelybahh above. I've been trying to deploy an update on this all day. It looks like npm is not updated with these changes yet. Anything you can do here @diegomura ?

joelybahh commented 1 month ago

@knuula I noticed some E2E tests failed on the merge of this, maybe the tests are checking against PDF output that is now technically different due to the jpeg fixes, meaning a false negative. For me it had a syntax error on the github action .yml file. So I submitted a PR fixing that but that seems to have uncovered an entire e2e test can of worms.

joelybahh commented 1 month ago

See this PR: https://github.com/diegomura/react-pdf/pull/2690