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

fix: jpeg precision metadata #2689

Merged
merged 2 commits into from
Mar 26, 2024
Merged

fix: jpeg precision metadata #2689

merged 2 commits into from
Mar 26, 2024

Conversation

joelybahh
Copy link
Contributor

@joelybahh joelybahh commented Mar 25, 2024

Fix #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.

Copy link

changeset-bot bot commented Mar 25, 2024

🦋 Changeset detected

Latest commit: 510baf2

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 diegomura changed the title Implemented fix for this.bits undefined, causing jpegs to not render in some PDF viewers fix: jpeg precision metadata Mar 26, 2024
Copy link
Owner

@diegomura diegomura left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@diegomura
Copy link
Owner

Thanks!

@diegomura diegomura merged commit e2d21a4 into diegomura:master Mar 26, 2024
@joelybahh
Copy link
Contributor Author

@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
Copy link
Sponsor

knuula commented Mar 27, 2024

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

See this PR: #2690

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.

PDF generated file can't open by Adobe Acrobat or other PDF readers except Chrome
3 participants