-
Notifications
You must be signed in to change notification settings - Fork 649
feat(oiiotool): eraseattrib_fromfile #4763
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
feat(oiiotool): eraseattrib_fromfile #4763
Conversation
Signed-off-by: Lydia Zheng <[email protected]>
|
Hi, Lydia, can you please fix the formatting? If you look at the CI clang-format run (look for the red and green highlights) you can see it's just a few lines that you could easily fix by hand if you don't have clang-format set up to run locally. |
Yes Larry! Already working on a clang-format fix right now! What I'm more worried about is the bleeding edge gcc14 test that failed? Digging into the log seems to imply that there are pixel differences from a test field that's completely untouched... |
Signed-off-by: Lydia Zheng <[email protected]>
|
|
||
| command += oiiotool (OIIO_TESTSUITE_IMAGEDIR+"/tahoe-gps.jpg --eraseattrib_fromfile src/regex_list.txt -o nomakegps.jpg") | ||
| command += info_command ("nomakegps.jpg", safematch=True) | ||
|
|
||
|
|
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.
Great test!
|
The "bleeding edge" failure is unrelated. I just submitted a fix here: #4765 |
|
Taking a look at incorporating all the comments after redo-ing the format fix! |
|
Because Lydia and I had talked about this offline, I want to fill in a bit of the motivation here for others who are reading: It's common in a studio to have tools insert all sorts of metadata into files as they're rendered, etc. But a lot of that are things that SHOULD NOT leave the facility -- like security issues from internal pathnames or network info, or secrets like plot details revealed by sequence or character names. So generally you want to scrub the metadata from the final images before they are shipped to the client or to marketing or whatever. oiiotool's
We thought a solution to facilitate this might be a version of --eraseattrib that takes a filename that contains a list of regexes, one per line of the text file. So that master list of things to scrub could be maintained in a central place and not need to update every oiiotool-calling script in order to update it. Furthermore, you could have the command say something like to make a single command that allows per-show customization. |
|
Question for onlookers: Lydia implemented this as a new command, An alternate design we discussed was just using the existing command but with a new optional modifier: Does anybody have a strong preference? Or any other feedback on the basic design? |
Signed-off-by: Lydia Zheng <[email protected]>
Signed-off-by: Lydia Zheng <[email protected]>
Signed-off-by: Lydia Zheng <[email protected]>
Signed-off-by: Lydia Zheng <[email protected]>
lgritz
left a comment
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.
It's really close. I pointed out one line that is no longer needed.
I'd like to hold this in review for a few days, simply for the sake of allowing others the chance to comment on design of the feature.
Also, before merging, we really should make sure that the documentation mentions this feature.
Sounds good Larry, I was checking the tests and it might have updated the branch to merge, feel free to cancel/ ignore. Adding a list of todos since there’s a chance that I might not be able to work on this soon:
|
|
Larry, in re: whether to have a separate command-line option to take the list from the file rather than an immediate argument: is there any precedent one way or the other with existing options, which might argue for consistency in the service of least surprise? And yeah it's an important thing, I wrote something once (as a shell script, so no one could accuse it of harboring a trojan) to strip out all sorts of stuff from the output of an exrheader command. One doesn't want a camera "notes" field that says "Quicksilver doesn't die in this take" to leak. Even with those precautions though producers wouldn't allow my contact to use it. Maybe they figured that someone could deduce a plot point from the difference between the display and data windows. |
|
Thanks for the feedback, Joseph. I don't believe that oiiotool has any other commands at the moment that read arguments from files. But maybe it will in the future, so I agree that we want to think carefully about what we establish as the precedent. Are there other utilities (outside of oiio) that have similar functionality? Ah, the one that popped into my mind for some reason is "rsync", and its man page includes: And I checked So they are using separate commands, and seem to follow a common "--foo-from filename" pattern. But they don't the kind of "optional command modifiers" that we do, so maybe they didn't consider that part of the design space? I'm mildly preferring the second option, simply because if someday we have many commands we want to operate like this, we don't have to make a completely new command for each instance. We can just make more commands use the same "fromfile" optional modifier. Use of optional modifiers in this way is already a very common oiiotool idioms that users expect. That's my current thought, anyway. If everybody strongly prefers Design 1, I won't object. |
Signed-off-by: Lydia Zheng <[email protected]>
…zheng/OpenImageIO into feat_eraseattrib_fromfile
Signed-off-by: Lydia Zheng <[email protected]>
I personally prefer the second option as well, for exactly the reasons you've alluded to:
|
|
Your docs are great, thanks. To do:
|
Description
Extended eraseattrib to a prototyped standalone flag "--eraseattrib_fromfile" that allows for erasure of metadata from a text file with a list of regex.
Erases all metadata attribute fields that match.
Tests
Added a new test to
oiiotool-attribswhich has been verified painstakenly to be working!Also tried running the command directly from dist/bin and verified with
oiiotool --info -v.Checklist:
need to update the documentation, for example if this is a bug fix that
doesn't change the API.)
(adding new test cases if necessary).
corresponding Python bindings (and if altering ImageBufAlgo functions, also
exposed the new functionality as oiiotool options).
already run clang-format before submitting, I definitely will look at the CI
test that runs clang-format and fix anything that it highlights as being
nonconforming.