Paths

Table of Contentst

DRW: Color Management improvement
ClosedPublic
Actions

Authored by Clément Foucault (fclem) on Feb 1 2020, 12:33 AM.

Details

Reviewers

Brecht Van Lommel (brecht)
Jeroen Bakker (jbakker)
Sergey Sharybin (sergey)

Commits

rB804e90b42d72: DRW: Color Management improvement

Summary

As described in T72420 this patch rewrite a good portion of how the viewport pipeline is define.

The main keypoint is that:

Render engines (Workbench/EEVEE/GPencil/Cycles) always output/blend into a scene refered linear colorspace RGBA16F buffer.
The overlays are rendered separately in a SRGB8_A8 buffer that allows correct blending in linear space.
The overlays and the final render buffer are composited in display linear space, and during the blit to window step to avoid using another extra buffer.

The fact that Render outputs are all in scene refered linear space is very important for viewport compositing (gpencil, future realtime compositor...).

For that to work, we modified OCIO implementation to allow creating a glsl shader from two processor.
This glsl shader does the final blending step in the correct colorspace (display linear).

The current display encoded to display linear transform is only a placeholder exponent transform that does the job (tm) for now.

The OCIO GLSL backend is now faster when adjusting values that are not related to the LUT.

The Overlays code have been cleaned up to centralize all colors inside draw_common.c in order to batch convert the color to the correct display space.

The Gizmos and callbacks are rendered inside the same SRGB8_A8 buffer as the overlays but without GL_FRAMEBUFFER_SRGB enable to ensure

The UV editor renders everything to the SRGB8_A8 buffer without GL_FRAMEBUFFER_SRGB for now to avoid more code refactoring.

The Render API is modified for external engines to draw in scene linear space. So this should not cause any breakage of compatibility.

This patch is also a great step towards HDR display of the viewport.

Diff Detail

Repository

rB Blender

Branch

draw-colormanagement

Build Status

Buildable 6521
Build 6521: arc lint + arc unit

Event Timeline

Clément Foucault (fclem) created this revision.Feb 1 2020, 12:33 AM

Harbormaster completed remote builds in B6451: Diff 21311.Feb 1 2020, 12:34 AM

Nabil Adel (mywa880) awarded a token.Feb 1 2020, 5:26 AM

Aaron Carlisle (Blendify) awarded a token.Feb 1 2020, 6:05 AM

Just a quick review. Still want to check this in more detail.

intern/opencolorio/fallback_impl.cc
698–699	processor_display
intern/opencolorio/ocio_impl_glsl.cc
574	We should clean up the code and remove this method can be done in a another patch.
694–695	just remove the state parameter name
source/blender/draw/engines/eevee/eevee_renderpasses.c
244	We need to check how we are going to deal with these passes. I can imagine that this design doesn't allow it currently. I added it to reflect how cycles displayed the passes. The draw engine somehow needs to communicate this back to the draw manager. Perhaps with a callback?
source/blender/draw/engines/workbench/workbench_private.h
429–430	remove else-if clause
source/blender/draw/intern/draw_color_management.c
46	Question: isn't a blit enough? We should find a better name as this is the only CM function that actually doesn't do any CM.
source/blender/draw/intern/draw_common.c
208	We could use a CPU OCIO processor for this. This could be stored in the DRWContextState during initialization. This can even be reused as the Scene Ref Space can only be changed after restarting blender.
source/blender/draw/intern/draw_manager.c
276	This function should check with what the draw engine actually draws (eg Active renderpass)
source/blender/gpu/intern/gpu_viewport.c
94	The end goal would be that the UV/Image editor would be part of the draw manager, where it needs to be rendered in SRS.
source/blender/gpu/shaders/gpu_shader_image_overlays_merge_frag.glsl
2	Comment needs to be updated.
source/blender/imbuf/intern/colormanagement.c
916	Should we add this to ocio config

