Regression: going in and out of full screen causes the compositor to re-calculate unnecessarily. #97575

New Issue

michael campbell · 2022-04-23T22:03:25+02:00

michael campbell commented

2022-04-23 22:03:25 +02:00

System Information
Operating system: Windows-10-10.0.19042-SP0 64 Bits
Graphics card: NVIDIA GeForce GTX 1070/PCIe/SSE2 NVIDIA Corporation 4.5.0 NVIDIA 472.12

Blender Version
Broken: version: 3.2.0 Alpha, branch: master, commit date: 2022-04-13 23:49, hash: 0ba061c3bc
Worked: 2.83.0
Worked: 2.82a

This is a regression caused by 23b5e10dbd

with the compositor workspace open drop in any image and then connect it to a denoise node or some other expensive node (lens distort set to one for example), connect it to a viewer and ensure backdrop is enabled.

Go to full screen by pressing ctrl space, it'll recalculate, then go back out of full screen by pressing ctrl space again, it'll recalculate again. I haven't tested, but virtually everything causes a frame change event so I wouldn't be surprised if this is the case here too.

**System Information** Operating system: Windows-10-10.0.19042-SP0 64 Bits Graphics card: NVIDIA GeForce GTX 1070/PCIe/SSE2 NVIDIA Corporation 4.5.0 NVIDIA 472.12 **Blender Version** Broken: version: 3.2.0 Alpha, branch: master, commit date: 2022-04-13 23:49, hash: `0ba061c3bc` Worked: 2.83.0 Worked: 2.82a This is a regression caused by 23b5e10dbd with the compositor workspace open drop in any image and then connect it to a denoise node or some other expensive node (lens distort set to one for example), connect it to a viewer and ensure backdrop is enabled. Go to full screen by pressing ctrl space, it'll recalculate, then go back out of full screen by pressing ctrl space again, it'll recalculate again. I haven't tested, but virtually everything causes a frame change event so I wouldn't be surprised if this is the case here too.

michael campbell commented

2022-04-23 22:03:25 +02:00

Added subscriber: @3di

Philipp Oeser commented

2022-04-25 12:51:31 +02:00

Added subscriber: @lichtwerk

Philipp Oeser commented

2022-04-25 12:51:31 +02:00

Changed status from 'Needs Triage' to: 'Confirmed'

Philipp Oeser commented

2022-04-25 12:51:31 +02:00

Can confirm, will check.

michael campbell commented

2022-04-25 13:42:25 +02:00

Thanks. Perhaps this also sheds some light on the other bug report you looked at regarding the persistent data being partially lost when going full screen. Perhaps render.render is being called each time full screen is entered/left with the composite node temporarily disabled to get the refresh?

Philipp Oeser commented

2022-04-25 14:04:52 +02:00

Added subscriber: @JulianEisel

Philipp Oeser commented

2022-04-25 14:04:52 +02:00

This is a regression caused by 23b5e10dbd

Not totally sure about design here, but a SpaceType refresh callback usually seems to be responsible for refreshing pretty much everything, so we might do a bit much here now.
For me, this raises following questions:

should screen editing really enforce such a prominent refresh?
is there another place where the VSE could ensure valid region sizes and remove handlers of invisible regions?

notes:

ED_screen_state_toggle does ED_area_tag_refresh
ED_area_do_refresh causes node_area_refresh

The regression is quite old and correctness is certainly more important than performance, but will still raise priority to get the eyes of developers looking at this...

CC @JulianEisel

This is a regression caused by 23b5e10dbd Not totally sure about design here, but a `SpaceType` `refresh` callback usually seems to be responsible for refreshing pretty much everything, so we *might* do a bit much here now. For me, this raises following questions: - should screen editing really enforce such a prominent refresh? - is there another place where the VSE could ensure valid region sizes and remove handlers of invisible regions? notes: - `ED_screen_state_toggle` does `ED_area_tag_refresh` - `ED_area_do_refresh` causes `node_area_refresh` The regression is quite old and correctness is certainly more important than performance, but will still raise priority to get the eyes of developers looking at this... CC @JulianEisel

Philipp Oeser changed title from ~~going in and out of full screen causes the compositor to re-calculate unnecessarily.~~ to Regression: going in and out of full screen causes the compositor to re-calculate unnecessarily.

