This was caused by a double lock of the DRW context mutex.
This changes the logic a bit by releasing the DRW context before rendering
with BKE_sequencer_give_ibuf and restoring it after.
Critical fix for 2.91
Details
Details
Diff Detail
Diff Detail
- Repository
- rB Blender
Event Timeline
| source/blender/editors/space_sequencer/sequencer_draw.c | ||
|---|---|---|
| 1295 | Can you elaborate on the logic here, and why you are not using context.view_id? | |
| source/blender/editors/space_sequencer/sequencer_draw.c | ||
|---|---|---|
| 1295 | Because this is what wm_draw_region_bind uses inside wm_draw_window_offscreen. And we need to follow that to ensure we bind the same viewport. | |
Comment Actions
This is weird to pass region to a function which generates imbuf. However, if it's for 2.90 probably is least reisky? But is there a better way to handle draw state related issue in the drawing code?
| source/blender/editors/space_sequencer/sequencer_draw.c | ||
|---|---|---|
| 1295 | Put this to the comment in the code, at a very least. | |
Comment Actions
I did make sure to test all callers to the function and they seems to be fine. So for me it's on the safe end.
Comment Actions
Is this still aimed for 2.90?
If it is, then, as Iv'e said before, this is safest way to move on.
If it's for 2.91/master then I am not fully convinced this is the right decision to make ImBuf rendering depend on a region, and some more detailed discussion is needed (as there are better approaches).
Comment Actions
Nope the orignal patch which introduce the issue was reverted. So 2.91
If it's for 2.91/master then I am not fully convinced this is the right decision to make ImBuf rendering depend on a region, and some more detailed discussion is needed (as there are better approaches).
So better do this save and restore outside of sequencer_ibuf_get ? I have no particular preference. This is just a sequencer function after all. Only used for drawing.
Comment Actions
Nope the orignal patch which introduce the issue was reverted. So 2.91
Please update the patch description then.
So better do this save and restore outside of sequencer_ibuf_get ? I have no particular preference. This is just a sequencer function after all. Only used for drawing.
Or not save/restore state at all. One thing we've discussed with @Brecht Van Lommel (brecht) is to have more explicit "prepare drawing data" pass, so that you can call sequencer_ibuf_get outside of the drawing code and "cache" it somewhere so that the drawing "simply" use it. Such preparation can as well happen in threads ;)
Comment Actions
@Clément Foucault (fclem) @Dalai Felinto (dfelinto), that is it what is blocking this review to happen?
Based on discussion between me, Brecht and Clement in the chat we do agree that at this point it is important to fix the crash, and work on a less sub-optimal solution later.