@Richard Antalik (ISS) @Sergey Sharybin (sergey) @Francesco Siddi (fsiddi)

Confusing thing is: compositor operates in the linear space, while sequencer operates in (roughly) display space. So the same input color and same color correction settings are likely to lead to different results in the compositor and sequencer. Although, I think the ASC-CDL is defined for display space, which means sequencer implementation is the correct one?

Some quick round of code review, mainly cosmetic ones.
Would be nice to have input about interface from Francesco and some additional testing from Richard :)

In D12575#328239, @Sergey Sharybin (sergey) wrote:

Confusing thing is: compositor operates in the linear space, while sequencer operates in (roughly) display space. So the same input color and same color correction settings are likely to lead to different results in the compositor and sequencer. Although, I think the ASC-CDL is defined for display space, which means sequencer implementation is the correct one?

That's true, but I guess you can configure it like that by selecting the right color space for the sequencer? When I use the ACES OCIO config, the ACEScg color space was selected automatically, which is linear as far as I know. But I think for video editing ACEScc or ACEScct are usually preferred, which are not linear.

In D12575#328239, @Sergey Sharybin (sergey) wrote:

Confusing thing is: compositor operates in the linear space, while sequencer operates in (roughly) display space. So the same input color and same color correction settings are likely to lead to different results in the compositor and sequencer. Although, I think the ASC-CDL is defined for display space, which means sequencer implementation is the correct one?

I don't think that ASC-CDL standard define which color space it has to be edit on.

The problem is more in the compositor where the file ../compositor/operations/COM_ColorBalanceLGGOperation.cc force to use LGG in display space with the function linearrgb_to_srgb(). This is actually quite a bad idea because this could be done through the OCIO config file setting the compositing role with any colorspace the user want. Could be sRGB, or Linear or any.
ASC-CDL instead in the compositor is truly working in linear. Having LGG with linearrgb_to_srgb() is a real problem because it does not allow to swap LGG to CDL with the same result.

VSE color balance implementation seems actually better because it allows to use LGG in linear, display or log space even without editing the OCIO config file. I personally use the colormanagement of the Sequncer in ACEScg which is working perfectly fine.
The only thing that is quite weird is the lift neutral value that is at 1.0 instead of 0.0. This is why I made this change on my build: https://github.com/vincentgires/blender/commit/2ff93b5f9857eb94853e731d66acc2fb587981d0

Thanks for providing this. A couple of questions:

• Is it possible to improve the "color space" enum to make it take less space and make it sortable?
• Could you turn the "correction method" radio selector in the Color Balance modifier into an enum?

In D12575#329169, @Francesco Siddi (fsiddi) wrote:

• Is it possible to improve the "color space" enum to make it take less space and make it sortable?

That's a good point, but it would be a topic for another patch. The problem here is that ACES has a lot of things defined and they are shown all. It would be a good idea to only show color spaces in the color space selection and only show IDTs for the video strip source setting.

• Could you turn the "correction method" radio selector in the Color Balance modifier into an enum?

It actually is an enum, but it es expanded in the widget. I am not sure if there are UI guidelines for that, but I guess it would really be better to not expand that because it is selected once to define the behaviour all of the options below.

In D12575#329295, @Josef Raschen (JosefR) wrote:

In D12575#329169, @Francesco Siddi (fsiddi) wrote:

• Is it possible to improve the "color space" enum to make it take less space and make it sortable?

That's a good point, but it would be a topic for another patch. The problem here is that ACES has a lot of things defined and they are shown all. It would be a good idea to only show color spaces in the color space selection and only show IDTs for the video strip source setting.

I actually meant "searchable", but I understand now that the large list appears only if you have a specific OCIO config. So that's indeed something for later.

• Could you turn the "correction method" radio selector in the Color Balance modifier into an enum?

It actually is an enum, but it es expanded in the widget. I am not sure if there are UI guidelines for that, but I guess it would really be better to not expand that because it is selected once to define the behaviour all of the options below.

Showing something expanded is useful when a list is short and fixed and a change of value causes subsequent options to change. In this case, since the labels are pretty long, I suggest not to expand it. If changing those values becomes very repetitive, then you could look into making it radio again.

• Opening an existing .blend and switching to the color processing of your patch makes all of the colors black(and the preview black). In a new file the default values seems okay.
• Some docs will be needed for this function or at least copied into the vse docs.
• Comparing with the color node the Offset value is black, but in the modifier in the sidebar Offset is white. Is there supposed to be this difference?

In D12575#332939, @Peter Fog (tintwotin) wrote:

• Opening an existing .blend and switching to the color processing of your patch makes all of the colors black(and the preview black). In a new file the default values seems okay.

I think I am setting some wrong default for old blend files. I will try to find where these are set.

• Some docs will be needed for this function or at least copied into the vse docs.

True, I will prepare a change for the documentation.

• Comparing with the color node the Offset value is black, but in the modifier in the sidebar Offset is white. Is there supposed to be this difference?

I did that to allow applying negative offset values and having the slider in the middle position by default. Otherwise, you would have to select the “invert” button to get a negative offset.
Technically it is more correct to have black there by default because this is the rgb value that will be used for the offset. I am actually doing that by correcting the value from the widget:

if (cb.flag & SEQ_COLOR_BALANCE_INVERSE_OFFSET) {
  cb.offset[c] = -1.0f * (cb.offset[c] - 1.0f);
}
else {
  cb.offset[c] = cb.offset[c] - 1.0f;
}

For consistency with the compositor it might really be better to change that. But the compositor is using this “basis” field for offsetting the color value to achieve negative values as there is no “invert” option there. I am not sure which one is the best solution.

(https://devtalk.blender.org/t/add-offset-power-slope-controls-for-color-balance-modifier-video-editor/19989/25)

In D12575#333157, @Josef Raschen (JosefR) wrote:

In D12575#332939, @Peter Fog (tintwotin) wrote:

• Opening an existing .blend and switching to the color processing of your patch makes all of the colors black(and the preview black). In a new file the default values seems okay.

I think I am setting some wrong default for old blend files. I will try to find where these are set.

This is done in versioning, see file versioning_300.c. I checked only if this breaks existing files in any way which it doesn't, but it would be nice to initialize values if they are set to 0 and SEQ_COLOR_BALANCE_METHOD_LIFTGAMMAGAIN is used.

In D12575#333159, @Richard Antalik (ISS) wrote:

In D12575#333157, @Josef Raschen (JosefR) wrote:

In D12575#332939, @Peter Fog (tintwotin) wrote:

• Opening an existing .blend and switching to the color processing of your patch makes all of the colors black(and the preview black). In a new file the default values seems okay.

I think I am setting some wrong default for old blend files. I will try to find where these are set.

This is done in versioning, see file versioning_300.c. I checked only if this breaks existing files in any way which it doesn't, but it would be nice to initialize values if they are set to 0 and SEQ_COLOR_BALANCE_METHOD_LIFTGAMMAGAIN is used.

Thanks, I will prepare a fix for that next week.