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

opus: implement range decoder. #116

Open
wants to merge 1 commit into
base: opus-dev
Choose a base branch
from

Conversation

thinking-tower
Copy link

Description

Part of the work for #8.

Part of implementing RFC 6716 Section 3.1 and 4.1.

@pdeljanov
Copy link
Owner

Hey @thinking-tower,

I'm not particularly familiar with range decoding so I can't comment on the correctness of the code/give a proper review.

Do you think it'd be a good idea to have some tests for these functions to gain some confidence?

@thinking-tower
Copy link
Author

thinking-tower commented Mar 23, 2022

@pdeljanov

I'm not particularly familiar with range decoding so I can't comment on the correctness of the code/give a proper review.

No worries, I'm just looking for more of a Rust code style review since I still need more work to verify it's correct.

Do you think it'd be a good idea to have some tests for these functions to gain some confidence?

For sure, I'm currently still debugging to get an idea what a "correct input" would look like.

@SunnyWar
Copy link

@pdeljanov

I'm not particularly familiar with range decoding so I can't comment on the correctness of the code/give a proper review.

No worries, I'm just looking for more of a Rust code style review since I still need more work to verify it's correct.

Do you think it'd be a good idea to have some tests for these functions to gain some confidence?

For sure, I'm currently still debugging to get an idea what a "correct input" would look like.

OPUS has test vectors available: https://opus-codec.org/docs/

@Sean-Der
Copy link

Hey @thinking-tower @pdeljanov I would love to help with this.

I started a Opus decoder for Go pion/opus, SILK works great. I haven't started CELT.

https://github.com/lu-zero/opus is MIT licensed and I learned a lot from it.

I would love to help with this so I can use it with https://github.com/webrtc-rs/webrtc

@pdeljanov
Copy link
Owner

Hi @Sean-Der,

That looks very cool! We've had a few false starts with Opus so far, but I'd really like to have support for it. Would love to have you on board.

My immediate goal is going to be addressing long-standing Symphonia's API issues. I think this is required for the health of the project. So I think the best strategy would be if we rebase an Opus branch to master and then let you and @thinking-tower work on it. I'll probably need to be hands off until the API changes are complete. However, I don't expect many (if any) decode API changes so it should be easy to merge when it comes time.

@Sean-Der
Copy link

Hi @thinking-tower are you still interested in working on this? I would love to follow your lead.

I could maybe sketch up a rough plan/explain how I did my Opus impl?

thanks!

@pdeljanov
Copy link
Owner

Hey @Sean-Der,

Since there hasn't been any activity on this for a while, I think you may have to take the reins on it. If @thinking-tower returns, then I'm sure you could both work together. You have a really great foundation to reference with your existing Go implementation.

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