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.

Sign up for GitHub

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

Jump to bottom

Color Space management #1170

Merged
merged 15 commits into from
Oct 19, 2022
Merged

Color Space management #1170

merged 15 commits into from
Oct 19, 2022

Conversation

demoulinv
Copy link
Contributor

@demoulinv demoulinv commented May 5, 2022 •
edited
Loading

Description

A new private metadata named "AliceVision:ColorSpace" is now managed.
Read and Write image update to enable image processing in a color space given as a parameter.
imageProcessing executable updated to take advantage of the above possibility.

Features list

  • AliceVision:ColorSpace metadata added to image file when writing. Possible values are in the list srgb, srgb_linear, aces, acescg.
  • Enable image processing in a color space given as a parameter.
  • Implement color space conversion with OCIO at image reading to enable the selection of another color space than linear or sRGB as working color space.
  • Check existence of the OCIO config file and throw an error if not found when needed. 3 environment variables are considered, ALICEVISION_OCIO, OCIO and ALICEVISION_ROOT, see implementation details.
  • Set the working color space as a parameter in the imageProcessing executable.

Implementation remarks

  • Function getColorConfigFilePath() defined for avoiding some code duplication.
  • OCIO Color config file is first searched by using the ALICEVISION_OCIO env var. If it is undefined or points an unexisting file, the standardized OCIO env var is considered. If it is undefined or points an unexisting file, the ALICEVISION_ROOT env var is considered to point the embedded config file. If the ALICEVISION_OCIO or OCIO environment variables are set, they must be set with the path of the OCIO configuration file to be used.
  • Function getImageColorSpace(imagePath) defined.
  • Color space conversion already implemented at image reading. The target color space, previously named outputcolorSpace , field of the imageReadOptions structure is renamed in workingColorSpace.
  • In order to perform the rigth conversion, the image writer check the AliceVision:ColorSpace metadata, set when processing is done, to identify the working color space.
  • For clarification the parameter imageColorSpace of the writeImage methods is renamed outputImageColorSpace.

@demoulinv demoulinv force-pushed the dev/AVColorSpaceMetadata branch 4 times, most recently from 901803d to fd7deef Compare June 1, 2022 19:37
@demoulinv demoulinv force-pushed the dev/AVColorSpaceMetadata branch from fd7deef to 90fe1f4 Compare June 17, 2022 07:21
@demoulinv demoulinv force-pushed the dev/AVColorSpaceMetadata branch from c7eadcb to f1e19a6 Compare August 2, 2022 12:04
@demoulinv demoulinv force-pushed the dev/AVColorSpaceMetadata branch 2 times, most recently from 7753256 to cb3c321 Compare September 9, 2022 07:29
@fabiencastan fabiencastan added this to the 2.5.0 milestone Sep 9, 2022
@demoulinv demoulinv force-pushed the dev/AVColorSpaceMetadata branch 2 times, most recently from a7a8ace to 6baec4b Compare September 21, 2022 12:26
fabiencastan
fabiencastan reviewed Sep 22, 2022
View reviewed changes
src/aliceVision/image/io.cpp Outdated
return configOCIOFilePath;
}

EImageColorSpace getImageColorSpace(const std::string imagePath)
Copy link
Member

Choose a reason for hiding this comment

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

const std::string&

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does not compile anymore on Windows.

@demoulinv demoulinv force-pushed the dev/AVColorSpaceMetadata branch 6 times, most recently from 4889a6b to 6340ede Compare September 30, 2022 18:53
@demoulinv demoulinv force-pushed the dev/AVColorSpaceMetadata branch from e38b85d to f362e3d Compare October 5, 2022 14:15
@fabiencastan fabiencastan force-pushed the dev/AVColorSpaceMetadata branch from 9f4a563 to da97c07 Compare October 7, 2022 11:27
@fabiencastan
Copy link
Member

I don't understand why we have new build errors on windows CI which does not seem to be really related to the PR:

  D:\a\AliceVision\vcpkg\installed\x64-windows-release\include\boost/mpl/aux_/preprocessor/def_params_tail.hpp(23,10): fatal  error C1083: Cannot open include file: 'boost/preprocessor/identity.hpp': No such file or directory (compiling source file D:\a\AliceVision\AliceVision\src\aliceVision\system\Logger.cpp) [D:\a\AliceVision\AliceVision\build\src\aliceVision\system\aliceVision_system.vcxproj]

@p12tic
Copy link
Contributor

p12tic commented Oct 7, 2022

@fabiencastan I think the commit
SRGB_LINEAR becomes LINEAR to be more compliant with the OCIO config should be combined into the parent commit [Add AliceVision Color Space metadata]. The parent commit renames LINEAR to SRGB_LINEAR and the child commit renames the colorspace back.

There are also a couple of left-overs EImageColorSpace::SRGB_LINEAR changes which I think should be fixed. It's interesting why they don't cause compile errors, maybe they are in code that's disabled..

@p12tic p12tic mentioned this pull request Oct 7, 2022
Merged
@demoulinv demoulinv force-pushed the dev/AVColorSpaceMetadata branch from 4033e60 to 4966c97 Compare October 10, 2022 13:38
@demoulinv demoulinv mentioned this pull request Oct 10, 2022
Merged
@demoulinv demoulinv force-pushed the dev/AVColorSpaceMetadata branch from 4966c97 to 3e995ff Compare October 10, 2022 16:06
@demoulinv demoulinv force-pushed the dev/AVColorSpaceMetadata branch from 3e995ff to 8410f55 Compare October 12, 2022 14:48
@fabiencastan fabiencastan force-pushed the dev/AVColorSpaceMetadata branch from b1abb06 to 5347687 Compare October 17, 2022 14:48
@fabiencastan fabiencastan force-pushed the dev/AVColorSpaceMetadata branch from 5347687 to c5bfaf5 Compare October 17, 2022 17:44
demoulinv and others added 11 commits October 17, 2022 19:45
@demoulinv @fabiencastan
Add AliceVision Color Space metadata
8b81b63 
Add AliceVision:ColorSpace metadata when writing image.
Manage AliceVision:ColorSpace metadata when reading image.

* Renaming

Complete renaming from outputColorSpace to workingColorSpace when using read image.

* OIIO colorConfig object definition and initialization.

OIIO colorConfig object is defined within io.cpp only. It is initialized with the default OCIO color config file that must be located in the directory src/aliceVision/image/share/aliceVision.
A function declared in io.hpp is available to set another OCIO config file if needed.

Search of OCIO config file strategy:
First try with the ALICEVISION_OCIO env variable.
If unsuccessful, second try with the standardized OCIO env variable.
If unsuccessful, third try with ALICEVISION_ROOT env variable to extract the embedded config file.

* image io

Add log trace when OCIO config file found.
Update OCIO config file by removing the possibility to call the OCIO V1 method to identify the color space from the file name. Default color space is still sRGB.
When reading image, avoid searching input image color space if no conversion is needed.
If conversion is needed, check color space validity after getting it from image metadate or image file name.
Move config.ocio file in subdirectory to ease testing.
Set ALICEVISION_ROOT environment variable in some tests related to image.

* Workflow

Add ALICEVISION_ROOT environment variable in continuous integration.

* Working Color Space implementation in imageProcessing

Transmit working color space to write image and convert to output color space if needed.
Working color space is the color space in which the processing is done. Input images are converted in this color space by the readImage method.
@demoulinv @fabiencastan
[software] Update some panorama nodes to enable computation in ACEScg.
0040210
@demoulinv @fabiencastan
Update ocio config file.
77d8435
@fabiencastan
[software] ImageProcessing: add option to keep image filename
6ebaeeb
@fabiencastan
[image] io: Update color space metadata extraction from read image.
c348728 
First, search AliceVision:ColorSpace metadata in oiio read buf.
If non-existent or invalid search oiio:ColorSpace metadata in read buf.
If still non-existent or invalid use filename to guess the color space by using the oiio::readmetadata function and then the OCIO configuration file if any.
@demoulinv @fabiencastan
[cmake] Update test environment: add env var ALICEVISION_ROOT definit…
81dba04 
…ion for Windows in the alicevision_add_test cmake function.
@fabiencastan
[ci] Windows fix: add CMAKE_INSTALL_PREFIX
8007486
@demoulinv @fabiencastan
[ci] Add ALICEVISION_ROOT definition in ci/env.sh
1ffd64a
@demoulinv @fabiencastan
[ci] Add Argument CMAKE_INSTALL_PREFIX in the UnitTests step of the c…
247c1f9 
…ontinuous integration github yml file.

CMAKE_INSTALL_PREFIX definition is used by the Cmake function "alicevision_add_test" defined in Helpers.cmake to set the env var ALICEVISION_ROOT for the unitary target target RUN_TESTS.
@fabiencastan
[image] use disabled code instead of commented code
b62aee3
@fabiencastan
[image] add log for errors
c5aadf3
@fabiencastan fabiencastan force-pushed the dev/AVColorSpaceMetadata branch from c5bfaf5 to 57c5578 Compare October 17, 2022 19:44
@fabiencastan fabiencastan merged commit d64eee5 into develop Oct 19, 2022
@fabiencastan fabiencastan deleted the dev/AVColorSpaceMetadata branch October 19, 2022 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@p12tic p12tic p12tic left review comments

@fabiencastan fabiencastan fabiencastan approved these changes

@servantftechnicolor servantftechnicolor Awaiting requested review from servantftechnicolor

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@demoulinv @fabiencastan @p12tic