search

roc-streaming / roc-toolkit

Real-time audio streaming over the network.
https://roc-streaming.org
Mozilla Public License 2.0
1.06k stars 211 forks source link

Refactor ChannelMapper #621

Closed dshil closed 10 months ago

dshil commented 11 months ago

gh-86

Based on patch suggested in https://github.com/roc-streaming/roc-toolkit/pull/607.

gavv commented 11 months ago

Thanks for PR.

On one hand, it's nice that ChannelMapper::setup_mapmatrix() was split. On the other hand, the new approach looks over-complicated. IMHO there is now too much entities to keep track of when reading the code, and they're not just data but each one is new abstraction with its own API.

I'd prefer to keep it simpler: keep all state inside ChannelMapper, but split setup_mapmatrix() into multiple functions. I incorporated some of you refactorings (notably for building index maps) and implemented this approach here: https://github.com/roc-streaming/roc-toolkit/pull/626

Could you take a look?

gavv commented 10 months ago

New version is better! I've pushed follow-up refactoring to #626 (I dropped my old version, but have redone some renames I did there).