2022-04-25 14:05:32 +02:00

Philipp Oeser commented

2022-04-25 14:09:31 +02:00

In #97575#1346211, @3di wrote:
Thanks. Perhaps this also sheds some light on the other bug report you looked at regarding the persistent data being partially lost when going full screen. Perhaps render.render is being called each time full screen is entered/left with the composite node temporarily disabled to get the refresh?

Would have to look at how renderengines react to this callback.
Does it (the issue from the other report, #96033 btw) behave better in 2.82a for you?

> In #97575#1346211, @3di wrote: > Thanks. Perhaps this also sheds some light on the other bug report you looked at regarding the persistent data being partially lost when going full screen. Perhaps render.render is being called each time full screen is entered/left with the composite node temporarily disabled to get the refresh? Would have to look at how renderengines react to this callback. Does it (the issue from the other report, #96033 btw) behave better in 2.82a for you?

Sergey Sharybin commented

2022-05-04 10:55:30 +02:00

Added subscribers: @Jeroen-Bakker, @Sergey

Sergey Sharybin commented

2022-05-04 10:55:30 +02:00

Doing space refresh on fullscreen changes sounds reasonable to me. I wouldn't know of another way of ensuring space is in the consistent state after the screen layout changed.

The actual problem to me seems that compositor uses area refresh and notifiers mechanisms to inform that re-compositing is needed. This is how it was initially implemented, and there is still comment about it: /* default now: refresh node is starting preview */.

The easiest solution which does not lead to a big refactor seems to be to set a special "need compositor job" flag from the notifier listener, and check for it from the area refresh: P2919

@Jeroen-Bakker, @JulianEisel, can you think of something better what we can do for 3.2?

Doing space refresh on fullscreen changes sounds reasonable to me. I wouldn't know of another way of ensuring space is in the consistent state after the screen layout changed. The actual problem to me seems that compositor uses area refresh and notifiers mechanisms to inform that re-compositing is needed. This is how it was initially implemented, and there is still comment about it: `/* default now: refresh node is starting preview */`. The easiest solution which does not lead to a big refactor seems to be to set a special "need compositor job" flag from the notifier listener, and check for it from the area refresh: [P2919](https://archive.blender.org/developer/P2919.txt) @Jeroen-Bakker, @JulianEisel, can you think of something better what we can do for 3.2?

Jeroen Bakker commented

2022-05-04 12:59:45 +02:00

Not really familiar with this mechanism and its reasoning

Julian Eisel commented

2022-05-04 13:13:50 +02:00

Doing space refresh on fullscreen changes sounds reasonable to me. I wouldn't know of another way of ensuring space is in the consistent state after the screen layout changed.

Exactly, the refresh shouldn't be an issue, the way the compositor uses it is.

Your proposed solution looks fine to me. Although naming could be a bit more descriptive, it's unclear what "job" it refers to (I guess this was just a quick patch though) :)

> Doing space refresh on fullscreen changes sounds reasonable to me. I wouldn't know of another way of ensuring space is in the consistent state after the screen layout changed. Exactly, the refresh shouldn't be an issue, the way the compositor uses it is. Your proposed solution looks fine to me. Although naming could be a bit more descriptive, it's unclear what "job" it refers to (I guess this was just a quick patch though) :)

Sergey Sharybin commented

2022-05-04 15:12:10 +02:00

Naming -- sure. It was really a quick PoC.
I'll do some improvements to it and submit as a more proper review.

Naming -- sure. It was really a quick PoC. I'll do some improvements to it and submit as a more proper review.

blender-admin commented

2022-05-05 09:33:10 +02:00

This issue was referenced by b891c72d2d

This issue was referenced by b891c72d2da6c0e04569baac32da23c8b736aacf

Sergey Sharybin commented

2022-05-05 15:40:05 +02:00

Changed status from 'Confirmed' to: 'Resolved'

Sergey Sharybin closed this issue

2022-05-05 15:40:05 +02:00

Sergey Sharybin self-assigned this 2022-05-05 15:40:05 +02:00

Sign in to join this conversation.

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Regression: going in and out of full screen causes the compositor to re-calculate unnecessarily. #97575