Repository navigation
PDFium CMYK setters, CMYK JPEG images, ICC output intents and vector recolour - #165
balazs-szucs wants to merge 6 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 Summary
Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check |  |
No pull request description was provided. The required change summary and checklist sections are missing. | Add a description with a “Description of Changes” section explaining what changed and why. Complete the General, Library changes, Native or FFM changes, and Testing checklist items, including test and build results and any applicable issue … |
-
Check name Status Explanation Title check The title clearly summarizes the primary changes: PDFium CMYK setters, CMYK JPEG image support, ICC output intents, and vector recoloring. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Description check
-
Resolution
Add a description with a “Description of Changes” section explaining what changed and why. Complete the General, Library changes, Native or FFM changes, and Testing checklist items, including test and build results and any applicable issue or changelog references.
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
balazs-szucs
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
31e61dc to
35a6644
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
3986f02 to
af1d011
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
af1d011 to
51aace1
Compare
51aace1 to
3be4f33
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| int ignored = (int) FormFillBindings.EPDFForm_RedrawFieldWidgets.invokeExact( | ||
| rawDoc, objNum, MemorySegment.NULL, 0L, MemorySegment.NULL); |
| int ignored = (int) FormFillBindings.EPDFForm_ResetField.invokeExact(rawDoc, objNum, | ||
| MemorySegment.NULL, 0L, MemorySegment.NULL); |
253efd4 to
b626b59
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
c61972e to
c1f4528
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments: 1
native/setup-pdfium.sh: 1
878-879: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winThe metadata patch still uses slash-prefixed keys, so it copies no keys.
A past review comment flagged this, and the comment is marked "Addressed in commit 5dff0bd". Lines 878-879 still contain
"/OC","/Intent"and the other slash-prefixed keys.
CPDF_Dictionarystores keys without the leading/. Line 915 in this same script usesdict->KeyExist("SMask"), which follows that rule.- Because of the slash,
old_dict->KeyExist(key)is always false.EPDFImageObj_SetJpegthen drops/OC,/Intent,/Interpolateand/Metadata.- Consequence: if an image belongs to an optional-content group, it becomes always visible after CMYK conversion. The script still prints "Patched ... to preserve image metadata".
/Alternateis not an image XObject key. The correct key isAlternates./Mattebelongs to the SMask dictionary, not to the base image dictionary.Line 859 checks for the text
JPDFium: preserve image metadata. That text is already in trees patched by the broken version. If you fix only the key list, those trees keep the broken code until someone runs--clean. Change the marker text so the fixed patch replaces the old one, or detect the old slash form and fail.Proposed fix
- for (const char* key : {"/OC", "/Intent", "/Interpolate", "/Name", - "/Metadata", "/Alternate", "/Matte"}) { + for (const char* key : {"OC", "Intent", "Interpolate", "Name", + "Metadata", "Alternates"}) {Add a regression test: convert an image that has
/OC, reopen the saved PDF, and assert that/OCis still present.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: Stirling-Tools/JPDFium/.coderabbit.yaml
- Review profile: ASSERTIVE
- Plan: Advanced
- Run ID:
afd04c9c-f7d1-4056-8c62-f4566a5f137c
⛔ Files ignored due to path filters: 1
native/rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing: 36
.editorconfig-checker.json.gitattributesNOTICEdocs/README.mddocs/color-pipeline.mddocs/editing.mdjpdfium/build.gradle.ktsjpdfium/src/main/java/stirling/software/jpdfium/doc/IccCmykTransform.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfColorConverter.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfIccProfiles.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfOutputIntent.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfRecolor.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfXConverter.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfXOptions.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfXStandard.javajpdfium/src/main/java/stirling/software/jpdfium/panama/ColorBindings.javajpdfium/src/main/java/stirling/software/jpdfium/panama/FormFillBindings.javajpdfium/src/main/java/stirling/software/jpdfium/panama/PageEditBindings.javajpdfium/src/main/java/stirling/software/jpdfium/transform/PdfPageGeometry.javajpdfium/src/main/resources/META-INF/native-image/com.stirling/jpdfium/reachability-metadata.jsonjpdfium/src/main/resources/icc/JPDFiumCompactCmyk.NOTICE.txtjpdfium/src/main/resources/icc/JPDFiumCompactCmyk.iccjpdfium/src/test/java/stirling/software/jpdfium/doc/PdfBinaryFreeApiTest.javanative/CMakeLists.txtnative/bridge/include/jpdfium.hnative/bridge/include/jpdfium_rust.hnative/bridge/jpdfium.defnative/bridge/src/jpdfium_color.cppnative/bridge/src/jpdfium_stub.cppnative/bridge/tests/smoke.cppnative/licenses/NOTICE.txtnative/rust/jpdfium-impl/Cargo.tomlnative/rust/jpdfium-impl/src/lib.rsnative/setup-pdfium.shnative/tools/gen_compact_icc.creachability-metadata/metadata/com.stirling/jpdfium/1.2.0/reachability-metadata.json
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
📜 Review details
🧰 Additional context used
📚 Code guidelines: 1
.github/CODERABBIT_REVIEW.md — configured
📓 Path-based instructions: 5
Apply the native ownership, C ABI, C++23, engine integration, memory, and SIMD rules from .github/CODERABBIT_REVIEW.md.
⚙️ CodeRabbit configuration file
Files:
native/bridge/tests/smoke.cppnative/bridge/src/jpdfium_stub.cppnative/bridge/include/jpdfium_rust.hnative/bridge/include/jpdfium.hnative/tools/gen_compact_icc.cnative/bridge/src/jpdfium_color.cpp
Check target-scoped flags, exported ABI, reproducibility, sanitizer instrumentation, dependency compatibility, and portable CPU targets.
⚙️ CodeRabbit configuration file
Files:
native/CMakeLists.txt
Enforce meaningful assertions, deterministic concurrency, failure-path cleanup, and explicit native/stub requirements.
⚙️ CodeRabbit configuration file
Files:
jpdfium/src/test/java/stirling/software/jpdfium/doc/PdfBinaryFreeApiTest.java
Apply the Java 25 and FFM rules from .github/CODERABBIT_REVIEW.md.
⚙️ CodeRabbit configuration file
Files:
jpdfium/src/main/java/stirling/software/jpdfium/panama/FormFillBindings.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfIccProfiles.javajpdfium/src/main/java/stirling/software/jpdfium/transform/PdfPageGeometry.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfXStandard.javajpdfium/src/main/java/stirling/software/jpdfium/panama/PageEditBindings.javajpdfium/src/test/java/stirling/software/jpdfium/doc/PdfBinaryFreeApiTest.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfRecolor.javajpdfium/src/main/java/stirling/software/jpdfium/doc/IccCmykTransform.javajpdfium/src/main/java/stirling/software/jpdfium/panama/ColorBindings.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfXOptions.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfOutputIntent.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfXConverter.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfColorConverter.java
Source excerpt: Use imports rather than package-qualified type names in method bodies and signatures.
📄 CodeRabbit inference engine (.github/CODERABBIT_REVIEW.md)
Files:
docs/README.mdnative/rust/jpdfium-impl/Cargo.tomljpdfium/build.gradle.ktsnative/bridge/jpdfium.defNOTICEreachability-metadata/metadata/com.stirling/jpdfium/1.2.0/reachability-metadata.jsonnative/bridge/tests/smoke.cppjpdfium/src/main/java/stirling/software/jpdfium/panama/FormFillBindings.javajpdfium/src/main/resources/icc/JPDFiumCompactCmyk.NOTICE.txtjpdfium/src/main/java/stirling/software/jpdfium/doc/PdfIccProfiles.javajpdfium/src/main/java/stirling/software/jpdfium/transform/PdfPageGeometry.javanative/licenses/NOTICE.txtnative/bridge/src/jpdfium_stub.cppjpdfium/src/main/resources/META-INF/native-image/com.stirling/jpdfium/reachability-metadata.jsonnative/bridge/include/jpdfium_rust.hjpdfium/src/main/java/stirling/software/jpdfium/doc/PdfXStandard.javajpdfium/src/main/java/stirling/software/jpdfium/panama/PageEditBindings.javadocs/editing.mddocs/color-pipeline.mdnative/CMakeLists.txtjpdfium/src/test/java/stirling/software/jpdfium/doc/PdfBinaryFreeApiTest.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfRecolor.javajpdfium/src/main/java/stirling/software/jpdfium/doc/IccCmykTransform.javanative/bridge/include/jpdfium.hjpdfium/src/main/java/stirling/software/jpdfium/panama/ColorBindings.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfXOptions.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfOutputIntent.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfXConverter.javanative/rust/jpdfium-impl/src/lib.rsnative/tools/gen_compact_icc.cjpdfium/src/main/java/stirling/software/jpdfium/doc/PdfColorConverter.javanative/setup-pdfium.shnative/bridge/src/jpdfium_color.cpp
🪛 GitHub Check: CodeQL
jpdfium/src/main/java/stirling/software/jpdfium/transform/PdfPageGeometry.java
[notice] 309-310: Unread local variable
Variable 'int ignored' is never read.
[notice] 315-316: Unread local variable
Variable 'int ignored' is never read.
🪛 LanguageTool
docs/color-pipeline.md
[uncategorized] ~82-~82: The official name of this software platform is spelled with a capital “H”.
Context: ...s future work. 4. Ghent gate in CI. .github/workflows/ghent.yml builds the patched...
(GITHUB)
🔇 Additional comments: 50
jpdfium/src/main/java/stirling/software/jpdfium/doc/PdfRecolor.java: 6
288-291: Fix the narrowing cast instretchbefore the clamp.
Math.round(double)returns along. The code casts it tointbeforeMath.clamp, so a largefactortruncates to arbitrary bits first. Forv > 128andfactor = +Infinity, the result is 0 instead of 255.NaNalso yields 0, which is wrong for most inputs.Pass the
longtoMath.clamp(long, int, int)directly. Also validate thatcontrastFactoris finite in the builder (Line 321).This is a prior finding. The current code still has the narrowing cast. It is a duplicate of the earlier review comment on
stretch, and the part about narrowing is still open.Proposed fix
- return Math.clamp((int) Math.round(centred), 0, 255); + return Math.clamp(Math.round(centred), 0, 255);- public Builder contrastFactor(double f) { this.contrastFactor = f; return this; } + public Builder contrastFactor(double f) { + if (!Double.isFinite(f)) { + throw new IllegalArgumentException("contrastFactor must be finite: " + f); + } + this.contrastFactor = f; + return this; + }As per coding guidelines: "Check conversions, integer arithmetic, sizes, alignment, finite floating-point inputs, and narrowing".
Source: Coding guidelines
194-197: Surface non-fatal downcall failures fromrecolorOne.The
catch (Throwable)callsrethrowFataland then returnsfalse. A non-fatal failure, such asWrongMethodTypeExceptionfrom a wrong handle signature, looks like "no recolour needed". If a binding is wrong, every object is skipped andapplyreturns 0 without any error. The same pattern exists at Lines 96-99, 117-120 and 126-129.Wrap non-fatal failures in
JPDFiumException, as theFPDFPage_GenerateContentguard does.This is a duplicate of the earlier review comment, which was marked as addressed. The current code still swallows these failures.
As per coding guidelines: "Do not catch
Throwablemerely to swallow or log it."Source: Coding guidelines
150-154: CallNativeRuntime.rethrowFatalbefore wrapping theFPDFPage_GenerateContentfailure.The
catch (Throwable t)wraps fatal errors, such asOutOfMemoryError, inJPDFiumException. Callers that catchJPDFiumExceptionthen treat a fatal VM error as an ordinary recolour failure. All other downcall guards in this file callrethrowFatalfirst.Move the
ok == 0check out of thetryblock. Then remove theJPDFiumExceptionrethrow branch.Proposed fix
- try { - int ok = (int) PageEditBindings.FPDFPage_GenerateContent.invokeExact(rawPage); - if (ok == 0) { - throw new JPDFiumException("FPDFPage_GenerateContent failed"); - } - } catch (JPDFiumException e) { - throw e; - } catch (Throwable t) { - throw new JPDFiumException("FPDFPage_GenerateContent failed", t); - } + int ok; + try { + ok = (int) PageEditBindings.FPDFPage_GenerateContent.invokeExact(rawPage); + } catch (Throwable t) { + NativeRuntime.rethrowFatal(t); + throw new JPDFiumException("FPDFPage_GenerateContent failed", t); + } + if (ok == 0) { + throw new JPDFiumException("FPDFPage_GenerateContent failed"); + }This is a duplicate of an earlier review comment. The current code still has the problem.
Source: Coding guidelines
307-307: Reject a nullModeinBuilder.mode.
mode(null)is accepted. Theswitch (mode)at Line 233 then throwsNullPointerExceptionon the first eligible object.recolorOnecatches that exception and returnsfalse, so the object is silently skipped. The caller sees no error.Validate the argument with
Objects.requireNonNull. Addimport java.util.Objects;.This is a duplicate of an earlier review comment. The current code still accepts null.
270-285: Skip writes whenHIGH_CONTRASTstretch andTINTleave the colour unchanged.The
contraststretch branch and thetintmethod returntrueeven whenoutequals the input. For example, black withfactor = 2.0stays(0,0,0). The object is still counted,dirtybecomes true, andFPDFPage_GenerateContentrewrites the page for no change. This contradicts themapJavadoc at Line 231. The threshold branch already has the equality check.Apply the equality check once in
map.Proposed fix
boolean map(int r, int g, int b, int[] out) { - switch (mode) { - case INVERT: - return invert(r, g, b, out); - case TINT: - return tint(r, g, b, out); - case HIGH_CONTRAST: - default: - return contrast(r, g, b, out); - } + boolean mapped = switch (mode) { + case INVERT -> invert(r, g, b, out); + case TINT -> tint(r, g, b, out); + case HIGH_CONTRAST -> contrast(r, g, b, out); + }; + return mapped && (out[0] != r || out[1] != g || out[2] != b); }This is a duplicate of an earlier review comment. The comment was marked as addressed, but the current code has no such check.
80-90: Rejectnullelements inpageIndicesand hoistpageCount().
for (int i : pageIndices)unboxes each element. Anullelement throwsNullPointerException.doc.pageCount()is also called on every iteration.This is a duplicate of an earlier review comment. The current code is unchanged.
native/setup-pdfium.sh: 1
620-690: LGTM!native/CMakeLists.txt: 2
646-649: These are still plain substring probes.Both probes still use
string(FIND ...). A comment or a longer identifier in either header can enableJPDFIUM_HAVE_CMYK_APIorJPDFIUM_HAVE_EPDF_JPEGwhen no declaration exists.jpdfium_color.cppthen fails to compile. An earlier review already reported this.Also applies to: 664-665
205-205: LGTM!native/bridge/src/jpdfium_stub.cpp: 2
1681-1684: The stub returnsJPDFIUM_ERR_NATIVEwhere the real bridge returnsJPDFIUM_ERR_NOT_FOUND.When the feature is unavailable, the real bridge returns
JPDFIUM_ERR_NOT_FOUNDfor these two calls. The stub returnsJPDFIUM_ERR_NATIVEthroughfail_native_bytes. An earlier review already reported this.Also applies to: 1692-1696
1346-1350: LGTM!Also applies to: 1675-1680, 1686-1690
NOTICE: 2
60-60: Thejpeg-encoderlicense still omits IJG.This line still says
MIT OR Apache-2.0. It lacks the IJG term and the required attribution. An earlier review already reported this.
18-21: LGTM!native/licenses/NOTICE.txt: 1
136-139: This entry still lacks the IJG license and attribution.This is the same root cause as
NOTICEline 60. An earlier review already reported this.native/bridge/src/jpdfium_color.cpp: 4
32-34: Theepdf_edit.hinclude is still unconditional.
CMakeLists.txtenablesJPDFIUM_HAVE_EPDF_JPEGwhenEPDFImageObj_SetJpegappears only infpdf_edit.h. In that layout this include fails to compile. An earlier review already reported this.
620-634: The XMP update still matches only the element form.When the source XMP uses the attribute form,
AddPdfxAttributesappends duplicate attributes, which makes the XML ill-formed. This covers our ownBuildPdfxXmpoutput and Acrobat files. An earlier review already reported this.
395-448: Windows staging and publishing still use narrow-path APIs on UTF-8 paths.
_sopen_s,std::filesystem::path(std::string)andMoveFileExAreceive the UTF-8 paths that come from Java. qpdf reads the same paths as UTF-8. An earlier review already reported this.
44-180: LGTM!Also applies to: 246-302, 668-952
jpdfium/src/main/java/stirling/software/jpdfium/panama/ColorBindings.java: 2
126-131: The ICC probes still bindunsigned longasJAVA_LONG.On LLP64 Windows,
unsigned longis 32-bit. The returned size can include garbage in the upper bits. An earlier review already reported this.
1-125: LGTM!Also applies to: 133-479
jpdfium/src/main/java/stirling/software/jpdfium/panama/PageEditBindings.java: 2
67-68: These bindings still useJAVA_LONGfor Cunsigned long.
FPDFFormObj_GetObjectandFPDFFormObj_GetObjectNumberboth use nativeunsigned long. On LLP64 Windows, the returned object number is unreliable. An earlier review already reported this.Also applies to: 79-80
5-5: LGTM!Also applies to: 31-34, 62-66, 70-78, 82-105
jpdfium/src/test/java/stirling/software/jpdfium/doc/PdfBinaryFreeApiTest.java: 2
74-83: The test still compares againstvectorCmykAvailable()only.
supportsColorSpace(CMYK)also requirescolorSpaceAvailable()andiccConversionAvailable(). An earlier review already reported this.
19-26: LGTM!Also applies to: 97-99
native/bridge/tests/smoke.cpp: 2
218-219: The capability check is still unconditional when Rust is enabled.The check fails on a supported build that has Rust but no
EPDFImageObj_SetJpeg. An earlier review already reported this.
150-151: LGTM!Also applies to: 209-217, 220-238
docs/editing.md: 2
199-199: The image-conversion probe description still omitsEPDFImageObj_SetJpeg.An earlier review already reported this.
180-198: LGTM!Also applies to: 200-259, 269-308
jpdfium/src/main/java/stirling/software/jpdfium/doc/PdfXConverter.java: 2
31-39: PDF/X-3 output still does not reject transparency.
allowsTransparency()is never checked, so a source with transparency still gets PDF/X-3 metadata. An earlier review already reported this.
88-88:java.nio.charset.StandardCharsetsis still package-qualified inline.An earlier review already reported this.
native/tools/gen_compact_icc.c: 1
105-111: Results ofcmsStageAllocCLut16bit,cmsPipelineInsertStageandcmsWriteTagare still unchecked.A failed call still produces a profile without the A2B0 or B2A0 tag, and the tool exits 0. An earlier review already reported this.
jpdfium/build.gradle.kts: 2
452-453:jpdfium.test.cmykIccis still not forwarded to the test JVM.
PdfColorPipelineTestandPdfXConverterTestread this property. Without it, the explicit-profile path never runs. An earlier review already reported this.
479-481: LGTM!jpdfium/src/main/resources/icc/JPDFiumCompactCmyk.NOTICE.txt: 1
10-10: The MIT statement still conflicts with the generator.
gen_compact_icc.cstill embeds "Public domain compact CMYK fallback profile" as the ICC copyright tag. An earlier review already reported this.native/bridge/include/jpdfium.h: 1
710-771: LGTM!native/bridge/include/jpdfium_rust.h: 1
115-120: LGTM!native/bridge/jpdfium.def: 1
18-18: LGTM!native/rust/jpdfium-impl/Cargo.toml: 1
22-27: LGTM!native/rust/jpdfium-impl/src/lib.rs: 1
19-31: LGTM!Also applies to: 138-138, 173-175, 319-319, 485-583
jpdfium/src/main/java/stirling/software/jpdfium/doc/IccCmykTransform.java: 1
1-145: LGTM!jpdfium/src/main/java/stirling/software/jpdfium/doc/PdfColorConverter.java: 1
7-665: LGTM!jpdfium/src/main/java/stirling/software/jpdfium/doc/PdfIccProfiles.java: 1
1-42: LGTM!jpdfium/src/main/java/stirling/software/jpdfium/doc/PdfOutputIntent.java: 1
1-139: LGTM!jpdfium/src/main/java/stirling/software/jpdfium/doc/PdfXStandard.java: 1
1-55: LGTM!jpdfium/src/main/java/stirling/software/jpdfium/doc/PdfXOptions.java: 1
1-197: LGTM!.editorconfig-checker.json: 1
13-14: LGTM!.gitattributes: 1
32-32: LGTM!docs/README.md: 1
18-18: LGTM!reachability-metadata/metadata/com.stirling/jpdfium/1.2.0/reachability-metadata.json: 1
233-267: 🗄️ Data Integrity & IntegrationThe registration-gap claim is refuted. Both metadata copies already contain the
SAMPLES_TO_CMYK_JPEG, CMYK setter, and CMYK getter signatures. No metadata change is required for these descriptors.jpdfium/src/main/resources/META-INF/native-image/com.stirling/jpdfium/reachability-metadata.json: 1
233-267: 🗄️ Data Integrity & IntegrationThe missing-registration claim is refuted.
ColorBindingsdeclares all listed CMYK, PDF/X, and ICC descriptors, andPageEditBindings.FPDFFormObj_GetObjectmatches the added(ADDRESS, ADDRESS, JAVA_LONG)descriptor. The removed seven-parameter signature is still registered at lines 1863-1871 forEmbedPdfDocumentBindings.EPDF_RenderAnnotBitmap, so the edit removes only a duplicate metadata entry. The stated native-image failure does not follow.
|
@coderabbitai full review |
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 6
ℹ️ Review info
⚙️ Run configuration
- Source mode: Auto
- Repository: Stirling-Tools/JPDFium
- Branch: main
- File: .coderabbit.yaml
- Commit: 5a35fef
- Layers: Repository YAML
- Review profile: ASSERTIVE
- Plan: Advanced
- Run ID:
b5200e49-c0a5-4ce7-8f8d-f12b84cbe002
⛔ Files ignored due to path filters: 1
native/rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing: 39
.editorconfig-checker.json.gitattributesNOTICEdocs/README.mddocs/color-pipeline.mddocs/editing.mdjpdfium/build.gradle.ktsjpdfium/src/main/java/stirling/software/jpdfium/doc/IccCmykTransform.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfColorConverter.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfIccProfiles.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfOutputIntent.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfRecolor.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfXConverter.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfXOptions.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfXStandard.javajpdfium/src/main/java/stirling/software/jpdfium/panama/ColorBindings.javajpdfium/src/main/java/stirling/software/jpdfium/panama/FormFillBindings.javajpdfium/src/main/java/stirling/software/jpdfium/panama/PageEditBindings.javajpdfium/src/main/java/stirling/software/jpdfium/transform/PdfPageGeometry.javajpdfium/src/main/resources/META-INF/native-image/com.stirling/jpdfium/reachability-metadata.jsonjpdfium/src/main/resources/icc/JPDFiumCompactCmyk.NOTICE.txtjpdfium/src/main/resources/icc/JPDFiumCompactCmyk.iccjpdfium/src/test/java/stirling/software/jpdfium/doc/IccCmykTransformTest.javajpdfium/src/test/java/stirling/software/jpdfium/doc/PdfBinaryFreeApiTest.javajpdfium/src/test/java/stirling/software/jpdfium/doc/PdfImageLosslessTest.javajpdfium/src/test/java/stirling/software/jpdfium/doc/PdfxMetadataSafetyTest.javanative/CMakeLists.txtnative/bridge/include/jpdfium.hnative/bridge/include/jpdfium_rust.hnative/bridge/jpdfium.defnative/bridge/src/jpdfium_color.cppnative/bridge/src/jpdfium_stub.cppnative/bridge/tests/smoke.cppnative/licenses/NOTICE.txtnative/rust/jpdfium-impl/Cargo.tomlnative/rust/jpdfium-impl/src/lib.rsnative/setup-pdfium.shnative/tools/gen_compact_icc.creachability-metadata/metadata/com.stirling/jpdfium/1.2.0/reachability-metadata.json
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
📜 Review details
🧰 Additional context used
📚 Code guidelines: 1
.github/CODERABBIT_REVIEW.md — configured
📓 Path-based instructions: 5
Apply the native ownership, C ABI, C++23, engine integration, memory, and SIMD rules from .github/CODERABBIT_REVIEW.md.
⚙️ CodeRabbit configuration file
Files:
native/bridge/tests/smoke.cppnative/bridge/include/jpdfium_rust.hnative/tools/gen_compact_icc.cnative/bridge/include/jpdfium.hnative/bridge/src/jpdfium_stub.cppnative/bridge/src/jpdfium_color.cpp
Check target-scoped flags, exported ABI, reproducibility, sanitizer instrumentation, dependency compatibility, and portable CPU targets.
⚙️ CodeRabbit configuration file
Files:
native/CMakeLists.txt
Enforce meaningful assertions, deterministic concurrency, failure-path cleanup, and explicit native/stub requirements.
⚙️ CodeRabbit configuration file
Files:
jpdfium/src/test/java/stirling/software/jpdfium/doc/PdfBinaryFreeApiTest.javajpdfium/src/test/java/stirling/software/jpdfium/doc/PdfImageLosslessTest.javajpdfium/src/test/java/stirling/software/jpdfium/doc/IccCmykTransformTest.javajpdfium/src/test/java/stirling/software/jpdfium/doc/PdfxMetadataSafetyTest.java
Apply the Java 25 and FFM rules from .github/CODERABBIT_REVIEW.md.
⚙️ CodeRabbit configuration file
Files:
jpdfium/src/test/java/stirling/software/jpdfium/doc/PdfBinaryFreeApiTest.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfIccProfiles.javajpdfium/src/main/java/stirling/software/jpdfium/transform/PdfPageGeometry.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfXStandard.javajpdfium/src/main/java/stirling/software/jpdfium/panama/FormFillBindings.javajpdfium/src/main/java/stirling/software/jpdfium/panama/PageEditBindings.javajpdfium/src/test/java/stirling/software/jpdfium/doc/PdfImageLosslessTest.javajpdfium/src/main/java/stirling/software/jpdfium/doc/IccCmykTransform.javajpdfium/src/test/java/stirling/software/jpdfium/doc/IccCmykTransformTest.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfXOptions.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfOutputIntent.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfXConverter.javajpdfium/src/main/java/stirling/software/jpdfium/panama/ColorBindings.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfRecolor.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfColorConverter.javajpdfium/src/test/java/stirling/software/jpdfium/doc/PdfxMetadataSafetyTest.java
Source excerpt: Use imports rather than package-qualified type names in method bodies and signatures.
📄 CodeRabbit inference engine (.github/CODERABBIT_REVIEW.md)
Files:
native/licenses/NOTICE.txtjpdfium/build.gradle.ktsdocs/README.mdnative/bridge/tests/smoke.cppreachability-metadata/metadata/com.stirling/jpdfium/1.2.0/reachability-metadata.jsonjpdfium/src/main/resources/icc/JPDFiumCompactCmyk.NOTICE.txtjpdfium/src/test/java/stirling/software/jpdfium/doc/PdfBinaryFreeApiTest.javaNOTICEjpdfium/src/main/java/stirling/software/jpdfium/doc/PdfIccProfiles.javanative/bridge/include/jpdfium_rust.hnative/bridge/jpdfium.defjpdfium/src/main/java/stirling/software/jpdfium/transform/PdfPageGeometry.javajpdfium/src/main/resources/META-INF/native-image/com.stirling/jpdfium/reachability-metadata.jsonjpdfium/src/main/java/stirling/software/jpdfium/doc/PdfXStandard.javanative/rust/jpdfium-impl/Cargo.tomljpdfium/src/main/java/stirling/software/jpdfium/panama/FormFillBindings.javajpdfium/src/main/java/stirling/software/jpdfium/panama/PageEditBindings.javanative/CMakeLists.txtjpdfium/src/test/java/stirling/software/jpdfium/doc/PdfImageLosslessTest.javadocs/editing.mddocs/color-pipeline.mdjpdfium/src/main/java/stirling/software/jpdfium/doc/IccCmykTransform.javajpdfium/src/test/java/stirling/software/jpdfium/doc/IccCmykTransformTest.javanative/tools/gen_compact_icc.cnative/bridge/include/jpdfium.hjpdfium/src/main/java/stirling/software/jpdfium/doc/PdfXOptions.javanative/bridge/src/jpdfium_stub.cppjpdfium/src/main/java/stirling/software/jpdfium/doc/PdfOutputIntent.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfXConverter.javajpdfium/src/main/java/stirling/software/jpdfium/panama/ColorBindings.javanative/rust/jpdfium-impl/src/lib.rsjpdfium/src/main/java/stirling/software/jpdfium/doc/PdfRecolor.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfColorConverter.javanative/setup-pdfium.shnative/bridge/src/jpdfium_color.cppjpdfium/src/test/java/stirling/software/jpdfium/doc/PdfxMetadataSafetyTest.java
🪛 GitHub Check: CodeQL
jpdfium/src/main/java/stirling/software/jpdfium/transform/PdfPageGeometry.java
[notice] 309-310: Unread local variable
Variable 'int ignored' is never read.
[notice] 315-316: Unread local variable
Variable 'int ignored' is never read.
🪛 LanguageTool
docs/color-pipeline.md
[uncategorized] ~85-~85: The official name of this software platform is spelled with a capital “H”.
Context: ...s future work. 4. Ghent gate in CI. .github/workflows/ghent.yml builds the patched...
(GITHUB)
🔇 Additional comments: 33
native/CMakeLists.txt: 1
646-649: Both PDFium probes still use substring matching.
string(FIND ...)matches comments and longer identifiers. In that case, theJPDFIUM_HAVE_CMYK_APIorJPDFIUM_HAVE_EPDF_JPEGdefinition is set without a real declaration, andjpdfium_color.cppthen fails to compile. The earlier comment gives a declaration-aware regex for both probes.Also applies to: 664-665
NOTICE: 1
60-60:jpeg-encoderis still listed asMIT OR Apache-2.0only.The earlier comment is marked as addressed, but this revision still omits the IJG term and the required "based in part on the work of the Independent JPEG Group" statement.
native/bridge/src/jpdfium_color.cpp: 1
498-504: Windows staging and publish still use narrow-path APIs.
_sopen_sandMoveFileExAinterpret the UTF-8 path fromColorBindingsin the ANSI code page. qpdf interprets the same path as UTF-8. For a non-ASCII path, these calls can address different files.Also applies to: 532-537
native/bridge/src/jpdfium_stub.cpp: 1
1681-1684: The stub still returnsJPDFIUM_ERR_NATIVEwhere the real bridge returnsJPDFIUM_ERR_NOT_FOUND.Also applies to: 1692-1696
native/bridge/tests/smoke.cpp: 1
218-219: The unconditional capability check is still present.
JPDFIUM_COLOR_CAP_CMYK_IMAGEalso requiresJPDFIUM_HAVE_EPDF_JPEG, and CMake treats that capability as optional. On a Rust build whose PDFium header lacksEPDFImageObj_SetJpeg,native_smokefails. The earlier comment is marked as addressed, but this revision still has the check. Assert only that the capability bit implies Rust.jpdfium/src/main/java/stirling/software/jpdfium/panama/ColorBindings.java: 1
134-139: The ICC getters are still bound withJAVA_LONGfor Cunsigned long.On LLP64 Windows,
unsigned longis 32 bits, so this descriptor does not match the native declaration. The earlier comment is marked as addressed in c61972e, but this revision still usesJAVA_LONG. The same mismatch exists inPageEditBindingsforFPDFFormObj_GetObjectNumberand theFPDFFormObj_GetObjectindex.jpdfium/src/test/java/stirling/software/jpdfium/doc/PdfBinaryFreeApiTest.java: 1
74-83: The test still compares CMYK support withvectorCmykAvailable()only.
supportsColorSpace(CMYK)also requirescolorSpaceAvailable()andiccConversionAvailable(). The earlier comment is marked as addressed, but this revision still uses the partial predicate.jpdfium/build.gradle.kts: 1
452-453: The forwarding filter still omitsjpdfium.test.*.The earlier comment is marked as addressed, but
-Djpdfium.test.cmykIccstill does not reach the forked test JVM.docs/editing.md: 1
199-199: Line 199 still omits theEPDFImageObj_SetJpegrequirement forsupportsImageConversion().jpdfium/src/main/java/stirling/software/jpdfium/panama/PageEditBindings.java: 1
67-68:FPDFFormObj_GetObjectandFPDFFormObj_GetObjectNumberstill useJAVA_LONGfor Cunsigned long.This is the LLP64 descriptor mismatch from the earlier comment, and it is still present.
Also applies to: 79-80
jpdfium/src/main/java/stirling/software/jpdfium/doc/PdfRecolor.java: 1
144-154: Several earlierPdfRecolorfindings are still present in this revision:
FPDFPage_GenerateContentfailures are wrapped withoutrethrowFatal.recolorOneturns non-fatal downcall errors into "unchanged".stretchnarrows alongtointbeforeMath.clamp.contrastFactoraccepts non-finite values.mode(null)is accepted.Also applies to: 194-197, 288-291, 307-307, 321-321
jpdfium/src/main/java/stirling/software/jpdfium/transform/PdfPageGeometry.java: 1
299-317: Two earlier findings are still open, and CodeQL flags the unreadignoredlocals.
- When
EPDFForm_SetFieldValuereturns0for a text or choice field, cleanup does not fail.- When
EPDFForm_RedrawFieldWidgetsis absent, the old/APappearance stays in the file.Both findings remain unresolved.
jpdfium/src/main/java/stirling/software/jpdfium/doc/PdfXConverter.java: 1
98-98: Line 98 still uses an inlinejava.nio.charset.StandardCharsetsname.jpdfium/src/main/resources/icc/JPDFiumCompactCmyk.NOTICE.txt: 1
10-10: The license conflict is still present.This notice says MIT.
native/tools/gen_compact_icc.cLine 116 still embeds "Public domain compact CMYK fallback profile" in the ICC copyright tag.native/tools/gen_compact_icc.c: 1
105-118: The stage allocation, pipeline insertion andcmsWriteTagresults are still unchecked.If any of these calls fails, the tool still writes a profile without
A2B0orB2A0and returns 0.native/licenses/NOTICE.txt: 1
136-139: Thejpeg-encoderentry still omits the IJG license term and attribution.native/bridge/include/jpdfium.h: 1
710-787: LGTM!native/bridge/jpdfium.def: 1
18-18: LGTM!native/rust/jpdfium-impl/Cargo.toml: 1
22-27: LGTM!native/rust/jpdfium-impl/src/lib.rs: 1
490-583: LGTM!.gitattributes: 1
32-32: LGTM!.editorconfig-checker.json: 1
13-14: LGTM!jpdfium/src/main/java/stirling/software/jpdfium/doc/IccCmykTransform.java: 1
1-235: LGTM!jpdfium/src/main/java/stirling/software/jpdfium/doc/PdfIccProfiles.java: 1
1-42: LGTM!jpdfium/src/test/java/stirling/software/jpdfium/doc/IccCmykTransformTest.java: 1
1-46: LGTM!docs/README.md: 1
18-18: LGTM!jpdfium/src/main/java/stirling/software/jpdfium/panama/FormFillBindings.java: 1
56-71: LGTM!jpdfium/src/main/java/stirling/software/jpdfium/doc/PdfOutputIntent.java: 1
1-161: LGTM!jpdfium/src/main/java/stirling/software/jpdfium/doc/PdfXOptions.java: 1
1-218: LGTM!jpdfium/src/main/java/stirling/software/jpdfium/doc/PdfXStandard.java: 1
1-62: LGTM!jpdfium/src/test/java/stirling/software/jpdfium/doc/PdfxMetadataSafetyTest.java: 1
1-141: LGTM!native/bridge/include/jpdfium_rust.h: 1
115-120: LGTM!jpdfium/src/main/resources/META-INF/native-image/com.stirling/jpdfium/reachability-metadata.json: 1
233-279: 🗄️ Data Integrity & IntegrationThe concern is refuted. All listed current FFM descriptor shapes are present in both JPDFium metadata files. The removed seven-parameter shape also remains once in each file, so a binding using that shape is covered. The scan found no missing descriptors.
221fc3d to
af097bd
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 6
ℹ️ Review info
⚙️ Run configuration
- Source mode: Auto
- Repository: Stirling-Tools/JPDFium
- Branch: main
- File: .coderabbit.yaml
- Commit: 5a35fef
- Layers: Repository YAML
- Review profile: ASSERTIVE
- Plan: Advanced
- Run ID:
41ad37fc-8dd6-4123-af82-573a85abe84a
⛔ Files ignored due to path filters: 1
native/rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing: 40
.editorconfig-checker.json.gitattributesNOTICEbuildSrc/src/main/kotlin/jpdfium.library-conventions.gradle.ktsdocs/README.mddocs/color-pipeline.mddocs/editing.mdjpdfium/build.gradle.ktsjpdfium/src/main/java/stirling/software/jpdfium/doc/IccCmykTransform.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfColorConverter.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfIccProfiles.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfOutputIntent.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfRecolor.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfXConverter.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfXOptions.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfXStandard.javajpdfium/src/main/java/stirling/software/jpdfium/panama/ColorBindings.javajpdfium/src/main/java/stirling/software/jpdfium/panama/FormFillBindings.javajpdfium/src/main/java/stirling/software/jpdfium/panama/PageEditBindings.javajpdfium/src/main/java/stirling/software/jpdfium/transform/PdfPageGeometry.javajpdfium/src/main/resources/META-INF/native-image/com.stirling/jpdfium/reachability-metadata.jsonjpdfium/src/main/resources/icc/JPDFiumCompactCmyk.NOTICE.txtjpdfium/src/main/resources/icc/JPDFiumCompactCmyk.iccjpdfium/src/test/java/stirling/software/jpdfium/doc/IccCmykTransformTest.javajpdfium/src/test/java/stirling/software/jpdfium/doc/PdfBinaryFreeApiTest.javajpdfium/src/test/java/stirling/software/jpdfium/doc/PdfImageLosslessTest.javajpdfium/src/test/java/stirling/software/jpdfium/doc/PdfxMetadataSafetyTest.javanative/CMakeLists.txtnative/bridge/include/jpdfium.hnative/bridge/include/jpdfium_rust.hnative/bridge/jpdfium.defnative/bridge/src/jpdfium_color.cppnative/bridge/src/jpdfium_stub.cppnative/bridge/tests/smoke.cppnative/licenses/NOTICE.txtnative/rust/jpdfium-impl/Cargo.tomlnative/rust/jpdfium-impl/src/lib.rsnative/setup-pdfium.shnative/tools/gen_compact_icc.creachability-metadata/metadata/com.stirling/jpdfium/1.2.0/reachability-metadata.json
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout: 9
- GitHub Check: prep-windows-icu
- GitHub Check: Corpus (generate + native)
- GitHub Check: JMH Benchmarks (FFM Downcall Overhead Gate)
- GitHub Check: GraalVM Native Image Smoke & E2E Test (Linux x64)
- GitHub Check: CLI correctness & no-corruption tests
- GitHub Check: Native bridge libFuzzer (ASan/UBSan, real PDFium)
- GitHub Check: build-natives (windows-arm64, windows-11-arm)
- GitHub Check: Restricted-env compat (no execstack + hermetic bundle)
- GitHub Check: CodeQL Analysis (c-cpp)
🧰 Additional context used
📚 Code guidelines: 1
.github/CODERABBIT_REVIEW.md — configured
📓 Path-based instructions: 5
Apply the native ownership, C ABI, C++23, engine integration, memory, and SIMD rules from .github/CODERABBIT_REVIEW.md.
⚙️ CodeRabbit configuration file
Files:
native/bridge/tests/smoke.cppnative/bridge/include/jpdfium_rust.hnative/tools/gen_compact_icc.cnative/bridge/src/jpdfium_stub.cppnative/bridge/include/jpdfium.hnative/bridge/src/jpdfium_color.cpp
Check target-scoped flags, exported ABI, reproducibility, sanitizer instrumentation, dependency compatibility, and portable CPU targets.
⚙️ CodeRabbit configuration file
Files:
native/CMakeLists.txt
Enforce meaningful assertions, deterministic concurrency, failure-path cleanup, and explicit native/stub requirements.
⚙️ CodeRabbit configuration file
Files:
jpdfium/src/test/java/stirling/software/jpdfium/doc/PdfImageLosslessTest.javajpdfium/src/test/java/stirling/software/jpdfium/doc/IccCmykTransformTest.javajpdfium/src/test/java/stirling/software/jpdfium/doc/PdfBinaryFreeApiTest.javajpdfium/src/test/java/stirling/software/jpdfium/doc/PdfxMetadataSafetyTest.java
Apply the Java 25 and FFM rules from .github/CODERABBIT_REVIEW.md.
⚙️ CodeRabbit configuration file
Files:
jpdfium/src/main/java/stirling/software/jpdfium/transform/PdfPageGeometry.javajpdfium/src/test/java/stirling/software/jpdfium/doc/PdfImageLosslessTest.javajpdfium/src/test/java/stirling/software/jpdfium/doc/IccCmykTransformTest.javajpdfium/src/test/java/stirling/software/jpdfium/doc/PdfBinaryFreeApiTest.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfIccProfiles.javajpdfium/src/test/java/stirling/software/jpdfium/doc/PdfxMetadataSafetyTest.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfXStandard.javajpdfium/src/main/java/stirling/software/jpdfium/doc/IccCmykTransform.javajpdfium/src/main/java/stirling/software/jpdfium/panama/FormFillBindings.javajpdfium/src/main/java/stirling/software/jpdfium/panama/PageEditBindings.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfOutputIntent.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfRecolor.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfXConverter.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfXOptions.javajpdfium/src/main/java/stirling/software/jpdfium/panama/ColorBindings.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfColorConverter.java
Source excerpt: Use imports rather than package-qualified type names in method bodies and signatures.
📄 CodeRabbit inference engine (.github/CODERABBIT_REVIEW.md)
Files:
buildSrc/src/main/kotlin/jpdfium.library-conventions.gradle.ktsNOTICEdocs/README.mdjpdfium/src/main/resources/icc/JPDFiumCompactCmyk.NOTICE.txtreachability-metadata/metadata/com.stirling/jpdfium/1.2.0/reachability-metadata.jsonjpdfium/src/main/resources/META-INF/native-image/com.stirling/jpdfium/reachability-metadata.jsonnative/rust/jpdfium-impl/Cargo.tomljpdfium/src/main/java/stirling/software/jpdfium/transform/PdfPageGeometry.javanative/licenses/NOTICE.txtnative/bridge/tests/smoke.cppnative/bridge/jpdfium.defjpdfium/src/test/java/stirling/software/jpdfium/doc/PdfImageLosslessTest.javanative/bridge/include/jpdfium_rust.hjpdfium/src/test/java/stirling/software/jpdfium/doc/IccCmykTransformTest.javajpdfium/src/test/java/stirling/software/jpdfium/doc/PdfBinaryFreeApiTest.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfIccProfiles.javajpdfium/build.gradle.ktsjpdfium/src/test/java/stirling/software/jpdfium/doc/PdfxMetadataSafetyTest.javadocs/color-pipeline.mdjpdfium/src/main/java/stirling/software/jpdfium/doc/PdfXStandard.javajpdfium/src/main/java/stirling/software/jpdfium/doc/IccCmykTransform.javanative/CMakeLists.txtjpdfium/src/main/java/stirling/software/jpdfium/panama/FormFillBindings.javajpdfium/src/main/java/stirling/software/jpdfium/panama/PageEditBindings.javadocs/editing.mdnative/tools/gen_compact_icc.cjpdfium/src/main/java/stirling/software/jpdfium/doc/PdfOutputIntent.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfRecolor.javajpdfium/src/main/java/stirling/software/jpdfium/doc/PdfXConverter.javanative/rust/jpdfium-impl/src/lib.rsjpdfium/src/main/java/stirling/software/jpdfium/doc/PdfXOptions.javanative/bridge/src/jpdfium_stub.cppjpdfium/src/main/java/stirling/software/jpdfium/panama/ColorBindings.javanative/setup-pdfium.shnative/bridge/include/jpdfium.hnative/bridge/src/jpdfium_color.cppjpdfium/src/main/java/stirling/software/jpdfium/doc/PdfColorConverter.java
🪛 ast-grep (0.45.3)
jpdfium/src/test/java/stirling/software/jpdfium/doc/PdfxMetadataSafetyTest.java
[warning] 117-117: Use a randomly-generated IV
Context: byte[] utf16 = ("\uFEFF" + xmp).getBytes(StandardCharsets.UTF_16LE);
Note: [CWE-329] Generation of Predictable IV with CBC Mode.
(random-iv)
🪛 GitHub Check: CodeQL
jpdfium/src/main/java/stirling/software/jpdfium/transform/PdfPageGeometry.java
[notice] 309-310: Unread local variable
Variable 'int ignored' is never read.
[notice] 315-316: Unread local variable
Variable 'int ignored' is never read.
🪛 LanguageTool
docs/color-pipeline.md
[uncategorized] ~85-~85: The official name of this software platform is spelled with a capital “H”.
Context: ...s future work. 4. Ghent gate in CI. .github/workflows/ghent.yml builds the patched...
(GITHUB)
🔇 Additional comments: 48
jpdfium/src/main/java/stirling/software/jpdfium/doc/PdfRecolor.java: 6
82-83: Duplicate of an earlier review comment. The code at these lines still has the issue.for (int i : pageIndices)unboxes each element, so anullelement throwsNullPointerException.doc.pageCount()is also called on every iteration. Hoist the page count and rejectnullelements.
150-153: Duplicate of an earlier review comment. Thecatch (Throwable t)aroundFPDFPage_GenerateContentdoes not callNativeRuntime.rethrowFatal(t). A fatal error such asOutOfMemoryErroris therefore wrapped inJPDFiumException. Every other downcall guard in this file callsrethrowFatalfirst.
96-99: Duplicate of an earlier review comment. Thesecatch (Throwable)blocks callrethrowFataland then swallow the failure by returning0, continuing, or returningfalse. A non-fatal binding failure, such asWrongMethodTypeException, is indistinguishable from "nothing to recolour".applycan then return0and look successful. Wrap these failures inJPDFiumException, as theGenerateContentpath does.Also applies to: 117-120, 126-129, 194-197
167-175: Duplicate of an earlier review comment. WhensourceSpaceis-1, the native color-space probe is unavailable or failed. Control then goes to the CMYK probe. If the CMYK probe also returnsfalse, the object reaches the RGB getter. ICC or Separation colors can then be rewritten as DeviceRGB. ReturnfalsewhensourceSpace < 0.
254-268: Duplicate of an earlier review comment. Three problems remain:
stretchcasts thelongfromMath.roundtointbeforeMath.clamp. A large or infinitecontrastFactortruncates to arbitrary bits instead of clamping. This violates the guideline "Check conversions, integer arithmetic, sizes, alignment, finite floating-point inputs, and narrowing".contrastFactor(double)does not reject non-finite values.tintand the stretch branch ofcontrastreturntrueeven whenoutequals the input. This contradicts themapJavadoc and causes no-op page rewrites that inflate the changed count.Pass the
longtoMath.clampdirectly, validate finiteness in the builder, and compareoutwith the input inmap.Also applies to: 280-290, 321-321
307-307: Duplicate of an earlier review comment.mode(null)is accepted. TheNullPointerExceptionthen occurs later inmap.recolorOnecatches and hides that exception. UseObjects.requireNonNull(m, "mode")in the builder.native/CMakeLists.txt: 2
646-648: The probes still usestring(FIND ...). This repeats the open declaration-aware probe comment.A substring match can find a comment or a longer identifier. In that case, CMake defines
JPDFIUM_HAVE_CMYK_APIorJPDFIUM_HAVE_EPDF_JPEG, andjpdfium_color.cppthen fails to compile. Apply the earlier proposed regex check to both probes.Also applies to: 664-665
205-205: LGTM!NOTICE: 2
60-60: Thejpeg-encoderentry still omits the IJG license and attribution.An earlier review marked this as addressed, but line 60 still reads
MIT OR Apache-2.0. The crate's license is(MIT OR Apache-2.0) AND IJG. The statically linked binary therefore ships without the required IJG attribution.
18-21: LGTM!native/bridge/src/jpdfium_stub.cpp: 2
1681-1684: The stub still returnsJPDFIUM_ERR_NATIVEfor features that are not available.In a real build without these features,
jpdfium_color_samples_to_cmyk_jpegandjpdfium_color_output_intent_bytesreturnJPDFIUM_ERR_NOT_FOUND. The stub returns a different code, so stub-backed tests check a different error mapping. This matches the earlier open comment.Also applies to: 1698-1702
1346-1350: LGTM!Also applies to: 1675-1680, 1686-1697, 1704-1720
native/bridge/tests/smoke.cpp: 2
218-219: The capability assertion is still unconditional when Rust is present.An earlier review marked this as addressed, but the code is unchanged. CMake treats
EPDFImageObj_SetJpegas optional. If a Rust-enabled build lacks it,jpdfium_has_rust() == 1whileJPDFIUM_COLOR_CAP_CMYK_IMAGEstays clear, andnative_smokefails. Assert only that the capability bit implies Rust.
150-151: LGTM!Also applies to: 209-217, 220-238
native/licenses/NOTICE.txt: 1
136-139: The IJG terms and attribution are still missing forjpeg-encoder.An earlier review marked this as addressed, but the entry still lists only
MIT OR Apache-2.0. The root cause is the same asNOTICEline 60.native/bridge/src/jpdfium_color.cpp: 1
498-504: The Windows narrow-path APIs are still in place.
_sopen_sandMoveFileExAinterpret UTF-8 path bytes in the ANSI code page, while qpdf treats the same strings as UTF-8. A non-ASCII path can therefore refer to different files during staging and publication. This matches the earlier open comment.Also applies to: 534-535
jpdfium/src/main/java/stirling/software/jpdfium/panama/ColorBindings.java: 1
134-139: The ICC getters are still bound withJAVA_LONGfor Cunsigned long.An earlier review marked this as addressed, but the descriptors are unchanged. On LLP64 Windows,
unsigned longis 4 bytes. The return value and thelengthargument therefore do not match the native ABI. UseLinker.nativeLinker().canonicalLayouts().get("long").As per coding guidelines: "Audit LP64/LLP64 explicitly."
jpdfium/src/main/java/stirling/software/jpdfium/doc/IccCmykTransform.java: 1
107-117: The ICC path without a target profile still parses the source profile for every object.An earlier review marked this as addressed, but
sourceToCmykis unchanged. For each fill and stroke, it callsstaticSourceSpaceand thenICC_Profile.getInstanceplusnew ICC_ColorSpace. The one-entry cache insourceSpaceis not used on this path.jpdfium/src/test/java/stirling/software/jpdfium/doc/PdfBinaryFreeApiTest.java: 2
74-83: The test still compares CMYK support withvectorCmykAvailable()alone.An earlier review marked this as addressed, but the code is unchanged.
supportsColorSpace(CMYK)also requirescolorSpaceAvailable()andiccConversionAvailable(). The assertion therefore fails on a build that exports the setters but not the probes.
19-26: LGTM!Also applies to: 97-99
docs/editing.md: 3
199-199: ThesupportsImageConversion()row still omits theEPDFImageObj_SetJpegrequirement.An earlier review marked this as addressed, but the text is unchanged. A build that has only the encoder reports
false.
233-233: The-1sentinel still does not mention partial page rewrites.If a page fails after earlier pages were converted,
convertImagesToCmykreturns-1and the document is partially rewritten. This matches the earlier open comment.
180-198: LGTM!Also applies to: 200-232, 234-314
jpdfium/src/main/resources/icc/JPDFiumCompactCmyk.NOTICE.txt: 1
10-10: The license statement still conflicts with the profile's embedded copyright tag.An earlier review marked this as addressed, but nothing changed. This notice says MIT.
gen_compact_icc.cline 116 writes "Public domain compact CMYK fallback profile" into the ICC file.native/tools/gen_compact_icc.c: 1
105-118: The stage allocations and thecmsWriteTagresults are still unchecked.An earlier review marked this as addressed, but nothing changed. If a stage allocation or tag write fails, the tool writes a profile without
A2B0orB2A0and still exits 0.native/setup-pdfium.sh: 1
381-951: LGTM!native/bridge/include/jpdfium.h: 1
710-786: LGTM!native/bridge/include/jpdfium_rust.h: 1
115-120: LGTM!native/bridge/jpdfium.def: 1
18-18: LGTM!native/rust/jpdfium-impl/Cargo.toml: 1
22-27: LGTM!native/rust/jpdfium-impl/src/lib.rs: 1
19-31: LGTM!Also applies to: 138-138, 173-175, 319-319, 485-583
reachability-metadata/metadata/com.stirling/jpdfium/1.2.0/reachability-metadata.json: 1
233-279: LGTM!Also applies to: 1838-1885
.gitattributes: 1
32-32: LGTM!.editorconfig-checker.json: 1
13-14: LGTM!jpdfium/src/main/java/stirling/software/jpdfium/doc/PdfColorConverter.java: 1
7-57: LGTM!Also applies to: 67-71, 85-90, 104-112, 126-139, 141-266, 268-366, 371-521, 529-700
jpdfium/src/test/java/stirling/software/jpdfium/doc/IccCmykTransformTest.java: 1
1-46: LGTM!jpdfium/src/test/java/stirling/software/jpdfium/doc/PdfImageLosslessTest.java: 1
1-170: LGTM!jpdfium/build.gradle.kts: 1
449-453: LGTM!Also applies to: 479-481
docs/README.md: 1
18-18: LGTM!docs/color-pipeline.md: 1
1-122: LGTM!jpdfium/src/main/java/stirling/software/jpdfium/doc/PdfOutputIntent.java: 1
1-161: LGTM!jpdfium/src/main/java/stirling/software/jpdfium/doc/PdfXConverter.java: 1
1-149: LGTM!jpdfium/src/main/java/stirling/software/jpdfium/doc/PdfXOptions.java: 1
1-218: LGTM!jpdfium/src/main/java/stirling/software/jpdfium/doc/PdfXStandard.java: 1
1-62: LGTM!jpdfium/src/main/java/stirling/software/jpdfium/doc/PdfIccProfiles.java: 1
1-42: LGTM!jpdfium/src/test/java/stirling/software/jpdfium/doc/PdfxMetadataSafetyTest.java: 1
1-153: LGTM!Also applies to: 158-182
buildSrc/src/main/kotlin/jpdfium.library-conventions.gradle.kts: 1
131-132: LGTM!jpdfium/src/main/resources/META-INF/native-image/com.stirling/jpdfium/reachability-metadata.json: 1
233-279: 🗄️ Data Integrity & IntegrationAll listed downcall signatures are present.
The metadata contains the three cited signatures, and the bundled metadata files are identical. The missing-signature concern does not apply.
| int beforeConverted = state.converted; | ||
| boolean beforeDirty = state.dirty; | ||
| scanFormObjects(obj, options, xform, r, g, b, a, cmyk, state); | ||
| if (state.converted != beforeConverted || state.dirty != beforeDirty) { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Stroke-only edits inside a form are lost when the page is already dirty.
The form is marked dirty only when state.converted or state.dirty changes. converted counts fill conversions only. In scanObject, a stroke conversion sets dirty = true and does not increment converted.
Trigger: an earlier object on the page converts, so beforeDirty is already true. A form then contains stroked paths with a DeviceRGB stroke and the default DeviceGray fill, which is preserved. The form's children are rewritten, but neither counter changes. As a result:
FPDFFormObj_MarkDirtyis not called.FPDFPage_GenerateContentdiscards the form edits.- The output keeps RGB strokes, which defeats the CMYK/PDF/X conversion.
The same gap exists for nested forms.
Fix: track an edit counter that every successful fill or stroke write increments, and compare that counter.
Proposed fix
private static final class ConversionState {
private final Set<Long> visitedForms;
private int converted;
+ private int edits;
private boolean dirty;
@@
- int beforeConverted = state.converted;
- boolean beforeDirty = state.dirty;
+ int beforeEdits = state.edits;
scanFormObjects(obj, options, xform, r, g, b, a, cmyk, state);
- if (state.converted != beforeConverted || state.dirty != beforeDirty) {
+ if (state.edits != beforeEdits) {
@@
if (convertColor(obj, options, xform, r, g, b, a, cmyk, true)) {
state.dirty = true;
+ state.edits++;
state.converted++;
}
if (convertColor(obj, options, xform, r, g, b, a, cmyk, false)) {
state.dirty = true;
+ state.edits++;
}| float[] comps = fill | ||
| ? ColorBindings.fillColorComponents(obj) | ||
| : ColorBindings.strokeColorComponents(obj); | ||
| byte[] sourceIcc = fill | ||
| ? ColorBindings.fillColorIccProfile(obj) | ||
| : ColorBindings.strokeColorIccProfile(obj); | ||
| if (comps == null || sourceIcc == null) return false; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy lift
🔎 Supported by static analysis
🏁 Script executed:
sed -n '500,584p' jpdfium/src/main/java/stirling/software/jpdfium/doc/PdfColorConverter.java
sed -n '224,274p' jpdfium/src/main/java/stirling/software/jpdfium/panama/ColorBindings.javaRepository: Stirling-Tools/JPDFium
Length of output: 5693
🏁 Script executed:
rg -n -F --glob '*.java' -e 'class IccCmykTransform' -e 'sourceSpace' -e 'convertIccToCmyk' -e 'fillColorIccProfile' -e 'strokeColorIccProfile' jpdfium/src/main/java
printf '\n--- transform and surrounding callers ---\n'
rg -n -F --glob '*.java' -e 'IccCmykTransform' jpdfium/src/main/java/stirling/software/jpdfium/doc/PdfColorConverter.java
printf '\n--- transform source files ---\n'
rg --files jpdfium/src/main/java | rg 'Icc|Color'Repository: Stirling-Tools/JPDFium
Length of output: 5076
🏁 Script executed:
printf '%s\n' '--- IccCmykTransform.java ---'
nl -ba jpdfium/src/main/java/stirling/software/jpdfium/doc/IccCmykTransform.java | sed -n '1,205p'
printf '%s\n' '--- PdfColorConverter dispatch ---'
nl -ba jpdfium/src/main/java/stirling/software/jpdfium/doc/PdfColorConverter.java | sed -n '430,470p'
printf '%s\n' '--- ColorBindings profile helper and handles ---'
nl -ba jpdfium/src/main/java/stirling/software/jpdfium/panama/ColorBindings.java | sed -n '190,270p'Repository: Stirling-Tools/JPDFium
Length of output: 16438
Avoid copying the ICC profile before cache lookup.
Each reached ICCBased fill or stroke conversion performs two native profile calls, allocates a confined arena buffer, and copies the full profile into a new byte[]. IccCmykTransform.sourceSpace compares that copied array only afterward, so the cache avoids profile parsing but not the per-object transfer and allocation.
The “gigabytes” estimate depends on the number of objects and profile sizes, so do not state it without measurement. If an ICC-heavy fixture shows material cost, provide a stable native profile identity so the converter can reuse the cached profile before copying. Profile length alone is not sufficient.
| StringBuilder hostile = new StringBuilder(10 * 1024 * 1024 + prefix.length() + suffix.length()); | ||
| hostile.append(prefix); | ||
| hostile.append("<rdf:li>".repeat(300_000)); | ||
| hostile.append(suffix); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The "huge" packet is below the 8 MiB cap, so this test never reaches the no-parse branch.
"<rdf:li>".repeat(300_000) is about 2.4 MB. kMaxXmpBytes is 8 MiB, so UpdateOrCreatePdfxMetadata sends the packet to pugixml. The parse fails on the unclosed tags, and the fallback replaces the packet. The test passes either way, so the xml.size() > kMaxXmpBytes branch has no coverage, although the test name claims it does. The 10 MiB StringBuilder capacity shows that the intent was a packet over the cap.
Proposed fix
- hostile.append("<rdf:li>".repeat(300_000));
+ // 8 bytes * 1.1M = ~8.8 MB, above the native 8 MiB parse cap.
+ hostile.append("<rdf:li>".repeat(1_100_000));As per path instructions: "Enforce meaningful assertions … Reject silently passing tests."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| StringBuilder hostile = new StringBuilder(10 * 1024 * 1024 + prefix.length() + suffix.length()); | |
| hostile.append(prefix); | |
| hostile.append("<rdf:li>".repeat(300_000)); | |
| hostile.append(suffix); | |
| StringBuilder hostile = new StringBuilder(10 * 1024 * 1024 + prefix.length() + suffix.length()); | |
| hostile.append(prefix); | |
| // 8 bytes * 1.1M = ~8.8 MB, above the native 8 MiB parse cap. | |
| hostile.append("<rdf:li>".repeat(1_100_000)); | |
| hostile.append(suffix); |
Source: Path instructions
| // An image with an /SMask or /Mask loses that mask when its stream dict is | ||
| // replaced by a re-encoded JPEG (CMYK JPEG has no alpha channel), so those | ||
| [[maybe_unused]] bool ImageHasTransparency(FPDF_DOCUMENT doc, FPDF_PAGE page, FPDF_PAGEOBJECT obj) { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
The comment for ImageHasTransparency stops mid-sentence.
The comment ends with "so those". Complete the sentence, for example: "so those images are skipped."
af097bd to
bd99c57
Compare
No description provided.