-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #4953: Resource leak when using --animate #4954
base: master
Are you sure you want to change the base?
Conversation
Create the OffscreenView only once when animating
I think the root cause is due to OffscreenView not being destructed appropriately? In theory it should close everything when dropped, and will not cause resource leakage (but it may be slow). |
Either way, allocating it only once works well and it's more efficient that
way.
…On Sun, Jan 28, 2024, 22:11 pca006132 ***@***.***> wrote:
I think the root cause is due to OffscreenView not being destructed
appropriately? In theory it should close everything when dropped, and will
not cause resource leakage (but it may be slow).
—
Reply to this email directly, view it on GitHub
<#4954 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIIZ5OGCHAELUJTRJTXT6LYQ4OP7AVCNFSM6AAAAABCOSV7LOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJTHEZDQOBQHA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
True. Maybe we can use a singleton instead of passing OffscreenView around? |
|
||
bool prepare_preview(Tree& tree, const ViewOptions& options, Camera& camera, OffscreenView& glview) | ||
{ | ||
PRINTD("prepare_preview_common"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was an old typo: -> PRINTD("prepare_preview");
try { | ||
glview = std::make_unique<OffscreenView>(camera.pixel_width, camera.pixel_height); | ||
glview = std::make_shared<OffscreenView>(camera.pixel_width, camera.pixel_height); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why switch to shared_ptr
when we're still maintaining a unique instance?
@@ -446,6 +446,13 @@ int cmdline(const CommandLine& cmd) | |||
render_variables.time = 0; | |||
return do_export(cmd, render_variables, export_format, root_file); | |||
} else { | |||
std::shared_ptr<OffscreenView> glview = nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
= nullptr
is superfluous
@@ -446,6 +446,13 @@ int cmdline(const CommandLine& cmd) | |||
render_variables.time = 0; | |||
return do_export(cmd, render_variables, export_format, root_file); | |||
} else { | |||
std::shared_ptr<OffscreenView> glview = nullptr; | |||
if ((export_format == FileFormat::ECHO || export_format == FileFormat::PNG) && (cmd.viewOptions.renderer == RenderType::OPENCSG || cmd.viewOptions.renderer == RenderType::THROWNTOGETHER)) { | |||
Camera camera = cmd.camera; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks a bit dubious: You're copying the camera, then passing the copy into create_offscreenview() which potentially modifies the camera, then passing the original camera to do_export() later.
Create the OffscreenView only once when using --animate