Remove streamIndex parameter and multi-stream related logic #506
Conversation
facebook-github-bot
added
the
CLA Signed
| videoStreamOptions.colorConversionLibrary.value_or(defaultLibrary); | ||
| } | ||
|
|
||
| void VideoDecoder::updateMetadataWithCodecContext( |
There was a problem hiding this comment.
I inlined this above, as it became hard to justify this being a single separate method with only one call-site.
| VideoDecoder::FrameOutput VideoDecoder::getNextFrameNoDemux() { | ||
| auto output = getNextFrameNoDemuxInternal(); | ||
| output.data = maybePermuteHWC2CHW(output.streamIndex, output.data); | ||
| VideoDecoder::FrameOutput VideoDecoder::getNextFrame() { |
There was a problem hiding this comment.
Removed all the NoDemux suffixes - they weren't informative (and wrong)
| break; | ||
| } | ||
| VideoDecoder::FrameOutput VideoDecoder::getFramePlayedAt(double seconds) { | ||
| StreamInfo& streamInfo = streamInfos_[activeStreamIndex_]; |
There was a problem hiding this comment.
removed the for loop over StreamInfos_ and just indented to the left, using activeStreamIndex_. This is probably a minor fix.
| OpsFrameOutput get_frame_at_index( | ||
| at::Tensor& decoder, | ||
| int64_t stream_index, | ||
| [[maybe_unused]] int64_t stream_index, |
There was a problem hiding this comment.
As mentioned in the PR description, I chose not to update the APIs of the ops and core APIs. Will deal with that in a follow-up.
| const StreamInfo& streamInfo, | ||
| int64_t pts) const { | ||
| int VideoDecoder::getKeyFrameIndexForPts(int64_t pts) const { | ||
| const StreamInfo& streamInfo = streamInfos_.at(activeStreamIndex_); |
There was a problem hiding this comment.
Using .at() instead of [] to preserve the const qualifier of the method. I would otherwise get:
/home/nicolashug/dev/torchcodec/src/torchcodec/decoders/_core/VideoDecoder.cpp:1475:65: error: passing ‘const std::map<int, facebook::torchcodec::VideoDecoder::StreamInfo>’ as ‘this’ argument discards qualifiers [-fpermissive]
1475 | const StreamInfo& streamInfo = streamInfos_[activeStreamIndex_];
| ^
@scotts is there a more idiomatic way of dealing with this?
There was a problem hiding this comment.
This is the correct thing to do - the [] operator on a map always returns a reference, and its semantics are that if the key is not there, a value is placed there with the default for that type. The at() member function can return a constant reference, and it does bounds-checking, so a key not being there is an exception.
|
@NicolasHug has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
I considered if we should eventually make the C++ video decoder just take the stream index as a parameter in its constructor, similar to what we do in Python. But then I remembered that there is one valid case of a C++ video decoder with no active stream: reading the metadata from the header. |
3 participants
Towards #476. Our VideoDecoder is inherently single-stream and almost all
streamIndexparameters aren't used, so this PR removes them. This also removes most but not all multi-stream related logic (there is some left in the scan for example).I haven't updates the custom ops APIs nor the core APIs because this may create conflicts internally and I want to deal with that separately as a follow-up PR.
Other clean-ups we could do afterwards:
StreamInfos_vec and only keep a singleStreamInfofor the active stream.