-
Notifications
You must be signed in to change notification settings - Fork 31
Audio decoding support: range-based core API #538
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
Merged
Merged
Changes from 27 commits
Commits
Show all changes
28 commits
Select commit
Hold shift + click to select a range
ae15304
Add basic range support
NicolasHug 29e0b8d
Add more tests
NicolasHug cad69da
Merge branch 'main' of github.com:pytorch/torchcodec into audioooooooo
NicolasHug 04f6282
Add separate audio decoding method
NicolasHug f8dfcda
Merge branch 'main' of github.com:pytorch/torchcodec into audioooooooo
NicolasHug da40954
More stuff
NicolasHug 3881586
Cleanups
NicolasHug 82bea4a
Remove old code
NicolasHug ce12f03
More validation, more tests
NicolasHug 59b0d15
remove next() support
NicolasHug f4bed23
Rename
NicolasHug fe04cd2
Add support for None stop_seconds
NicolasHug 98fee85
Remove pre-alloc logic
NicolasHug d2357fe
Add test
NicolasHug f3b56f8
Add proper error when backward seek is neede
NicolasHug 5f2800a
Cleanup
NicolasHug 2f020f2
Add TODO
NicolasHug 0c11f72
Put back original compilation flags
NicolasHug de4facc
Fix
NicolasHug b5f2df0
nit
NicolasHug 09e6f44
Oops, fix
NicolasHug 3d955c1
Add case for start=stop
NicolasHug d791d2a
Simplify
NicolasHug 0c0f62b
Don't use a lambda
NicolasHug 893c358
Merge branch 'main' of github.com:pytorch/torchcodec into audioooooooo
NicolasHug c35ae47
Fix
NicolasHug c453a3c
Address comments
NicolasHug dafb927
Add comment
NicolasHug File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Q about C++ best practices: I realize we're already doing it in a few places (like custom ops), but is it a good practice to use exceptions for control flow? Maybe the
reachedEOF
flag fromdecodeAVFrame()
could be a stateful attribute instead? (not that I find statefulness appealing either!)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.
Since we shouldn't ordinarily decode past the end of a file, I think it makes sense for us to throw exceptions when we reach the end of a file. Here, we're not really using an exception for control flow per se. That is, we're not trying to read past the end of the file, we just have to handle the case that we might.
With that said, I do find it more natural when the "normal" stop conditions are explicitly part of the while loop's condition, as opposed to setting a boolean inside the loop. But since you're depending on the internal state of the decoder to know the last decoded frame info, I don't know if that's possible. When I implemented something similar, I ended up using a priming read to get around this problem.
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.
Also, nit: for local variables used in a small space with a clear purpose, I prefer shorter names. So even
stop
as the boolean would make this easier for me to read.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.
OK. I'll use
finished
instead ofstop
, because we already have local variables namedstopPts
andstopSeconds
in this function.