-
Notifications
You must be signed in to change notification settings - Fork 30
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
Move stream options and frame output structs to dedicated headers #620
Conversation
Signed-off-by: Dmitry Rogozhkin <[email protected]>
Signed-off-by: Dmitry Rogozhkin <[email protected]>
Signed-off-by: Dmitry Rogozhkin <[email protected]>
Signed-off-by: Dmitry Rogozhkin <[email protected]>
src/torchcodec/_core/Stream.h
Outdated
|
||
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.h
renamed toStreamOptions.h
- Created
Medata.h
to holdStreamMetadata
andContainerMetadata
@NicolasHug , @scotts : The reason why I moved metadata to header was that StreamMetadata
is used by FrameBatchOutput
which I also moved:
torchcodec/src/torchcodec/_core/SingleStreamDecoder.h
Lines 181 to 184 in 8b19f45
explicit FrameBatchOutput( | |
int64_t numFrames, | |
const VideoStreamOptions& videoStreamOptions, | |
const StreamMetadata& streamMetadata); |
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 <[email protected]>
Signed-off-by: Dmitry Rogozhkin <[email protected]>
Fixes: #618
Commit moves stream options and frame output structs to dedicated headers:
Frame.h
now definesFrameOutput
,FrameBatchOutput
andAudioFrameOutput
Stream.h
now definesVideoStreamOptions
,AudioStreamOptions
,StreamMetadata
andContainerMetadata
Commit also resolves circular dependency between
DeviceInterface.h
andSingleStreamDecoder.h
, forward declaration ofclass DeviceInterface
no longer needed.CC: @scotts, @NicolasHug