Repository navigation
RTNR: Decouple switch-enable control from params blob - #5618
Conversation
There was a problem hiding this comment.
Just as I mentioned in https://github.com/thesofproject/sof/pull/5616/files#r838305750
I am worried about that rtnr_copy_func may be out of sync. What would happen if rtnr_copy_func is changed while it is under processing?
Will need the comment from the expert.
There was a problem hiding this comment.
How about using single copy_func, but check the process_enable flag inside copy_func?
There was a problem hiding this comment.
Yes that would be better than the current code. rtnr_set_value() only do setting cd->process_enable value.
In rtnr_copy(), check cd->process_enable to determine the path. I think it could minimize the risk of asynchronous issue.
A quick question, when this is implemented, it means we can switch path in units of copy() (data sample chunk in one scheduled task). Would rtnr effect be degraded since it will obtain intermittent data samples, e.g. enabled during 1st to 100th task and then bypassed, re-enabled from 200th task. Fow now I could think of some factors which may drop the effect and even generate wrong output data.
- is there sample latency in rtnr? That is, the processed data has a certain delay than the source data.
- due to the discontinuity of data sample chunks, the waveform could be distorted if some algorithm uses a processing window larger than one chunk.
That's also the reason we tend to change path/config during the component restart (reset and prepare). In so doing the component actually regard each segment as an individual stream.
There was a problem hiding this comment.
Hi Johny,
There will be latency, but in overall it should be acceptable unless the user is toggling on&off rapidly.
e1707f9 to
13bfa37
Compare
There was a problem hiding this comment.
as much as this is appreciated as this reduces the call stack, I don't think this i needed. We just need the control interface modified.
There was a problem hiding this comment.
@cujomalainey Do you mean that we should use the original bypass path provided by RTNR, instead of introducing a new one?
There was a problem hiding this comment.
yea i don't see any issue with using that. the concern was the TLV interface only.
There was a problem hiding this comment.
Force-pushed a new commit. Please check it, thanks.
29039ef to
f1e2bc4
Compare
|
Can one of the admins verify this patch? |
There was a problem hiding this comment.
why not just use !val for everything here so we can remove the if?
f1e2bc4 to
32be360
Compare
|
SOFCI TEST |
|
Unblock the CI. |
This PR is based on thesofproject#5616 It decouples the switch-typed control for enabling/disabling from the control bytes. Previously, control blobs should be sent from userspace to to toggle RTNR on/off. With this PR, RTNR can be toggled by setting the UCM switch value. cset "name='RTNR10.0 rtnr_enable_10' off" cset "name='RTNR10.0 rtnr_enable_10' on" or from command line amixer -c0 cset name='RTNR10.0 rtnr_enable_10' 1 amixer -c0 cget name='RTNR10.0 rtnr_enable_10' 0 Signed-off-by: Pin-chih Lin <johnylin@google.com> Signed-off-by: Ming Jen Tai <mingjen_tai@realtek.com> merge copy_func
32be360 to
947867d
Compare
@lgirdwood Can this PR be merged now? Thanks! |
5 participants
This PR is based on #5616
It decouples the switch-typed control for enabling/disabling from the control bytes.
Previously, control blobs should be sent from userspace to to toggle RTNR on/off.
With this PR, RTNR can be toggled by setting the UCM switch value.
cset "name='RTNR10.0 rtnr_enable_10' off"
cset "name='RTNR10.0 rtnr_enable_10' on"
or from command line
amixer -c0 cset name='RTNR10.0 rtnr_enable_10' 1
amixer -c0 cget name='RTNR10.0 rtnr_enable_10' 0
Signed-off-by: Pin-chih Lin johnylin@google.com
Signed-off-by: Ming Jen Tai mingjen_tai@realtek.com