search

nagata-yoshiteru / pbm-ppm-pgm-viewer-for-vscode

MIT License
14 stars 9 forks source link

P6 with maxval > 255 not rendered correctly #35

Closed DanielS6 closed 1 year ago

DanielS6 commented 1 year ago

When a P6 has a maxval (denominator) greater than 255, each color is stored in two bytes but the extension code is only reading 1 byte, meaning that a completely wrong image is shown (because each pixel's 6 bytes are interpreted as 2 different pixels, the first from the RRG bytes and the second from the GBB bytes). See https://linux.die.net/man/5/ppm and https://github.com/nagata-yoshiteru/pbm-ppm-pgm-viewer-for-vscode/blob/2d5f0052845120f51eb5204234f2bcd1d8de4b24/src/parsing.ts#L209-L217

BenWeisz commented 1 year ago

Ahh yup that makes sense. We can try to fix it so that if the max value specified is greater than or equal to 2^8 then we use 2 bytes per channel otherwise just 1. I think the man page says the max is 2^16 - 1 so those 2 cases should cover everything.

I'm a bit busy ATM but I can take a look at this some time later next week.

DanielS6 commented 1 year ago

Ahh yup that makes sense. We can try to fix it so that if the max value specified is greater than or equal to 2^8 then we use 2 bytes per channel otherwise just 1. I think the man page says the max is 2^16 - 1 so those 2 cases should cover everything.

I'm a bit busy ATM but I can take a look at this some time later next week.

I created a patch to fix this myself, but for the life of me I couldn't figure out how to get the extension to run with my local version, so my fix is untested.

BenWeisz commented 1 year ago

Hey @DanielS6, I took a look at https://netpbm.sourceforge.net/doc/ppm.html which I guess you could consider gospel for the file type. I think this same sort of thing is valid for the P5 format too. I was wondering if you could add it for that case as well and then I could take a look at how it performs locally.

We might have to remove the getPixel function later for performance (to limit function calls because I'm not sure if that function is optimized away) but I think we can do that later in a separate PR as long as this version runs fast enough.

Thanks!

DanielS6 commented 1 year ago

Hey @DanielS6, I took a look at https://netpbm.sourceforge.net/doc/ppm.html which I guess you could consider gospel for the file type. I think this same sort of thing is valid for the P5 format too. I was wondering if you could add it for that case as well and then I could take a look at how it performs locally.

Done