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

Dynamic lifetime issues and the node collection #469

Merged
merged 4 commits into from
Mar 6, 2024

Conversation

orottier
Copy link
Owner

Apologies for the complex PR. I will let it simmer for a while and put some more thought in it, but wanted to share already.

Introduce AudioProcess::has_side_effects

A processor with no side effect will be dropped when the control handle
is gone and there are not output ports connected.

This makes the cleanup of AudioParams easier and more robust.

Relates to #397 and #468

@orottier orottier requested a review from b-ma February 29, 2024 12:31
orottier added 4 commits March 6, 2024 08:34
This issue arises when a graph involves nodes connecting to audio
params.  E.g. constant src -> playback rate -> buffer src.

The playbackrate param is dropped alongside the buffer src if it has
ended, but this leaves an edge from the constant src in place.

To solve this we probably need a bit more refactoring, stay tuned
Introduce AudioProcess::has_side_effects

A processor with no side effect will be dropped when the control handle
is gone and there are not output ports connected.

This makes the cleanup of AudioParams easier and more robust.

Relatest to #397 and #468
@orottier orottier force-pushed the bugfix/panics-in-nodecollection branch from 88e103c to 85d873b Compare March 6, 2024 07:36
@orottier
Copy link
Owner Author

orottier commented Mar 6, 2024

/bench

Copy link

github-actions bot commented Mar 6, 2024

Benchmark result:


bench_ctor
  Instructions:             2761321 (-1.472882%)
  L1 Accesses:              4178740 (-1.329129%)
  L2 Accesses:                27509 (+0.043641%)
  RAM Accesses:               61624 (-0.019469%)
  Estimated Cycles:         6473125 (-0.867549%)

bench_audio_buffer_decode
  Instructions:             7372803 (-0.000163%)
  L1 Accesses:             10167726 (-0.001465%)
  L2 Accesses:                28338 (+0.514312%)
  RAM Accesses:               32160 (-0.027977%)
  Estimated Cycles:        11435016 (+0.002283%)

bench_sine
  Instructions:            68776515 (-0.320959%)
  L1 Accesses:            100311675 (-0.302505%)
  L2 Accesses:               267026 (+1.560533%)
  RAM Accesses:               62476 (-0.024003%)
  Estimated Cycles:       103833465 (-0.273132%)

bench_sine_gain
  Instructions:            73027923 (-0.429922%)
  L1 Accesses:            106810892 (-0.394051%)
  L2 Accesses:               269252 (-0.599169%)
  RAM Accesses:               62572 (-0.019174%)
  Estimated Cycles:       110347172 (-0.389146%)

bench_sine_gain_delay
  Instructions:           105860309 (-0.384253%)
  L1 Accesses:            153172557 (-0.358576%)
  L2 Accesses:               504111 (-0.640178%)
  RAM Accesses:               64129 (-0.020268%)
  Estimated Cycles:       157937627 (-0.358291%)

bench_buffer_src
  Instructions:            18298022 (-1.075672%)
  L1 Accesses:             26870516 (-0.997920%)
  L2 Accesses:               138011 (+3.236737%)
  RAM Accesses:              100672 (-0.020855%)
  Estimated Cycles:        31084091 (-0.797692%)

bench_buffer_src_delay
  Instructions:            75266383 (-0.382703%)
  L1 Accesses:            106373406 (-0.375810%)
  L2 Accesses:               342416 (+3.239345%)
  RAM Accesses:              100821 (-0.019833%)
  Estimated Cycles:       111614221 (-0.311042%)

bench_buffer_src_iir
  Instructions:            80302608 (-0.275095%)
  L1 Accesses:            114705628 (-0.258431%)
  L2 Accesses:               133766 (+0.455846%)
  RAM Accesses:              100748 (-0.019848%)
  Estimated Cycles:       118900638 (-0.247382%)

bench_buffer_src_biquad
  Instructions:            57683971 (-0.845705%)
  L1 Accesses:             77882678 (-0.780861%)
  L2 Accesses:               182297 (-3.689243%)
  RAM Accesses:              100870 (-0.015859%)
  Estimated Cycles:        82324613 (-0.781479%)

bench_stereo_positional
  Instructions:            95544000 (-1.573564%)
  L1 Accesses:            139796232 (-1.473760%)
  L2 Accesses:               399463 (+4.077235%)
  RAM Accesses:              100998 (-0.014850%)
  Estimated Cycles:       145328477 (-1.366454%)

bench_stereo_panning_automation
  Instructions:            29485493 (-0.971009%)
  L1 Accesses:             44416339 (-0.905535%)
  L2 Accesses:               159628 (+10.69212%)
  RAM Accesses:              100786 (-0.021824%)
  Estimated Cycles:        48741989 (-0.671583%)

bench_analyser_node
  Instructions:            37441279 (-0.588119%)
  L1 Accesses:             52473369 (-0.567977%)
  L2 Accesses:               187114 (+1.717821%)
  RAM Accesses:              101280 (-0.015795%)
  Estimated Cycles:        56953739 (-0.497044%)

bench_hrtf_panners
  Instructions:           320109462 (-0.084709%)
  L1 Accesses:            562456729 (-0.064506%)
  L2 Accesses:             10853194 (-0.099714%)
  RAM Accesses:              120791 (-0.076107%)
  Estimated Cycles:       620950384 (-0.067663%)

bench_sine_gain_with_worklet
  Instructions:            73521626 (-0.396689%)
  L1 Accesses:            107450201 (-0.354992%)
  L2 Accesses:               266740 (-1.431924%)
  RAM Accesses:               62747 (-0.015934%)
  Estimated Cycles:       110980046 (-0.361389%)


running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s


@orottier
Copy link
Owner Author

orottier commented Mar 6, 2024

Right, I think we should probably merge this as it solves a panic in the audio param dynamic lifetimes.
@b-ma did you manage to reproduce the panic you experienced in your complex graph? I hope this PR would solve it.

@b-ma
Copy link
Collaborator

b-ma commented Mar 6, 2024

No I didn't have time to make a simpler test bed, but I can test my app against this branch right now, the panic was rather easy to get, I let you now in the afternoon

@b-ma
Copy link
Collaborator

b-ma commented Mar 6, 2024

Just tested and I didn't manager to make it crash!

@b-ma
Copy link
Collaborator

b-ma commented Mar 6, 2024

Didn't understood all the details, but I will check that when needed and it looks all good to me

@orottier
Copy link
Owner Author

orottier commented Mar 6, 2024

Cool, good to hear!

@orottier orottier merged commit 8753217 into main Mar 6, 2024
3 checks passed
@orottier orottier deleted the bugfix/panics-in-nodecollection branch March 6, 2024 16:09
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.

2 participants