-
Notifications
You must be signed in to change notification settings - Fork 32
Add sort and dedup logic in C++ with new getFramesDisplayedByTimestamps
method / core API
#282
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
Changes from all commits
823c8a3
61b4937
f7a70ba
133c213
f391582
d475890
14e2876
9c9e462
b8284cc
be80996
12c0e29
4dda5b7
7d26623
3a8839d
d43dd91
8dd9b0a
52dd9ed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1090,6 +1090,53 @@ VideoDecoder::BatchDecodedOutput VideoDecoder::getFramesAtIndices( | |
return output; | ||
} | ||
|
||
VideoDecoder::BatchDecodedOutput VideoDecoder::getFramesDisplayedByTimestamps( | ||
int streamIndex, | ||
const std::vector<double>& timestamps) { | ||
validateUserProvidedStreamIndex(streamIndex); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe add a TODO saying long term we should not require scanning the file for time-based frame extraction |
||
validateScannedAllStreams("getFramesDisplayedByTimestamps"); | ||
|
||
// The frame displayed at timestamp t and the one displayed at timestamp `t + | ||
// eps` are probably the same frame, with the same index. The easiest way to | ||
// avoid decoding that unique frame twice is to convert the input timestamps | ||
// to indices, and leverage the de-duplication logic of getFramesAtIndices. | ||
Comment on lines
+1096
to
+1102
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The long term thing to do here is to not require a scan for this function. Scan should only be required for index-based functions where indexes need to be exact. The index-based function can then call this function if the de-duping is done here and this doesn't need a scan. That said you can merge this as-is and do the long-term thing later There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ahmadsharif1 do you think it will be possible to correctly de-dup pts-based frame queries without going through indices? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is what I mean by having explicit seeking modes (approximate versus exact) is a big conceptual change. :) It might only requires changing a few dozen lines of code, but it will require changes in many places, and we'll always have to be aware of what mode is active. At the moment, we're implicitly exact when we need to do things with indices. I think we should just keep being that way as needed, and when we implement the different modes, we can reason about what changes to make wholistically. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should be possible to dedup based on time, but it needs to be done on the fly -- i.e. when you decode a frame you see the extent of that frame on the timeline and can make copies of that for the timepoints that the user specified. It could be a bit more involved than that because you don't know the extent of the frame by looking at the frame itself -- you need to read (not decode) the next frame. Doing it holistically in a future PR sounds good |
||
// This means this function requires a scan. | ||
// TODO: longer term, we should implement this without requiring a scan | ||
|
||
const auto& streamMetadata = containerMetadata_.streams[streamIndex]; | ||
const auto& stream = streams_[streamIndex]; | ||
double minSeconds = streamMetadata.minPtsSecondsFromScan.value(); | ||
double maxSeconds = streamMetadata.maxPtsSecondsFromScan.value(); | ||
|
||
std::vector<int64_t> frameIndices(timestamps.size()); | ||
for (auto i = 0; i < timestamps.size(); ++i) { | ||
auto framePts = timestamps[i]; | ||
TORCH_CHECK( | ||
framePts >= minSeconds && framePts < maxSeconds, | ||
"frame pts is " + std::to_string(framePts) + "; must be in range [" + | ||
std::to_string(minSeconds) + ", " + std::to_string(maxSeconds) + | ||
")."); | ||
|
||
auto it = std::lower_bound( | ||
stream.allFrames.begin(), | ||
stream.allFrames.end(), | ||
framePts, | ||
[&stream](const FrameInfo& info, double framePts) { | ||
return ptsToSeconds(info.nextPts, stream.timeBase) <= framePts; | ||
}); | ||
int64_t frameIndex = it - stream.allFrames.begin(); | ||
// If the frame index is larger than the size of allFrames, that means we | ||
// couldn't match the pts value to the pts value of a NEXT FRAME. And | ||
// that means that this timestamp falls during the time between when the | ||
// last frame is displayed, and the video ends. Hence, it should map to the | ||
// index of the last frame. | ||
frameIndex = std::min(frameIndex, (int64_t)stream.allFrames.size() - 1); | ||
frameIndices[i] = frameIndex; | ||
} | ||
|
||
return getFramesAtIndices(streamIndex, frameIndices); | ||
} | ||
|
||
VideoDecoder::BatchDecodedOutput VideoDecoder::getFramesInRange( | ||
int streamIndex, | ||
int64_t start, | ||
|
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.
For the function names, I based it on the existing
getFramesDisplayedByTimestampsInRange()
. I'm happy to bikeshed, but not on this PR please.Note that our naming is becoming a bit inconsistent, e.g. the python names, ops names and C++ names are not always aligned. We probably want to clean that up, but that's for later.
We'll also have to re-think our public
VideoDecoder
method names very soon I think, because adding the 2 new "get_frames_..." that we recently added may conflict with existing names.