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

@@ -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<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
+    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

doc/snippets/MagnumTrade.cpp

@@ -140,7 +140,7 @@ Containers::Pointer<Trade::AbstractImporter> importer;
 /* [AbstractImporter-usage-data] */
 Utility::Resource rs{"data"};
 Containers::ArrayView<const char> 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<char>&& 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<char>{NoInit, data.size()};

src/Magnum/Trade/AbstractImporter.cpp

@@ -152,6 +152,18 @@ void AbstractImporter::doOpenData(Containers::Array<char>&& data, const DataFlag
     #endif
 }
 
+bool AbstractImporter::openMemory(Containers::ArrayView<const void> 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<char>{const_cast<char*>(static_cast<const char*>(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", {});

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<const void> 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<const void> 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<char>&& data, DataFlags dataFlags);
 

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

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.

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

src/MagnumPlugins/TgaImporter/Test/TgaImporterTest.cpp

@@ -28,6 +28,7 @@
 #include <Corrade/Containers/Optional.h>
 #include <Corrade/TestSuite/Tester.h>
 #include <Corrade/TestSuite/Compare/Container.h>
+#include <Corrade/Utility/Algorithms.h>
 #include <Corrade/Utility/DebugStl.h>
 #include <Corrade/Utility/Directory.h>
 #include <Corrade/Utility/FormatStl.h>
@@ -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<const void>);
+} OpenMemoryData[]{
+    {"data", [](AbstractImporter& importer, Containers::ArrayView<const void> data) {
+        /* Copy to ensure the original memory isn't referenced */
+        Containers::Array<char> copy{NoInit, data.size()};
+        Utility::copy(Containers::arrayCast<const char>(data), copy);
+        return importer.openData(copy);
+    }},
+    {"memory", [](AbstractImporter& importer, Containers::ArrayView<const void> 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<AbstractImporter> 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<Trade::ImageData2D> 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<char>({
+        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<AbstractImporter> importer = _manager.instantiate("TgaImporter");
 

src/MagnumPlugins/TgaImporter/TgaImporter.cpp

@@ -64,7 +64,7 @@ void TgaImporter::doOpenData(Containers::Array<char>&& 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<char>{NoInit, data.size()};