Clément Foucault (fclem) marked 4 inline comments as done.Feb 5 2020, 12:25 AM

Clément Foucault (fclem) added inline comments.

source/blender/draw/engines/eevee/eevee_renderpasses.c
244	The issue is, Grease pencil and maybe other engine can draw on top of it. So maybe it is simpler to build this color management exception inside the drawmanager, just like we do for lookdev and rendered view.
source/blender/draw/intern/draw_color_management.c
46	Yes but blitting requires setting up framebuffers. For simplicity sake I would leave this function as is for now. We can rename this function to `DRW_draw_texture_fullscreen` or something like that.
source/blender/draw/intern/draw_common.c
208	We definitely will do that. Although I would prefer to first cache these theme colors first as the transform can be slower with OCIO.
source/blender/gpu/intern/gpu_viewport.c
94	Yes that's what I also meant.
source/blender/imbuf/intern/colormanagement.c
916	The end goal would be to use an ocio role that contains the correct transform for the display. This is what the TODO is about.

Description sounds all good to me. But i am not sure what is the difference between processor and processor_display. Is it described anywhere?

Also, usual comments on style. Just an initial pass, very quickly.

intern/opencolorio/ocio_impl_glsl.cc
73	`TODO: Remove` or `TODO(fclem): Remove`
74	Could become simply: `struct OCIO_GLSLCurveMappingParameters { ... };` Same applies to other structures in this file.
208–210	`shader_desc`, `cache_id`.
211–283	Always use parenthesis.
312–313	Use `OBJECT_GUARDED_NEW`/`OBJECT_GUARDED_DELETE` on the `shader` itself. It will call appropriate constructor/destructor without you needing to worry about individual fields.
325	`const std::string& cacheId`

This revision now requires changes to proceed.Feb 6 2020, 9:42 AM

Cleanup: Update comments & fix some style issues
OCIO: Style fixes
Merge branch 'master' into draw-colormanagement

Harbormaster completed remote builds in B6521: Diff 21478.Feb 7 2020, 2:10 AM

@Sergey Sharybin (sergey) processor is the OCIO processor containing the Scene Refered Linear > Display Linear transform. The processor_display only contains the Display Linear > Display Encoded transform. For now we do this double transform but this might be avoided in some cases when we don't have any overlays.

This seems good to me (I didn't look at the draw/ module changes, assuming that's fine).

It's breaking compatibility a little bit in the sense that some render engine may not be using our GLSL display shader, but I think that's acceptable if we document it.

It would be nice if we could come up with a better term than "display linear". I think that would typically be interpreted as "color space that has a linear relation to the display space", which isn't the case here. Maybe "UI linear" or something like that.

intern/opencolorio/gpu_shader_display_transform.glsl
154–156	I don't think applying dither at this stage will work well? I can see the problem that you want to dither the render but not the overlays, but I'm not sure that's possible efficiently. You could transform this color to display space and back just for the dither, but that seems too inefficient.
source/blender/imbuf/intern/colormanagement.c
108–109	It could help to be a bit more verbose here, like using `processor_scene_to_ui` and `processor_ui_to_display`.

Address reviewers comments

Harbormaster completed remote builds in B6567: Diff 21587.Feb 11 2020, 1:49 AM

I think that would typically be interpreted as "color space that has a linear relation to the display space", which isn't the case here.

The Linear term here is meant to contrast with display encoded color space. The color space is linear w.r.t. the display chromaticity and radiometry (amount of light). Not with the display encoded space. I think it is confusing to think about display space only as the display encoded space.

intern/opencolorio/gpu_shader_display_transform.glsl
154–156	Yes this is a concern. Dithering should happen last on the pipeline just before the quantization step. But doing so would mean doing it after overlays have been merged. Doing it only on the render means having one more transform (just like you pointed out) and would be inefficient. I wouldn't mind if we dither the overlays to be honest but users could be confused.

In D6729#161033, @Clément Foucault (fclem) wrote:

I think that would typically be interpreted as "color space that has a linear relation to the display space", which isn't the case here.

The Linear term here is meant to contrast with display encoded color space. The color space is linear w.r.t. the display chromaticity and radiometry (amount of light). Not with the display encoded space. I think it is confusing to think about display space only as the display encoded space.

I understand what you mean, it's just different than existing terminology in Blender and OpenColorIO. They would be considered different color spaces. For me "color space" is about semantics and "encoding" is about compression.

Anyway, not a big deal either way as long as there is a good comment or docs explaining what these color spaces are.

Brecht Van Lommel (brecht) accepted this revision.Feb 11 2020, 12:55 PM

Think same naming suggestion from Brecht can be applied to C-API.
Other than that don't have much to comment about. Seems fine.

This revision is now accepted and ready to land.Feb 11 2020, 2:13 PM

Add comment about UI color space
Style: Rename processor arguments in C api and glsl impl.
Fix crash when selecting in edit mode with eevee

Harbormaster completed remote builds in B6577: Diff 21605.Feb 11 2020, 2:55 PM

Closed by commit rB804e90b42d72: DRW: Color Management improvement (authored by Clément Foucault (fclem)). · Explain WhyFeb 11 2020, 3:24 PM

This revision was automatically updated to reflect the committed changes.

Clément Foucault (fclem) added a commit: rB804e90b42d72: DRW: Color Management improvement.

Rémi Verschelde (akien) mentioned this in T77572: Reference image opacity value doesn't work with Blender 2.83+ (Intel HD 630 with Mesa 20, regression from D6729).Aug 11 2020, 12:33 PM

Aditia A. Pratama (aditiapratama) added a subscriber: Aditia A. Pratama (aditiapratama).Jan 20 2021, 6:54 AM

Revision Contents
Changeset List

Path

Size

intern/

opencolorio/

fallback_impl.cc

41 lines

gpu_shader_display_transform.glsl

183 lines

15 lines

42 lines

40 lines

45 lines

813 lines

source/

blender/

draw/

CMakeLists.txt

6 lines

DRW_engine.h

6 lines

engines/

basic/

basic_engine.c

1 line

eevee/

45 lines

80 lines

3 lines

58 lines

eevee_temporal_sampling.c

4 lines

external/

external_engine.c

7 lines

gpencil/

gpencil_engine.h

1 line

gpencil_engine.c

1 line

overlay/

overlay_antialiasing.c

6 lines

257 lines

112 lines

4 lines

16 lines

53 lines

60 lines

8 lines

overlay_motion_path.c

1 line

overlay_private.h

11 lines

overlay_shader.c

29 lines

shaders/

armature_envelope_solid_frag.glsl

2 lines

armature_shape_solid_vert.glsl

2 lines

armature_sphere_solid_frag.glsl

2 lines

background_frag.glsl

73 lines

clipbound_vert.glsl

13 lines

image_frag.glsl

9 lines

select/

select_engine.c

1 line

workbench/

shaders/

workbench_background_lib.glsl

workbench_data_lib.glsl

5 lines

workbench_deferred_background_frag.glsl

14 lines

workbench_forward_composite_frag.glsl

16 lines

6 lines

6 lines

60 lines

68 lines

workbench_effect_aa.c

13 lines

workbench_forward.c

39 lines

workbench_private.h

22 lines

workbench_render.c

2 lines

intern/

DRW_render.h

14 lines

draw_cache.c

28 lines

	draw_color_management.h
	draw_view.h

17 lines

draw_color_management.c

63 lines

28 lines

34 lines

469 lines

40 lines

2 lines

89 lines

shaders/

common_globals_lib.glsl

28 lines

editors/

screen/

glutil.c

4 lines

space_image/

space_image.c

7 lines

gpu/

2 lines

1 line

2 lines

2 lines

11 lines

intern/

11 lines

2 lines

7 lines

6 lines

3 lines

6 lines

