search

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

Introduce SampleSpec lcass #444

Closed ghost closed 3 years ago

ghost commented 3 years ago

Pull request for issue #378

Hi Victor, I checked if we could remove packet::num_channels() completely and put it into SampleSpec. I saw that in some cases, num_channels() is being used only with channel mask (e.g. test_pcm_funcs.cpp:79)

Let me know so I can make the changes accoridngly!

gavv commented 3 years ago

Hi Victor, I checked if we could remove packet::num_channels() completely and put it into SampleSpec. I saw that in some cases, num_channels() is being used only with channel mask (e.g. test_pcm_funcs.cpp:79)

Regarding these usages.

In packet_writer.h and packet_reader.h I think we can replace channel mask with either sample spec or number of channels. It seems we already have both defined in roc_pipeline tests which are using packet writer and reader.

In test_sox_sink.cpp we can make FrameDuration a field instead of a constant and use sample_spec to compute it. We already have sample_spec in setup() method.

It seems the remaining usages are only two: test_frame_encoder_decoder.cpp and test_pcm_funcs.cpp. Both are in roc_audio tests. So I think we can just copy-paste packet::num_channels() to a static function in these files, or maybe place it to src/tests/roc_audio/test_helpers.

This way we can get rid of packet::num_channels() by the cost of duplicating channel calculation logic in SampleSpec and a couple of tests. The benefit is that we can be sure that, except tests, there is only one way to work with sample rate and channel mask - SampleSpec - and all components have to use it. I think it's worth it.

ghost commented 3 years ago

Hi Victor, I made the changes according to your review. Let me know if I need to change something else as well!

Edit: I added a num_channels to test_units.cpp, but I see fmt is messing with the format. I get CI errors if I add test_units.cpp to .fmtignore (see this commit). Maybe you have an idea on how to resolve this?

gavv commented 3 years ago

@nythoang Sorry for so long delay a thanks a lot for your work! I finally found time to review the PR carefully. BTW, this PR is quite important because it's the first step to implement #86, which is a must-have. So, thanks!

I've found two minor problems in the new pipeline construction code of ReceiverSession. Since you were waiting for review for so long, I'm going to merge this now and prepare a small follow-up commit by myself. I'll fix the formatting issue as well. Will post it here soon.

gavv commented 3 years ago

Here are the follow-up commits: