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

ImageBufAlgo: Initial implementation of st_warp transform function #3379

Merged

Conversation

nrusch
Copy link
Contributor

@nrusch nrusch commented Mar 30, 2022

Description

Add a new ImageBufAlgo::st_warp function, intended to closely match the behavior of Nuke's STMap node.

The function takes a source ImageBuf to warp, and a secondary ImageBuf containing channels to treat as normalized image space (i.e. st) coordinates. For each pixel in the st buffer, the coordinates are used to look up a source pixel position, which is then sampled using a filter to produce a new output pixel.

The transform is currently only defined over the geometry of the st buffer (again, matching the behavior of the Nuke node), with the expectation that any desired scaling should be done to the st buffer prior to warping.

One notable difference between the Nuke node and this implementation is the filtering behavior: for better or worse, Nuke's filtering API does some extra level of interpolation/smoothing on top of the raw filter results (possibly just an extra linear filtering pass).

This set of changes has been developed off of dev-2.3 and then rebased onto master for the PR, so it is known to be portable between those branches.

Tests

I have not added any new tests at this point. I didn't see any examples of testing the spatial IBA functions in imagebufalgo_test.cpp, so I'm not sure what the policy is there, or how best to ensure strict correctness.

Checklist:

  • I have read the contribution guidelines.
  • If this is more extensive than a small change to existing code, I
    have previously submitted a Contributor License Agreement
    (individual, and if there is any way my
    employers might think my programming belongs to them, then also
    corporate).
  • I have updated the documentation, if applicable.
  • I have ensured that the change is tested somewhere in the testsuite
    (adding new test cases if necessary).
  • My code follows the prevailing code style of this project.

src/include/OpenImageIO/imagebufalgo.h Show resolved Hide resolved
src/libOpenImageIO/imagebufalgo_xform.cpp Outdated Show resolved Hide resolved
src/libOpenImageIO/imagebufalgo_xform.cpp Show resolved Hide resolved
src/libOpenImageIO/imagebufalgo_xform.cpp Outdated Show resolved Hide resolved
@lgritz
Copy link
Collaborator

lgritz commented Apr 1, 2022

I would recommend (if you think you can tackle it) adding oiiotool --st_warp, and then it becomes much easier to have robust tests for this by adding something appropriate to testsuite/oiiotool-xform/run.py.

@nrusch
Copy link
Contributor Author

nrusch commented Apr 4, 2022

I've added oiiotool --st_warp, and also added a new test to testsuite/oiiotool-xform/run.py.

However, I wanted to run something strange by you.

Initially, I generated my test reference from the existing reference resize.tif in the OIIO repo:
oiiotool -i testsuite/oiiotool-xform/ref/resize.tif --pattern fill:topleft=0,0,0:topright=1,0,0:bottomleft=0,1,0:bottomright=1,1,0 256x256 3 --powc 1.2 --st_warp -o testsuite/oiiotool-xform/ref/st_warped.tif

However, using that as my reference image was causing consistent test failures due to differences above the minimum difference threshold when compared with the output that used the resize.tif generated by the testsuite:

Comparing "st_warped.tif" and "ref/st_warped.tif"
256 x 256, 4 channel
  Mean error = 0.000463329
  RMS error = 0.00135965
  Peak SNR = 57.3314
  Max error  = 0.0078432 @ (13, 41, R)  values are 0.752941, 0.423529, 0.423529, 0 vs 0.745098, 0.423529, 0.423529, 0
  0 pixels (0%) over 0.008
  190 pixels (0.29%) over 0.004
FAILURE

I checked out the raw diff in Nuke, and the pixel differences are all 0-2 8-bit code values apart.

I then went back and regenerated my reference image from the resize.tif generated by the testsuite:

oiiotool -i build/platform-linux/gcc-6.3.1/boost\=\=1.70.0/python-2.7/\!nuke/testsuite/oiiotool-xform/resize.tif --pattern fill:topleft=0,0,0:topright=1,0,0:bottomleft=0,1,0:bottomright=1,1,0 256x256 3 --powc 1.2 --st_warp -o src/testsuite/oiiotool-xform/ref/st_warped.tif

Using this new reference image does not cause the same test failure, and this is the reference image I have added to the PR.

I wanted to mention this because I don't know how to explain it; the reference testsuite/oiiotool-xform/ref/resize.tif and the resize.tif generated by the testsuite on my end compare identical in Nuke and using idiff -warn 1e-08, so I'm not sure how they could be producing different results after the st_warp.

I also tested what would happen if I generated my reference image directly from the original grid.tif found in the oiio-images repo:
oiiotool -i ../oiio-images/grid.tif --resize 256x256 --pattern fill:topleft=0,0,0:topright=1,0,0:bottomleft=0,1,0:bottomright=1,1,0 256x256 3 --powc 1.2 --st_warp -o testsuite/oiiotool-xform/ref/st_warped.tif

This also causes test failures when compared with the resize.tif generated by the testsuite, with even greater pixel differences. However, if I change my test case to also start from grid.tif and do the resize on the fly before warping, it works.

Let me know if you have any insights or concerns.

@nrusch nrusch requested a review from lgritz April 11, 2022 17:13
Copy link
Collaborator

@lgritz lgritz left a comment

Choose a reason for hiding this comment

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

This looks really good. Some minor changes are suggested (see comments inline).

I would encourage you to also add docs for the new oiiotool command in oiiotool.rst (though I can give that a stab after this merges, if you just don't have the time for it).

src/include/OpenImageIO/imagebufalgo.h Outdated Show resolved Hide resolved
src/libOpenImageIO/imagebufalgo_xform.cpp Show resolved Hide resolved
src/libOpenImageIO/imagebufalgo_xform.cpp Outdated Show resolved Hide resolved
src/libOpenImageIO/imagebufalgo_xform.cpp Outdated Show resolved Hide resolved
src/libOpenImageIO/imagebufalgo_xform.cpp Show resolved Hide resolved
src/oiiotool/oiiotool.cpp Outdated Show resolved Hide resolved
@nrusch nrusch requested a review from lgritz April 15, 2022 01:49
@nrusch
Copy link
Contributor Author

nrusch commented Apr 15, 2022

I would encourage you to also add docs for the new oiiotool command in oiiotool.rst (though I can give that a stab after this merges, if you just don't have the time for it).

Whoops, I missed this one. I'll take a look at this tomorrow.

@nrusch
Copy link
Contributor Author

nrusch commented Apr 15, 2022

OK, I've added an --st_warp section to oiiotool.rst (and hopefully fixed the issue that broke the CI jobs).

@lgritz
Copy link
Collaborator

lgritz commented Apr 15, 2022

LGTM!

@lgritz lgritz merged commit fdafe1c into AcademySoftwareFoundation:master Apr 15, 2022
@lgritz
Copy link
Collaborator

lgritz commented Apr 15, 2022

@nrusch, do you want this backported to 2.3?

@nrusch
Copy link
Contributor Author

nrusch commented Apr 15, 2022

Yes please, that would be great. Thanks!

lgritz pushed a commit to lgritz/OpenImageIO that referenced this pull request Apr 15, 2022
…cademySoftwareFoundation#3379)

* Initial implementation of `ImageBufAlgo::st_warp`

* Simplify default filter logic, and share it with `ImageBufAlgo::warp`

* Add st_warp to oiiotool

* Add tests for bespoke `st_warp` validations that are done before IBAprep

* Add st_warp test to oiiotool-xform testsuite

* Address review notes

- Remove ST buffer datatype guard
- Simplify additional ROI prep
- Remove unnecessary buffer reallocation

* Use sample accumulator type for source/dest iterator USERT

* Add `--st_warp` section to `oiiotool.rst`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants