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

Recording options and Bug Fixes #108

Merged
merged 29 commits into from
Apr 26, 2024
Merged

Conversation

borrmann
Copy link
Contributor

@borrmann borrmann commented Mar 26, 2024

Adds support for RecordingOptions and addresses the following issues

Fixes #103
Fixes #104
Fixes #73 (for iOS)
Fixes #49
Fixes #32 (for Android, iOS seemed to have worked already, did not look into windows)

I have added the fix to set the speed on iOS, but I think an API change could be considered for setting the speed in the future.
On android setting the speed is via a method and not a setter, so I think it should be changed to a method instead a setter as well.

@jfversluis
Copy link
Owner

Wow great work @borrmann I will review asap!

Copy link
Owner

@jfversluis jfversluis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor things for the first pass!

src/Plugin.Maui.Audio/AudioPlayer/AudioPlayer.macios.cs Outdated Show resolved Hide resolved
src/Plugin.Maui.Audio/AudioPlayer/AudioPlayer.macios.cs Outdated Show resolved Hide resolved
src/Plugin.Maui.Audio/AudioRecordingOptions.cs Outdated Show resolved Hide resolved
src/Plugin.Maui.Audio/AudioRecordingOptions.cs Outdated Show resolved Hide resolved
src/Plugin.Maui.Audio/AudioRecordingOptions.cs Outdated Show resolved Hide resolved
src/Plugin.Maui.Audio/AudioRecordingOptions.cs Outdated Show resolved Hide resolved
src/Plugin.Maui.Audio/AudioRecordingOptions.cs Outdated Show resolved Hide resolved
@bijington
Copy link
Collaborator

This is going to conflict with #101 in some areas. I would personally prefer to get 101 in first and then that solves the recording bug. I'll be happy to assist with whatever merge conflicts arise from that.

borrmann and others added 13 commits March 29, 2024 17:29
Co-authored-by: Gerald Versluis <gerald@verslu.is>
Co-authored-by: Gerald Versluis <gerald@verslu.is>
Co-authored-by: Gerald Versluis <gerald@verslu.is>
Co-authored-by: Gerald Versluis <gerald@verslu.is>
Co-authored-by: Gerald Versluis <gerald@verslu.is>
Co-authored-by: Gerald Versluis <gerald@verslu.is>
Co-authored-by: Gerald Versluis <gerald@verslu.is>
Co-authored-by: Gerald Versluis <gerald@verslu.is>
Co-authored-by: Gerald Versluis <gerald@verslu.is>
@borrmann
Copy link
Contributor Author

borrmann commented Apr 2, 2024

I added support for windows and added two more encodings: Alac (Apple Lossless) and Flac. Those work on iOS and Windows, but not Android.

@jfversluis
Copy link
Owner

OK so some conflicts here because I merged the other one, but I'd really love to have this!

Can we work these out? @borrmann are you still interested? Else we'll just take it from here and get this over the finish line :)

@bijington
Copy link
Collaborator

OK so some conflicts here because I merged the other one, but I'd really love to have this!

Can we work these out? @borrmann are you still interested? Else we'll just take it from here and get this over the finish line :)

Considering my PR caused the conflicts in more than happy to help out if required

@borrmann
Copy link
Contributor Author

borrmann commented Apr 23, 2024

@jfversluis @bijington no worries. I'll look into it asap. Unless you prefer to take control, no issue with that either.

@jfversluis
Copy link
Owner

I'd love for you to take it home as its your brain child! But if you feel it's going to take too long (whatever that is) or you don't have the time right now please just let us know and we'll take it!

@borrmann
Copy link
Contributor Author

@bijington I merged your changes into my PR. When starting a Recording session, the app crashes on iOS with:

