Plugin.Maui.Audio icon indicating copy to clipboard operation
Plugin.Maui.Audio copied to clipboard

Silence detection sample

Open ArturWyszomirski opened this issue 1 year ago • 9 comments

For clarity I've made a separate page for silence detection sample, but I can also merge it into existing audio recorder page if you prefer.

ArturWyszomirski avatar Sep 11 '24 08:09 ArturWyszomirski

I think the separate page works fine, thank you!

jfversluis avatar Sep 11 '24 12:09 jfversluis

@ArturWyszomirski thank you so much for this effort! I really like the concept.

I am wondering if we can consider refactoring this implementation down to something that might be a little easier to use. This is fully intended as a discussion and not me telling you want should be done so please feel free to push back if you don't agree :).

I am thinking of potentially dropping the DetectSilenceAsync in favour of one or both of the following:

Enhance StartAsync

We could introduce a parameter to the StartAsync method that will allow a user to define recording until silence is detected.

Example usage would be:

await audioRecorder.StartAsync(Until.SilenceIsDetected(thresholdOf: 0.05, forAtLeast: TimeSpan.FromSeconds(1)));

Enhance StopAsync

We could introduce a parameter to the StopAsync method that will allow a user to state that the recording will stop when silence is detected.

await audioRecorder.StopAsync(When.SilenceIsDetected(thresholdOf: 0.05, forAtLeast: TimeSpan.FromSeconds(1)));

With the Until or When classes providing the ability to provide a rule for when the recording would stop. This could allow us to introduce further rules down the line like When.TimeHasElapsed(TimeSpan.FromSeconds(10)) or When.RecordingReachesLengthOf(TimeSpan.FromSeconds(10)).

I think I personally prefer enhancing just the StopAsync method but I know that my thoughts don't always correlate with all users.

So to summarise I am not suggesting we remove any of the key logic just how we expose it to the users.

What do you think?

bijington avatar Sep 12 '24 18:09 bijington

@bijington your proposal looks super cool! The syntax is so much readable that even my wife is able to get it :P Yet, I am not sure if I got your idea right, so below I have linked my implementation where I created a static class When with a static method IsSilenceDetected providing parameters for silence detection. Let me know if this approach is correct?

And, please, don't ever hesitate to make any reviews, remarks and suggestion :) I am very grateful for every kind of feedback on my work, especially the one which levers up my skills and way of thinking about designing software.

https://github.com/ArturWyszomirski/Plugin.Maui.Audio/compare/silence-detection-documentation...ArturWyszomirski:Plugin.Maui.Audio:silence-detection-enhanced-start-stop

ArturWyszomirski avatar Sep 15 '24 12:09 ArturWyszomirski

@ArturWyszomirski thank you, that is nice to hear! I assume your wife is non-technical? If so then I guess the point kind of works, the aim is to make some readable and therefore developers don't have to think about what the code does as it should be descriptive itself.

I have created this PR: https://github.com/ArturWyszomirski/Plugin.Maui.Audio/pull/1

To show what I was thinking. This does a bit more than your PR originally intended but it leaves it open for other ways to potentially control how a recording is stopped.

What do you think?

bijington avatar Sep 15 '24 17:09 bijington

@bijington i think: WOW! I am truly impressed by your solution!

However, I was still concerned about canceling ongoing stop rules when one of them will be fulfilled (e.g. user push stop button and fire stop immediately rule when there is already awaited silence detection). So I came up with the idea that if one stop rule would be fulfilled, then all other should be cancelled, so API consumer won't have to bother about cancelling tasks (but, if he decide to do it on his own, the cancellation token is still exposed in EnforceStop parameters).

Let me know what do you think about it: https://github.com/bijington/Plugin.Maui.Audio/pull/1

ArturWyszomirski avatar Sep 16 '24 17:09 ArturWyszomirski

Thank you!

Oh sorry I realise that I didn't really update the sample app other than to allow it to compile. I have opened https://github.com/bijington/Plugin.Maui.Audio/pull/2 to show the changes that I think need to be made in order to satisfy your scenario. I'll be honest I haven't tested this but my reasoning is that the cancellationToken will throw an exception when cancelled if the code is still waiting for silence to be detected which will halt the When.SilenceIsDetected rule from completing, then the call to StopAsync without a When rule supplied (defaults to Immediately) will then cause the recording to stop.

