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

Numba-optimize findpulses and add inputfreq argument to LDDecode #768

Merged
merged 2 commits into from
Aug 7, 2022

Conversation

oyvindln
Copy link
Contributor

@oyvindln oyvindln commented Aug 7, 2022

Upstream the optimized version of findpulses used in vhs-decode - the function itself is significantly faster, but it doesn't seem to make a much difference on the overall speed when I tested.

  • Not using the lower threshold, not sure what it's purpose would be anyhow since if anything actually hits it it will probably mess up the computation in whichever version.
  • The min/max length args are used in vhs-decode but just set them to not have any effect here, removing the condition didn't seem to have any speed impact anyhow.
  • Benchmarking the func itself indicates it's 3-4x faster than the old one, probably still a lot of room for optimizing it more though. Ideally one would maybe re-use the array buffers between calls, use a vec-type data structure rather than list (not sure what the list in numba actually is implemented as) etc. There are other bottleneck functions that would benefit more from porting to numba/cython or other native code more though, major standout ones from some quick profiling is compute_line_bursts and downscale_audio.

Enable numba on unpack_data_4_40 (probably commented it out by accident when making the PR for that, makes it 2x as fast but the function time is pretty insignificant compared to the rest of the program anyhow.)

add inputfreq argument to LDDecode - mainly to optionally allow decoding at input frequency in vhs-decode rather than resampling to 40mhz which could have some speed benefits since many people capture at lower freqs with cx etc.

@atsampson
Copy link
Collaborator

Regarding the inputfreq change, that's how I did it originally back in 2019 (#247). I changed it to resample instead (#303) because the filters in ld-decode were all tuned with a sample rate of 40 MHz, and if you ran it at a different sample rate, then the output quality was significantly worse. It'd be worth checking whether that's still the case...

@oyvindln
Copy link
Contributor Author

oyvindln commented Aug 7, 2022

Yeah not planning to run at native sample rate by default, at least not any time soon, the change just allows passing a different sample rate to the constructor so the option is there for testing it.

@happycube happycube merged commit 8e3c161 into happycube:master Aug 7, 2022
@oyvindln oyvindln deleted the upstream_stuff branch August 7, 2022 23:11
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.

3 participants