-
Notifications
You must be signed in to change notification settings - Fork 66
Move stream options and frame output structs to dedicated headers #620
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
Conversation
Signed-off-by: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
Signed-off-by: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
Signed-off-by: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
Signed-off-by: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
|
|
||
| std::optional<int> sampleRate; | ||
| }; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @dvrogozh . Was it necessary to move the StreamMetadata and ContainerMetadata classes into this new header?
If yes, then I'd suggest moving them into their own metadata.h header
If not, then it might be best to leave them in the SingleStreamDecoder files
In both cases, I'd also suggest to rename Stream.h into just StreamOptions.h since this file should only contain option-related structs.
Otherwise, this PR LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed with @NicolasHug - if we need to move the metadata definitions, let's put them in their own file. If we don't need to, let's leave them be for now. And also agreed on naming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the last update I've pushed:
Stream.hrenamed toStreamOptions.h- Created
Medata.hto holdStreamMetadataandContainerMetadata
@NicolasHug , @scotts : The reason why I moved metadata to header was that StreamMetadata is used by FrameBatchOutput which I also moved:
That being said, note that I moved ContainerMetadata only for consistency - it's not used by any of the structures we really need to be in headers. Let me know if you want to avoid moving ContainerMetadata. Also, if we don't want to separately expose FrameBatchOutput we can avoid moving metadata to separate header entirely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dvrogozh, looks good, thanks for cleaning this up!
Signed-off-by: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
Signed-off-by: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
Fixes: #618
Commit moves stream options and frame output structs to dedicated headers:
Frame.hnow definesFrameOutput,FrameBatchOutputandAudioFrameOutputStream.hnow definesVideoStreamOptions,AudioStreamOptions,StreamMetadataandContainerMetadataCommit also resolves circular dependency between
DeviceInterface.handSingleStreamDecoder.h, forward declaration ofclass DeviceInterfaceno longer needed.CC: @scotts, @NicolasHug