When another viewport is merged in the one that has started the rendering process, the region data gets freed which leads finally to a crash. This is a classical use-after-free bug which is caused by another thread. The patch suggests to make copies of the relevant data in case the rendering viewport gets collapsed by the user.
Details
Details
- Reviewers
Julian Eisel (Severin) - Group Reviewers
User Interface EEVEE & Viewport - Maniphest Tasks
- T82000: Viewport animation render crashes when collapsing the viewport.
Diff Detail
Diff Detail
- Repository
- rB Blender
- Branch
- Fix-T820000 (branched from master)
- Build Status
Buildable 10899 Build 10899: arc lint + arc unit
Event Timeline
Comment Actions
I am not sure if this is the best possible solution. I mean it's also kind of a design decision what should happen after another viewport is merged in the one, that triggered the animation rendering? Maybe my solution is okay?
Comment Actions
I think the better solution is to copy the relevant v3d and rv3d data rather than keep a pointer to it.
The fix is also not correct, region itself can be freed.
Comment Actions
- Revision of Bugfix
Created copies of ARegion and View3D, because this memory is getting freed if viewport is collapsed.
| source/blender/editors/render/render_opengl.c | ||
|---|---|---|
| 815 | There should be a deep copy so that all data owned by View3D is copied too. E.g. if the viewport is closed now and View3D.shading.prop of the render data is accessed, there would still be undefined behavior because the properties were owned by the viewport and were freed. So oglrender->prevsa->type->duplicate() should be called here (better add a new BKE_spacedata_copy() for that, see BKE_spacedata_copylist()). That will lead to a call to view3d_duplicate(). An issue is that view3d_duplicate() may override the shading type, which is not nice. You could just set the value from the copied viewport again after the duplication (not nice either, but acceptable imho). | |
Comment Actions
Julian, I have tried to incorporate your review to the best of my current knowledge. I hope that I was able to improve the patch.
Merci,
Janusch
| source/blender/blenkernel/intern/screen.c | ||
|---|---|---|
| 535–541 ↗ | (On Diff #30680) | Why is this needed? view3d_duplicate() already copies the properties, so I wouldn't expect this here. |
| source/blender/blenkernel/intern/screen.c | ||
|---|---|---|
| 535–541 ↗ | (On Diff #30680) | Do you mean line 539? "An issue is that view3d_duplicate() may override the shading type, which is not nice. You could just set the value from the copied viewport again after the duplication (not nice either, but acceptable imho)." Maybe I have an misunderstanding? But this is the problem you have linked to down below. This overwriting Problem seems to be still an issue. |
| source/blender/blenkernel/intern/image.c | ||
|---|---|---|
| 5366 ↗ | (On Diff #32402) | This has to be checked. |
| source/blender/blenkernel/intern/screen.c | ||
| 535–541 ↗ | (On Diff #30680) | Ehm yah, that was kinda stupid from me. It is corrected now. |