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

@@ -372,6 +372,10 @@ See also:
 
 @subsubsection changelog-latest-changes-trade Trade library
 
+-   A changed signature of the @ref Trade::AbstractImporter::doOpenData(Containers::Array<char>&&, 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

doc/snippets/MagnumTrade.cpp

@@ -25,6 +25,7 @@
 
 #include <unordered_map>
 #include <Corrade/Containers/Optional.h>
+#include <Corrade/Utility/Algorithms.h>
 #include <Corrade/Utility/Directory.h>
 #include <Corrade/Utility/Resource.h>
 
@@ -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<char> _in;
+
+/* [AbstractImporter-doOpenData-ownership] */
+void doOpenData(Containers::Array<char>&& 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<char>{NoInit, data.size()};
+        Utility::copy(data, _in);
+    }
+
+    DOXYGEN_IGNORE()
+}
+/* [AbstractImporter-doOpenData-ownership] */
+} importer;
+}
+
 {
 /* [AbstractSceneConverter-usage-file] */
 PluginManager::Manager<Trade::AbstractSceneConverter> manager;

src/Magnum/Trade/AbstractImporter.cpp

@@ -131,13 +131,26 @@ bool AbstractImporter::openData(Containers::ArrayView<const char> 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<char>{const_cast<char*>(data.data()), data.size(), Implementation::nonOwnedArrayDeleter}, {});
     return isOpened();
 }
 
+#ifdef MAGNUM_BUILD_DEPRECATED
 void AbstractImporter::doOpenData(Containers::ArrayView<const char>) {
     CORRADE_ASSERT_UNREACHABLE("Trade::AbstractImporter::openData(): feature advertised but not implemented", );
 }
+#endif
+
+void AbstractImporter::doOpenData(Containers::Array<char>&& data, const DataFlags) {
+    #ifndef MAGNUM_BUILD_DEPRECATED
+    CORRADE_ASSERT_UNREACHABLE("Trade::AbstractImporter::openData(): feature advertised but not implemented", );
+    static_cast<void>(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<char>{const_cast<char*>(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<Containers::ArrayView<const char>> data = _fileCallback(filename, InputFileCallbackPolicy::LoadTemporary, _fileCallbackUserData);
         if(!data) {
             Error() << "Trade::AbstractImporter::openFile(): cannot open file" << filename;
             return;
         }
-        doOpenData(*data);
+        doOpenData(Containers::Array<char>{const_cast<char*>(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);
     }
 }
 

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<const char> 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<char>&& data, DataFlags dataFlags);
+
+        #ifdef MAGNUM_BUILD_DEPRECATED
+        /**
+         * @brief Implementation for @ref openData()
+         * @m_deprecated_since_latest Implement @ref doOpenData(Containers::Array<char>&&, DataFlags)
+         *      instead.
+         */
+        virtual CORRADE_DEPRECATED("implement doOpenData(Containers::Array<char>&&, DataFlags) instead") void doOpenData(Containers::ArrayView<const char> data);
+        #endif
 
         /** @brief Implementation for @ref openState() */
         virtual void doOpenState(const void* state, const std::string& filePath);

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 {
     /**

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<char>&& 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<const char> 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<const char> data) override {
+        void doOpenData(Containers::Array<char>&& 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<const char>) override {
+        void doOpenData(Containers::Array<char>&&, DataFlags) override {
             _opened = true;
         }
 
@@ -948,7 +989,7 @@ void AbstractImporterTest::setFileCallbackOpenFileDirectly() {
             _opened = true;
         }
 
-        void doOpenData(Containers::ArrayView<const char>) override {
+        void doOpenData(Containers::Array<char>&&, 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<const char> data) override {
+        void doOpenData(Containers::Array<char>&& 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<const char> data) override {
+        void doOpenData(Containers::Array<char>&& data, DataFlags dataFlags) override {
             CORRADE_COMPARE_AS(data,
                 Containers::arrayView({'\xb0'}),
                 TestSuite::Compare::Container);
+            CORRADE_COMPARE(dataFlags, DataFlags{});
             _opened = true;
         }