Skip to content

Don't get ICC color space files if required API options are missing #19669

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 16, 2025
Merged

Don't get ICC color space files if required API options are missing #19669

merged 1 commit into from
Mar 16, 2025

Conversation

timvandermeij
Copy link
Contributor

@timvandermeij timvandermeij commented Mar 16, 2025 •
edited
Loading

This commit improves validation of the API options for the ICC color space logic. If useWasm is true but the corresponding wasmUrl or iccUrl API options are not provided we can avoid requesting files with null URLs which always results in a 404 response.

Fixes #19668.

The diff is slightly shorter with ?w=1; see https://github.com/mozilla/pdf.js/pull/19669/files?w=1.

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Mar 16, 2025 •
edited
Loading

This seems generally reasonable, but one question:
Is there any indication that these resources couldn't be loaded, or should we perhaps warn in the various !isUsable-cases since otherwise there may not be any indication of why a PDF document renders "poorly" and what the user can do to fix it?

@timvandermeij
Copy link
Contributor Author

timvandermeij commented Mar 16, 2025 •
edited
Loading

Good point; I have updated the patch. Is this more like what you had in mind?

Initially I was a bit worried about such warnings being logged multiple times due to the number of invocations of isUsable, but it's not an issue if we solve it inside the isUsable getter because of the shadowing which makes it only computed (and logged) once.

I have tested the following scenarios with this version of the patch:

  • useWasm = false: nothing is logged because WASM is explicitly disabled. No requests with 404 are visible. The PDF file renders without improved ICC support.
  • useWasm = true without wasmUrl and iccUrl: "No ICC color space support due to missing wasmUrl API option" is logged, but because the ICC color space is not usable and the CMYK-to-RGB conversion relies on that the CMYK profile warning is not logged to avoid double logging for the same issue. No requests with 404 are visible. The PDF file renders without improved ICC support.
  • useWasm = true with wasmUrl but without iccUrl: "No CMYK ICC profile support due to missing iccUrl API option" is logged. No requests with 404 are visible. The PDF file renders with improved ICC support but without improved CMYK-to-RGB conversion.
  • useWasm = true with wasmUrl and iccUrl: nothing is logged because all is fine. No requests with 404 are visible. The PDF file renders with improved ICC support and with improved CMYK-to-RGB conversion.

@timvandermeij
Don't get ICC color space files if required API options are missing
f44cba8 
This commit improves validation of the API options for the ICC color
space logic. If `useWasm` is `true` but the corresponding `wasmUrl`
or `iccUrl` API options are not provided we can avoid requesting
files with `null` URLs which always results in a 404 response.
@timvandermeij timvandermeij changed the title Don't get colorspace files if wasmUrl or iccUrl are missing Don't get ICC color space files if required API options are missing Mar 16, 2025
Snuffleupagus
Snuffleupagus approved these changes Mar 16, 2025
View reviewed changes
Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

r=me, with passing tests; thank you.

@Snuffleupagus
Copy link
Collaborator

/botio test

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/989d2de546ac131/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.193.163.58:8877/4ad1416cf0f063e/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/989d2de546ac131/output.txt

Total script time: 30.05 mins

  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: Passed

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/4ad1416cf0f063e/output.txt

Total script time: 59.78 mins

  • Unit tests: FAILED
  • Integration Tests: FAILED
  • Regression tests: Passed

@timvandermeij timvandermeij merged commit e738566 into mozilla:master Mar 16, 2025
9 checks passed
@timvandermeij timvandermeij deleted the icc-colorspace-404 branch March 16, 2025 17:45
@timvandermeij timvandermeij mentioned this pull request Mar 16, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Snuffleupagus Snuffleupagus Snuffleupagus approved these changes

Assignees
No one assigned
Labels
core image-conversion image-quality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: Invalid requests performed if wasmUrl or iccUrl are not provided but useWasm is
3 participants
@timvandermeij @Snuffleupagus @moz-tools-bot