From c857c9d4f57e3003ac782bdb78496952f617cb62 Mon Sep 17 00:00:00 2001 From: Brecht Van Lommel Date: Mon, 22 Sep 2025 19:20:42 +0200 Subject: [PATCH] Refactor: Move video CICP logic to be reusable for images Ref #145612 Pull Request: https://projects.blender.org/blender/blender/pulls/146606 --- source/blender/imbuf/IMB_colormanagement.hh | 11 +- .../blender/imbuf/intern/colormanagement.cc | 152 +++++++++++++++++- source/blender/imbuf/intern/format_jpeg.cc | 2 +- source/blender/imbuf/intern/format_webp.cc | 2 +- .../imbuf/intern/oiio/openimageio_support.cc | 2 +- .../blender/imbuf/movie/intern/movie_read.cc | 51 +----- .../blender/imbuf/movie/intern/movie_write.cc | 84 +++------- 7 files changed, 188 insertions(+), 116 deletions(-) diff --git a/source/blender/imbuf/IMB_colormanagement.hh b/source/blender/imbuf/IMB_colormanagement.hh index 265eeed7905..605720885b6 100644 --- a/source/blender/imbuf/IMB_colormanagement.hh +++ b/source/blender/imbuf/IMB_colormanagement.hh @@ -71,8 +71,15 @@ bool IMB_colormanagement_space_name_is_data(const char *name); bool IMB_colormanagement_space_name_is_scene_linear(const char *name); bool IMB_colormanagement_space_name_is_srgb(const char *name); -/* Get binary ICC profile contents for a colorspace. */ -blender::Vector IMB_colormanagement_space_icc_profile(const ColorSpace *colorspace); +/* Get binary ICC profile contents for a colorspace. + * For describing the colorspace for standard dynamic range image files. */ +blender::Vector IMB_colormanagement_space_to_icc_profile(const ColorSpace *colorspace); +/* Get CICP code for colorspace. + * For describing the colorspace of videos and high dynamic range image files. */ +bool IMB_colormanagement_space_to_cicp(const ColorSpace *colorspace, + const bool video, + int cicp[4]); +const ColorSpace *IMB_colormanagement_space_from_cicp(const int cicp[4], const bool video); /* Get identifier for colorspaces that works with multiple OpenColorIO configurations, * as defined by the ASWF Color Interop Forum. */ diff --git a/source/blender/imbuf/intern/colormanagement.cc b/source/blender/imbuf/intern/colormanagement.cc index e2d4a22241b..5bbe72e9795 100644 --- a/source/blender/imbuf/intern/colormanagement.cc +++ b/source/blender/imbuf/intern/colormanagement.cc @@ -1373,7 +1373,7 @@ const char *IMB_colormanagement_srgb_colorspace_name_get() return global_role_default_byte; } -blender::Vector IMB_colormanagement_space_icc_profile(const ColorSpace *colorspace) +blender::Vector IMB_colormanagement_space_to_icc_profile(const ColorSpace *colorspace) { /* ICC profiles shipped with Blender are named after the OpenColorIO interop ID. */ blender::Vector icc_profile; @@ -1425,6 +1425,156 @@ blender::Vector IMB_colormanagement_space_icc_profile(const ColorSpace *co return icc_profile; } +/* Primaries */ +static const int CICP_PRI_REC709 = 1; +static const int CICP_PRI_REC2020 = 9; +static const int CICP_PRI_P3D65 = 12; +/* Transfer functions */ +static const int CICP_TRC_BT709 = 1; +static const int CICP_TRC_G22 = 4; +static const int CICP_TRC_SRGB = 13; +static const int CICP_TRC_PQ = 16; +static const int CICP_TRC_G26 = 17; +static const int CICP_TRC_HLG = 18; +/* Matrix */ +static const int CICP_MATRIX_RGB = 0; +static const int CICP_MATRIX_BT709 = 1; +static const int CICP_MATRIX_REC2020_NCL = 9; +/* Range */ +static const int CICP_RANGE_FULL = 1; + +bool IMB_colormanagement_space_to_cicp(const ColorSpace *colorspace, const bool video, int cicp[4]) +{ + const StringRefNull interop_id = colorspace->interop_id(); + if (interop_id.is_empty()) { + return false; + } + + /* References: + * ASWF Color Interop Forum defined display spaces. + * https://en.wikipedia.org/wiki/Coding-independent_code_points + * https://www.w3.org/TR/png-3/#cICP-chunk + * + * For images we always use RGB matrix as that is the only thing supported for PNG. + * For video we specify an appropriate matrix to YUV or similar. This should also + * be used for HEIF and AVIF which are based on video codecs. */ + + if (interop_id == "pq_rec2020_display") { + cicp[0] = CICP_PRI_REC2020; + cicp[1] = CICP_TRC_PQ; + cicp[2] = (video) ? CICP_MATRIX_REC2020_NCL : CICP_MATRIX_RGB; + cicp[3] = CICP_RANGE_FULL; + return true; + } + if (interop_id == "hlg_rec2020_display") { + cicp[0] = CICP_PRI_REC2020; + cicp[1] = CICP_TRC_HLG; + cicp[2] = (video) ? CICP_MATRIX_REC2020_NCL : CICP_MATRIX_RGB; + cicp[3] = CICP_RANGE_FULL; + return true; + } + if (interop_id == "pq_p3d65_display") { + /* Rec.2020 matrix may seem odd, but follows Color Interop Forum recommendation. */ + cicp[0] = CICP_PRI_P3D65; + cicp[1] = CICP_TRC_PQ; + cicp[2] = (video) ? CICP_MATRIX_REC2020_NCL : CICP_MATRIX_RGB; + cicp[3] = CICP_RANGE_FULL; + return true; + } + if (interop_id == "g26_p3d65_display") { + /* BT.709 matrix may seem odd, but follows Color Interop Forum recommendation. */ + cicp[0] = CICP_PRI_P3D65; + cicp[1] = CICP_TRC_G26; + cicp[2] = (video) ? CICP_MATRIX_BT709 : CICP_MATRIX_RGB; + cicp[3] = CICP_RANGE_FULL; + return true; + } + if (interop_id == "g22_rec709_display") { + cicp[0] = CICP_PRI_REC709; + cicp[1] = CICP_TRC_G22; + cicp[2] = (video) ? CICP_MATRIX_BT709 : CICP_MATRIX_RGB; + cicp[3] = CICP_RANGE_FULL; + return true; + } + if (interop_id == "g24_rec2020_display") { + /* There is no gamma 2.4 trc, but BT.709 is close. */ + cicp[0] = CICP_PRI_REC2020; + cicp[1] = CICP_TRC_BT709; + cicp[2] = (video) ? CICP_MATRIX_REC2020_NCL : CICP_MATRIX_RGB; + cicp[3] = CICP_RANGE_FULL; + return true; + } + if (interop_id == "g24_rec709_display") { + /* There is no gamma 2.4 trc, but BT.709 is close. */ + cicp[0] = CICP_PRI_REC709; + cicp[1] = CICP_TRC_BT709; + cicp[2] = (video) ? CICP_MATRIX_BT709 : CICP_MATRIX_RGB; + cicp[3] = CICP_RANGE_FULL; + return true; + } + if (interop_id == "srgb_p3d65_display" || interop_id == "srgbx_p3d65_display") { + /* For video we use BT.709 to match default sRGB writing, even though it is wrong. + * But we have been writing sRGB like this forever, and there is the so called + * "Quicktime gamma shift bug" that complicates things. */ + cicp[0] = CICP_PRI_P3D65; + cicp[1] = (video) ? CICP_TRC_BT709 : CICP_TRC_SRGB; + cicp[2] = (video) ? CICP_MATRIX_BT709 : CICP_MATRIX_RGB; + cicp[3] = CICP_RANGE_FULL; + return true; + } + if (interop_id == "srgb_rec709_display") { + /* Don't write anything for backwards compatibility. Is fine for PNG + * and video but may reconsider when JXL or AVIF get added. */ + return false; + } + + return false; +} + +const ColorSpace *IMB_colormanagement_space_from_cicp(const int cicp[4], const bool video) +{ + StringRefNull interop_id; + + /* We don't care about matrix or range, we assume decoding handles that and we get + * full range RGB values out. */ + if (cicp[0] == CICP_PRI_REC2020 && cicp[1] == CICP_TRC_PQ) { + interop_id = "pq_rec2020_display"; + } + else if (cicp[0] == CICP_PRI_REC2020 && cicp[1] == CICP_TRC_HLG) { + interop_id = "hlg_rec2020_display"; + } + else if (cicp[0] == CICP_PRI_P3D65 && cicp[1] == CICP_TRC_PQ) { + interop_id = "pq_p3d65_display"; + } + else if (cicp[0] == CICP_PRI_P3D65 && cicp[1] == CICP_TRC_G26) { + interop_id = "g26_p3d65_display"; + } + else if (cicp[0] == CICP_PRI_REC709 && cicp[1] == CICP_TRC_G22) { + interop_id = "g22_rec709_display"; + } + else if (cicp[0] == CICP_PRI_REC2020 && cicp[1] == CICP_TRC_BT709) { + interop_id = "g24_rec2020_display"; + } + else if (cicp[0] == CICP_PRI_REC709 && cicp[1] == CICP_TRC_BT709) { + if (video) { + /* Arguably this should be g24_rec709_display, but we write sRGB like this. + * So there is an exception for now. */ + interop_id = "srgb_rec709_display"; + } + else { + interop_id = "g24_rec709_display"; + } + } + else if (cicp[0] == CICP_PRI_P3D65 && (cicp[1] == CICP_TRC_SRGB || cicp[1] == CICP_TRC_BT709)) { + interop_id = "srgb_p3d65_display"; + } + else if (cicp[0] == CICP_PRI_REC709 && cicp[1] == CICP_TRC_SRGB) { + interop_id = "srgb_rec709_display"; + } + + return (interop_id.is_empty()) ? nullptr : g_config->get_color_space_by_interop_id(interop_id); +} + StringRefNull IMB_colormanagement_space_get_interop_id(const ColorSpace *colorspace) { return colorspace->interop_id(); diff --git a/source/blender/imbuf/intern/format_jpeg.cc b/source/blender/imbuf/intern/format_jpeg.cc index 7299e2e7689..3a29f52aebc 100644 --- a/source/blender/imbuf/intern/format_jpeg.cc +++ b/source/blender/imbuf/intern/format_jpeg.cc @@ -624,7 +624,7 @@ static void write_jpeg(jpeg_compress_struct *cinfo, ImBuf *ibuf) /* Write ICC profile if there is one associated with the colorspace. */ const ColorSpace *colorspace = ibuf->byte_buffer.colorspace; if (colorspace) { - blender::Vector icc_profile = IMB_colormanagement_space_icc_profile(colorspace); + blender::Vector icc_profile = IMB_colormanagement_space_to_icc_profile(colorspace); if (!icc_profile.is_empty()) { icc_profile.prepend({'I', 'C', 'C', '_', 'P', 'R', 'O', 'F', 'I', 'L', 'E', 0, 0, 1}); jpeg_write_marker(cinfo, diff --git a/source/blender/imbuf/intern/format_webp.cc b/source/blender/imbuf/intern/format_webp.cc index 33ba64d8224..880fb4f105b 100644 --- a/source/blender/imbuf/intern/format_webp.cc +++ b/source/blender/imbuf/intern/format_webp.cc @@ -224,7 +224,7 @@ bool imb_savewebp(ImBuf *ibuf, const char *filepath, int /*flags*/) /* Write ICC profile if there is one associated with the colorspace. */ const ColorSpace *colorspace = ibuf->byte_buffer.colorspace; if (colorspace) { - blender::Vector icc_profile = IMB_colormanagement_space_icc_profile(colorspace); + blender::Vector icc_profile = IMB_colormanagement_space_to_icc_profile(colorspace); if (!icc_profile.is_empty()) { WebPData icc_chunk = {reinterpret_cast(icc_profile.data()), size_t(icc_profile.size())}; diff --git a/source/blender/imbuf/intern/oiio/openimageio_support.cc b/source/blender/imbuf/intern/oiio/openimageio_support.cc index b2a1144b756..d1eb815399e 100644 --- a/source/blender/imbuf/intern/oiio/openimageio_support.cc +++ b/source/blender/imbuf/intern/oiio/openimageio_support.cc @@ -457,7 +457,7 @@ ImageSpec imb_create_write_spec(const WriteContext &ctx, int file_channels, Type ctx.ibuf->float_buffer.colorspace : ctx.ibuf->byte_buffer.colorspace; if (colorspace) { - Vector icc_profile = IMB_colormanagement_space_icc_profile(colorspace); + Vector icc_profile = IMB_colormanagement_space_to_icc_profile(colorspace); if (!icc_profile.is_empty()) { file_spec.attribute("ICCProfile", OIIO::TypeDesc(OIIO::TypeDesc::UINT8, icc_profile.size()), diff --git a/source/blender/imbuf/movie/intern/movie_read.cc b/source/blender/imbuf/movie/intern/movie_read.cc index 5e13590d561..1e8488dbdc3 100644 --- a/source/blender/imbuf/movie/intern/movie_read.cc +++ b/source/blender/imbuf/movie/intern/movie_read.cc @@ -121,51 +121,14 @@ static void probe_video_colorspace(MovieReader *anim, char r_colorspace_name[IM_ } #ifdef WITH_FFMPEG - const AVColorTransferCharacteristic color_trc = anim->pCodecCtx->color_trc; - const AVColorPrimaries color_primaries = anim->pCodecCtx->color_primaries; + /* Note that the ffmpeg enums are documented to match CICP codes. */ + const int cicp[4] = {anim->pCodecCtx->color_primaries, + anim->pCodecCtx->color_trc, + anim->pCodecCtx->colorspace, + anim->pCodecCtx->color_range}; + const bool for_video = true; + const ColorSpace *colorspace = IMB_colormanagement_space_from_cicp(cicp, for_video); - /* ASWF Color Interop Forum defined display spaces. The CICP codes there match the enum - * values defined by ffmpeg. Keep in sync with movie_write.cc. - * - * Note that pCodecCtx->color_space is ignored because it is only about choice of YUV - * encoding for best compression, and ffmpeg will decode to RGB for us. */ - blender::StringRefNull interop_id; - - if (color_primaries == AVCOL_PRI_BT2020 && color_trc == AVCOL_TRC_SMPTEST2084) { - interop_id = "pq_rec2020_display"; - } - else if (color_primaries == AVCOL_PRI_BT2020 && color_trc == AVCOL_TRC_ARIB_STD_B67) { - interop_id = "hlg_rec2020_display"; - } - else if ((color_primaries == AVCOL_PRI_SMPTE432 && color_trc == AVCOL_TRC_IEC61966_2_1) || - (color_primaries == AVCOL_PRI_SMPTE432 && color_trc == AVCOL_TRC_BT709)) - { - interop_id = "srgb_p3d65_display"; - } - else if (color_primaries == AVCOL_PRI_SMPTE432 && color_trc == AVCOL_TRC_SMPTEST2084) { - interop_id = "pq_p3d65_display"; - } - else if (color_primaries == AVCOL_PRI_SMPTE432 && color_trc == AVCOL_TRC_SMPTE428) { - interop_id = "g26_p3d65_display"; - } - else if (color_primaries == AVCOL_PRI_BT709 && color_trc == AVCOL_TRC_GAMMA22) { - interop_id = "g22_rec709_display"; - } - else if (color_primaries == AVCOL_PRI_BT2020 && color_trc == AVCOL_TRC_BT709) { - interop_id = "g24_rec2020_display"; - } - else if (color_primaries == AVCOL_PRI_BT709 && color_trc == AVCOL_TRC_IEC61966_2_1) { - interop_id = "srgb_rec709_display"; - } - else if (color_primaries == AVCOL_PRI_BT709 && color_trc == AVCOL_TRC_BT709) { - /* Arguably this should be g24_rec709_display, but we write sRGB like this. */ - interop_id = "srgb_rec709_display"; - } - - if (interop_id.is_empty()) { - return; - } - const ColorSpace *colorspace = IMB_colormanagement_space_from_interop_id(interop_id); if (colorspace == nullptr) { return; } diff --git a/source/blender/imbuf/movie/intern/movie_write.cc b/source/blender/imbuf/movie/intern/movie_write.cc index 36232cebf97..eb513c37478 100644 --- a/source/blender/imbuf/movie/intern/movie_write.cc +++ b/source/blender/imbuf/movie/intern/movie_write.cc @@ -9,6 +9,8 @@ #include "movie_write.hh" +#include "BLI_string_ref.hh" + #include "DNA_scene_types.h" #include "MOV_write.hh" @@ -788,79 +790,33 @@ static void set_quality_rate_options(const MovieWriter *context, } } -static void set_colorspace_options(AVCodecContext *c, blender::StringRefNull interop_id) +static void set_colorspace_options(AVCodecContext *c, const ColorSpace *colorspace) { const AVPixFmtDescriptor *pix_fmt_desc = av_pix_fmt_desc_get(c->pix_fmt); const bool is_rgb_format = (pix_fmt_desc->flags & AV_PIX_FMT_FLAG_RGB); + const bool for_video = true; - /* Full range for most color spaces. */ - c->color_range = AVCOL_RANGE_JPEG; - - /* ASWF Color Interop Forum defined display spaces. The CICP codes there match the enum - * values defined by ffmpeg. Keep in sync with movie_read.cc. */ - if (interop_id == "pq_rec2020_display") { - c->color_primaries = AVCOL_PRI_BT2020; - c->color_trc = AVCOL_TRC_SMPTEST2084; - c->colorspace = AVCOL_SPC_BT2020_NCL; + int cicp[4]; + if (IMB_colormanagement_space_to_cicp(colorspace, for_video, cicp)) { + /* Note ffmpeg enums are documented to match CICP. */ + c->color_primaries = AVColorPrimaries(cicp[0]); + c->color_trc = AVColorTransferCharacteristic(cicp[1]); + c->colorspace = (is_rgb_format) ? AVCOL_SPC_RGB : AVColorSpace(cicp[2]); + c->color_range = AVCOL_RANGE_JPEG; } - else if (interop_id == "hlg_rec2020_display") { - c->color_primaries = AVCOL_PRI_BT2020; - c->color_trc = AVCOL_TRC_ARIB_STD_B67; - c->colorspace = AVCOL_SPC_BT2020_NCL; - } - else if (interop_id == "pq_p3d65_display") { - c->color_primaries = AVCOL_PRI_SMPTE432; - c->color_trc = AVCOL_TRC_SMPTEST2084; - c->colorspace = AVCOL_SPC_BT2020_NCL; - } - else if (interop_id == "g26_p3d65_display") { - c->color_primaries = AVCOL_PRI_SMPTE432; - c->color_trc = AVCOL_TRC_SMPTE428; - c->colorspace = AVCOL_SPC_BT709; - } - else if (interop_id == "g22_rec709_display") { - c->color_primaries = AVCOL_PRI_BT709; - c->color_trc = AVCOL_TRC_GAMMA22; - c->colorspace = AVCOL_SPC_BT709; - } - else if (interop_id == "g24_rec2020_display") { - /* There is no gamma 2.4 trc, but BT.709 is supposed to be close. But it's not - * clear this is right, as we use the same trc for sRGB which is clearly different. */ - c->color_primaries = AVCOL_PRI_BT2020; - c->color_trc = AVCOL_TRC_BT709; - c->colorspace = AVCOL_SPC_BT2020_NCL; - } - else if (interop_id == "g24_rec709_display") { - /* There is no gamma 2.4 trc, but BT.709 is supposed to be close. But now this - * is identical to how we write sRGB so at least of the two must be wrong? */ - c->color_primaries = AVCOL_PRI_BT709; - c->color_trc = AVCOL_TRC_BT709; - c->colorspace = AVCOL_SPC_BT709; - } - else if (ELEM(interop_id, "srgb_p3d65_display", "srgbx_p3d65_display")) { - c->color_primaries = AVCOL_PRI_SMPTE432; - /* This should be AVCOL_TRC_IEC61966_2_1, but Quicktime refuses to open the file. - * And we're currently also writing srgb_rec709_display the same way. */ - c->color_trc = AVCOL_TRC_BT709; - c->colorspace = AVCOL_SPC_BT709; - } - /* Don't write sRGB as we weren't doing it before either, but maybe we should. */ -# if 0 - else if (interop_id == "srgb_rec709_display") { - c->color_primaries = AVCOL_PRI_BT709; - c->color_trc = AVCOL_TRC_IEC61966_2_1; - c->colorspace = AVCOL_SPC_BT709; - } -# endif - /* If we're not writing RGB, we must write a colorspace to define how - * the conversion to YUV happens. */ else if (!is_rgb_format) { + /* Note BT.709 is wrong for sRGB. + * But we have been writing sRGB like this forever, and there is the so called + * "Quicktime gamma shift bug" that complicates things. */ c->color_primaries = AVCOL_PRI_BT709; c->color_trc = AVCOL_TRC_BT709; c->colorspace = AVCOL_SPC_BT709; /* TODO(sergey): Consider making the range an option to cover more use-cases. */ c->color_range = AVCOL_RANGE_MPEG; } + else { + /* We don't set anything for pure sRGB writing, for backwards compatibility. */ + } } static AVStream *alloc_video_stream(MovieWriter *context, @@ -1141,11 +1097,7 @@ static AVStream *alloc_video_stream(MovieWriter *context, /* Set colorspace based on display space of image. */ const ColorSpace *display_colorspace = IMB_colormangement_display_get_color_space( &imf->display_settings); - const blender::StringRefNull interop_id = (display_colorspace) ? - IMB_colormanagement_space_get_interop_id( - display_colorspace) : - ""; - set_colorspace_options(c, interop_id); + set_colorspace_options(c, display_colorspace); /* xasp & yasp got float lately... */