Skip to content
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

Reduce renderer memory consumption when exporting #140

Merged
merged 4 commits into from
Jul 8, 2022

Conversation

twstokes
Copy link
Collaborator

@twstokes twstokes commented Jun 30, 2022

Description

Memory consumption grows constantly when exporting multiple images due to calls to output in the renderer. In instances where there's no delegate to process the renderer's pixel buffer, we can avoid running the function altogether.

This will help alleviate WPiOS out-of-memory crashes:

@twstokes twstokes self-assigned this Jun 30, 2022
@twstokes twstokes changed the title Reduce Renderer memory consumption when exporting Reduce renderer memory consumption when exporting Jun 30, 2022
@twstokes twstokes added stories Related to the Stories project enhancement New feature or request labels Jun 30, 2022
@twstokes twstokes added this to the 1.4.3 milestone Jun 30, 2022
@twstokes twstokes marked this pull request as ready for review June 30, 2022 16:51
@twstokes
Copy link
Collaborator Author

@afterxleep, @bjtitus - would you mind giving this a look? 🙇 Hopefully this change is straightforward - it seems like if there's no delegate to display a renderer's output we shouldn't bother with the output method. Yet, I do see where it's mutating self.filteredPixelBuffer and I'm not totally sure of the ramifications of that.

A WPiOS build that includes these changes is here, but I could use your guidance if we need to test Tumblr. Thank you.

@twstokes twstokes force-pushed the fix/multi-export-crash branch from 4d715e0 to dd7b104 Compare July 6, 2022 13:32
twstokes added 2 commits July 6, 2022 09:34
MultiEditorViewController: Reload the current editor during export process
}
}
})

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes are from #141.

Copy link
Collaborator

@bjtitus bjtitus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested in Tumblr and seems to work.

Thanks for tracking this down!

@twstokes
Copy link
Collaborator Author

twstokes commented Jul 6, 2022

Tested in Tumblr and seems to work.

Thanks for tracking this down!

Thank you for the review @bjtitus!

@afterxleep afterxleep merged commit 2743f5a into main Jul 8, 2022
@afterxleep afterxleep deleted the fix/multi-export-crash branch July 8, 2022 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request stories Related to the Stories project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants