feat(raw): make the crop match in-camera JPEG

AcademySoftwareFoundation / OpenImageIO

Reading, writing, and processing images in a wide variety of file formats, using a format-agnostic API, aimed at VFX applications.

https://openimageio.readthedocs.org

Apache License 2.0

1.98k stars 597 forks source link

feat(raw): make the crop match in-camera JPEG #4397

Closed antond-weta closed 2 months ago

antond-weta commented 2 months ago

Description

Make the 'display window' match the default crop, which is applied to in-camera JPEG.

Tests

Checklist:

[x] I have read the contribution guidelines.
[x] I have updated the documentation, if applicable.
[x] I have ensured that the change is tested somewhere in the testsuite (adding new test cases if necessary).
[x] If I added or modified a C++ API call, I have also amended the corresponding Python bindings (and if altering ImageBufAlgo functions, also exposed the new functionality as oiiotool options).
[x] My code follows the prevailing code style of this project. If I haven't 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.

antond-weta commented 2 months ago

This seems to work with the images I tested with, coming from a Canon, Panasonic, and Blackmagic cameras I have on hand. TODO:

raw bayer mode. (will try to fix the issue mentioned in #3125 as well)
accepting a custom crop box from user
ideally testing with more cameras

antond-weta commented 2 months ago

Hi @lgritz, Could you please help me with the unit tests here. There are multiple ref/out*.txt files for different OSes and LibRaw versions, I'm not sure what to do with them.

lgritz commented 2 months ago

As far as the testsuite is concerned, a test "passes" as long as its output matches any one of the available reference outputs.

Most testsuite tests only need one reference output, but there are a few tests we have that need to rely on this multi-ref behavior, and raw is one of them because LibRaw tends to change so much from release to release. Thus, testsuite/raw sometimes needs to update several reference outputs, different ones for different CI matrix platforms.

I've got this fixed up for you and I will push an additional commit to your branch that contains the updated reference outputs that cover all of the test cases we need.

lgritz commented 2 months ago

I also rebased/merged to be on top of the current master.

This LGTM, should we mark as "ready for review" and get it merged? Or was there something else you hoped to do in this PR? (Like I've said before, I'm always in favor of a small PR that moves the ball forward in some concrete way without breaking anything. Nothing wrong with making more related changes in a subsequent PR.)

antond-weta commented 2 months ago

Yes, I think this is good to go. I'm still not certain whether we want to treat raw un-debayered output differently. I reckon it might be worth adding an option to suppress any cropping regardless of whether we demosaic or not. We can do this later if needed.

Thanks for fixing the tests!

lgritz commented 2 months ago

The crop doesn't alter the pixel data, it just sets the "full" (a.k.a. "displaywindow" in OpenEXR lingo) metadata, so there is no data loss. Somebody who receives the image and doesn't want the crop can just clear that or ignore it.

antond-weta commented 2 months ago

True, but this 'soft' crop is applied on top of the 'hard' crop LibRaw does. Which, I believe, was the reason for #3125. Some users may still be interested in seeing the masked pixels outside of the image area. I thought having an option for doing that might be useful.

lgritz commented 2 months ago

You're probably right. Like an open-with-config hint like "raw:nocrop" or something?