Support rgb files #8
Conversation
Support for RGB only (as opposed to RGBA) images is desired. This provides a baseline to test support against. The assumption is that when loading an RGB image it will be converted to the internal RGBA format. This provides a good baseline of support without needing to change other parts of the module. In the future the color type (RGB vs RGBA) may be supported directly and preserved with a round-trip, however that is beyond the scope of initial support.
| for i in range(0, len(rgb), 3): | ||
| rgba += rgb[i:i+3] | ||
| rgba.append(255) | ||
| assert len(rgba) == len(rgb) / 3 * 4 |
There was a problem hiding this comment.
I'd rather not use assert in production code.
There was a problem hiding this comment.
What exactly is the concern with using assert? I can remove it, but I don't think the performance advantage of removing it is really worth it. I think it adds some useful documentation, and helps to make sure any modifications to the function internals don't break stuff.
But if it is a big concern I can just remove it.
|
Would you mind making the changes I suggested? |
1. Removed unwanted assert 2. Added numeric constants for PNG color types. 3. Added data structure for supported types. 4. Slight rework of main loading loop to make logic clearer.
| step_size = 1 + row_size | ||
| bytes_per_pixel = SAMPLES_PER_PIXEL[color_type] | ||
| bytes_per_row = bytes_per_pixel * width | ||
| bytes_per_rgba_row = SAMPLES_PER_PIXEL[COLOR_TYPE_TRUECOLOR_WITH_ALPHA] * width |
There was a problem hiding this comment.
Oh, now I understand why you need both. I missed a few things in the previous version. You may want to add a comment here saying "We need the RGBA length regardless since we cast RGB to RGBA".
|
I think this looks good enough so I'm gonna merge. You can do any improvements based on my suggestions in a follow-up. Thank you very much for all your efforts! |
This supports loading RGB files (rather than just RGBA files).
It isn't great. It converts things to RGBA internally, rather than tracking the format type. Additionally, it is kind of slow as the rgb2rgba isn't exactly fast.
Despite these limitations it does provide functionality that at least one user (me!) needs, and shouldn't impact anyone not using the feature.
There is a relatively simple test accompanying the changes.