-
Notifications
You must be signed in to change notification settings - Fork 0
tests/Makefile: properly use cornersync mode from Makefile variable #9
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
base: trunk
Are you sure you want to change the base?
Conversation
Signed-off-by: Stefan Westerfeld <[email protected]>
Summary of ChangesHello @swesterfeld, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a change to the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
This PR fixes a configuration bug in the test harness where the watermark extraction command was ignoring the CORNERSYNC_MODE variable. The Makefile already defined this variable (defaulting to 'auto') and used it to conditionally exclude rotation/crop attacks when set to 'on', but the imagewmark get command wasn't receiving the --cornersync flag. The fix adds --cornersync $(CORNERSYNC_MODE) to the extraction command on line 125, ensuring the test configuration is honored end-to-end. This aligns the extraction behavior with the embedding and attack-selection logic that already respected the mode setting, making the test framework consistent when users want to validate cornersync-specific watermarking scenarios.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| tests/Makefile | 5/5 | Added --cornersync $(CORNERSYNC_MODE) flag to extraction command to honor test configuration |
Confidence score: 5/5
- This PR is safe to merge with minimal risk
- Score reflects a targeted bug fix that adds a missing configuration flag to match existing infrastructure; the change is isolated to the test harness and ensures consistency with pre-existing Makefile variables and conditional logic
- No files require special attention
Sequence Diagram
sequenceDiagram
participant User
participant Make
participant wmtool.py
participant imagewmark
participant Extract
User->>Make: "make all"
Make->>Make: "check-nproc (validate job count)"
Note over Make: Create/link input images
Make->>Make: "create inputs/ directory"
Note over Make: Embed watermarks
loop For each input image
Make->>imagewmark: "add --test-key <input> <output> <WATERMARK>"
imagewmark-->>Make: "watermarked image in wmarked/"
end
Note over Make: Generate attack images
loop For each attack type
Make->>wmtool.py: "--seed <SEED> --attack <params> <input> <output>"
wmtool.py-->>Make: "attacked image in attack/"
end
Note over Make: Extract watermarks (FIXED)
loop For each attack image
Make->>imagewmark: "get --test-key --expect <WATERMARK> --cornersync <CORNERSYNC_MODE> <image> --json"
imagewmark-->>Make: "extraction result JSON"
alt extraction failed and WITHOUT_ORIGINAL=false
Make->>imagewmark: "get --test-key --expect <WATERMARK> --original <original> --json"
imagewmark-->>Make: "retry with alignment"
end
Make->>Extract: "save to extract/*.json"
end
Note over Make: Generate reports
Make->>Extract: "collect all JSON results"
Make->>Make: "report.py <WATERMARK>"
Make-->>User: "report.html, report.pdf, flat.txt, fails.txt"
1 file reviewed, no comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request updates the tests/Makefile to use the CORNERSYNC_MODE variable when calling imagewmark get. The change is a good improvement, but it's incomplete. The --cornersync flag is missing from the fallback command that runs when the initial watermark extraction fails. I've added a comment to point this out so it can be fixed for consistency.
| $(QECHO) GET $@ | ||
| $Q mkdir -p $(dir $@) | ||
| $Q $(SRCDIR)/imagewmark get $(__TESTKEY) --expect $(WATERMARK) $< --json > $@.tmp | ||
| $Q $(SRCDIR)/imagewmark get $(__TESTKEY) --expect $(WATERMARK) --cornersync $(CORNERSYNC_MODE) $< --json > $@.tmp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While adding --cornersync $(CORNERSYNC_MODE) here is correct, the same flag is missing from the fallback imagewmark get call on line 128. This leads to inconsistent behavior, as the CORNERSYNC_MODE will not be respected if the initial extraction fails. For consistent behavior, please also add --cornersync $(CORNERSYNC_MODE) to the command on line 128.
Thanks for that. What about the fallback case that uses the original image though? extract.py currently contains this logic: Do you prefer to leave it as is and then also add the --cornersync mode argument in tests/Makefile to the fallback case? |
No description provided.