fix: revoke Blob URLs created by p5.File - #9155
slash-init wants to merge 7 commits into
Conversation
|
Hi, I'd like to take this on. |
|
@rk-3001 i think you should read contributor guidelines first. |
Vaivaswat2244
left a comment
Vaivaswat2244
left a comment
There was a problem hiding this comment.
Thanks for the pr @slash-init . Matches what we discussed on the issue, left a few comments.
de025d3 to
6cbdcea
Compare
| this._userNode = node; | ||
| this._curElement = null; | ||
| this._elements = []; | ||
| this._blobUrls = new Set(); |
There was a problem hiding this comment.
Does this need to be on the p5 instance? Can it not be fully handled by p5.File itself? For cleaning up when the sketch is removed, use the remove lifecycle hook which is designed for this kind of use.
There was a problem hiding this comment.
@limzykenneth Thanks for the feedback! I looked through the lifecycle code and I think I understand the direction now: move the Blob URL cleanup into p5.File and use the existing remove lifecycle hook, rather than keeping _blobUrls on the p5 instance.
The one thing I'm still unsure about is how you'd like the File to keep track of which p5 instance it belongs to. Right now _load() gets pInst explicitly for that, but from your comment on dom.js, it sounds like you'd prefer not to pass it through like this.
I could move the cleanup into a single remove hook, but I don't want to introduce another registry or ownership mechanism if there's already a pattern in p5.js that I'm missing.
Is there a particular approach you had in mind for handling the per-instance cleanup here?
| const pInst = this; | ||
| const handleFileSelect = function (event) { | ||
| for (const file of event.target.files) { | ||
| File._load(file, callback); | ||
| File._load(file, callback, pInst); |
There was a problem hiding this comment.
Per the above, this should not be necessary.
| /** | ||
| * Revokes the Blob URL associated with this file, if one was created. | ||
| * | ||
| * When video or audio files are loaded via | ||
| * <a href="#/p5/createFileInput">createFileInput()</a> or | ||
| * <a href="#/p5.Element/drop">myElement.drop()</a>, p5 creates a Blob URL | ||
| * pointing to the media in browser memory. Calling `revoke()` releases that | ||
| * resource immediately instead of waiting for the sketch to be removed. | ||
| * | ||
| * @method revoke | ||
| * @for p5.File | ||
| * | ||
| * @example | ||
| * // Load a video file and release its URL when replacing it. | ||
| * let video; | ||
| * let previousFile; | ||
| * | ||
| * function setup() { | ||
| * createCanvas(100, 100); | ||
| * createFileInput(handleFile); | ||
| * } | ||
| * | ||
| * function handleFile(file) { | ||
| * if (file.type === 'video') { | ||
| * if (video) { | ||
| * video.remove(); | ||
| * previousFile.revoke(); | ||
| * } | ||
| * | ||
| * video = createVideo(file.data); | ||
| * previousFile = file; | ||
| * } | ||
| * } | ||
| */ | ||
| revoke() { | ||
| if (this._isBlobUrl && this.data) { | ||
| URL.revokeObjectURL(this.data); | ||
| if (this._pInst && this._pInst._blobUrls) { | ||
| this._pInst._blobUrls.delete(this.data); | ||
| } | ||
| this._isBlobUrl = false; | ||
| } |
There was a problem hiding this comment.
It needs to be clearer when and why a user might want to revoke a URL object manually like this and what the consequence of this would be.
4 participants
Resolves #9134
Changes:
p5.File._load()on the owning p5 instance.p5.remove()is called.p5.File.revoke()for explicitly releasing a file's Blob URL.p5.File.revoke()idempotent and remove revoked URLs from the instance's tracking set.createFileInput()andElement.drop()so media files are tracked correctly.PR Checklist
npm run lintpasses