Add write support for 16-bit PNG #763
Conversation
|
Because this implementation supports both 8- and 16-bit channels by treating them as 1 or 2 bytes and using a multiply by the component count, it will slow down the common case of 1-byte channels unacceptably. It needs to use a separate path for the pixel loops. |
WearerOfShoes
force-pushed
the
png16bit
branch
from
7ffb71e to
07f714a
Compare
WearerOfShoes
force-pushed
the
png16bit
branch
from
07f714a to
d882013
Compare
|
Thanks for review, Sean. |
|
ok, yeah, that works too. i didn't realize it was a loop-constant that compiler MIGHT have fixed, but still worth changing. |
| case 2: for (i=pixel_bytes; i < width*n; ++i) line_buffer[i] = z[i] - z[i-signed_stride]; break; | ||
| case 3: for (i=pixel_bytes; i < width*n; ++i) line_buffer[i] = z[i] - ((z[i-pixel_bytes] + z[i-signed_stride])>>1); break; | ||
| case 4: for (i=pixel_bytes; i < width*n; ++i) line_buffer[i] = z[i] - stbiw__paeth(z[i-pixel_bytes], z[i-signed_stride], z[i-signed_stride-pixel_bytes]); break; | ||
| case 5: for (i=pixel_bytes; i < width*n; ++i) line_buffer[i] = z[i] - (z[i-pixel_bytes>>1]); break; |
There was a problem hiding this comment.
There's a bug here, the code went from z[...]>>1 to z[...>>1 right before 'break']. I'm trying to fix this by changing the code back to using 'n' so that less code will be changed. I.e. change passed in parameter n to "channel_count", then say 'int n = pixel_bytes', and then most of the code is unchanged.
While doing that, though, I realized that the current code still loops i up to width * n, not width * pixel_bytes. That can't possibly be right, can it? But if it's wrong, then half of the image would have failed to encode, which I think you would have noticed.
There was a problem hiding this comment.
@WearerOfShoes , were you able to take a look at this comment by Sean? I'm very interested in being able to save into any type deeper than 8 bits...
There was a problem hiding this comment.
@WearerOfShoes, I'm very interested in this, too...
There was a problem hiding this comment.
@nothings: So it took me a while to figure out, but apart from the right-shift bug you pointed out, the code is actually correct with regards to width * n vs width * pixel_bytes in the for-loops because width has already been multiplied by comp_bytes (i.e. 1 for 8-bit PNGs, 2 for 16-bit PNGs) in stbiw__write_png_to_mem_core():
// all calculations in PNG are based on bytes in 16-bit mode,
// so just treated it as a twice-as-wide 8-bit image works in most cases
// exceptions are patches up by sending along comp_bytes
x *= comp_bytes;
I also ran the tests in image_write_test and all generated PNGs are seemingly correct.
(BTW, I can make a new PR based on a fork of @WearerOfShoes work so we can finish this if you like)
This patch adds write support for 16-bit PNG.
I tried to mimic the way it is done on the read side, by having a special _16 function. I didn't modify the interface of any previously exposed functions.
Most of the functionality is shared with the 8-bit PNG writer, and I expect it to get a comparable performance and compression ratio. But I admit I haven't measured it.