feat(oiiotool): oiiotool new expression eval tokens IS_CONSTANT, IS_BLACK #4610
Conversation
src/oiiotool/expressions.cpp
Outdated
|
|
||
| } else if (metadata == "IS_CONSTANT") { | ||
| std::vector<float> color((*img)(0, 0).nchannels()); | ||
| if (ImageBufAlgo::isConstantColor((*img)(0, 0), color)) { |
There was a problem hiding this comment.
I think this is not quite correct for the declaration of isConstantColor. I believe you want
| if (ImageBufAlgo::isConstantColor((*img)(0, 0), color)) { | |
| if (ImageBufAlgo::isConstantColor((*img)(0, 0), 0.0f, color)) { |
The 2nd parameter is the threshold amount (0.0f would mean a "true constant", exact same value everywhere), and the 3rd parameter is the place to store the constant value it finds.
src/oiiotool/expressions.cpp
Outdated
| auto pixstat = ImageBufAlgo::computePixelStats((*img)(0, 0)); | ||
| std::vector<float> color((*img)(0, 0).nchannels()); | ||
| // Check constant first to guard against false positive average of 0 with negative values i.e. -2, 1, 1 | ||
| if (ImageBufAlgo::isConstantColor((*img)(0, 0), color)) { |
There was a problem hiding this comment.
| if (ImageBufAlgo::isConstantColor((*img)(0, 0), color)) { | |
| if (ImageBufAlgo::isConstantColor((*img)(0, 0), 0.0f, color)) { |
src/oiiotool/expressions.cpp
Outdated
|
|
||
|
|
||
| } else if (metadata == "IS_BLACK") { | ||
| auto pixstat = ImageBufAlgo::computePixelStats((*img)(0, 0)); |
There was a problem hiding this comment.
I don't think you need to call computePixelStats. You can save the expense, since you are calling isConstantColor() and that will deposit the constant color it finds into color, then you can just check if the values of color[0..nchans-1] are all 0.0f to know if it's black.
src/oiiotool/expressions.cpp
Outdated
| for (size_t i = 0; i < pixstat.avg.size(); ++i) { | ||
| // If any of the pixel doesn't have an average of (0,0,0), then we don't have a black frame | ||
| // Is this the best way to check? Feels fishy to have the specified number...should we look at 0.0f or other formats? | ||
| if (pixstat.avg[i] == 0.000000) { |
There was a problem hiding this comment.
0.0f is sufficient. But you don't need to look at the image's average color -- you can just use color[0] since isConstantColor returns that constant color.
src/oiiotool/expressions.cpp
Outdated
| result = "1"; | ||
| } else { | ||
| result = "0"; |
There was a problem hiding this comment.
I think this logic is incorrect. Doesn't this end up setting result based solely on whether the LAST channel is black or not? You want the result to be "0" if any channel is not black, right?
Thank you so much for taking such a thorough look, Larry! |
lgritz
changed the title
lgritz
added
enhancement
Signed-off-by: Lydia Zheng <[email protected]>
lydia-zheng
force-pushed
the
feat_black_constant_frames
branch
from
a55baa0 to
9efbfd5
Compare
Signed-off-by: Lydia Zheng <[email protected]>
|
Hi @lgritz ! Took another jab at this with your amazing feedback, and fixed the feedstock/git commits such that CLs are all green. Let me know if this is good to merge or if the new logic still looks off! |
Signed-off-by: Larry Gritz <[email protected]>
lgritz
force-pushed
the
feat_black_constant_frames
branch
from
634b877 to
616553b
Compare
|
Just before merging, I got cold feet. I didn't feel good about this not having a test, so I took the liberty of adding one and amending your PR (it's just a few lines). I'll merge after it passes PR. |
That’s super fair and thanks for adding the tests and checking, Larry! Actually had some local files in test suite but was waiting for the base PR to go through first, though what you did was much better for ensuring it worked. |
…LACK (AcademySoftwareFoundation#4610) Included two additional metadata boolean checks for constant and black frame images, so that handling for those image cases are facilitated. IS_CONSTANT and IS_BLACK return 1 when true, and 0 when false. Signed-off-by: Lydia Zheng <[email protected]>
Description
Included two additional metadata boolean checks for constant and black frame images, so that handling for those image cases are facilitated.
IS_CONSTANT and IS_BLACK return 1 when true, and 0 when false.
Tests
No new tests added to testsuite for now, but tested with local build copy of oiiotool and test images to ensure it behaved as expected.
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.