From 27044a4fefa9ac4d61b7bfc29040658658253fd6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Sat, 23 Oct 2021 21:20:39 +0200 Subject: [PATCH] Trade: ability to open non-temporary memory. Using openMemory() instead of openData() allows the implementation to assume the data will stay in scope for as long as needed, which can prevent unnecessary copies in some plugin implementations. It warranted a new flag, DataFlag::ExternallyOwned, to describe this kind of memory. I couldn't reuse Owned as that's used for allocations owned by the instance, which is too little for certain future use cases. For example returning *Data instances referencing an Owned memory would mean the user has to assume the memory is gone when the importer instance is gone, and that's generally not true for memory passed to openMemory(). Originally I thought I would do this later, but then realized the existing plugin implementations would need to get all updated again to be aware of the new flag, with some being forgotten, and it's just easier to do the whole thing in a single step. --- doc/changelog.dox | 8 ++- doc/snippets/MagnumTrade.cpp | 4 +- src/Magnum/Trade/AbstractImporter.cpp | 12 ++++ src/Magnum/Trade/AbstractImporter.h | 61 ++++++++++++++----- src/Magnum/Trade/Data.cpp | 2 + src/Magnum/Trade/Data.h | 18 +++++- .../Trade/Test/AbstractImporterTest.cpp | 30 +++++++++ .../TgaImporter/Test/TgaImporterTest.cpp | 46 +++++++++++++- src/MagnumPlugins/TgaImporter/TgaImporter.cpp | 2 +- 9 files changed, 159 insertions(+), 24 deletions(-) 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()};