335 lines

shaders/

gpu_shader_image_linear_frag.glsl

gpu_shader_image_overlays_merge_frag.glsl

42 lines

imbuf/

IMB_colormanagement.h

3 lines

intern/

colormanagement.c

70 lines

makesrna/

intern/

rna_render.c

15 lines

Commit	Tree	Parents	Author	Summary	Date
c7b319279e4e	eae4e517e456	863632e7e26d 807aefd6234f	Clément Foucault	Merge branch 'master' into draw-colormanagement (Show More…)	Feb 7 2020, 2:05 AM
863632e7e26d	f268b783cc08	61d66b528e75	Clément Foucault	OCIO: Style fixes	Feb 7 2020, 2:03 AM
61d66b528e75	ebb4bdf20b7f	c0675868a3fa	Clément Foucault	Cleanup: Update comments & fix some style issues	Feb 5 2020, 12:25 AM
c0675868a3fa	6569085d27f4	f9914a8ef991	Clément Foucault	Overlay: Fix edit text overlay invert logic op not working (Show More…)	Jan 31 2020, 11:58 PM
f9914a8ef991	6d4ab28065ba	fe327c0970ad	Clément Foucault	Overlay: Armature: Fix look regression because of linear space rendering (Show More…)	Jan 31 2020, 7:40 PM
fe327c0970ad	19bc0ffd4eaf	691c6f94bbbd	Clément Foucault	Overlay: Put all colors inside draw_common.c for correct colormanagment. (Show More…)	Jan 31 2020, 6:59 PM
691c6f94bbbd	a47c16b7b169	a7ac3a3ee3c9	Clément Foucault	Cleanup: Workbench: Remove background code (Show More…)	Jan 31 2020, 4:21 AM
a7ac3a3ee3c9	4f9574fe4f7e	711c4bba54b3	Clément Foucault	Overlay: Support viewport background modes (Show More…)	Jan 31 2020, 4:19 AM
711c4bba54b3	37db9f23420e	a3ee3b8f245e	Clément Foucault	Cleanup: DRW: Remove unused code and redundant state changes	Jan 31 2020, 4:18 AM
a3ee3b8f245e	68809cdfadb2	66e1ca26f0b9	Clément Foucault	Overlay: Move Alt+B clipping bounds display from workbench to overlay	Jan 29 2020, 4:02 AM
66e1ca26f0b9	b16b1714f23e	b180c8b84757	Clément Foucault	Cleanup: OCIO: Remove immVertexFormat in GLSL implementation (Show More…)	Jan 28 2020, 2:25 AM
b180c8b84757	8c66ab82c966	089b55885c2b	Clément Foucault	Render API: Make external engine render in linear space for the viewport (Show More…)	Jan 28 2020, 2:17 AM
089b55885c2b	8de085a0e6a3	5cab93d89f8a	Clément Foucault	OCIO: Use GroupTransform to avoid doing 3 transforms instead of 2 (Show More…)	Jan 28 2020, 12:23 AM
5cab93d89f8a	8a7ba8198749	8de055452d83	Clément Foucault	DRW: Remove drw_engines_draw_overlays routine (Show More…)	Jan 27 2020, 1:28 AM
8de055452d83	753f4141c6e2	da240defef27	Clément Foucault	Overlays: Fix wireframe shading not working	Jan 27 2020, 1:24 AM
da240defef27	3e8def56cb3d	bef1c0d11e05	Clément Foucault	Overlay: Don't always allocate the antialiasing buffer and draw background... (Show More…)	Jan 26 2020, 9:36 PM
bef1c0d11e05	4b97bf2f8c3e	be68e899488a	Clément Foucault	DRW: ColorManagement: Fix ghosting with cycles.	Jan 26 2020, 9:08 PM
be68e899488a	29606b67d336	5db185d1c485	Clément Foucault	DRW: ColorManagement: Fix viewport render pipeline (Show More…)	Jan 26 2020, 8:45 PM
5db185d1c485	693f9a2fdca3	2616a0a9d6eb	Clément Foucault	GPUViewport: Fix image uv editor.	Jan 26 2020, 5:33 PM
2616a0a9d6eb	c8c63daeb3ac	adf98e6360d9	Clément Foucault	GPUViewport: Add fallback colormanagment (Show More…)	Jan 26 2020, 5:32 PM
adf98e6360d9	bc5af843f7e1	419a818eec87	Clément Foucault	DRW: Move color management to the GPUViewport (Show More…)	Jan 26 2020, 5:09 PM
419a818eec87	dc70065a419e	281fb484408f	Clément Foucault	OCIO: Support overlay blending in linear colorspace in GLSL backend (Show More…)	Jan 26 2020, 5:03 PM
281fb484408f	ba66fbd21f79	e18f0a4cfb51	Clément Foucault	DRW: Color Management: Rework code to accept new color transform needed	Jan 25 2020, 1:11 AM
e18f0a4cfb51	a5d5c13284d5	e9411b8ccf27	Clément Foucault	Overlay: Add other background types (checker and gradient)	Jan 24 2020, 7:24 PM
e9411b8ccf27	e1949956467e	05dda64cf29a	Clément Foucault	Overlay: Cleanup Image background implementation (Show More…)	Jan 24 2020, 6:08 PM
05dda64cf29a	6dae7b3608c4	a25bb245e994	Clément Foucault	DRW: Color Management: Do some rearangement of the draw code (Show More…)	Jan 24 2020, 4:23 PM
a25bb245e994	b6b77504c6b6	34fd04f3b94a	Clément Foucault	Overlay: Fix background covering the outline	Jan 24 2020, 4:21 PM
34fd04f3b94a	d5e679dd70ba	7d25c7461522	Clément Foucault	OCIO: Fix curve cache preventing curvemapping update	Jan 23 2020, 1:57 AM
7d25c7461522	6facde8634ee	a6bfab516ebb	Clément Foucault	DRW: Remove DrawEngineType draw_background function (Show More…)	Jan 23 2020, 12:16 AM
a6bfab516ebb	d56cb2a6af53	af37c863ebf7	Clément Foucault	OCIO: Add back IMM hack (Show More…)	Jan 23 2020, 12:01 AM
af37c863ebf7	b027a98bd788	45dc1139e84b	Clément Foucault	Cleanup: EEVEE: Remove alpha checker drawing (Show More…)	Jan 22 2020, 7:34 PM
45dc1139e84b	2bfdabfafc71	a23f7a85ca8a	Clément Foucault	DRW: Move Color-Management handling to draw manager (Show More…)	Jan 22 2020, 7:34 PM
a23f7a85ca8a	5dbf9dc1d9f3	73f612fa3bd4	Clément Foucault	Cleanup: EEVEE: Move debug draw to eevee_renderpass.c	Jan 22 2020, 6:31 PM
73f612fa3bd4	cdbf93a592a6	986ff34d1ea4	Clément Foucault	GPU: Add sRGB framebuffer support (Show More…)	Jan 22 2020, 5:47 PM
986ff34d1ea4	21d74053be62	e00455381f55	Clément Foucault	GPUViewport: Cleanup and use always use RGBA16F	Jan 22 2020, 5:44 PM
e00455381f55	e38abee00af6	fa010ddddac8	Clément Foucault	Cleanup: OpenColorIO: Remove state tracking (Show More…)	Jan 22 2020, 5:05 PM
fa010ddddac8	9877732c4129	a0283355fcf4	Clément Foucault	DRW: Use Batch API instead of IMM for color managment (Show More…)	Jan 22 2020, 4:59 PM
a0283355fcf4	d5992ec1a78c	e769be1f340b	Clément Foucault	OpenColorIO: Improve/Cleanup Implementation (Show More…)	Jan 22 2020, 2:55 PM