From 116d327a6426a5b2bf811535cd9a366117ead01c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Wed, 20 Oct 2021 17:00:23 +0200 Subject: [PATCH] Trade: AbstractImporter::doOpenData() now takes a rvalue Array. This allows to better describe memory ownership and transfer it instead of forcing the plugins to allocate their own local copy if the import happens in-place on the imported data. Right now that's mainly for the openFile() use case, which implicitly allocated an Array with file contents only to pass it to openData() which then made a copy because it could not make any assumption about data scope. In other words, certain plugins (TgaImporter, KtxImporter, DdsImporter, CgltfImporter and possibly others) will now have their peak memory usage *halved*. --- doc/changelog.dox | 7 +++ doc/snippets/MagnumTrade.cpp | 26 +++++++++ src/Magnum/Trade/AbstractImporter.cpp | 29 ++++++++-- src/Magnum/Trade/AbstractImporter.h | 43 ++++++++++++++- src/Magnum/Trade/Data.h | 5 +- .../Trade/Test/AbstractImporterTest.cpp | 53 +++++++++++++++++-- 6 files changed, 150 insertions(+), 13 deletions(-) diff --git a/doc/changelog.dox b/doc/changelog.dox index f9eef268f..5840aa5d2 100644 --- a/doc/changelog.dox +++ b/doc/changelog.dox @@ -372,6 +372,10 @@ 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 + importer implementations - @ref Trade::AbstractImageConverter::doConvertToFile() and @ref Trade::AbstractSceneConverter::doConvertToFile() are now @cpp protected @ce instead of @cpp private @ce to allow calling them from @@ -621,6 +625,9 @@ See also: @ref MAGNUM_BUILD_DEPRECATED is enabled, the returned type acts as a @ref Corrade::Containers::Optional but has (deprecated) implicit conversion to a @ref Corrade::Containers::Pointer to avoid breaking user code. +- @ref Trade::AbstractImporter::doOpenData() taking an array view is + deprecated in favor of the more flexible signature that takes a r-value + @relativeref{Corrade,Containers::Array} - @cpp Trade::ImageConverterFeature::ConvertImage @ce and @cpp Trade::ImageConverterFeature::ConvertCompressedImage @ce; @cpp Trade::AbstractImageConverter::exportToImage() @ce and diff --git a/doc/snippets/MagnumTrade.cpp b/doc/snippets/MagnumTrade.cpp index 33750cbd6..6abc866f6 100644 --- a/doc/snippets/MagnumTrade.cpp +++ b/doc/snippets/MagnumTrade.cpp @@ -25,6 +25,7 @@ #include #include +#include #include #include @@ -216,6 +217,31 @@ importer->setFileCallback([](const std::string& filename, /* [AbstractImporter-setFileCallback-template] */ } +{ +struct: Trade::AbstractImporter { + Trade::ImporterFeatures doFeatures() const override { return {}; } + bool doIsOpened() const override { return false; } + void doClose() override {} + + Containers::Array _in; + +/* [AbstractImporter-doOpenData-ownership] */ +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) { + _in = std::move(data); + } else { + _in = Containers::Array{NoInit, data.size()}; + Utility::copy(data, _in); + } + + DOXYGEN_IGNORE() +} +/* [AbstractImporter-doOpenData-ownership] */ +} importer; +} + { /* [AbstractSceneConverter-usage-file] */ PluginManager::Manager manager; diff --git a/src/Magnum/Trade/AbstractImporter.cpp b/src/Magnum/Trade/AbstractImporter.cpp index f6573ab51..177944e4d 100644 --- a/src/Magnum/Trade/AbstractImporter.cpp +++ b/src/Magnum/Trade/AbstractImporter.cpp @@ -131,13 +131,26 @@ bool AbstractImporter::openData(Containers::ArrayView data) { 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(data); + doOpenData(Containers::Array{const_cast(data.data()), data.size(), Implementation::nonOwnedArrayDeleter}, {}); return isOpened(); } +#ifdef MAGNUM_BUILD_DEPRECATED void AbstractImporter::doOpenData(Containers::ArrayView) { CORRADE_ASSERT_UNREACHABLE("Trade::AbstractImporter::openData(): feature advertised but not implemented", ); } +#endif + +void AbstractImporter::doOpenData(Containers::Array&& data, const DataFlags) { + #ifndef MAGNUM_BUILD_DEPRECATED + CORRADE_ASSERT_UNREACHABLE("Trade::AbstractImporter::openData(): feature advertised but not implemented", ); + static_cast(data); + #else + CORRADE_IGNORE_DEPRECATED_PUSH + doOpenData(data); + CORRADE_IGNORE_DEPRECATED_POP + #endif +} bool AbstractImporter::openState(const void* state, const std::string& filePath) { CORRADE_ASSERT(features() & ImporterFeature::OpenState, @@ -179,7 +192,13 @@ bool AbstractImporter::openFile(const std::string& filename) { Error() << "Trade::AbstractImporter::openFile(): cannot open file" << filename; return isOpened(); } - doOpenData(*data); + /** @todo it might be useful to use LoadPermanent and DataFlag::Owned + here, but it kinda depends on the plugin if it wants to keep a copy + of the data... which means it's probably the most straightforward + if we just provide an explicit doOpenFile() implementation there. + + Same in doOpenFile() below. */ + doOpenData(Containers::Array{const_cast(data->data()), data->size(), Implementation::nonOwnedArrayDeleter}, {}); _fileCallback(filename, InputFileCallbackPolicy::Close, _fileCallbackUserData); /* Shouldn't get here, the assert is fired already in setFileCallback() */ @@ -192,14 +211,14 @@ void AbstractImporter::doOpenFile(const std::string& filename) { CORRADE_ASSERT(features() & ImporterFeature::OpenData, "Trade::AbstractImporter::openFile(): not implemented", ); /* If callbacks are set, use them. This is the same implementation as in - openFile(), see the comment there for details. */ + openFile(), see the comments there for details. */ if(_fileCallback) { const Containers::Optional> data = _fileCallback(filename, InputFileCallbackPolicy::LoadTemporary, _fileCallbackUserData); if(!data) { Error() << "Trade::AbstractImporter::openFile(): cannot open file" << filename; return; } - doOpenData(*data); + doOpenData(Containers::Array{const_cast(data->data()), data->size(), Implementation::nonOwnedArrayDeleter}, {}); _fileCallback(filename, InputFileCallbackPolicy::Close, _fileCallbackUserData); /* Otherwise open the file directly */ @@ -209,7 +228,7 @@ void AbstractImporter::doOpenFile(const std::string& filename) { return; } - doOpenData(Utility::Directory::read(filename)); + doOpenData(Utility::Directory::read(filename), DataFlag::Owned|DataFlag::Mutable); } } diff --git a/src/Magnum/Trade/AbstractImporter.h b/src/Magnum/Trade/AbstractImporter.h index 586244403..cd2690a00 100644 --- a/src/Magnum/Trade/AbstractImporter.h +++ b/src/Magnum/Trade/AbstractImporter.h @@ -1636,8 +1636,47 @@ class MAGNUM_TRADE_EXPORT AbstractImporter: public PluginManager::AbstractManagi /** @brief Implementation for @ref isOpened() */ virtual bool doIsOpened() const = 0; - /** @brief Implementation for @ref openData() */ - virtual void doOpenData(Containers::ArrayView data); + /** + * @brief Implementation for @ref openData() + * @m_since_latest + * + * The @p data is mutable or owned depending on the value of + * @p dataFlags. This can be used for example to avoid allocating a + * local copy in order to preserve its lifetime. The following cases + * are possible: + * + * - If @p dataFlags is empty, @p data is a @cpp const @ce + * non-owning view. This happens when the function is called from + * @ref openData(). You have to assume the data go out of scope + * after the function exists, so if the importer needs to access + * the data after, it has to allocate a local copy. + * - If @p dataFlags is @ref DataFlag::Owned and + * @ref DataFlag::Mutable, @p data is an owned memory. This + * happens when the implementation is called from the default + * @ref doOpenFile() implementation --- it reads a file into a + * newly allocated array and passes it to this function. You can + * take ownership of the @p data instance instead of allocating a + * local copy. + * + * Example workflow in a plugin that needs to preserve access to the + * input data but wants to avoid allocating a copy if possible: + * + * @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. + */ + virtual void doOpenData(Containers::Array&& data, DataFlags dataFlags); + + #ifdef MAGNUM_BUILD_DEPRECATED + /** + * @brief Implementation for @ref openData() + * @m_deprecated_since_latest Implement @ref doOpenData(Containers::Array&&, DataFlags) + * instead. + */ + virtual CORRADE_DEPRECATED("implement doOpenData(Containers::Array&&, DataFlags) instead") void doOpenData(Containers::ArrayView data); + #endif /** @brief Implementation for @ref openState() */ virtual void doOpenState(const void* state, const std::string& filePath); diff --git a/src/Magnum/Trade/Data.h b/src/Magnum/Trade/Data.h index cd9a171fc..b6314967c 100644 --- a/src/Magnum/Trade/Data.h +++ b/src/Magnum/Trade/Data.h @@ -41,10 +41,13 @@ namespace Magnum { namespace Trade { @brief Data flag @m_since{2020,06} +Used to describe data contained in various classes returned from +@ref AbstractImporter interfaces and also data passed internally in the +importer itself. @see @ref DataFlags, @ref AnimationData::dataFlags(), @ref ImageData::dataFlags(), @ref MaterialData::attributeDataFlags(), @ref MaterialData::layerDataFlags(), @ref MeshData::indexDataFlags(), - @ref MeshData::vertexDataFlags() + @ref MeshData::vertexDataFlags(), @ref AbstractImporter::doOpenData() */ enum class DataFlag: UnsignedByte { /** diff --git a/src/Magnum/Trade/Test/AbstractImporterTest.cpp b/src/Magnum/Trade/Test/AbstractImporterTest.cpp index 77eed6768..e74781611 100644 --- a/src/Magnum/Trade/Test/AbstractImporterTest.cpp +++ b/src/Magnum/Trade/Test/AbstractImporterTest.cpp @@ -70,6 +70,9 @@ struct AbstractImporterTest: TestSuite::Tester { void setFlagsNotImplemented(); void openData(); + #ifdef MAGNUM_BUILD_DEPRECATED + void openDataDeprecatedFallback(); + #endif void openFileAsData(); void openFileAsDataNotFound(); @@ -308,6 +311,9 @@ AbstractImporterTest::AbstractImporterTest() { &AbstractImporterTest::setFlagsNotImplemented, &AbstractImporterTest::openData, + #ifdef MAGNUM_BUILD_DEPRECATED + &AbstractImporterTest::openDataDeprecatedFallback, + #endif &AbstractImporterTest::openFileAsData, &AbstractImporterTest::openFileAsDataNotFound, @@ -630,12 +636,43 @@ void AbstractImporterTest::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, DataFlags{}); + /* 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.openData({&a5, 1})); + CORRADE_VERIFY(importer.isOpened()); + + importer.close(); + CORRADE_VERIFY(!importer.isOpened()); +} + +#ifdef MAGNUM_BUILD_DEPRECATED +void AbstractImporterTest::openDataDeprecatedFallback() { + struct: AbstractImporter { + ImporterFeatures doFeatures() const override { return ImporterFeature::OpenData; } + bool doIsOpened() const override { return _opened; } + void doClose() override { _opened = false; } + + CORRADE_IGNORE_DEPRECATED_PUSH void doOpenData(Containers::ArrayView data) override { CORRADE_COMPARE_AS(data, Containers::arrayView({'\xa5'}), TestSuite::Compare::Container); _opened = true; } + CORRADE_IGNORE_DEPRECATED_POP bool _opened = false; } importer; @@ -648,6 +685,7 @@ void AbstractImporterTest::openData() { importer.close(); CORRADE_VERIFY(!importer.isOpened()); } +#endif void AbstractImporterTest::openFileAsData() { struct: AbstractImporter { @@ -655,10 +693,13 @@ void AbstractImporterTest::openFileAsData() { bool doIsOpened() const override { return _opened; } void doClose() override { _opened = false; } - void doOpenData(Containers::ArrayView data) override { + void doOpenData(Containers::Array&& data, DataFlags dataFlags) override { CORRADE_COMPARE_AS(data, Containers::arrayView({'\xa5'}), TestSuite::Compare::Container); + CORRADE_COMPARE(dataFlags, DataFlag::Owned|DataFlag::Mutable); + /* I.e., we can take over the array, it's not just a view */ + CORRADE_VERIFY(!data.deleter()); _opened = true; } @@ -680,7 +721,7 @@ void AbstractImporterTest::openFileAsDataNotFound() { bool doIsOpened() const override { return _opened; } void doClose() override { _opened = false; } - void doOpenData(Containers::ArrayView) override { + void doOpenData(Containers::Array&&, DataFlags) override { _opened = true; } @@ -948,7 +989,7 @@ void AbstractImporterTest::setFileCallbackOpenFileDirectly() { _opened = true; } - void doOpenData(Containers::ArrayView) override { + void doOpenData(Containers::Array&&, DataFlags) override { /* Shouldn't be called because FileCallback is supported */ openDataCalledNotSureWhy = true; } @@ -982,10 +1023,11 @@ void AbstractImporterTest::setFileCallbackOpenFileThroughBaseImplementation() { AbstractImporter::doOpenFile(filename); } - void doOpenData(Containers::ArrayView data) override { + void doOpenData(Containers::Array&& data, DataFlags dataFlags) override { CORRADE_COMPARE_AS(data, Containers::arrayView({'\xb0'}), TestSuite::Compare::Container); + CORRADE_COMPARE(dataFlags, DataFlags{}); _opened = true; } @@ -1058,10 +1100,11 @@ void AbstractImporterTest::setFileCallbackOpenFileAsData() { openFileCalled = true; } - void doOpenData(Containers::ArrayView data) override { + void doOpenData(Containers::Array&& data, DataFlags dataFlags) override { CORRADE_COMPARE_AS(data, Containers::arrayView({'\xb0'}), TestSuite::Compare::Container); + CORRADE_COMPARE(dataFlags, DataFlags{}); _opened = true; }