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

[FEATURE REQUEST] Add additional approximate diff methods to oiiotool #4276

Open
reinecke opened this issue May 28, 2024 · 14 comments
Open

[FEATURE REQUEST] Add additional approximate diff methods to oiiotool #4276

reinecke opened this issue May 28, 2024 · 14 comments
Labels
Dev Days ASWF Dev Days suitable project devdays24 Dev Days 2024 difficulty/medium Days to weeks for veterans, very challenging for newcomers feature request help wanted A task that is desired, but needs somebody to commit the effort to implement it. image processing Related to ImageBufAlgo or other image processing topic. oiiotool oiiotool

Comments

@reinecke
Copy link

reinecke commented May 28, 2024

oiiotool has options for absolute tolerance based diff as well as pdiff.

It can be useful to have additional diff methods based on whether the image differences are "perceptually significant". One use for this is to prevent false fails in automated validations of image generation/processing systems where very minor or "imperceptible" differences are allowable.

Some potential diff methods could be:

  • Tolerances as described by the formula in the ACES CLF implementation guide - an example using oiiotool with this methodology can be found in an OpenColorIO script
@lgritz
Copy link
Collaborator

lgritz commented May 28, 2024

Others I've been meaning to try, or that people have suggested over the years:

I'm sure there's been lots of work in this area over the years since Yee, I'm not even sure which are considered best of class these days.

@ThiagoIze
Copy link
Collaborator

What @lgritz cited sounds good to me. However, now that we have HDR displays, another thing to consider is whether we want our difference metric to also account for that. And if we do, we probably need the help of ocio so that these computations are done in the proper display space.

@pleprince
Copy link

FWIW, I have found Flip extremely useful in CI over the last few years.

@lgritz
Copy link
Collaborator

lgritz commented Jun 10, 2024

Chris points out that the Flip has sample code here: https://github.com/NVlabs/flip/blob/main/cpp/FLIP.h

@lgritz lgritz added image processing Related to ImageBufAlgo or other image processing topic. feature request oiiotool oiiotool difficulty/medium Days to weeks for veterans, very challenging for newcomers help wanted A task that is desired, but needs somebody to commit the effort to implement it. labels Jun 10, 2024
@colerjstevenson
Copy link

Planning on taking a swing at implementing Flip for DevDays

@lgritz
Copy link
Collaborator

lgritz commented Sep 26, 2024

Great, Cole! I know people have wanted this feature for a long time, and we'll certainly use it at work. It's bigger than most dev days tasks, so don't be discouraged if it takes more than a day to get finalized, because it's a high-impact task and worth the wait.

@lgritz lgritz added Dev Days ASWF Dev Days suitable project devdays24 Dev Days 2024 labels Sep 26, 2024
@lgritz
Copy link
Collaborator

lgritz commented Sep 26, 2024

Also, @colerjstevenson, it's ok to break it into several sized chunks, potentially even as separate PRs to get them reviewed and merged sequentially rather than feeling that you must have every last detail. Roughly speaking, what you need for functionality like this is:

  • Make a new ImageBufAlgo function that implements the flip differencing, and some kind of minimal contrived unit test in imagebufalgo_test.
  • Add Python bindings for this new IBA function, test in testsuite/python-imagebufalgo
  • Expose it as a command-line argument to oiiotool (maybe --flipdiff?), including tests in testsuite/diff
  • Documentation for all the above (either separately, or each part as it gets done)

@colerjstevenson
Copy link

Thanks Larry! This gives be a great starting point to work off of! yeah I have a suspicious this might a project that requires work past the deadline of dev days

@lgritz
Copy link
Collaborator

lgritz commented Sep 26, 2024

It is not a problem for it to take longer. Even when people get all the coding done in a day, it's quite common for it to simmer a few more days to get review cycle completed, make minor requested changes, etc. No big deal. Also, it is fine to break into multiple PRs.

If you are comfortable with the scope of task you chose to take on, I'm fine with it extending past the day. Especially because this one is something we've really wanted and will definitely be used right away.

@colerjstevenson
Copy link

just as an update, this is definetly a project that I'll need to continue post devdays but now I have a better idea of the scope so I'll keep picking away at it over the next little bit and keep you posted

@lgritz
Copy link
Collaborator

lgritz commented Sep 28, 2024

All good! I look forward to seeing it completed. Please don't hesitate to ask questions or at any point show us what you have for feedback.

@colerjstevenson
Copy link

colerjstevenson commented Nov 4, 2024

Progress update: I have a "working" version of my implementation pushed to my fork, there is still lot of work to be done, including documentation, tests, python bindings, and general cleaning, but this version can successful use oiiotool on the command line to compute the FLIP difference of both LDR and HDR images
colerjstevenson#2

@lgritz
Copy link
Collaborator

lgritz commented Nov 4, 2024

I just took a look and made some comments on your fork. But I think it would be better to make it a real PR against this project (you can mark it as a "draft"), so that it's easier to get feedback as you go and that we can all make suggestions in small steps. The last thing you want is to work on something for a long time and then find out only at the final review that it has long since veered off into some kind of design that doesn't fit in with our requirements or conventions.

The first place I want to steer you can be summarized by two points:

  1. Just like all other functions in imagebufalgo.h, the flip difference function needs to operate on ImageBuf objects. There's no reason to think that the images we're comparing exist on disk.
  2. It's unclear to me (so, for you to figure out) how much of the NVIDIA flip.h code -- if any -- can be directly used. I suspect that the right thing is to scavenge bits and pieces of that code that are the core per-pixel computations and the overall logic, but of course you'll need to replace all of the parts that access image data with ImageBuf::Iterator and whatnot, and probably can entirely jettison 90% of the NVIDIA code as irrelevant (like reading images from disk).

I recommend proceeding in truly bite-sized chunks -- like, I would make the first instalment for feedback just be the changes to imagebufalgo.h that shows the new function declaration and a detailed comment providing the "specification" for what that IBA function does (from a user's point of view, essentially writing the documentation first to make sure it's an API design that makes sense).

@colerjstevenson
Copy link

That makes total sense, I'll start looking into that, I had a suspicion this was only a version 1, I'll submit a pull request and then start picking away at it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dev Days ASWF Dev Days suitable project devdays24 Dev Days 2024 difficulty/medium Days to weeks for veterans, very challenging for newcomers feature request help wanted A task that is desired, but needs somebody to commit the effort to implement it. image processing Related to ImageBufAlgo or other image processing topic. oiiotool oiiotool
Projects
None yet
Development

No branches or pull requests

5 participants