Add SampleSpec class with just members, constructor, getters and setters

roc-streaming / roc-toolkit

Real-time audio streaming over the network.

https://roc-streaming.org

Mozilla Public License 2.0

1.06k stars 213 forks source link

Add SampleSpec class with just members, constructor, getters and setters #430

Closed soapdogg closed 3 years ago

soapdogg commented 3 years ago

Pull request for issue #378

gavv commented 3 years ago

Thanks for the PR! I'll take a look in upcoming days.

The CI build is failing, this needs to be fixed. See 1 and 2 for hints how you can run everything locally.

Also please note that we should use snake_case for functions / methods / variables.

soapdogg commented 3 years ago

Thanks for the feedback. I'll send out a revision soon!

gavv commented 3 years ago

Hi, thanks again for the PR and sorry for delay. Big work!

You can find some comments below.

gavv commented 3 years ago

Also:

please fix travis build; the code should be compatible with old non-C++11 compilers; if desired, you can run travis build locally: https://roc-streaming.org/toolkit/docs/development/continuous_integration.html#run-locally
please go through all places where we pass SampleSpec to methods and ctors and replace SampleSpec or SampleSpec & with const SampleSpec &.
please go through all places where we declare SampleSpec field in a class and make it const if it's not modified after ctor

gavv commented 3 years ago

IFrameEncoder and IFrameDecoder (and their implementations) don't use sample rate, only channel mask. And it would be nice to make it clear that they only do channel remapping, but not rate conversion. For this reason, could we revert the change in those classes and keep them using channel_mask_t?

github-actions[bot] commented 3 years ago

:umbrella: The latest upstream change (presumably these) made this pull request unmergeable. Please resolve the merge conflicts.