New plugin: IcoImporter #79
Conversation
Codecov Report
@@ Coverage Diff @@
## master #79 +/- ##
=========================================
+ Coverage 91.64% 91.94% +0.3%
=========================================
Files 76 79 +3
Lines 4606 4543 -63
=========================================
- Hits 4221 4177 -44
+ Misses 385 366 -19
Continue to review full report at Codecov.
|
| BI_ALPHABITFIELDS = 6, | ||
| BI_CMYK = 11, | ||
| BI_CMYKRLE8 = 12, | ||
| BI_CMYKRLE4 = 13 |
There was a problem hiding this comment.
Wait what, BMP can internally use a PNG or JPEG compression? 😱 Cannot unsee. I wanted to skip the BMPs because I knew there was RLE (which I have no interest in learning/implementing), but wasn't aware there's CMYK and all this also.
| imageData = Containers::Array<char>{Containers::NoInit, iconDirEntry.imageDataSize}; | ||
| std::copy(imageDataBegin, imageDataEnd, imageData.begin()); | ||
| } else { | ||
| /* BMP data in ICO files isn't valid :( */ |
There was a problem hiding this comment.
Hmm, Wikipedia said BMP header is omitted in the ICO file and so I thought a dirty hack fairly clean solution could be concatenating a fake header with the data and delegating that to a BmpImporter. But looks like you already got past that.
Unless you really want to suffer through this and finish the BMP loading (I wouldn't, assuming the main use for *.icos would be to have an icon for your own app, meaning you have the format fully under control), I'd say let's strip this down to just PNG loading and make the plugin simply fail with "sorry, not supported" for BMPs.
| _pngImporter = manager()->loadAndInstantiate("PngImporter"); | ||
| } | ||
| CORRADE_ASSERT(_pngImporter, | ||
| "Trade::IcoImporter::image2D(): the correct image importer is unavailable", Containers::NullOpt); |
There was a problem hiding this comment.
I'd make this just an Error{} + return NullOpt, not an assert that takes down the whole app.
Similarly for the other assertions -- "bad data" usually isn't a programmer error, and thus the error handling should be graceful in that case. Ideally with a test case covering each -- for that I think it could be simple enough to craft a bunch of bytes into a const char[] array and pass those into openData(), then checking that opening failed and the expected error message got printed.
also fixed an error message "reserved data incorrect"/"image data out of bounds" errors in an IconDirEntry now skips that entry instead of failing
| # doesn't support dynamic plugins on iOS, this sorta works around that. Should | ||
| # be revisited when updating Travis to newer Xcode (xcode7.3 has CMake 3.6). | ||
| if(NOT BUILD_PLUGINS_STATIC) | ||
| set(ICOIMPORTER_PLUGIN_FILENAME $<TARGET_FILE:IcoImporter>) |
There was a problem hiding this comment.
To make the CI pass also for dynamic builds, you'll need to add this here:
if(WITH_STBIMAGEIMPORTER)
set(STBIMAGEIMPORTER_PLUGIN_FILENAME $<TARGET_FILE:StbImageImporter>)
endif()and then this to Test/configure.h.cmake:
#cmakedefine STBIMAGEIMPORTER_PLUGIN_FILENAME "${STBIMAGEIMPORTER_PLUGIN_FILENAME}"and finally load the plugin in the test, right after loading the IcoImporter:
#ifdef STBIMAGEIMPORTER_PLUGIN_FILENAME
CORRADE_INTERNAL_ASSERT(_manager.load(STBIMAGEIMPORTER_PLUGIN_FILENAME) & PluginManager::LoadState::Loaded);
#endifHope this helps :)
|
Ok, I'm not sure what causes the failure. Tests run on emscripten pass on my machine... except when i use the ci script |
|
I'm getting those too (and only on the CI), somehow the previous emscripten versions don't handle unaligned reads/stores well and the new ones do. That's a minor thing tho, I can look at that when merging. What's left to do here? IIRC it's just removing the commented-out BMP code (or should we keep it?) and adding tests for the (currently uncovered) error handling code paths. |
|
(Finally) merged in 8761c97. I removed the commented-out parts that handle BMP loading and extended the tests a bit to verify also the "file too short" cases. Turns out the DevIlImageImporter can handle BMP ICOs so there's at least something for those (OTOH it crashes with some PNG ICOs and produces bad output for other, heh.) Thanks for pushing this 90% of the way :) |
No description provided.