[0:] An error occurred: 'The operation couldn’t be completed. (OSStatus error -50.)'. Callstack: '   at Plugin.Maui.Audio.ActiveSessionHelper.InitializeSession(BaseOptions options) in C:\Users\maxbo\source\repos\borrmann\Plugin.Maui.Audio\src\Plugin.Maui.Audio\ActiveSessionHelper.macios.cs:line 15
   at Plugin.Maui.Audio.AudioRecorder.StartAsync(String filePath, AudioRecordingOptions options) in C:\Users\maxbo\source\repos\borrmann\Plugin.Maui.Audio\src\Plugin.Maui.Audio\AudioRecorder\AudioRecorder.macios.cs:line 43
   at Plugin.Maui.Audio.AudioRecorder.StartAsync() in C:\Users\maxbo\source\repos\borrmann\Plugin.Maui.Audio\src\Plugin.Maui.Audio\AudioRecorder\AudioRecorder.macios.cs:line 33
   at Plugin.Maui.Audio.Sample.ViewModels.AudioRecorderPageViewModel.Start() in C:\Users\maxbo\source\repos\borrmann\Plugin.Maui.Audio\samples\Plugin.Maui.Audio.Sample\ViewModels\AudioRecorderPageViewModel.cs:line 170
   at System.Threading.Tasks.Task.<>c.<ThrowAsync>b__128_0(Object state)
   at Foundation.NSAsyncSynchronizationContextDispatcher.Apply() in /Users/builder/azdo/_work/1/s/xamarin-macios/src/Foundation/NSAction.cs:line 176
   at UIKit.UIApplication.UIApplicationMain(Int32 argc, String[] argv, IntPtr principalClassName, IntPtr delegateClassName) in /Users/builder/azdo/_work/1/s/xamarin-macios/src/UIKit/UIApplication.cs:line 58
   at UIKit.UIApplication.Main(String[] args, Type principalClass, Type delegateClass) in /Users/builder/azdo/_work/1/s/xamarin-macios/src/UIKit/UIApplication.cs:line 94
   at Plugin.Maui.Audio.Sample.Program.Main(String[] args) in C:\Users\maxbo\source\repos\borrmann\Plugin.Maui.Audio\samples\Plugin.Maui.Audio.Sample\Platforms\iOS\Program.cs:line 13
   at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args)
   at System.Reflection.MethodBaseInvoker.InvokeDirectByRefWithFewArgs(Object obj, Span`1 copyOfArgs, BindingFlags invokeAttr)'

I don't think line 15 in ActiveSessionHelper should throw an error, and rather trace a warning

@bijington
Copy link
Collaborator

@borrmann Sorry I hope I haven't left things in a state for you. Let me know if I can assist with any merging, etc.

@borrmann
Copy link
Contributor Author

no problem, I replaced it with Trace (for now) to ensure there is no app crash so I could test my changes. I think it makes sense you take it from there, since the error could mean, that the options are not always applied as intended, but that should not effect this PR. I saw you were planning to add some features to the sample to play around with the different options anyways, so that should help to figure out if the error does effect the behavior.

Aside from that, I did quick tests on Windows, iOS and Android (Emulator). I think the changes from this PR work as inteded.

@bijington @jfversluis let me know if anything else is needed for this. I did notice Windows does not reliably play the audio file. If it does, position and duration are not updated. I don't think that has something to do with my changes here, since this shoul not have effected the Windows Audio player at all. If windows works as expected on your main, please let me know.

However, I think I might have worked on these issues in my other PR, so I will update that and check whether the changes there resolved these issues already.

@bijington
Copy link
Collaborator

Thanks. I'll have to investigate the issue as I suspect something must be wrong there. I can do that outside of this PR as you suggested

@bijington
Copy link
Collaborator

Thanks. I'll have to investigate the issue as I suspect something must be wrong there. I can do that outside of this PR as you suggested

I have fixed the issue here: #111

@jfversluis
Copy link
Owner

Thank you so much for this and seeing this through @borrmann!

@jfversluis jfversluis merged commit d88f00d into jfversluis:main Apr 26, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants