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

Remove base64-stream dependency #73

Merged
merged 1 commit into from
Jul 24, 2017
Merged

Conversation

lpinca
Copy link

@lpinca lpinca commented Jul 24, 2017

This is an attempt to fix #31.

Currently as mentioned in the issue description the workflow is:

  • savePixels() generates a readable stream -- r
  • base64-stream generates a writable stream -- w
  • r.pipe(w) pipes r to w
  • a listener for the 'finish' event is attached to the pipe which is responsible for converting the image data to a DataURI.

Again as mentioned in the issue description it seems that the 'finish' event is not emitted.

This probably happens as a consequence of backpressure handling. The write queue of the transform stream (w) is filled so no more data is read from the source (r) until the 'drain' event is emitted but this never happens because no one is reading from the readable side of w.

This also explains why it work with a small 16x16 image. If the image is smaller than highWaterMark which defaults to 16384 bytes, then the 'finish' event should always be emitted.

A possible fix was to add a listener for the 'data' event to w, save all data chunks and merge them on 'finish'.

This patch saves and merges all data chunks but also removes the base64-stream dependency. Conversion to base64 is done after all chunks are concatenated and using the built-in buf.toString('base64') method.

@ccpandhare
Copy link
Collaborator

Wow, this indeed looks interesting!
What do you think, @jywarren?

@jywarren
Copy link
Member

I like it -- looks clean and reduces dependencies -- very cool. Besides that it passes all current tests, would we like to add a test which fails the current implementation and passes in this one, based on #31?

Either way, I'm fine merging this if you are, @ccpandhare -- I leave it to your judgement!

Thanks a million, @lpinca !!

@ccpandhare
Copy link
Collaborator

Okay @jywarren ! Merging!
Thanks a lot @lpinca !
Also I will make that test you mentioned here :-)

@ccpandhare ccpandhare merged commit 246130c into publiclab:master Jul 24, 2017
@ccpandhare
Copy link
Collaborator

@lpinca I'd like to request you to to claim your bounty from Bounty Source if you haven't yet.
Thanks a lot for helping us! That's the least we can do :-)

@lpinca
Copy link
Author

lpinca commented Jul 24, 2017

Yes a test is needed, I didn't add one as I'm not familiar enough with the code base.
I can spend some more time checking the existing tests if you want and try to add one.

@ccpandhare sure will do.

Thank you.

@lpinca lpinca deleted the gh-31 branch July 24, 2017 18:38
@ccpandhare
Copy link
Collaborator

@lpinca No worries, I will write the test. I think A test to check whether of not PNG, GIF images work fine should do, right?

@lpinca
Copy link
Author

lpinca commented Jul 24, 2017

Yes, that should work.

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

Successfully merging this pull request may close these issues.

None yet

3 participants