Skip to content
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

separated stream for each seek #664

Merged
merged 2 commits into from
Apr 9, 2021
Merged

separated stream for each seek #664

merged 2 commits into from
Apr 9, 2021

Conversation

philippe44
Copy link
Contributor

Per #654 it's probably a better idea to have a separated stream every time the user seeks through the same track. It also makes code simpler and solves the issue with potentially missing BoS. The only case where the EoS might miss is when skipping tracks.

@philippe44
Copy link
Contributor Author

Opinions?

Copy link
Contributor

@Johannesd3 Johannesd3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't review the actual logic behind it, but again a few comments about your Rust.

audio/src/passthrough_decoder.rs Show resolved Hide resolved
audio/src/passthrough_decoder.rs Outdated Show resolved Hide resolved
Comment on lines +27 to +29
eos: bool,
bos: bool,
ofsgp_page: u64,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I never understood why C programmers are afraid of using many letters for variables.

Copy link
Contributor Author

@philippe44 philippe44 Mar 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, I started a long time ago to be a DSP programmer as well as doing x86 assembly code. C was a luxury and you had to count every byte, so I kept the "frugal" mindset from these days. Even when I do Perl, which is my 2nd lmost used language after C, I have the same habit. And I like Perl because of the compactness of expressions you can write ;-)

Having said that, it's also because the Rust formater shrinks everything to 80 columns and that, I find it absolutely crazy in 2021. I think these days are long gone and even Linux Torvald :-) thinks that should be changed. So that 80 column pushed the habit of short variable names because having 10 lines of text for one line of code is ugly, IMHO.

Last but not least, I'm always trying to fit with the style of the code I'm adding my changes to and this started from the "ogg/vorbis" library that uses that same short naming for offset and granule.

Co-authored-by: Johannesd3 <51954457+Johannesd3@users.noreply.github.com>
@sashahilton00
Copy link
Member

sashahilton00 commented Mar 26, 2021

From a code perspective this looks fine. I also can't comment on the logic though, since I'm not particularly familiar with the vorbis spec. If anyone has any views on why this shouldn't be merged, voice them, otherwise I'll assume we're good to merge this in a few days.

@philippe44
Copy link
Contributor Author

philippe44 commented Mar 27, 2021

@peet1993, does that work for you?

@poettig
Copy link

poettig commented Mar 27, 2021

It works exactly how you described it, the BOS is there, so yes :) Thanks for implementing it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants