This is Yaron Cohen-Tal from CEGUI team. Our sample framework fails to run with the Irrlicht renderer due to a bug in the way Irrlicht identifies a jpg file from its data. Attached is a fix.
Thanks for the patch. Do you have some test-images for that?
Also please no STL in Irrlicht, we might switch to STL one day, but so far Irrlicht does not use STL.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I've only tested the image files that come with CEGUI - nothing else. Previously only some of them were correctly identified, now they all are. I've used the signatures from here.
I assume you're refering to my use of "std::memcmp", and u mean the standard C++ library (and not STL - Standard Template Library). "std::memcmp" and "memcmp" are the same function - "std::memcmp" is simply a "link" to "memcmp". I've investigated the matter and it seems like (unfortunately) indeed the prefered way in that case is to "#include <string.h>" and use "memcmp" rather than "#include <cstring>" and use "std::memcmp" (although they're definitely the same function). I'll change that.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Yeah, avoid anything in the std namespace for now.
Can you link me to one of those CEGUI image files? I just always like to have an example to test a patch and you seemed to have one already to have found this bug.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Yeah.. In CEGUI, we first load a resource to memory, then open it with a custom "irr::io::IReadFile" (using "irr::video::IVideoDriver::createImageFromFile (io::IReadFile *file)" that reads from memory. At this point - the extension is lost, and then Irrlicht tries to identify the "file" by its contents, and fails.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Any reason why you check for a specific jfif minor version? (the 01 at the end)
Ah wait - that's the major version. Still... same question for that one :-)
Last edit: Michael Zeilfelder 2016-07-18
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Just explaining why I'm going through this so much. Your patch adds more detailed checks than we had before. So I'm a little worried if it might add this pictures, but we might lose others in the process. I started reading a little about the format and I guess we couldn't handle RAW's so far. But we check now for FF E0 and FF E1 and first doc I read so far says this is applicationn specific (although it rather looks format specific as it's about testinf for JFIF and EXIF). But might there be other En's which maybe get dropped now? I can't tell without investing some time and learning about the jpg format more. So ... did you check that stuff already? In that case I'll just trust you and add it like that. But if you didn't - then I guess I'll have to find some time to learn more about it first.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
It's totally understood. Forgive me for my laziness - I only checked the signatures on Wikipedia (which, as we all know, is 100% accurate ;) ), and I prolly wouldn't accept such a submission to CEGUI myself.. Anyway I've now checked further and there seem to be many JPEG formats, more than the 3 I check for. However, it seems like JPEG always starts with hex "FF D8" (SOI - Start of Image) signature, so I suggest to simply only check for that. I've checked Qt's source code and that's exactly what they check. Indeed it's a short signature, but I guess there aren't other image formats that start with it. It would be simpler if I had the ISO standards, but unless you're willing to pay them 198 CHF I guess we'll have to do without them.. I say if it's good enough for Qt, it should b good enough for us..
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I did use FF D8 FF now - next block has to start again with FF it seems and we got one more byte at least. Checked to svn r5325 and r5326 for 1.8 branch and main branch (trunk). Thought we just released Irrlicht 1.8 version days before this report, so might take quite a while until it get's into an official release (sorry, but building releases always takes a weekend so I'm only doing it when a few things have collected).
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Thanks for the patch. Do you have some test-images for that?
Also please no STL in Irrlicht, we might switch to STL one day, but so far Irrlicht does not use STL.
I've only tested the image files that come with CEGUI - nothing else. Previously only some of them were correctly identified, now they all are. I've used the signatures from here.
I assume you're refering to my use of "std::memcmp", and u mean the standard C++ library (and not STL - Standard Template Library). "std::memcmp" and "memcmp" are the same function - "std::memcmp" is simply a "link" to "memcmp". I've investigated the matter and it seems like (unfortunately) indeed the prefered way in that case is to "#include <string.h>" and use "memcmp" rather than "#include <cstring>" and use "std::memcmp" (although they're definitely the same function). I'll change that.
Yeah, avoid anything in the std namespace for now.
Can you link me to one of those CEGUI image files? I just always like to have an example to test a patch and you seemed to have one already to have found this bug.
Attached is the new patch version.
Here is an example of a jpeg image that wasn't identified as jpeg and now it is.
Thanks! I didn't get to it this weekend, but will add it next days.
Hm, that jpg seems to load fine with existing Irrlicht (1.8).
Ok, I think I got it. Normal loading it only checks file extension and it already finds that one.
Yeah.. In CEGUI, we first load a resource to memory, then open it with a custom "irr::io::IReadFile" (using "irr::video::IVideoDriver::createImageFromFile (io::IReadFile *file)" that reads from memory. At this point - the extension is lost, and then Irrlicht tries to identify the "file" by its contents, and fails.
Any reason why you check for a specific jfif minor version? (the 01 at the end)
Ah wait - that's the major version. Still... same question for that one :-)
Last edit: Michael Zeilfelder 2016-07-18
Just explaining why I'm going through this so much. Your patch adds more detailed checks than we had before. So I'm a little worried if it might add this pictures, but we might lose others in the process. I started reading a little about the format and I guess we couldn't handle RAW's so far. But we check now for FF E0 and FF E1 and first doc I read so far says this is applicationn specific (although it rather looks format specific as it's about testinf for JFIF and EXIF). But might there be other En's which maybe get dropped now? I can't tell without investing some time and learning about the jpg format more. So ... did you check that stuff already? In that case I'll just trust you and add it like that. But if you didn't - then I guess I'll have to find some time to learn more about it first.
It's totally understood. Forgive me for my laziness - I only checked the signatures on Wikipedia (which, as we all know, is 100% accurate ;) ), and I prolly wouldn't accept such a submission to CEGUI myself.. Anyway I've now checked further and there seem to be many JPEG formats, more than the 3 I check for. However, it seems like JPEG always starts with hex "FF D8" (SOI - Start of Image) signature, so I suggest to simply only check for that. I've checked Qt's source code and that's exactly what they check. Indeed it's a short signature, but I guess there aren't other image formats that start with it. It would be simpler if I had the ISO standards, but unless you're willing to pay them 198 CHF I guess we'll have to do without them.. I say if it's good enough for Qt, it should b good enough for us..
I did use FF D8 FF now - next block has to start again with FF it seems and we got one more byte at least. Checked to svn r5325 and r5326 for 1.8 branch and main branch (trunk). Thought we just released Irrlicht 1.8 version days before this report, so might take quite a while until it get's into an official release (sorry, but building releases always takes a weekend so I'm only doing it when a few things have collected).
Thanx!