added programmatic dissection of cumulatively rendered gifs #18
Conversation
There was a problem hiding this comment.
I'm sorry I didn't fully understand what this algorithm entailed.. it strikes me as expensive since it involves repeated draws and subsequent reads from a canvas 2d context.. and it also will only work in a web browser. gifFrames is designed to work in Node.js as well. A few main thoughts:
- I think we can do this without using a canvas. We can probably just modify the code that you deleted which updates the
frameobject, and we already have access toframesInfothere so we can check the disposal method, etc. before we send anything tosavePixels. - I see that we can remove the
cumulativeoption, but we should make sure not to impose a significant performance penalty on users who have GIFs without fancy transparency stuff that causes problems. By using the frame object instead of a canvas I think we can do that. - Check this comment which specifies the logic for what to do depending on the disposal method specified. I think it's basically the same as what is implemented here, but it could be helpful for reimplementing without canvas.
- I think the new algorithm assumes all frames are collected. What we ought to do is collect all frames up to the highest frame index, and return those that are requested.
| @@ -114,6 +132,9 @@ function gifFrames (options, callback) { | |||
| resolve(frameData); | |||
| }); | |||
|
|
|||
| // programatically fixes | |||
| promise.then((frameData) => {return (renderCumulativeFrames(frameData))}); | |||
There was a problem hiding this comment.
Just for consistency I'd like to stick to ES5, so no arrow functions.
| @@ -114,6 +132,9 @@ function gifFrames (options, callback) { | |||
| resolve(frameData); | |||
| }); | |||
|
|
|||
| // programatically fixes | |||
| promise.then((frameData) => {return (renderCumulativeFrames(frameData))}); | |||
|
|
|||
| return promise; | |||
There was a problem hiding this comment.
I think we would need to return the result of promise.then above, not the initial promise value. I think the only reason your code works is because that callback gets queued first, and it mutates frameData. But if renderCumulativeFrames returned a new frameData object, the code wouldn't work. So I think return promise.then(...) would be better.
| current.width = firstFrameCanvas.width; | ||
| current.height = firstFrameCanvas.height; | ||
|
|
||
| for (const frame of frameData) { |
There was a problem hiding this comment.
If we can use ES5 here (e.g. forEach) that would be better. thanks
| previousContext.drawImage(current, 0, 0); | ||
|
|
||
| // Draw the current frame to the cumulative buffer. | ||
| const canvas = frame.getImage(); |
There was a problem hiding this comment.
var please (here and elsewhere)... thanks!
| currentContext.clearRect(0, 0, current.width, current.height); | ||
| currentContext.drawImage(previous, 0, 0); | ||
| } | ||
| frame.getImage = () => canvas; |
There was a problem hiding this comment.
function() { } please... thanks!
| const {disposal} = frameInfo; | ||
| // If the disposal method is clear to the background color, then clear the canvas. | ||
| if (disposal === 2) { | ||
| currentContext.clearRect(frameInfo.x, frameInfo.y, frameInfo.width, frameInfo.height); |
There was a problem hiding this comment.
Did you test this on the heart GIF from the other issue thread (https://media0.giphy.com/media/3HHvQSvdCMYD2bY1X2/giphy.gif)? Here I noticed that the reason for that issue was because the background color was red. I would think we would need to be reading the background color from the framesInfo.
|
I apologize for being so clumsy with syntax standards, this was my first pull request I've made on an unfamiliar repository. I recently got some work that will keep me busy for these next few days, but I'll try to look over all of it and try to submit a more optimized and conforming pull request within this week! |
|
No worries about the formatting! I definitely appreciate the PR, and my norm is to leave comments. Doesn't mean anything bad 😄. BTW I've been thinking about my earlier comment:
Actually what could be more efficient and skip unneeded computation, is to collect the frames beginning by iterating in reverse order. I actually have something in my head so I might suggest a code update to the PR later today, if you wouldn't mind. |
|
Not at all, do your thing. Like I said, I'm busy for a few days, but I'll
be back by mid/end-of-week to see where I can clean things up or help a bit
more with this. Also, I just wanted to say thank you so much for this
library! It was an integral part of my first webdev portfolio project which
is the main thing I'm using to upwork contracts and apply for my first dev
jobs right now. If your interested in seeing an integrated use case, i've
got it hosted here <https://reactiongifme.herokuapp.com/>. Anyway, don't
mean to shill, It's just been really transformational in my life
opportunities recently and I'm super psyched about it.
…On Mon, Apr 20, 2020 at 2:13 PM Ben Wiley ***@***.***> wrote:
No worries about the formatting! I definitely appreciate the PR, and my
norm is to leave comments. Doesn't mean anything bad 😄.
BTW I've been thinking about my earlier comment:
1. I think the new algorithm assumes all frames are collected. What we
ought to do is collect all frames up to the highest frame index, and return
those that are requested.
Actually what could be more efficient and skip unneeded computation, is to
collect the frames beginning by iterating in reverse order. I actually have
something in my head so I might suggest a code update to the PR later
today, if you wouldn't mind.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#18 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHIRCBZZ7UJWDIIZ3DP6VYLRNSUH3ANCNFSM4MMEUAYQ>
.
|
|
@pnkfluffy cool! The concept for your app seems cool from what I can see, but I wasn't able to try because the signup confirmation email got flagged as spam by Gmail and it won't let me click the link. Let me know if you get that sorted out.. thanks for sharing! |
|
@pnkfluffy just kidding, I was able to get it to work in the desktop web version of gmail by bypassing the spam filter, but I got stuck in the app on this screen (there's no prompt to access the camera, and I don't see any JS console errors):
|
Removed cumulative option. I deleted the stuff that had to do with your cumulative rendering and just added the code from TrevorSundberg. Tested it on a few cumulative and non-cumulative gifs and it worked with no issue.