Skip to content
This repository has been archived by the owner on Jan 23, 2018. It is now read-only.

Add ncolorpalette gameboy color cycling! #36

Merged
merged 1 commit into from
Sep 1, 2014

Conversation

kirbysayshi
Copy link
Contributor

The ncolorpalette takes the pixels of an image, and clusters them into n groups... in this case 4, and applies a gameboy palette to the groups.

@kid-icarus
Copy link
Contributor

Woah this looks amazing!!!! Can you squash those commits? Thanks! 🍰 🍰 🍰 🍰

The ncolorpalette takes the pixels of an image, and clusters them into n groups... in this case 4, and applies a gameboy palette to the groups.
@kirbysayshi
Copy link
Contributor Author

@kid-icarus done!

kid-icarus added a commit that referenced this pull request Sep 1, 2014
Add ncolorpalette gameboy color cycling!
@kid-icarus kid-icarus merged commit 1fa864c into revisitors:master Sep 1, 2014
@kid-icarus
Copy link
Contributor

Thank you! This is awesome! 👍

@kirbysayshi
Copy link
Contributor Author

Cool, thanks!

BTW, more effects are available at http://kirbysayshi.github.com/ncolorpalette.

Are there any limits to how many entries can be added to the hub?

@operatorjen
Copy link

@kirbysayshi hey just checked out your stuff! looks good and is on prod :) a few things:

  1. some animated gifs generate as broken through your service - for example, run this one through your service and see the result -
    the-internet
  2. http://revisit.svs.ncolorpalett.es/kirby_50.png seems to not exist for the icon?
  3. No limits to entries! :)

@kirbysayshi kirbysayshi deleted the patch-1 branch September 1, 2014 18:26
@kirbysayshi
Copy link
Contributor Author

@ednapiranha wow I'm so sorry how piecemeal this has been. Got a PR up to fix the sample image. Thanks for sending the sample gif, looks like I was putting a content length limit of 1MB on the entire request, and not each individual media item. The spec is slightly ambiguous there (or at least ambiguous to me). Does that mean 1MB limit on the content.data value, or 1MB including the json structure of content: { .. }?

I asked about the entries limits because the way the service is setup, it's relatively easy to make more combinations of color palettes, animations, and timings. For example, http://revisit.svs.ncolorpalett.es/gameboy will apply just the gameboy palette without the color cycling. And a custom palette is pretty easy to define as well, but not sure how to do that within revisit.link, since params aren't a thing.

@kirbysayshi
Copy link
Contributor Author

Also, request limit has been lifted! nginx also had a limit in addition to my app.

@operatorjen
Copy link

Hey @kirbysayshi - yeah it should in theory be 1 MB on the entire content.data part but it wasn't really clear :)

I've set it up on the hub to block anything over 1 MB but if someone bypasses it and goes directly to your system, then it will go over unless you have your limit.

The other thing is that if it passes through other services that mutate gifs, they could increase the size by the time it goes to you - I'm thinking upping the limit on nginx would be a safe bet now and assume it won't go over 3 MB (at worst). That's something we'll have to play around with and see what works well for everyone (since this is all mutating as we experiment). Hope that helps! :)

@kirbysayshi
Copy link
Contributor Author

@ednapiranha maybe in the spec clarify if the limit means:

  • data:...;base64... can't exceed 1mb
  • data:...;base64... decoded can't exceed 1mb
  • subset of the json body, including JSON stuff, can't exceed 1mb (content, audio, etc)

They're all hard to enforce because they require loading the contents of the request into memory fully, although I suppose the last (and current) is best because you could at least do conditional JSON parsing/streaming using JSONStream or something like it.

@operatorjen
Copy link

@kirbysayshi might be tough to conform to 1 MB post-animated-gif filtering for some people so I might just reword it to say incoming on the hub can only be 1 MB..

@kirbysayshi
Copy link
Contributor Author

@ednapiranha yeah, and I don't want to make it harder for people to build a service, just want to be able to provide a sane level of performance / expectations for users.

@operatorjen
Copy link

@kirbysayshi that's fine and is understood. you do realize this is an experiment that is barely 2 weeks old? :) keep in mind that our specs will likely change frequently and things will break. such is life

@kid-icarus
Copy link
Contributor

Thanks for the comments, I've moved this to an issue here: #41

@revisitors revisitors locked and limited conversation to collaborators Sep 2, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants