Add core C++ support for otioz and otiod, take 2#2021
Conversation
Codecov Report❌ Patch coverage is ❌ Your changes status has failed because you have indirect coverage changes. Learn more about Unexpected Coverage Changes and reasons for indirect coverage changes. Additional details and impacted files@@ Coverage Diff @@
## main #2021 +/- ##
==========================================
- Coverage 84.51% 83.13% -1.38%
==========================================
Files 181 180 -1
Lines 13239 13398 +159
Branches 1202 1234 +32
==========================================
- Hits 11189 11139 -50
- Misses 1867 2087 +220
+ Partials 183 172 -11
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
ssteinbach
left a comment
There was a problem hiding this comment.
Thanks for doing this work! Some questions and notes about the interface. As far as I can tell, this is 100% compatible with the way bundles work right now, right? Or is it the new library that would warrant the version bump?
The file format is the same, I was just thinking about the library change. Like if we wanted to identify if a particular bundle was written with the original code vs. the new code. On second thought that probably shouldn't change the spec version, if it was useful we could add it as metadata to the timeline. |
meshula
left a comment
There was a problem hiding this comment.
overall looks really good. general notes
- maybe clean up TempDir as noted?
- I think you could wire through much more descriptive error reporting for the CLI (or please correct me if I'm not spotting a verbose forensics path)
- please beware of string construction stringstream, as I noted it's not especially problematic (aside from performance) where you are using it, but especially for string construction it is loaded with footguns due to locale, and you haven't enforced the C locale which at least makes its behavior reproducible from computer to computer.
Please consider using zstd; to my knowledge it's bullet proof vs common attacks, offers excellent compression at I believe best speeds, and is also a very lightweight library that is easy to vendor. https://github.com/facebook/zstd
These are minor compared to the overall solidity of the work.
|
That's a different compression scheme from deflate? We could possibly add that as a separate feature at some point. Somewhat related, what do you think about having otioz files conform to ISO/IEC 21320-1? (https://en.wikipedia.org/wiki/ZIP_(file_format)) As I understand it, it's a minimal ZIP format for the same use cases as otioz. What do you suggest instead of |
|
afaik USD itself uses ISO/IEC 21320-1? so that makes sense to me. What do you suggest instead of stringstream? Aren't we constrained by our C++ version from using std::format? Allocate space and fill it in trivially C style. C still works :) |
meshula
left a comment
There was a problem hiding this comment.
Thanks for addressing the feedback!
Signed-off-by: Darby Johnston <darbyjohnston@yahoo.com>
Signed-off-by: Darby Johnston <darbyjohnston@yahoo.com>
Signed-off-by: Darby Johnston <darbyjohnston@yahoo.com>
Signed-off-by: Darby Johnston <darbyjohnston@yahoo.com>
Signed-off-by: Darby Johnston <darbyjohnston@yahoo.com>
Signed-off-by: Darby Johnston <darbyjohnston@yahoo.com>
Signed-off-by: Darby Johnston <darbyjohnston@yahoo.com>
Signed-off-by: Darby Johnston <darbyjohnston@yahoo.com>
Signed-off-by: Darby Johnston <darbyjohnston@yahoo.com>
Signed-off-by: Darby Johnston <darbyjohnston@yahoo.com>
Signed-off-by: Darby Johnston <darbyjohnston@yahoo.com>
Signed-off-by: Darby Johnston <darbyjohnston@yahoo.com>
Signed-off-by: Darby Johnston <darbyjohnston@yahoo.com>
Co-authored-by: Stephan Steinbach <61017+ssteinbach@users.noreply.github.com> Signed-off-by: Darby Johnston <darbyjohnston@yahoo.com>
Signed-off-by: Darby Johnston <darbyjohnston@yahoo.com>
Signed-off-by: Darby Johnston <darbyjohnston@yahoo.com>
Signed-off-by: Darby Johnston <darbyjohnston@yahoo.com>
Signed-off-by: Darby Johnston <darbyjohnston@yahoo.com>
Signed-off-by: Darby Johnston <darbyjohnston@yahoo.com>
Signed-off-by: Darby Johnston <darbyjohnston@yahoo.com>
Signed-off-by: Darby Johnston <darbyjohnston@yahoo.com>
Signed-off-by: Darby Johnston <darbyjohnston@yahoo.com>
Signed-off-by: Darby Johnston <darbyjohnston@yahoo.com>
Signed-off-by: Darby Johnston <darbyjohnston@yahoo.com>
Signed-off-by: Darby Johnston <darbyjohnston@yahoo.com>
Signed-off-by: Darby Johnston <darbyjohnston@yahoo.com>
Signed-off-by: Darby Johnston <darbyjohnston@yahoo.com>
Signed-off-by: Darby Johnston <darbyjohnston@yahoo.com>
b12d7ef to
e18401e
Compare
Fixes #1869
This PR adds support for otioz and otiod bundles to the C++ API. This PR is a second attempt at adding the functionality, superseding PR #1901.
The python otioz and otioz adapters still exist and work the same, but now they call into the C++ code for the bundle functionality.
Support for image sequences and multiple media references has been added.
Other changes:
Questions:
Follow up work for other PRs:
Assisted-by: Claude:claude-opus-4-7 [code-review] [debugging]