kevva / to-ico

Convert PNG to ICO in memory
MIT License
137 stars 19 forks source link

Chokes on 512x512px image #13

Open salomvary opened 7 years ago

salomvary commented 7 years ago

If it can't downscale, it should fail with a user friendly exception ("Images larger than 256x256 px are not supported").

$ file icon_512x512.png 
icon_512x512.png: PNG image data, 512 x 512, 8-bit/color RGBA, non-interlaced

$ node_modules/.bin/to-ico icon_512x512.png 
TypeError: "value" argument is out of bounds
    at checkInt (buffer.js:1040:11)
    at Buffer.writeUInt8 (buffer.js:1088:5)
    at createDirectory (node_modules/to-ico/index.js:32:6)
    at Promise.all.then.data (node_modules/to-ico/index.js:98:16)
    at process._tickCallback (internal/process/next_tick.js:103:7)

$ npm list | grep to-ico
└─┬ to-ico-cli@1.0.0
  └─┬ to-ico@1.1.3
kevva commented 7 years ago

Instead of throwing, we could just resize it to 256x256, regardless if resize is true or not. I'm also thinking about setting resize to true by default since it's really fast anyway and you'd want it in most cases.

albinekb commented 6 years ago

@kevva if you can tell me which way you'd like it I'd love to send a PR 🙌

albinekb commented 6 years ago

maybe @sindresorhus has the magic answer 🦄 😏

kevva commented 6 years ago

@albinekb, I think the first step is to just resize the image to 256x256 if it's larger. A PR would be greatly appreciated :+1:.

DanielRuf commented 4 years ago

Right.

Also related: https://github.com/kevva/to-ico/issues/20

DanielRuf commented 4 years ago

Generally setting the option {resize: true} should prevent that.