Voice is high pitched on Web.

MichealReed commented 3 months ago

It seems something changed with the data types after the PR? There are pitch changes happening with the recorder now like it sounds when someone inhales helium from a balloon.

DaniilAlpha commented 3 months ago

Does this happen in the example app?

Edit: This is due to mismatch of channel count and/or sample rate. I fixed the example, but until there will be a better way of getting sound from recorder, AudioData options should match recorder options.

MichealReed commented 3 months ago

now seeing compile issues on web and windows

web:

./lib/generator.dart:103:45: Error: The getter 'availableFloatCount' isn't defined for the class 'PlatformGenerator'.
 - 'PlatformGenerator' is from 'package:minisound_platform_interface/minisound_platform_interface.dart'
 ('/hosted/pub.dev/minisound_platform_interface-1.5.0/lib/minisound_platform_interface.dart').
Try correcting the name to the name of an existing getter, or defining a getter or field named 'availableFloatCount'.
  int get availableFloatCount => _generator.availableFloatCount;
                                            ^^^^^^^^^^^^^^^^^^^
../lib/recorder.dart:92:44: Error: The getter 'availableFloatCount' isn't defined for the class 'PlatformRecorder'.
 - 'PlatformRecorder' is from 'package:minisound_platform_interface/minisound_platform_interface.dart'
 ('hosted/pub.dev/minisound_platform_interface-1.5.0/lib/minisound_platform_interface.dart').
Try correcting the name to the name of an existing getter, or defining a getter or field named 'availableFloatCount'.
  int get availableFloatCount => _recorder.availableFloatCount;

Still many like this on Windows.

 error C2143: syntax error: missing '{' before '==' [minisound\minisound\example\build\windows\x64\plugins\minisound_ffi\shared\minisound_ffi.vcxproj]
\minisound\minisound\example\windows\flutter\ephemeral\.plugin_symlinks\minisound_ffi\src\src\sound.c(38,32): error C2059: syntax error: '==' [minisound\minisound\example\build\windows\x64\plugins\minisound_ffi\shared\minisound_ffi.vcxproj]

DaniilAlpha commented 3 months ago

Are you on dev branch? If not, you should run on the dev as master have published dependencies in its pubspecs

MichealReed commented 3 months ago

Tried from both dev and master. Both web and windows no longer compile. Can confirm the pubspec is setup for local.

DaniilAlpha commented 3 months ago

That's strange. I can build both for linux and for web normally. Have you cleaned the example project? Maybe try clean all the projects (example, minisound, platform interface, ffi and web)

MichealReed commented 3 months ago

It was a package cache problem, got web to build after incrementing to 1.5.1

Windows build still fails.
The recording playback is now broken entirely and results in a high pitched continuous buzz on web,
the Aborted(Assertion failed: notification.pDevice != ((void*)0), at: /home/Daniil/_work/_projects/_flutter/minisound/minisound_ffi/src/external/miniaudio/src/miniaudio.c,7218,ma_device__on_notification) error has returned
I can only get sounds to play if I do not try to access the recorder first, out of bounds and access errors have returned and you must use the -sfsanitize arg compile and linker arg to clean up the memory.

Not sure what all changed after the PR but maybe we should roll back to it? All tested platforms were very stable. I think maybe it was underestimated how many hours were spent resolving issues that have now returned from post-PR changes.

DaniilAlpha commented 3 months ago

No, i am just improving stuff incrementally. It was working on the default settings, but some code just was poorly written, and some was not accounting different channel count, and there were excessive memory allocations in some places. I tested the most of it on linux and can confirm that it works as it was before PR. There are just some fixes needed on the web. I always hated MSVC for its quirks, and here i think it either uses C++ mode, or does not support _Static_assert, which is used to spot problems if interface will be changed for some reason. Maybe its better to #if them for windows.

DaniilAlpha commented 3 months ago

Hey, your sound.c file which is reporting an error, seems outdated. Can you try building on windows after pulling dev branch?

MichealReed commented 3 months ago

I spent a week working on these additions, I have cleaned and built many times. After digging deeper, the issue is the new compile options and link flags that were added after the PR

[ ] cl : command line error D8016: '/RTC1' and '/Og' command-line options are incompatible

I think when you run emscripten with -O0 compile and -O2 link with -fsanitize on both you will see the memory issues that led me to some of my decisions for the PR. I think I will preserve the PR and changes and refactor some in a fork, it is hard to spend time troubleshooting incrementally when what I delivered was stable. Wish you well here 👍

DaniilAlpha commented 3 months ago

There was some opinion divergency, so maybe it's for the better. I'll finalize the feature though, possibly in a different way that you wanted it. Thanks anyway.

MichealReed commented 3 months ago

I would just say in retrospect that maybe in the future to not hold contributors to a higher standard than you hold yourself in your code/repository structure and to maybe not have someone refactor so much if you plan to undo and refactor extensively after without discussing impact with author. I spent significant time and contemplated starting my own implementation before contributing here, it might be better to reject to implement yourself and recommend a fork upfront if it diverges too far from what you have in mind.

I appreciate your ffi structure here, wish the collaboration could have gone better, and hope that the contributions are still valued in some way.

DaniilAlpha / minisound

Voice is high pitched on Web. #9