-
Notifications
You must be signed in to change notification settings - Fork 67
Add sort and dedup logic in C++ to getFramesAtIndices
#280
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
| if (options.colorConversionLibrary == | ||
| ColorConversionLibrary::FILTERGRAPH) { | ||
| output.frames[indexInOutput] = singleOut.frame; | ||
| } |
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.
Why is this needed when we are passing output.frames[indexInOutput] in getFrameAtIndex?
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.
It's because the pre-allocated buffer isn't used with filtergraph, only with swscale.
(Just a note that this is not something that was introduced in this PR, you'll see the same pattern in other callers)
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.
This seems like the wrong behavior. When a user passes in a pre-allocated tensor it should work with either color conversion library.
I guess this behavior is introduced by the PR to add pre-allocated tensor. That PR should have done it for either color conversion library. Maybe make that change first before merging in this PR? That would be my vote because otherwise the caller has to think about what color conversion library was used.
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.
| std::is_sorted(frameIndices.begin(), frameIndices.end()); | ||
|
|
||
| std::vector<size_t> argsort; | ||
| if (!indicesAreSorted) { |
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.
Digging a bit, I think we're probably better off not checking to see if the sequence is already sorted, and just always sorting. Modern implementations of std::sort seem to be Introsort, which was designed to be nearly linear with an already sorted sequence. I'm also fine if we commit this as is. We can always investigate more later if it becomes important.
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.
Nit: I like to keep object definitions as close as possible to their initial use. So I'd prefer to see lines 1037-1040 appear after the sorting, right before we use output in the loop. (I just ran into this trying to find the definition of what output is when reading the loop.)
Benchmark shows no regression against main, and up to 10X speedup when indices are shuffled.
Still TODO for other PRs:
VideoDecoder