Does this make sense? I could well be missing something

bijington avatar Sep 16 '24 18:09 bijington

@bijington em, I must have not been clear enough and you might missed some key parts of my pr: https://github.com/bijington/Plugin.Maui.Audio/pull/1/files

What I tried to achieve here is to follow to idea of making API user's life easier, so that he won't have to bother with task cancellation at all. I put the logic responsible for cancellation of all ongoing stop tasks inside the AudioRecorder class. If one reason for stop is fulfilled all other stop tasks will automatically be cancelled. In a result the viewmodel part could be as simple as that:

async Task StartStopRecordToggleAsync()
{
    if (!IsRecording)
    {
        await audioRecorder.StartAsync();

        audioSource = await audioRecorder.StopAsync(When.SilenceIsDetected(SilenceTreshold, SilenceDuration));
    }
    else
    {
        audioSource = await audioRecorder.StopAsync(When.Immediately());
    }
}

ArturWyszomirski avatar Sep 17 '24 07:09 ArturWyszomirski

Ah I understand your point now.

I know we didn't do this overly in the past but I believe it is best practice to allow developers to supply a cancellation token when providing an asynchronous method to give them the power to cancel it themselves. I like your idea but I think it lends itself better to a fire and forget model? By this I mean with your example you couldn't await the first Stop call otherwise this command method won't execute a second time (at least not by default if using something like RelayCommand) from the MVVM toolkit. This might be a bit of an edge case scenario though.

I'm wondering if there is some middle ground here although I'd be keen to hear what @jfversluis thinks on this topic?

bijington avatar Sep 17 '24 08:09 bijington

I left the cancellation token as a parameter of EnforceStop, so if someone will decide to use their own cancellation token, it is still possible to do. The automated cancellation of all stop tasks is fired only if cancellation token is left as default:

if (cancellationToken == default)
{
	cancellationToken = stopCancelTokenSource.Token;
}

bool isStopRuleFulfilled = await (stopRule ?? When.Immediately()).EnforceStop(this, cancellationToken);

if (isStopRuleFulfilled)
{
	stopCancelTokenSource.Cancel();
	stopCancelTokenSource = new();
}

I did some tests using community toolkit's RelayCommand and I didn't spot any issues besides the fact that it is not possible to make directly the toggle type button because the button stays inactive until the RelayCommand's method finishes its tasks. However I use this workaround to make start/stop button enabled all the time:

async Task StartStopRecordToggleAsync()
{
    if (!IsRecording)
    {
        MainThread.BeginInvokeOnMainThread(async () =>
        {
            await audioRecorder.StartAsync();
        
            audioSource = await audioRecorder.StopAsync(When.SilenceIsDetected(SilenceTreshold, SilenceDuration));
        }
    }
    else
    {
        audioSource = await audioRecorder.StopAsync(When.Immediately());
    }
}

ArturWyszomirski avatar Sep 17 '24 08:09 ArturWyszomirski

@jfversluis any hope this feature can be merged in? It would come in so handy with the openAI realtime API stuff

cbdg-mitch avatar Nov 24 '24 00:11 cbdg-mitch

@jfversluis any hope this feature can be merged in? It would come in so handy with the openAI realtime API stuff

There is no implementation for macOS or iOS in this PR. We would need to add that as a minimum to get this PR in

bijington avatar Nov 26 '24 19:11 bijington

@jfversluis @jfversluis we love this plugin and we used it our app , unfortunately some users complained that when the phone was on silent it still played the sound, is it possible to merge this feature , what we need if the phone is on silent dont play it.

hope make sense thanks

gabsamples6 avatar Mar 21 '25 14:03 gabsamples6

@jfversluis @jfversluis we love this plugin and we used it our app , unfortunately some users complained that when the phone was on silent it still played the sound, is it possible to merge this feature , what we need if the phone is on silent dont play it.

hope make sense

thanks

This PR will not solve your issue. Please open a separate issue covering your scenario and try to include all details such as what platform this happens on, etc.

bijington avatar Mar 21 '25 14:03 bijington

@bijington thanks for your prompt reply, will do

gabsamples6 avatar Mar 21 '25 17:03 gabsamples6

Thanks for your amazing work here! We have implemented this in #166 for all platforms!

jfversluis avatar Apr 09 '25 12:04 jfversluis