diff --git a/doc/changelog.dox b/doc/changelog.dox index 8179ab6ae..7b27561ac 100644 --- a/doc/changelog.dox +++ b/doc/changelog.dox @@ -228,6 +228,9 @@ See also: @relativeref{Trade::AbstractImporter,clearFlags()} convenience helpers that are encouraged over @relativeref{Trade::AbstractImporter,setFlags()} as it avoid accidentally clearing default flags potentially added in the future. +- New @ref Trade::AbstractImporter::openMemory() function for passing + non-temporary memory to the importer, allowing the implementations to avoid + allocating an internal copy - Ability to convert also 1D and 3D images with the @ref magnum-imageconverter "magnum-imageconverter" utility, as well as combining layers into images of one dimension more (or vice versa) and @@ -373,8 +376,9 @@ See also: @subsubsection changelog-latest-changes-trade Trade library - A changed signature of the @ref Trade::AbstractImporter::doOpenData(Containers::Array&&, DataFlags) - function allows importers to take over the ownership of passed data instead - of allocating a local copy, saving as much as half memory in certain + function and a new @ref Trade::DataFlag::ExternallyOwned flag that allows + importers to reason about ownership of passed data instead of being forced + to allocate a local copy, saving as much as half memory in certain importer implementations - @ref Trade::AbstractImageConverter::doConvertToFile() and @ref Trade::AbstractSceneConverter::doConvertToFile() are now diff --git a/doc/snippets/MagnumTrade.cpp b/doc/snippets/MagnumTrade.cpp index 6abc866f6..5627ebc29 100644 --- a/doc/snippets/MagnumTrade.cpp +++ b/doc/snippets/MagnumTrade.cpp @@ -140,7 +140,7 @@ Containers::Pointer importer; /* [AbstractImporter-usage-data] */ Utility::Resource rs{"data"}; Containers::ArrayView data = rs.getRaw("image.png"); -if(!importer->openData(data)) +if(!importer->openData(data)) /* or openMemory() */ Fatal{} << "Can't open image data with AnyImageImporter"; // import & use the image like above ... @@ -229,7 +229,7 @@ struct: Trade::AbstractImporter { void doOpenData(Containers::Array&& data, Trade::DataFlags dataFlags) override { /* Take over the existing array or copy the data if we can't */ - if(dataFlags & Trade::DataFlag::Owned) { + if(dataFlags & (Trade::DataFlag::Owned|Trade::DataFlag::ExternallyOwned)) { _in = std::move(data); } else { _in = Containers::Array{NoInit, data.size()}; diff --git a/src/Magnum/Trade/AbstractImporter.cpp b/src/Magnum/Trade/AbstractImporter.cpp index ec82f5e53..fb6a96c08 100644 --- a/src/Magnum/Trade/AbstractImporter.cpp +++ b/src/Magnum/Trade/AbstractImporter.cpp @@ -152,6 +152,18 @@ void AbstractImporter::doOpenData(Containers::Array&& data, const DataFlag #endif } +bool AbstractImporter::openMemory(Containers::ArrayView memory) { + CORRADE_ASSERT(features() & ImporterFeature::OpenData, + "Trade::AbstractImporter::openMemory(): feature not supported", {}); + + /* We accept empty data here (instead of checking for them and failing so + the check doesn't be done on the plugin side) because for some file + formats it could be valid (e.g. OBJ or JSON-based formats). */ + close(); + doOpenData(Containers::Array{const_cast(static_cast(memory.data())), memory.size(), Implementation::nonOwnedArrayDeleter}, DataFlag::ExternallyOwned); + return isOpened(); +} + bool AbstractImporter::openState(const void* state, const std::string& filePath) { CORRADE_ASSERT(features() & ImporterFeature::OpenState, "Trade::AbstractImporter::openState(): feature not supported", {}); diff --git a/src/Magnum/Trade/AbstractImporter.h b/src/Magnum/Trade/AbstractImporter.h index a34628e83..76345467c 100644 --- a/src/Magnum/Trade/AbstractImporter.h +++ b/src/Magnum/Trade/AbstractImporter.h @@ -49,7 +49,11 @@ namespace Magnum { namespace Trade { @see @ref ImporterFeatures, @ref AbstractImporter::features() */ enum class ImporterFeature: UnsignedByte { - /** Opening files from raw data using @ref AbstractImporter::openData() */ + /** + * Opening files from raw data or non-temporary memory using + * @ref AbstractImporter::openData() or + * @relativeref{AbstractImporter,openMemory()} + */ OpenData = 1 << 0, /** Opening already loaded state using @ref AbstractImporter::openState() */ @@ -199,9 +203,9 @@ expected to always check the count before attempting an import. Importers are commonly implemented as plugins, which means the concrete importer implementation is loaded and instantiated through a @relativeref{Corrade,PluginManager::Manager}. A file is opened using either -@ref openFile(), @ref openData() or, in rare cases, @ref openState() amd it -stays open until the importer is destroyed, @ref close() is called or another -file is opened. +@ref openFile(), @ref openData() / @ref openMemory() or, in rare cases, +@ref openState(). Then it stays open until the importer is destroyed, +@ref close() is called or another file is opened. With a file open you can then query the importer for particular data. Where possible, the import is performed lazily only when you actually request that @@ -236,9 +240,9 @@ importer plugins. @subsection Trade-AbstractImporter-usage-callbacks Loading data from memory, using file callbacks Besides loading data directly from the filesystem using @ref openFile() like -shown above, it's possible to use @ref openData() to import data from memory -(for example from @relativeref{Corrade,Utility::Resource}). Note that the -particular importer implementation has to support +shown above, it's possible to use @ref openData() / @ref openMemory() to import +data from memory (for example from @relativeref{Corrade,Utility::Resource}). +Note that the particular importer implementation has to support @ref ImporterFeature::OpenData for this method to work: @snippet MagnumTrade.cpp AbstractImporter-usage-data @@ -629,12 +633,32 @@ class MAGNUM_TRADE_EXPORT AbstractImporter: public PluginManager::AbstractManagi * * Closes previous file, if it was opened, and tries to open given raw * data. Available only if @ref ImporterFeature::OpenData is supported. - * Returns @cpp true @ce on success, @cpp false @ce otherwise. The - * @p data is not expected to be alive after the function exits. - * @see @ref features(), @ref openFile() + * Returns @cpp true @ce on success, @cpp false @ce otherwise. + * + * The @p data is not expected to be alive after the function exits. + * Using @ref openMemory() instead as can avoid unnecessary copies in + * exchange for stricter requirements on @p data lifetime. + * @see @ref features(), @ref openFile(), @ref openState() */ bool openData(Containers::ArrayView data); + /** + * @brief Open a non-temporary memory + * @m_since_latest + * + * Closes previous file, if it was opened, and tries to open given raw + * data. Available only if @ref ImporterFeature::OpenData is supported. + * Returns @cpp true @ce on success, @cpp false @ce otherwise. + * + * Unlike @ref openData(), this function expects @p memory to stay in + * scope until the importer is destructed, @ref close() is called or + * another file is opened. This allows the implementation to directly + * operate on the provided memory, without having to allocate a local + * copy to extend its lifetime. + * @see @ref features(), @ref openFile(), @ref openState() + */ + bool openMemory(Containers::ArrayView memory); + /** * @brief Open already loaded state * @param state Pointer to importer-specific state @@ -648,7 +672,8 @@ class MAGNUM_TRADE_EXPORT AbstractImporter: public PluginManager::AbstractManagi * * See documentation of a particular plugin for more information about * type and contents of the @p state parameter. - * @see @ref features(), @ref openData() + * @see @ref features(), @ref openData(), @ref openMemory(), + * @ref openFile() */ bool openState(const void* state, const std::string& filePath = {}); @@ -662,7 +687,8 @@ class MAGNUM_TRADE_EXPORT AbstractImporter: public PluginManager::AbstractManagi * callback to load the file and passes the memory view to * @ref openData() instead. See @ref setFileCallback() for more * information. - * @see @ref features(), @ref openData() + * @see @ref features(), @ref openData(), @ref openMemory(), + * @ref openState() */ bool openFile(const std::string& filename); @@ -1637,7 +1663,7 @@ class MAGNUM_TRADE_EXPORT AbstractImporter: public PluginManager::AbstractManagi virtual bool doIsOpened() const = 0; /** - * @brief Implementation for @ref openData() + * @brief Implementation for @ref openData() and @ref openMemory() * @m_since_latest * * The @p data is mutable or owned depending on the value of @@ -1657,6 +1683,10 @@ class MAGNUM_TRADE_EXPORT AbstractImporter: public PluginManager::AbstractManagi * newly allocated array and passes it to this function. You can * take ownership of the @p data instance instead of allocating a * local copy. + * - If @p dataFlags is @ref DataFlag::ExternallyOwned, it can be + * assumed that @p data will stay in scope until @ref doClose() is + * called or the importer is destructed. This happens when the + * function is called from @ref openMemory(). * * Example workflow in a plugin that needs to preserve access to the * input data but wants to avoid allocating a copy if possible: @@ -1664,8 +1694,9 @@ class MAGNUM_TRADE_EXPORT AbstractImporter: public PluginManager::AbstractManagi * @snippet MagnumTrade.cpp AbstractImporter-doOpenData-ownership * * The @p dataFlags can never be @ref DataFlag::Mutable without - * @ref DataFlag::Owned. The case of @ref DataFlag::Owned without - * @ref DataFlag::Mutable is currently unused but reserved for future. + * any other flag set. The case of @ref DataFlag::Mutable with + * @ref DataFlag::ExternallyOwned is currently unused but reserved for + * future. */ virtual void doOpenData(Containers::Array&& data, DataFlags dataFlags); diff --git a/src/Magnum/Trade/Data.cpp b/src/Magnum/Trade/Data.cpp index 5317e37e8..65f6ab3ff 100644 --- a/src/Magnum/Trade/Data.cpp +++ b/src/Magnum/Trade/Data.cpp @@ -36,6 +36,7 @@ Debug& operator<<(Debug& debug, const DataFlag value) { /* LCOV_EXCL_START */ #define _c(v) case DataFlag::v: return debug << "::" #v; _c(Owned) + _c(ExternallyOwned) _c(Mutable) #undef _c /* LCOV_EXCL_STOP */ @@ -47,6 +48,7 @@ Debug& operator<<(Debug& debug, const DataFlag value) { Debug& operator<<(Debug& debug, const DataFlags value) { return Containers::enumSetDebugOutput(debug, value, "Trade::DataFlags{}", { DataFlag::Owned, + DataFlag::ExternallyOwned, DataFlag::Mutable}); } diff --git a/src/Magnum/Trade/Data.h b/src/Magnum/Trade/Data.h index b6314967c..3a4519c5a 100644 --- a/src/Magnum/Trade/Data.h +++ b/src/Magnum/Trade/Data.h @@ -51,12 +51,24 @@ importer itself. */ enum class DataFlag: UnsignedByte { /** - * Data is owned by the instance. If this flag is not set, the instance - * might be for example referencing a memory-mapped file or a constant - * memory. + * Data is owned by the instance, meaning it stays in scope for as long as + * the instance. If neither this flag nor @ref DataFlag::ExternallyOwned is + * set, the data is considered to be just a temporary allocation and no + * assumptions about its lifetime can be made. */ Owned = 1 << 0, + /** + * Data has an owner external to the instance, for example a memory-mapped + * file or a constant memory. In general the data lifetime exceeds lifetime + * of the instance wrapping it. If neither this flag nor + * @ref DataFlag::ExternallyOwned is set, the data is considered to be just + * a temporary allocation and no assumptions about its lifetime can be + * made. + * @m_since_latest + */ + ExternallyOwned = 3 << 0, + /** * Data is mutable. If this flag is not set, the instance might be for * example referencing a readonly memory-mapped file or a constant memory. diff --git a/src/Magnum/Trade/Test/AbstractImporterTest.cpp b/src/Magnum/Trade/Test/AbstractImporterTest.cpp index e74781611..d8277a8fa 100644 --- a/src/Magnum/Trade/Test/AbstractImporterTest.cpp +++ b/src/Magnum/Trade/Test/AbstractImporterTest.cpp @@ -73,6 +73,7 @@ struct AbstractImporterTest: TestSuite::Tester { #ifdef MAGNUM_BUILD_DEPRECATED void openDataDeprecatedFallback(); #endif + void openMemory(); void openFileAsData(); void openFileAsDataNotFound(); @@ -314,6 +315,7 @@ AbstractImporterTest::AbstractImporterTest() { #ifdef MAGNUM_BUILD_DEPRECATED &AbstractImporterTest::openDataDeprecatedFallback, #endif + &AbstractImporterTest::openMemory, &AbstractImporterTest::openFileAsData, &AbstractImporterTest::openFileAsDataNotFound, @@ -687,6 +689,34 @@ void AbstractImporterTest::openDataDeprecatedFallback() { } #endif +void AbstractImporterTest::openMemory() { + struct: AbstractImporter { + ImporterFeatures doFeatures() const override { return ImporterFeature::OpenData; } + bool doIsOpened() const override { return _opened; } + void doClose() override { _opened = false; } + + void doOpenData(Containers::Array&& data, DataFlags dataFlags) override { + CORRADE_COMPARE_AS(data, + Containers::arrayView({'\xa5'}), + TestSuite::Compare::Container); + CORRADE_COMPARE(dataFlags, DataFlag::ExternallyOwned); + /* The array should have a custom no-op deleter */ + CORRADE_VERIFY(data.deleter()); + _opened = true; + } + + bool _opened = false; + } importer; + + CORRADE_VERIFY(!importer.isOpened()); + const char a5 = '\xa5'; + CORRADE_VERIFY(importer.openMemory({&a5, 1})); + CORRADE_VERIFY(importer.isOpened()); + + importer.close(); + CORRADE_VERIFY(!importer.isOpened()); +} + void AbstractImporterTest::openFileAsData() { struct: AbstractImporter { ImporterFeatures doFeatures() const override { return ImporterFeature::OpenData; } diff --git a/src/MagnumPlugins/TgaImporter/Test/TgaImporterTest.cpp b/src/MagnumPlugins/TgaImporter/Test/TgaImporterTest.cpp index fcca130f8..748ad659a 100644 --- a/src/MagnumPlugins/TgaImporter/Test/TgaImporterTest.cpp +++ b/src/MagnumPlugins/TgaImporter/Test/TgaImporterTest.cpp @@ -28,6 +28,7 @@ #include #include #include +#include #include #include #include @@ -59,7 +60,7 @@ struct TgaImporterTest: TestSuite::Tester { void rleTooLarge(); - /* no openData() as all tests use openData() and openFile() is used below */ + void openMemory(); void openTwice(); void importTwice(); @@ -124,6 +125,22 @@ const struct { "RLE file too short at pixel 0"} }; +/* Shared among all plugins that implement data copying optimizations */ +const struct { + const char* name; + bool(*open)(AbstractImporter&, Containers::ArrayView); +} OpenMemoryData[]{ + {"data", [](AbstractImporter& importer, Containers::ArrayView data) { + /* Copy to ensure the original memory isn't referenced */ + Containers::Array copy{NoInit, data.size()}; + Utility::copy(Containers::arrayCast(data), copy); + return importer.openData(copy); + }}, + {"memory", [](AbstractImporter& importer, Containers::ArrayView data) { + return importer.openMemory(data); + }}, +}; + TgaImporterTest::TgaImporterTest() { addTests({&TgaImporterTest::openEmpty}); @@ -149,6 +166,9 @@ TgaImporterTest::TgaImporterTest() { &TgaImporterTest::rleTooLarge}); + addInstancedTests({&TgaImporterTest::openMemory}, + Containers::arraySize(OpenMemoryData)); + addTests({&TgaImporterTest::openTwice, &TgaImporterTest::importTwice}); @@ -410,6 +430,30 @@ void TgaImporterTest::rleTooLarge() { CORRADE_COMPARE(out.str(), "Trade::TgaImporter::image2D(): RLE data larger than advertised Vector(2, 3) pixels at byte 28\n"); } +void TgaImporterTest::openMemory() { + /* same as dxt1() except that it uses openData() & openMemory() to test + data copying on import */ + + auto&& data = OpenMemoryData[testCaseInstanceId()]; + setTestCaseDescription(data.name); + + Containers::Pointer importer = _manager.instantiate("TgaImporter"); + /* Eh fuck off, GCC, why can't you convert the array to an ArrayView on + your own if it's passed to a function pointer?! Clang can. */ + CORRADE_VERIFY(data.open(*importer, Containers::arrayView(Color24))); + + Containers::Optional image = importer->image2D(0); + CORRADE_VERIFY(image); + CORRADE_COMPARE(image->storage().alignment(), 1); + CORRADE_COMPARE(image->format(), PixelFormat::RGB8Unorm); + CORRADE_COMPARE(image->size(), Vector2i(2, 3)); + CORRADE_COMPARE_AS(image->data(), Containers::arrayView({ + 3, 2, 1, 4, 3, 2, + 5, 4, 3, 6, 5, 4, + 7, 6, 5, 8, 7, 6 + }), TestSuite::Compare::Container); +} + void TgaImporterTest::openTwice() { Containers::Pointer importer = _manager.instantiate("TgaImporter"); diff --git a/src/MagnumPlugins/TgaImporter/TgaImporter.cpp b/src/MagnumPlugins/TgaImporter/TgaImporter.cpp index 2f038fee9..84d3e2341 100644 --- a/src/MagnumPlugins/TgaImporter/TgaImporter.cpp +++ b/src/MagnumPlugins/TgaImporter/TgaImporter.cpp @@ -64,7 +64,7 @@ void TgaImporter::doOpenData(Containers::Array&& data, const DataFlags dat } /* Ttake over the existing array or copy the data if we can't */ - if(dataFlags & DataFlag::Owned) { + if(dataFlags & (DataFlag::Owned|DataFlag::ExternallyOwned)) { _in = std::move(data); } else { _in = Containers::Array{NoInit, data.size()};