-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[core] audio speed change does not affect pitch #244
base: master
Are you sure you want to change the base?
[core] audio speed change does not affect pitch #244
Conversation
src/switch_ivr_play_say.c: In function 'switch_ivr_play_file': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you update patch to match coding guidelines, particularly i notice white space around if/else
Uhh, I have completely missed that. Corrected. |
wondering if this has any effect upon your patch: https://github.com/signalwire/freeswitch/pull/342/files |
I think not. I have just merged that into this PR. |
Would it be possible to have this reviewed? Not the most crucial of features, but other alternatives are slow and may not even work real-time (e.g., |
Is there a technical reason why this hasn't been merged? I know that it is being used in production in a patched fork of FreeSWITCH but it would be nice to have this in the main one. Also, the requested change has been done months ago. Thank you! |
Thread on the freeswitch-users mailing list: |
can you rebase and squash it, so its just one commit ? |
Hi @dragos-oancea ! Sorry for the delay, I moved away from the Telecom industry, but I am keen on working on this PR as I understand how important it is for @toraritte's use case. Unfortunately, I am not that experienced with git. I have read about the squash / rebase workflow in general (for example here, but what happens with the merge commits? When I hit |
9ae7adf
to
3f2f1e6
Compare
Hi @dragos-oancea , squashed and rebased. If merged, #649 may also be closed. Thanks! |
Scan-build found bugs: https://public-artifacts.signalwire.cloud/drone/signalwire/freeswitch/372/index.html |
"Dead store Dead assignment switch_ivr_play_say.c switch_ivr_play_file 1841 1 View Report" . @toraritte |
This will require a test before we merge it- to exercise the code so we can check for runtime memory problems. |
@dragos-oancea any interest in giving this a try- just a simple test in FS core to execute the function so ASAN can check it. We can listen to the recording generated by the test to spot check the result. |
it's leaking in 2 places, lines 1800 and 1803 . https://github.com/signalwire/freeswitch/pull/244/files#diff-2c79cd0fa8304ec5c3a6352dfe031b430e19db9b90e82d24bef823944fd0911eR1803
But indeed, the pitch during speed-up seems preserved by my ears (tried both the original and with this branch). |
sp_prev_idx += olen; | ||
|
||
if (cont) { | ||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
switch_safe_free(data);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch, thanks!
src/switch_ivr_play_say.c
Outdated
int extra = datalen - olen; | ||
short* data = NULL; | ||
short* currp = NULL; | ||
switch_zmalloc(data, datalen * 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
malloc on every frame seems wasteful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure how to improve this as the size is not constant. What do you suggest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
heap buf max sized with realloc or a static buf sized to SWITCH_RECOMMENDED_BUFFER_SIZE which is designed to fit sane data frames.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I have switched to using a static buffer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rucc looks like you missed on removing the switch_zmalloc
and the data
variable, it is not needed anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ar45 Thanks for your suggestions... data is still needed as a buffer but zmalloc is not needed. I made some changes here
yois615@96ad560
I will open a PR against @rucc branch
src/switch_ivr_play_say.c
Outdated
short sp_has_overlap = 0; | ||
int sp_prev_idx = 0; | ||
int sp_prev_cap = 0; | ||
short* sp_prev = NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to make sure switch_safe_free(sp_prev) is done every time this loops (continue, break, end of statement)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be enough to free only after the 'inner' cycle ends (there are no jumps from the inner cycle)? (as I did it in my latest rev)
Offering a bounty of $150 to fix this patch and make it ready to merge. |
d2e5ca5
to
1c9e61c
Compare
@dragos-oancea That leak check is awesome! I am impressed. I hope there are no more left after my last update. |
@rucc as soon as it gets approved I will reach out to you about the bounty payment, I really hope this gets in soon as the difference it makes to playback with speed change is just impressive. |
If time permits, would you please review this? Thank you. |
src/switch_ivr_play_say.c
Outdated
@@ -1929,6 +1986,10 @@ SWITCH_DECLARE(switch_status_t) switch_ivr_play_file(switch_core_session_t *sess | |||
break; | |||
} | |||
} | |||
if (sp_prev) { | |||
switch_safe_free(sp_prev); | |||
sp_prev = NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't have to do this, switch_safe_free(sp_prev) would set sp_prev to NULL after it frees it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, this was redundant, a simple switch_safe_free(sp_prev);
is enough. This is no longer an issue since the sp_prev was moved to static storage.
src/switch_ivr_play_say.c
Outdated
int extra = datalen - olen; | ||
short* data = NULL; | ||
short* currp = NULL; | ||
switch_zmalloc(data, datalen * 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rucc looks like you missed on removing the switch_zmalloc
and the data
variable, it is not needed anymore.
src/switch_ivr_play_say.c
Outdated
|
||
|
||
if (cont) { | ||
switch_safe_free(data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove line switch_safe_free(data);
src/switch_ivr_play_say.c
Outdated
last_speed = fh->speed; | ||
switch_safe_free(data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove switch_safe_free(data);
line
src/switch_ivr_play_say.c
Outdated
short sp_ovrlap[32]; | ||
short sp_has_overlap = 0; | ||
int sp_prev_idx = 0; | ||
short sp_prev[SWITCH_RECOMMENDED_BUFFER_SIZE]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe initialize to 0?
short sp_prev[SWITCH_RECOMMENDED_BUFFER_SIZE] = { 0 };
bp++; | ||
if (extra > 0) { | ||
int cont = 0; | ||
currp = data; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, something is off here too
we'd like to get this merged, but the patch is very intrusive - it would be much easier to get it merged if most of this code would be enabled through some sort of flag. |
a channel flag perhaps. |
It would probably be, but never heard of a bugfix being made optional by a flag. Or do you mean like a flag for an experimental feature, because one does not know what this change may cause in the long term? This I understand, but it would have to be an interim solution nonetheless. Is my thinking flawed? That is, if a PR introduces regressions or causes behaviour in a FreeSWITCH instance then such a PR would never be accepted, flag or no flag. Please throw some crumbs as to how this change is so intrusive; no long winded report need, links to affected source files, module, anthying would suffice to even start understand the scale of disruption this PR would cause. |
Relating to signalwire#244, this commit makes better use of stack and heap memory allocations
Relating to signalwire#244, this commit makes better use of stack and heap memory allocations
Relating to signalwire#244, this commit makes better use of stack and heap memory allocations
@dragos-oancea I've pushed new changes regarding memory allocation. Can you have CI run the unit tests here? It seems like it's blocked. And can you and/or @crienzo review? |
bf4f13a
to
3d09804
Compare
Relating to signalwire#244, this commit makes better use of stack and heap memory allocations
3d09804
to
1d65f6e
Compare
1d65f6e
to
ae2e4e8
Compare
We can merge this as long as we are now sure that it's stable and does no harm when not enabled. Since the code path is limited to people who purposely turn it on, it's probably safe, and they will tell us if it's not working. When we merge, fix the last few places where there are missing spaces and usage of "short" (ambiguous type) short* foo |
ae2e4e8
to
af5b28e
Compare
@anthmFS - Thanks. I ran this through ASAN and it comes back clean. Correct, this code branch only runs if (fh->speed !=0) short has been replaced with int16_t and int where appropriate. A redundant if statement checking (fh->speed) again has been removed. Whitespace cleaned up. Note that I only made these changes today. They are tested and work but this doesn't have the benefit of running in my prod environment for the last few weeks. |
af5b28e
to
f3d032d
Compare
Would you please merge this or raise actionable objections? |
@toraritte I mentioned this PR at an Office Hours about a year ago, and @andywolk was concerned about some malloc's that aren't explicitly freed. I mentioned that ASAN doesn't show any issue, and I've had this running in prod for 2 years with no sign of memory leak. I have little interest in recoding/recompiling at this point but if someone wants to 'fix' the issue with unfreed memory it should allow this to be merged. |
The code as it is now segfaults with heap buffer overflow at line 1949 from the buffer created in 1888. This took me a lot of effort to reproduce, this condition is not hit normally, but if I just keep returning random speed, volume, and seek values it eventually will hit on a forward seek, I think only when the audio is transcoded.
|
When a file is being played with the playback application and the speed is changed with eg. uuid_fileman speed:+1 the audio pitch increases. This commit tries to preserve the pitch even when the speed is changed. How? + Use longer sample chunks (where original freq is preserved) + Search for the best cut position using cross-correlation + Crossfade adjacent chunks to each other This updated co-authored commit from the original attempts to optimize memory usage by allocating memory on the heap for samples of the previous frame, but using stack memory for the modified data that is flushed to the buffer. Co-authored-by: hari <[email protected]> Co-authored-by: Joseph Nadiv <[email protected]>
f3d032d
to
bc6842a
Compare
The logic at 1947 was updated to check if newlen is < sp_fadeLen. newlen gets reset to 1 on speed change which is the way to occasionally reproduce this bug. |
When a file is being played with the playback application and the speed is changed with eg. uuid_fileman speed:+1 the audio pitch increases. This PR tries to preserve the pitch even when the speed is changed.
How?
Use longer sample chunks (where original freq is preserved)
Search for the best cut position using cross-correlation
Crossfade adjacent chunks to each other
This is basically a copy of my unreviewed PR from the old system