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; }