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

Can lose image when out of memory, including auto-save if you interact #24

Closed
1j01 opened this issue Dec 20, 2017 · 8 comments
Closed

Comments

@1j01
Copy link
Owner

1j01 commented Dec 20, 2017

When chrome runs out of memory, canvases get erased, and when you see your image is gone you'll probably naturally try to undo (speaking from experience), but that actually then destroys the last memory of the image, because it saves over the autosave with more nothingness.

So: we should detect if the canvas suspiciously becomes blank. Maybe also the undo history canvases (just the one, when undoing/redoing, I guess should work).

I wonder if there's a good way to simulate out of memory conditions, rather than just loading web apps with terribly expensive memory usage.

@1j01 1j01 added the data-loss label Dec 20, 2017
@1j01 1j01 added this to the 1.0 / good enough to share milestone Dec 20, 2017
@1j01
Copy link
Owner Author

1j01 commented Jun 19, 2018

Could check canvas.ctx.getImageData(0, 0, canvas.width, canvas.height).data.some((v)=> v > 0)
but we don't want to show a warning if you purposely clear the canvas, so, we could maybe check that it's not within some amount of time of the last update ($canvas.on("change", ...))
Also it would be good to lock the canvas / editing so you can't accidentally make a change that triggers saving over the backup

@1j01 1j01 changed the title Can lose image unnecessarily when out of memory Can lose image stupidly when out of memory Jun 30, 2018
@1j01 1j01 changed the title Can lose image stupidly when out of memory Can lose image when out of memory, including auto-save if you interact Sep 11, 2018
@1j01
Copy link
Owner Author

1j01 commented Feb 15, 2019

Data loss confirmed in the wild: https://youtu.be/GTOntYcgIqM?t=80
It's especially a problem if you're actively drawing when it happens

@1j01
Copy link
Owner Author

1j01 commented May 18, 2019

I don't think this happens all the time, but here all canvases gone transparent (including tool options and the color wells (which use canvases so that dark mode browser extensions don't change the colors)):

out of memory or whatever, unloads canvases

If it always happened with all canvases at once, we could run detection of datalessness on a color well, or even have a specific 1x1 sentinel canvas to detect this, but sadly I don't think that's the case.
The bigger canvas(es including undos/redos) gets unloaded first.

@1j01
Copy link
Owner Author

1j01 commented Jun 29, 2019

Oh, another possibility for mitigating this might be to store undos not as canvases, but as imagedata arrays - hopefully then the browser won't be so callus with erasing it!
This would allow you to undo when this happens, theoretically.
Or if it crashes the tab, that's still better, because we do have an autosave, and in that case the autosave shouldn't be overwritten. (we still need to worry about the main canvas tho)

Also we could save multiple autosaves! Once every N edits, up to some number of saves per session.

And as for detecting data loss on the canvas, it would be simple if jspaint was architected to always update state discretely for undoable operations - if it used a pattern of undoable(function action(){}) everywhere, then we could have a flag for allowing it to go fully transparent during the undoable action callback, and could check elsewhere if this flag was set and if not and the canvas became transparent, then we know what's going on... But it could go and erase the memory while doing an action for all I know, so I don't know how helpful that would be.

Anyways, I think improving autosave is the most important, giving it multiple save points per session and making it more accessible/discoverable by popping up with a recovery dialog instead of just having it tucked away in a menu (File > Manage Storage).

@1j01
Copy link
Owner Author

1j01 commented Oct 7, 2019

@1j01
Copy link
Owner Author

1j01 commented Oct 18, 2019

btw I have switched over to ImageData, which led to implementing a helper layer for tools so I could refactor them so they didn't use horrible hacks of accessing the undo stack (which was canvases and is now imagedata), and that led to implementing The Grid... which was a much more exciting feature so I didn't remember to put an update here, but...

so it should now crash the tab fully instead, or if the browser just clears the main canvas, you should still be able to undo (theoretically)
BUT, if the main canvas is cleared and you do something that triggers it to save over the autosave, and you don't undo (maybe you don't have time to because it crashes the tab soon after that), your work will still be lost.
And I haven't really tested this. Chrome might treat ImageData as second-class / volatile, same as canvas, or with a middle priority level or something.

Multiple autosave is still needed, and/or at least addressing the browser-clears-the-canvas thing directly by detecting when it goes transparent. I think it would be better to have a dialog that's kind of annoying/weird if you clear the canvas to transparent on purpose than leaving surprised users to fend for themselves in trying to recover their work. Something like "Woah, did you do that? [Yes, I want a blank canvas] [Restore my work]"
of course, testing to make sure restoring is actually possible via undo

@1j01
Copy link
Owner Author

1j01 commented Oct 21, 2019

Alright, tested this and found that editing very large images it does tend to crash the page instead, which is good! (I'm guessing with a smaller image over a longer period of time it would likely still lose the canvases, because it wouldn't be trying to allocate as much memory at once.)

Also, I found a way to consistently & easily cause chrome to clear all the canvases: make a selection and press numpad plus repeatively to enlarge the selection exponentially.(If you keep pressing it while it's lagging or otherwise do it too many times you can still get it to crash the tab, or freeze up your entire browser, so you have to be a little careful.
Edit: often it gets this instead: Uncaught RangeError: Failed to execute 'getImageData' on 'CanvasRenderingContext2D': Out of memory at ImageData creation

@1j01
Copy link
Owner Author

1j01 commented Oct 25, 2019

Implemented a dialog when the canvas becomes blank:
image
and it expands if you change the canvas other than undoing:
image
It's a little overly complex, but it's good for now, better than nothing!

Limitations:

  • It shows (unnecessarily) also when you Ctrl+A Delete in Transparent mode, or undo to a completely transparent state. (but it's too hard to detect why it's gone transparent, and it's better safe than sorry)
  • It doesn't detect the selection canvas going transparent
  • The application's functionality isn't fully restored when recovering from canvas memory evacuation (but I've started handling webgl context loss and restoring the brushes...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant