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

hifi resampler #670

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

hifi resampler #670

wants to merge 6 commits into from

Conversation

dvdsk
Copy link
Collaborator

@dvdsk dvdsk commented Jan 4, 2025

This is a mess, it is far from done and going to require significant changes to the core of rodio. I am opening this PR mainly so I have a place to dump notes.

@dvdsk
Copy link
Collaborator Author

dvdsk commented Jan 4, 2025

  • trying to get a unified interface to resamplers. Matching rubato's interface to the current inhouse solution does not work. Rubato always outputs f32, inhouse matches input. You can not have an Iterator that sometimes outputs a fixed type and sometimes is generic (generic generics don't exist). Possible solution: do away with DataConverter and make the resampler convert to the target sample type. Now both rubato and inhouse are generic, both output the target sample type
  • rubato allocates a significant amount of data. Currently resamplers are
    guarenteed a fixed number of channels and sample rate. The cost of that is
    that they are re-created every frame. For rubato that cost is too high.
    Solutions:
    • instead of into_inner provide an into_source_and_parts. Split resamplers
      into Parts and the Resampler. Only creating Parts allocates. Use the
      parts to generate a new resampler for each frame.
      Advantage: few changes from current UniformSourceIterator. No need to
      change ChannelConvertor.
    • do not recreate the resampler and channel converter every frame instead make them
      implement Source and respond to changes in number of channels and
      sample-rate.
      Advantage: Rubato can natively respond to changing sample-rate, while inhouse
      could be wrapped with current solution. Probably better performance.
      Disadvantage: ChannelConvertor needs to be changed

dvdsk added 2 commits January 4, 2025 15:19
Matching rubato's interface to the current inhouse solution does not
work. Rubato always outputs f32, inhouse matches input. You can not have
an Iterator that sometimes outputs a fixed type and sometimes is generic
(generic generics don't exist).

Solution: do away with DataConverter and make the resampler convert to
the target sample type. Now both rubato and inhouse are generic, both
output the target sample type
@roderickvd
Copy link

Probably you could also make hifi_rubato work over all sample types by making it generic over Sample and calling to_sample on its input and output? For Sample<f32> that will be zero-cost and for the others you need conversion anyway.

I am not against generics when they are fairly readable. Like the Sample type, which is also intuitive.

Making the sample converter more flexible and able to hold objects I also like, because there may be more use cases. For example: dithering, that would needed to be done in that step and where you also want to store a Rng and a noise buffer.

@dvdsk
Copy link
Collaborator Author

dvdsk commented Jan 6, 2025

Probably you could also make hifi_rubato work over all sample types by making it generic over Sample and calling to_sample on its input and output? For Sample<f32> that will be zero-cost and for the others you need conversion anyway.

Thats sort of what this does, rubato works on all sample types with this PR. To get a single resampler trait you have to do output conversion in the same trait implementation as you do resampling. That is needed since the inhouse fully generic resampler returns the same output type as you pass in.

Basically its a limitation of the rust type system. As far as I know you can not have a trait that expands iterator and has both:

  • Iterator::Item always f32 regardless of the input, and
  • Iterator::Item matching the input type.

Regarding calling to sample on rubato output to make its output generic. Yes you can do that but now you are converting between types more then necessary. The compiler is not allowed to 'optimize' that out since it has effects.

Anyway I propose we halt the discussion and work on this till we have figured out if the generics even stay. If they do not and the pipeline becomes completely f32 this PR can be massively simplified.

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

Successfully merging this pull request may close these issues.

2 participants