Freestyle render removes texture image user #74417

New Issue

Gurpreet Singh · 2020-03-04T06:10:55+01:00

Gurpreet Singh commented

2020-03-04 06:10:55 +01:00

System Information
Operating system: Windows-10-10.0.18362-SP0 64 Bits
Graphics card: GeForce GTX 1660 Ti/PCIe/SSE2 NVIDIA Corporation 4.5.0 NVIDIA 441.41

Blender Version
Broken: version: 2.82 (sub 7), branch: master, commit date: 2020-02-12 16:20, hash: 77d23b0bd7
Worked: (optional)

Short description of error
Freestyle linestyle texture goes missing every time I open the file.

Exact steps for others to reproduce the error

PFA zip file. It has 2 files .blend and png for linestyle.
When you open the file you will notice that there's no texture called in image texture node.
Call the texture image in zip file.
Render and it should render freestyle lines.
Save and close the file.
Open the saved file again and you will notice texture goes missing in image texture node.

freestyle.zip

**System Information** Operating system: Windows-10-10.0.18362-SP0 64 Bits Graphics card: GeForce GTX 1660 Ti/PCIe/SSE2 NVIDIA Corporation 4.5.0 NVIDIA 441.41 **Blender Version** Broken: version: 2.82 (sub 7), branch: master, commit date: 2020-02-12 16:20, hash: `77d23b0bd7` Worked: (optional) **Short description of error** Freestyle linestyle texture goes missing every time I open the file. **Exact steps for others to reproduce the error** 1. PFA zip file. It has 2 files .blend and png for linestyle. 2. When you open the file you will notice that there's no texture called in image texture node. 3. Call the texture image in zip file. 4. Render and it should render freestyle lines. 5. Save and close the file. 6. Open the saved file again and you will notice texture goes missing in image texture node. [freestyle.zip](https://archive.blender.org/developer/F8389259/freestyle.zip)

Gurpreet Singh commented

2020-03-04 06:10:55 +01:00

Added subscriber: @Oye_LKY

Brecht Van Lommel commented

2020-03-04 08:27:16 +01:00

Added subscriber: @brecht

Brecht Van Lommel commented

2020-03-04 08:27:16 +01:00

Changed status from 'Needs Triage' to: 'Needs User Info'

Brecht Van Lommel commented

2020-03-04 08:27:16 +01:00

It seems no file got attached?

Ankit Meel changed title from ~~Freestyle texture Bug~~ to Freestyle texture not getting saved despite getting rendered

2020-03-04 14:40:31 +01:00

Gurpreet Singh commented

2020-03-05 09:49:50 +01:00

freestyle.zip

There you go. I think this should work.

[freestyle.zip](https://archive.blender.org/developer/F8389257/freestyle.zip) There you go. I think this should work.

Brecht Van Lommel commented

2020-03-05 11:32:17 +01:00

Changed status from 'Needs User Info' to: 'Confirmed'

Brecht Van Lommel changed title from ~~Freestyle texture not getting saved despite getting rendered~~ to Freestyle render removes texture image user

2020-03-05 11:32:32 +01:00

Sybren A. Stüvel self-assigned this 2020-03-17 17:55:20 +01:00

Sybren A. Stüvel commented

2020-03-17 18:41:35 +01:00

I think I found the culprit.

When rendering starts:

In BlenderStrokeRenderer::RenderStrokeRepBasic(…) there is a call to GetStrokeShader(…, do_id_user=false).
That function reduces the user count of the "stroke_shader" material to zero.
Because of do_id_user=false, the ntreeCopyTree_ex() does not increase the user count for the copied node tree.

When rendering stops:

All copied materials are looped over and freed.
Freeing the material recursively frees everything it uses.
This leaves the texture node's image with a user count that's one too little.

Now the image datablock has no users, and will not be saved. Rendering twice shows an error on the console about a negative user count.

The simplest fix is just to remove the freeing of all the materials. This works, and when Blender quits it doesn't complain of any memory leaks. I'll do some more tests with ASAN to check whether this is really the right way to go.

diff --git a/source/blender/freestyle/intern/blender_interface/BlenderStrokeRenderer.cpp b/source/blender/freestyle/intern/blender_interface/BlenderStrokeRenderer.cpp
index c88d5f24b5d..22718c29745 100644
--- a/source/blender/freestyle/intern/blender_interface/BlenderStrokeRenderer.cpp
+++ b/source/blender/freestyle/intern/blender_interface/BlenderStrokeRenderer.cpp
@@ -200,15 +200,6 @@ BlenderStrokeRenderer::~BlenderStrokeRenderer()
     }
   }
 
-  // release materials
-  Link *lnk = (Link *)freestyle_bmain->materials.first;
-
-  while (lnk) {
-    Material *ma = (Material *)lnk;
-    lnk = lnk->next;
-    BKE_id_free(freestyle_bmain, ma);
-  }
-
   BLI_ghash_free(_nodetree_hash, NULL, NULL);
 
   DEG_graph_free(freestyle_depsgraph);

I think I found the culprit. When rendering starts: - In `BlenderStrokeRenderer::RenderStrokeRepBasic(…)` there is a call to `GetStrokeShader(…, do_id_user=false)`. - That function reduces the user count of the "stroke_shader" material to zero. - Because of `do_id_user=false`, the `ntreeCopyTree_ex()` does *not* increase the user count for the copied node tree. When rendering stops: - All copied materials are looped over and freed. - Freeing the material recursively frees everything it uses. - This leaves the texture node's image with a user count that's one too little. Now the image datablock has no users, and will not be saved. Rendering twice shows an error on the console about a negative user count. The simplest fix is just to remove the freeing of all the materials. This works, and when Blender quits it doesn't complain of any memory leaks. I'll do some more tests with ASAN to check whether this is really the right way to go. ``` diff --git a/source/blender/freestyle/intern/blender_interface/BlenderStrokeRenderer.cpp b/source/blender/freestyle/intern/blender_interface/BlenderStrokeRenderer.cpp index c88d5f24b5d..22718c29745 100644 --- a/source/blender/freestyle/intern/blender_interface/BlenderStrokeRenderer.cpp +++ b/source/blender/freestyle/intern/blender_interface/BlenderStrokeRenderer.cpp @@ -200,15 +200,6 @@ BlenderStrokeRenderer::~BlenderStrokeRenderer() } } - // release materials - Link *lnk = (Link *)freestyle_bmain->materials.first; - - while (lnk) { - Material *ma = (Material *)lnk; - lnk = lnk->next; - BKE_id_free(freestyle_bmain, ma); - } - BLI_ghash_free(_nodetree_hash, NULL, NULL); DEG_graph_free(freestyle_depsgraph); ```

Gurpreet Singh commented

2020-03-17 19:21:13 +01:00

So I tried it again. I found a trick for this.
This time I turned on the fake user. When I reopened the file, the image was present.

Don't know if this will really help. But I think I'll just inform.

So I tried it again. I found a trick for this. This time I turned on the fake user. When I reopened the file, the image was present. Don't know if this will really help. But I think I'll just inform.

Sybren A. Stüvel commented

2020-03-19 10:27:24 +01:00

Added subscribers: @IRIEShinsuke, @kjym3

Sybren A. Stüvel commented

2020-03-19 10:27:24 +01:00

