AndreRenaud / PDFGen

Simple C PDF Writer/Generation library
The Unlicense
504 stars 120 forks source link

PNG support overhaul #99

Closed JulianGmp closed 3 years ago

JulianGmp commented 3 years ago

I wanted to add some PNG images but noticed some weren't working, specifically ones with with 8-Bit grayscale info and 8-Bit indexing (color type 0 and 3). So I decided to implement it.

One part I am a bit unsure about is how I handled the colour_space string. It's manually allocated and freed since it might be just 12 or around 2000 bytes long, and I thought that I shouldn't just make the 2k buffer in the cases where I don't even need it. At times like this I miss std::string, but if you have a suggestion on how to do this better please let me know.

Disclaimer: I'm sending this PR at 1 am this might as well just be garbage that seems to work.

Changes

*this also allows fairly easily adding 8-Bit BMP data by modifying pdf_add_bmp_data, but that's out of scope for this PR.

Resources

These might be useful to understand what some of the code is even trying to do.

AndreRenaud commented 3 years ago

Hi Julian, Thanks for this - it looks really good.

I'd sort of like to split it into two PRs just for review purposes - one just the type changes (going to uin32_t etc...), and the other for the PNG stuff. I think the type change stuff is pretty much a no-brainer, and makes things better. But equally, you've done the work, so I'm not too fussed about this. If you'd rather stick with it all in this PR, that's fine as well.

The PNG thing I'm not quite clear on how it works with greyscale and palette'd images. Are you able to stick some example files into the data directory, and make tests/main.c output them? Superficially, it all looks good though.

Andre

JulianGmp commented 3 years ago

Hi Julian, Thanks for this - it looks really good.

I'd sort of like to split it into two PRs just for review purposes - one just the type changes (going to uin32_t etc...), and the other for the PNG stuff. I think the type change stuff is pretty much a no-brainer, and makes things better. But equally, you've done the work, so I'm not too fussed about this. If you'd rather stick with it all in this PR, that's fine as well.

The PNG thing I'm not quite clear on how it works with greyscale and palette'd images. Are you able to stick some example files into the data directory, and make tests/main.c output them? Superficially, it all looks good though.

Andre

Hi Andre I just created a seperate PR for the ints (#100). It was in a seperate commit anyway. I'll look through your review points for this PR then.

JulianGmp commented 3 years ago

The PNG thing I'm not quite clear on how it works with greyscale and palette'd images. Are you able to stick some example files into the data directory, and make tests/main.c output them? Superficially, it all looks good though.

I created some example files, you can check their colour mode with the file command or with magick identify..

As for storing them in the PDF: The greyscale images are fairly simple, we only tell the PDF that we have one colour channel and pass /DeviceGray. The image data gets streamed in the same way as with RGB data.

The indexed data requires some more attention. We stream in the image data the same way as with the grey images (one 8-bit channel). The values in the image data refer to colour in the palette. In the PNG stream the palette is stored with the PLTE chunk, each color is a 24-bit RGB value, and its index is derived from its position within the stream. Since we have one byte of index range we can have up 256 colors in the palette, but we may also have less than that (the example file has around 170 iirc).

When streaming the palette into PDF we define the color space as indexed. The indexed value then maps to an actual color space (DeviceRGB, although PDF would technically allow us to also use CMYK for example). The 174 defines the max index (i.e. the number of colors in the palette - 1). After that we write all 175 RGB values in the < > as hex values (trimmed for readability here)

/Name /Image36
/Subtype /Image
/ColorSpace [ /Indexed
  /DeviceRGB
  174
  <FFAA33 88DD33 33FF33 AAFFAA FFFFFF 3355FF AACC33 BBBBFF...... >
]
\Width 128
and so on

The position of the RGB value within the < > also determine its index value, like it does in the PNG PLTE chunk. So we only need to write them in the same order as we read them.

It's important to note that the max index and the number of values need to match. I mistook the value for being the length as opposed to the max index (length-1) and my PDF viewer would refuse to show the image.

JulianGmp commented 3 years ago

I rebased from master and addressed your suggestions.