engine: common: imagelib: add simple decoder/encoder for 8-bit RGB/RGBA PNG images #48
Conversation
nekonomicon
force-pushed
the
png
branch
2 times, most recently
from
cb70e13
to
85f9beb
Compare
engine/common/imagelib/img_png.c
Outdated
| buf_p += sizeof( png_sign ); | ||
|
|
||
| // get IHDR chunk length | ||
| chunk_len = *buf_p++ << 24; |
There was a problem hiding this comment.
This may cause crash on ARM due to unaligned access.
There was a problem hiding this comment.
A note on my prev comment: reading and writing on ARM is possible only if pointer aligned by 32 bits. So moving pointer by one or two bytes will cause a crash.
I think you can read 32-bits and swap it for big endian, if needed. Engine already have these handy macros: https://github.com/FWGS/xash3d-fwgs/blob/master/common/xash3d_types.h#L88
There was a problem hiding this comment.
I need swap bytes on little endian, so this macros useless for me.
There was a problem hiding this comment.
Seems I must use ntohl for little endian,
https://www.w3.org/TR/PNG/#7Integers-and-byte-order
There was a problem hiding this comment.
It's better to use network conversions here, but keep in mind, that we're still not linking to ws32 on Windows yet directly. So it will fail Windows builds.
engine/common/miniz.h
Outdated
| @@ -198,15 +198,15 @@ extern "C" { | |||
| typedef unsigned long mz_ulong; | |||
|
|
|||
| /* mz_free() internally uses the MZ_FREE() macro (which by default calls free() unless you've modified the MZ_MALLOC macro) to release a block allocated from the heap. */ | |||
| void mz_free(void *p); | |||
| static void mz_free(void *p); | |||
There was a problem hiding this comment.
Instead of re-defining them as static, you could define MINIZ_HEADER_FILE_ONLY before including miniz.
miniz implementation already defined in filesystem.c, but if you want, you can move it to dummy file.
There was a problem hiding this comment.
I want to refactor this library anyway.
There was a problem hiding this comment.
I have generated it as a single-header in hope that it will not change, except obvious updates. :)
engine/common/imagelib/img_png.c
Outdated
| buffer = (byte *)Mem_Calloc( host.imagepool, outsize ); | ||
|
|
||
| // write PNG header | ||
| buffer[0] = 0x89; |
There was a problem hiding this comment.
I think, you can memcpy png_sign here, as well as other signatures(IDAT, IHDR) or fill structure and then copy it at once.
|
Given the STB libraries are already utilised in the MainUI project, would it not make sense to use stb_png for this implementation? I used it for PNG reading in my fork and I'm pretty sure it was less complex than this. |
|
STB library in MainUI C++ is optional, used as a last hope when we can't use nor Win32 APIs nor FreeType, as it only roughly renders TrueType font(doesn't have hinting). And it doesn't changed at all. I'm quite unhappy with stb_image and stb_image_write library code, as it bloated(~7.5+1.5k loc) and implements different image formats instead of splitting it to small libraries one per image format. It's not the same as PNG loader. Making stb_image smaller(in lines of code) means forking it and harder updates and testing. That's why I'm also unhappy with @nekonomicon's decision to make changes in miniz library. |
|
I have a bad experience with stb_image. This library has a bad performance=\ |
|
OK, I guess it might be better not to use STB then. Once this goes in I'll replace the PNG implementation in my fork with this instead. |
nekonomicon
force-pushed
the
png
branch
6 times, most recently
from
b75fc7f
to
4c0dc51
Compare
|
I'm OK to merge it. |
No description provided.