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

Improve memory pressure while exporting GIFs #115

Merged
merged 1 commit into from
Jun 1, 2021

Conversation

SergioEstevao
Copy link
Collaborator

This PR updates the exporting loops for creating GIF to be wrapped with autorelease blocks so the memory pressure is smaller when exporting then.

Related bug: https://jira.tumblr.net/browse/PROD-15147

How to test:

@SergioEstevao SergioEstevao added the bug Something isn't working label May 31, 2021
@SergioEstevao SergioEstevao requested a review from pinarol May 31, 2021 17:12
@pinarol
Copy link
Collaborator

pinarol commented Jun 1, 2021

I used the iPhone 8 Plus (iOS 14.4) simulator to test this but I am having this assertion failure after these steps:

  • Open example app
  • Tap "Open Kanvas Dashboard"
  • Tap on the media library icon on the bottom left
  • Select the gif here https://zikkimyeyin.tumblr.com/post/651438616631033856 (I had dragged&dropped it into the sim, so it is now in the device lib)
  • Tap on the Post button in the next screen (The circle blue one with arrow)
  • Crash:

Screen Shot 2021-06-01 at 13 48 46

@pinarol
Copy link
Collaborator

pinarol commented Jun 1, 2021

Another thing, Tumblr was not depending on the main branch, not sure about the most up to date state but you may need to target another branch.

@SergioEstevao
Copy link
Collaborator Author

SergioEstevao commented Jun 1, 2021 via email

@danielebogo
Copy link
Collaborator

@SergioEstevao Pinar is right. You have to merge this first and then cherry pick this commit in a new PR targeting a base branch created from 1.0.3 tag, also bumping the version to 1.0.4 for CanvasKamera.
Tumblr doesn't use Kanvas yet, but its old version.

Copy link
Collaborator

@pinarol pinarol left a comment

Choose a reason for hiding this comment

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

OK I can confirm that the the crash doesn't happen anymore.

Even though I think autoreleasepool is not the most ideal solution it is prolly the most time efficient one currently so LGTM. 👍

(Don't forget to update the target branch.)

@danielebogo
Copy link
Collaborator

@pinarol I think this can be merged on main. We can cherry pick the commit on a new base so the fix will be there even when we will update to the most recent Kanvas

@pinarol
Copy link
Collaborator

pinarol commented Jun 1, 2021

Ok then 👍

@SergioEstevao SergioEstevao merged commit e522036 into main Jun 1, 2021
@SergioEstevao SergioEstevao deleted the sergio/fix_memory_pressure_gif branch June 1, 2021 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants