Page MenuHome

Fix T77564: VSE (and compositor background) lost stereoscopy preview
ClosedPublic

Authored by Clément Foucault (fclem) on Aug 5 2020, 3:25 PM.

Details

Summary

Issue introduced on fe045b2b77dc6d7f0b552619fe824b496d34db6c.

Since the stereoscopy compositing (anaglyph, ...) is only done for
viewports the VSE preview and compositor need to use viewports.

However, we need a different setting in GPUViewport to differenciate between
doing color management, and whether to use display colorspace.

Diff Detail

Repository
rB Blender

Event Timeline

Dalai Felinto (dfelinto) requested review of this revision.Aug 5 2020, 3:25 PM
source/blender/editors/space_node/node_draw.c
1766

I think this is unecessary. If you want to draw something in display space, just draw to the overlay framebuffer. Also check wether or not you want sRGB blending enabled (false for all the node drawing) and use glEnable/Disable(GL_FRAMEBUFFER_SRGB); couple around the area you want to disable it (see draw_image_buffer).

source/blender/gpu/intern/gpu_viewport.c
1052 ↗(On Diff #27437)

Theses clear operations are costly and should not be done "implicitly". Prefer using a getter to get the main framebuffer and the overlay framebuffer. This way you remove the bad level calls.

source/blender/gpu/intern/gpu_viewport.c
119 ↗(On Diff #27437)

display_colorspace is ambiguous and doesn't feel it fully explains what it actually means.

From the discussions here at the studio it seems that it's something what denotes whether input buffers you're drawing are in scene linear space (which needs to converted to the display space) or whether they are in display space already (which do not need extra transform to the display space).

So in a way it is something more like is_input_colormanaged.

But to make things even more interesting, you can also thing that linear buffers are to be colormanaged by the GPU
viewport (do_color_management=true) and the buffers which are already in the display space are to be not (do_color_management=false).

The relation between do_color_management and is_input_colormanaged is not really clear to me at this time. Need more input before giving any further suggestions about clear API.

Updating the patch

The cleanups were committed separately. Also simplified the patch a bit.

I still need to address the naming issue brought up by @Sergey Sharybin (sergey)

Dalai Felinto (dfelinto) marked 2 inline comments as done.Aug 6 2020, 4:53 PM
Clément Foucault (fclem) requested changes to this revision.Aug 6 2020, 11:05 PM

Like I said, you don't need this colorspace trick. Just render everything on the overlay framebuffer.

source/blender/editors/space_node/node_draw.c
1765

Also don't separate the clear color from the clear command.

1777

The above comment is still not done. You shouldn't bind framebuffer_default Also disable GL_FRAMEBUFFER after that for correct blending.

source/blender/gpu/intern/gpu_viewport.c
555 ↗(On Diff #27483)

And remove this.

This revision now requires changes to proceed.Aug 6 2020, 11:05 PM

Small update, not yet the changes Clément suggests

Changes suggested by review, however now the bugs persist

The previous update is the last one where the bug was fixed. Updating the patch still
to simplify the conversation on top of this.

@Clément Foucault (fclem) there is clearly something I'm not understanding from what you are suggesting. I did what you suggested but now both editors have their color fundamentally very wrong.

@Clément Foucault (fclem) this is the sample file I'm using:

(open and click on Compositor "Backdrop" twice to have it all working).

This is the image:
https://github.com/AcademySoftwareFoundation/openexr-images/blob/master/MultiView/Balls.exr

Remember to check the greasepencil as well.

Dalai Felinto (dfelinto) requested changes to this revision.Aug 12 2020, 11:13 AM

@Clément Foucault (fclem) how did you test this? Because it gives me wrong color with the sample file I attached in this conversation:

The compositor works fine though, great work.

This revision now requires changes to proceed.Aug 12 2020, 11:13 AM

@Clément Foucault (fclem) how did you test this? Because it gives me wrong color with the sample file I attached in this conversation:

The compositor works fine though, great work.

I was not able to test the sequencer view (I don't know how to test? putting the exr where it should did not fixed it). Other than that everything was fine.

You are right, for the sequencer I used the jpg version of the image:

Just copy both images to //multiview/ and re-open the test file.

Fix the sequencer preview and encapsulate the changes into a GPUFrambuffer function.

Fix missing header declaration

@Clément Foucault (fclem) can you share with me the file you are using for testing? Because here the last one not only didn't fix the sequencer, but it broke the compositor again:

For the records, the image editor has the ground-truth image.

Rebase on master & fix broken patch

Note: Building without OCIO has broken EXR display but that's not caused by this patch. The wrong color management comes from elsewhere.

This revision is now accepted and ready to land.Aug 18 2020, 3:02 PM