Stream and Channel Mapping Support#2015
Stream and Channel Mapping Support#2015reinecke wants to merge 8 commits intoAcademySoftwareFoundation:mainfrom
Conversation
|
@mrlimbic here is my first pass at stream/channel mapping. Please feel free to provide feedback here. |
Signed-off-by: Eric Reinecke <ereinecke@netflix.com>
Signed-off-by: Eric Reinecke <ereinecke@netflix.com>
Signed-off-by: Eric Reinecke <ereinecke@netflix.com>
Signed-off-by: Eric Reinecke <ereinecke@netflix.com>
Signed-off-by: Eric Reinecke <ereinecke@netflix.com>
Signed-off-by: Eric Reinecke <ereinecke@netflix.com>
Signed-off-by: Eric Reinecke <ereinecke@netflix.com>
Signed-off-by: Eric Reinecke <ereinecke@netflix.com>
0d6c0cf to
b3bae93
Compare
darbyjohnston
left a comment
There was a problem hiding this comment.
Overall the C++ code looks fine, it seems to follow all of our conventions, I just left a few minor comments. Thanks for including the Doxygen markup and export macros.
I only skimmed the Python code, it would be good to get someone with more Python experience to take a look.
At some point it might be nice to add C++ tests, but for now the Python tests exercise the functionality.
|
|
||
| ## Overview | ||
|
|
||
| OTIO's stream and channel mapping support allows media references to describe the individual streams they contain (video eyes, audio channels, etc.) and for timeline items to select or remix those streams via effects. The system has three layers: **addressing** a stream within a container, **describing** a stream's semantic role, and **selecting or mixing** streams on a clip. |
There was a problem hiding this comment.
I haven't seen "video eyes" before, is that a convention?
| 2. **Additional streams** SHOULD prefix an `Identifier` value to form a unique key within the media reference. | ||
|
|
||
| ```python | ||
| # A music stem alongside the audio mix |
There was a problem hiding this comment.
I thought this was a typo ('stem' -> 'stream'), but I guess stem is an audio term?
| | Class | Use when | | ||
| |---|---| | ||
| | `IndexStreamAddress` | The container uses a single integer index per stream (e.g. WAV channel index, ffmpeg stream index) | | ||
| | `StreamChannelIndexStreamAddress` | The container organises media into tracks, each with one or more channels (e.g. MP4/MOV, MXF) | |
There was a problem hiding this comment.
Could we simplify this class name a bit, maybe ChannelStreamAddress?
| /// @brief This struct provides the AudioMixMatrix schema. | ||
| struct Schema | ||
| { | ||
| static char constexpr name[] = "AudioMixMatrix"; |
There was a problem hiding this comment.
Thanks, I like the explicit type here vs. the static auto constexpr name we have in the rest of the headers. :)
| OTIO_API explicit IndexStreamAddress(int64_t index = 0); | ||
|
|
||
| /// @brief Return the stream index. | ||
| OTIO_API int64_t index() const noexcept { return _index; } |
There was a problem hiding this comment.
I'm not sure if the export macros are needed on inline functions, we should double check that.
|
|
||
| using Parent = SerializableObject; | ||
|
|
||
| OTIO_API StreamAddress(); |
There was a problem hiding this comment.
We could use = default here to save a bit of code since the constructor is empty. Just a suggestion though, I'm not sure we use default anywhere else yet. At some point we could also do a separate pass over the code replacing empty ctors/dtors with default.
| /// @brief An effect that selects specific named output streams from an item. | ||
| /// | ||
| /// Use this to select a stereo view, specific audio channels, etc. | ||
| /// The clip will expose these streams out with the same naming. |
There was a problem hiding this comment.
Minor, but I thought the wording on this was a little confusing:
The clip will expose these streams out with the same naming.
Maybe:
The stream names match the names within the clip.
|
|
||
| using Parent = StreamAddress; | ||
|
|
||
| OTIO_API explicit StringStreamAddress(std::string const& address = std::string()); |
There was a problem hiding this comment.
Minor, but we could add a Doxygen comment here for completeness:
/// @brief Create a new StringStreamAddress.
| from . import ( | ||
| track_algo | ||
| ) | ||
| from . import track_algo |
There was a problem hiding this comment.
It looks like the changes in timeline_algo.py and track_algo.py are just formatting?
|
|
||
| Stream effects are `Effect` subclasses that attach to a clip's `effects` list and transform which streams are visible downstream and under what names. They operate on the stream identifiers declared in `available_streams`. | ||
|
|
||
| ### StreamSelector |
There was a problem hiding this comment.
Just curious if we need the StreamSelector class, it seems like you could do the same thing with StreamMapper?
|
Just to continue the discussion from today's OTIO meeting: If multiple media references expose different stream information, how is that handled by the stream effects? For example, say you have a clip with two media references, one with 5.1 audio and the other with mono:
How would you use an
Or use multiple
|
Link the Issue(s) this Pull Request is related to.
If there is an associated issue, link it in the form:
Fixes #145
Summarize your change.
To for an explanation of the implementation, check out the docs.
This PR implements the proposal that was created to address #145 and add the ability to do stream/channel addressing and mapping from sources into the composition.
The C++ implementation of the mappings was built following the pattern of Clip.media_references.
Reference associated tests.
Unittests were added for the new features.