Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Zero-copy importer plugin APIs #240

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Zero-copy importer plugin APIs #240

wants to merge 4 commits into from

Conversation

mosra
Copy link
Owner

@mosra mosra commented Mar 26, 2018

The end goal for this:

  • use Utility::Directory::mapRead() to memory-map a huge image / audio / scene file
  • open the memory-mapped file via an importer and parse it without copying the data
  • provide a view on the data to the user (again, no copy)
  • take small slices of the imported data and upload them to the GPU / play them on the soundcard / ..., again no copy
  • have AnimationData, MeshData, ImageData ... work directly on the memory-mapped data

The progress, as of 2021-10-24:

image

TODO:

  • rework doOpenData() to accept an r-value Array and DataFlags describing whether it's temporary, owned by the importer, make openFile() then pass over the allocated array ownership there -- 116d327
    • make a protected openData() with the same signature and the usual assertions from the public openData() so proxy plugins such as AnyImageImporter or opening images from within other plugins can make use of the memory ownership annotations as well
  • implement openMemory() as a zero-copy counterpart to openData() in Trade::AbstractImporter + DataFlag::ExternallyOwned, delegate to the above new `doOpenData() -- 27044a4
    • a mutable overload of it, passing DataFlag::Mutable to doOpenData()
    • expose in magnum-sceneconverter -- 03aeb49
    • make magnum-sceneconverter memory-map also referenced files via a callback
  • implement openMemory() as a zero-copy counterpart to openData() in Audio::AbstractImporter these interfaces will eventually get deprecated and merged into Trade::AbstractImporter
  • implement doOpenData() and openMemory() equivalently in Text::AbstractFont as well
  • redesign Trade::MeshData2D / Trade::MeshData3D to eat an array (view) instead of vectors -- done in Mesh Data rework #371 instead
  • add an ability to put an array view to Trade::ImageData2D / ... (convert it to an array with a custom no-op deleter internally) -- done in Mesh Data rework #371 instead
  • implement Trade::AbstractImporter::mesh2DMemory() / ... that's able to pass through the mapped data in case the file was opened via openMemory() done differently in Mesh Data rework #371
  • implement Trade::AbstractImporter::image2DMemory() / ... that's able to pass through the mapped data in case the file was opened via openMemory() done differently in Mesh Data rework #371
  • implement Audio::Importer::data() that's able to pass through the mapped data in case the file was opened via openMemory() these interfaces will eventually get deprecated and merged into Trade::AbstractImporter
  • Add an ImporterFlag::ZeroCopy flag, which will enable importers to pass the input data from openMemory() through to AnimationData / MeshData / ImageData / ... without copies
    • and silently falling back to copies if the data is not directly representable there (because it needs to be modified, flipped, swizzled, deinterleaved, whatever)
    • if the importer doesn't support it, it behaves same as without the flag (no error)
  • Add an ImporterFlag::ForceZeroCopyThing for animations, images, mesh index/vertex buffers, ... that turns the above opt-in behavior to enforced
    • and a ImporterFeature::ZeroCopyThing with which the importer advertises ability to zero-copy certain data, which then gets assert for presence if the corresponding flag gets set
    • useful for example when editing image/mesh/scene files in-place, to ensure we're not editing an in-memory copy
    • or to ensure the app doesn't have any suboptimal behavior by accident
    • should the output data be checked? probably not, the base class doesn't remember the memory range anywhere
    • Also ImporterFeature::ZeroCopyScenes and ImporterFlag::ForceZeroCopyScenes once SceneData rework #525 is in
    • And apparently SkinData needs to expose jointDataFlags() & inverseBindMatrixDataFlags()
    • All those inflate the flag/feature enums which now need to be 16-bit (or maybe more with the YUp etc flags?), bump importer plugin interfaces for that
    • AnyImageImporter etc. flag propagation now needs to handle the cases of the flag not being supported in the target and provide a runtime error instead of an assert
  • ImporterFlag::YUp & ImageConverterFlag::YUp to control whether images get Y-flipped on import and on conversion (otherwise it's not really possible to implement zero-copy import), similarly for ZBackward (and XLeft?)
    • ImageFlag::YUp to annotate what orientation the image is in so this information doesn't get lost after
    • Probably also need ImporterFlag::YDown / ImageConverterFlag::YDown and a presence of neither meaning "keep it in whatever orientation it was" to make the zero-copy import not annoying to set up
    • Decide how should pixels() behave -- they should probably return the view with X right, Y down, Z forward always to be consistent? That'll break existing code tho.
  • Implement this in WavAudioImporter once Trade::AbstractImporter can do audio as well
  • Implement this in FreeTypeFont
  • Implement this in TgaImporter once the YUp flag is in (it also converts from ARGB or something, what to do ?)
  • Implement this in DdsImporter once the YUp flag is in
  • Implement this in KtxImporter once the YUp flag is in
  • Implement this in StanfordImporter in progress, zerocopy branch
  • Implement this in TinyGltfImageImporter (impossible) CgltfImporter
  • Optional, for fun: implement ImageView::slice() and then upload a slice to GPU memory

Open questions / followup tasks:

@mosra mosra mentioned this pull request Sep 13, 2019
70 tasks
@mosra mosra changed the base branch from master to meshdata December 2, 2019 23:30
@mosra mosra added this to the 2019.1c milestone Dec 2, 2019
@mosra mosra mentioned this pull request Jan 1, 2020
87 tasks
@mosra mosra force-pushed the meshdata branch 7 times, most recently from 98c057b to b4666a6 Compare January 23, 2020 16:30
@mosra mosra force-pushed the meshdata branch 2 times, most recently from 414347d to 0e5385f Compare January 24, 2020 15:45
@mosra mosra force-pushed the meshdata branch 5 times, most recently from 8c0503e to 41d57f0 Compare February 8, 2020 19:51
@mosra mosra force-pushed the meshdata branch 6 times, most recently from 8889ee2 to e09b741 Compare February 16, 2020 10:41
@mosra mosra force-pushed the meshdata branch 6 times, most recently from 6f62217 to a4bf0e6 Compare March 11, 2020 13:39
@mosra mosra closed this Mar 11, 2020
@mosra mosra reopened this Mar 11, 2020
@mosra mosra changed the base branch from meshdata to master March 11, 2020 19:33
@mosra mosra mentioned this pull request Mar 11, 2020
61 tasks
@mosra mosra modified the milestones: 2020.0a, 2020.0b Jun 24, 2020
@mosra mosra marked this pull request as draft October 24, 2021 14:53
@mosra mosra changed the title [WIP] Zero-copy importer plugin APIs Zero-copy importer plugin APIs Oct 24, 2021
It's great to see how the design nicely fermented over the three and
half (!!) years. This is a completely opt-in feature for all plugins
involved, so if the plugin decides to not do anything about it, it
doesn't have to. The actual enforcements will come later.
Because there's no format ever that would support zero-copy everything
(except the upcoming Magnum blobs, hah!), it makes no sense to force
zero-copy globally. Instead, zero-copy can be forced just for particular
data of interest. For example a glTF can zero-copy-import mesh and
animation data, but not always image data (except if it embeds an
uncompressed KTX), and never scene or material data, which are stored in
the textual JSON.
@codecov
Copy link

codecov bot commented Oct 25, 2021

Codecov Report

Merging #240 (dfea5bd) into master (9870cd7) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head dfea5bd differs from pull request most recent head 4446300. Consider uploading reports for the commit 4446300 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master     #240   +/-   ##
=======================================
  Coverage   79.97%   79.98%           
=======================================
  Files         511      511           
  Lines       32354    32363    +9     
=======================================
+ Hits        25875    25885   +10     
+ Misses       6479     6478    -1     
Impacted Files Coverage Δ
src/Magnum/Trade/AbstractImporter.h 100.00% <ø> (ø)
src/Magnum/Trade/AbstractImporter.cpp 100.00% <100.00%> (ø)
src/MagnumPlugins/ObjImporter/ObjImporter.cpp 94.35% <0.00%> (+0.40%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9870cd7...4446300. Read the comment docs.

Meant to be used from within plugin implementations to avoid copies in
the delegated importers.

TODO: the heck, why can't I call a protected function from a subclass
   but another instance?
TODO: figure out a way to test this
TODO: update docs and behavior for the ZeroCopy flag
TODO: adapt AnySceneImporter also
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

1 participant