@kjym3 @IRIEShinsuke what do you think about the fix I described in my previous comment? From what I can see it works well (I also tested on https://download.blender.org/demo/test/AtvBuggy.zip) and causes no crashes or warnings about memory leaks.

I did some more testing, and with or without my code, there are the same number of calls to material_free_data() and ntree_free_data(), strengthening my hypothesis that the removed code is unnecessary.

@kjym3 @IRIEShinsuke what do you think about the fix I described in my previous comment? From what I can see it works well (I also tested on https://download.blender.org/demo/test/AtvBuggy.zip) and causes no crashes or warnings about memory leaks. I did some more testing, and with or without my code, there are the same number of calls to `material_free_data()` and `ntree_free_data()`, strengthening my hypothesis that the removed code is unnecessary.

Tamito Kajiyama commented

2020-03-24 16:17:25 +01:00

@Oye_LKY Thanks for the problem report. Using fake users fully makes sense as a workaround, because the reported bug is caused by a wrong decrement of image data block user counts.

@dr.sybren Many thanks for looking into the issue. I confirmed that your proposed fix indeed resolved the problem and no side-effect was seen.

The code segment you proposed to remove was intended to release a material auto-generated by BlenderStrokeRenderer::GetStrokeShader() every time a Freestyle rendering process takes place.

Removing the code seems inappropriate because of a risk of memory leaks. I have no idea why no such leak is seen though.

I tried to address the reported issue by properly incrementing the user counts of image data blocks referenced from texture nodes in Freestyle line style node trees.

diff --git a/source/blender/freestyle/intern/blender_interface/BlenderStrokeRenderer.cpp b/source/blender/freestyle/intern/blender_interface/BlenderStrokeRenderer.cpp
index c88d5f24b5d..00898f2b5c4 100644
--- a/source/blender/freestyle/intern/blender_interface/BlenderStrokeRenderer.cpp
+++ b/source/blender/freestyle/intern/blender_interface/BlenderStrokeRenderer.cpp
@@ -252,6 +252,13 @@ Material *BlenderStrokeRenderer::GetStrokeShader(Main *bmain,
     // make a copy of linestyle->nodetree
     ntree = ntreeCopyTree_ex(iNodeTree, bmain, do_id_user);

+    // loop over nodes to increment the user count of image data blocks
+    for (bNode *node = (bNode *)ntree->nodes.first; node; node = node->next) {
+      if (ELEM(node->type, SH_NODE_TEX_IMAGE, SH_NODE_TEX_ENVIRONMENT)) {
+        id_us_plus(node->id);
+      }
+    }
+
     // find the active Output Line Style node
     for (bNode *node = (bNode *)ntree->nodes.first; node; node = node->next) {
       if (node->type == SH_NODE_OUTPUT_LINESTYLE && (node->flag & NODE_DO_OUTPUT)) {

I have a few questions with respect to my solution:

Is maintaining ID reference counts in this way the right thing to do? Is there a better way to achieve the same effect?
Are there any other ID data blocks referenced from material nodes?

If you or some other main developers could answer these questions, I would be greatly appreciate it.

@Oye_LKY Thanks for the problem report. Using fake users fully makes sense as a workaround, because the reported bug is caused by a wrong decrement of image data block user counts. @dr.sybren Many thanks for looking into the issue. I confirmed that your proposed fix indeed resolved the problem and no side-effect was seen. The code segment you proposed to remove was intended to release a material auto-generated by BlenderStrokeRenderer::GetStrokeShader() every time a Freestyle rendering process takes place. Removing the code seems inappropriate because of a risk of memory leaks. I have no idea why no such leak is seen though. I tried to address the reported issue by properly incrementing the user counts of image data blocks referenced from texture nodes in Freestyle line style node trees. ``` diff --git a/source/blender/freestyle/intern/blender_interface/BlenderStrokeRenderer.cpp b/source/blender/freestyle/intern/blender_interface/BlenderStrokeRenderer.cpp index c88d5f24b5d..00898f2b5c4 100644 --- a/source/blender/freestyle/intern/blender_interface/BlenderStrokeRenderer.cpp +++ b/source/blender/freestyle/intern/blender_interface/BlenderStrokeRenderer.cpp @@ -252,6 +252,13 @@ Material *BlenderStrokeRenderer::GetStrokeShader(Main *bmain, // make a copy of linestyle->nodetree ntree = ntreeCopyTree_ex(iNodeTree, bmain, do_id_user); + // loop over nodes to increment the user count of image data blocks + for (bNode *node = (bNode *)ntree->nodes.first; node; node = node->next) { + if (ELEM(node->type, SH_NODE_TEX_IMAGE, SH_NODE_TEX_ENVIRONMENT)) { + id_us_plus(node->id); + } + } + // find the active Output Line Style node for (bNode *node = (bNode *)ntree->nodes.first; node; node = node->next) { if (node->type == SH_NODE_OUTPUT_LINESTYLE && (node->flag & NODE_DO_OUTPUT)) { ``` I have a few questions with respect to my solution: * Is maintaining ID reference counts in this way the right thing to do? Is there a better way to achieve the same effect? * Are there any other ID data blocks referenced from material nodes? If you or some other main developers could answer these questions, I would be greatly appreciate it.

blender-admin commented

2020-03-26 01:14:08 +01:00

This issue was referenced by ba8d819c9b

This issue was referenced by ba8d819c9b1ccb7dae50167755167dc3e54bd657

Brecht Van Lommel commented

2020-03-26 01:35:44 +01:00

Temporary datablocks used for rendering should not increase user counts on datablocks that are actually part of the .blend file, or modify them in any way that might be visible in the user interface or Python API.

This is all more complicated than it should be though. There is no reason for the freestyle_bmain to exist beyond the lifetime of BlenderStrokeRenderer, that's a leftover from Blender Internal. We can just free the entire thing and not bother trying to manually free particular datablocks.

Temporary datablocks used for rendering should not increase user counts on datablocks that are actually part of the .blend file, or modify them in any way that might be visible in the user interface or Python API. This is all more complicated than it should be though. There is no reason for the `freestyle_bmain` to exist beyond the lifetime of `BlenderStrokeRenderer`, that's a leftover from Blender Internal. We can just free the entire thing and not bother trying to manually free particular datablocks.

Brecht Van Lommel commented

2020-03-26 01:36:02 +01:00

Changed status from 'Confirmed' to: 'Resolved'

Brecht Van Lommel closed this issue

2020-03-26 01:36:02 +01:00

Gurpreet Singh commented

2020-03-26 04:43:54 +01:00

Awesome. You guys are the best.

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

Freestyle render removes texture image user #74417