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

feat(windows, linux) : Added setBalance method #1248

Merged
merged 13 commits into from
Sep 23, 2022

Conversation

novikov-studio
Copy link
Contributor

@novikov-studio novikov-studio commented Aug 17, 2022

Description

Added method:

/// -1 - The left channel is at full volume; the right channel is silent.
///  1 - The right channel is at full volume; the left channel is silent.
///  0 - Both channels are at the same volume.
AudioPlayer.setBalance(double value)

Platforms

Windows - yes
Linux - yes

Related Issues

closes #58

@Gustl22
Copy link
Collaborator

Gustl22 commented Aug 17, 2022

Nice 👍 I'd prefer to add the integration tests first in #1238 and then can add this feature to the example / tests, too.

@Gustl22 Gustl22 mentioned this pull request Sep 4, 2022
@Gustl22
Copy link
Collaborator

Gustl22 commented Sep 8, 2022

@novikov-studio Would you mind adding buttons to the example and adding tests, now that #1238 is merged. Need to rebase or merge main branch first.
I thought of adding 5 Buttons to Controls Tab with values: [-1, -0.5, 0, 0.5, 1] like done here.

@novikov-studio
Copy link
Contributor Author

@Gustl22, done.

Copy link
Collaborator

@Gustl22 Gustl22 left a comment

Choose a reason for hiding this comment

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

Thanks for your work.

You also have to overwrite setBalance in web implementation, otherwise the test will fail.

It would also be good to overwrite it in the other implementations (darwin, android) and throw an explicit unimplemented error or add a logger error, although it's not mandatory, but would give a better error message, what is not implemented yet.

Best regards

@novikov-studio
Copy link
Contributor Author

@Gustl22, I make setBalance to work for web (inspired by this).
But it only works if I set player.crossOrigin = "anonymous" and that makes me doubt,
Need some help with that.

@Gustl22
Copy link
Collaborator

Gustl22 commented Sep 21, 2022

Hi @novikov-studio,
this wasn't my intention, that you need to implement the balance for web platform, you could have just thrown an UnimplementedError. But nice that you tried it.

It also works for me, but dart:web_audio isn't exposed to pub. So even if it works, we shouldn't use it as it's not officially supported. Also the flutter linter would not allow this. See flutter/flutter#49849
I saw @luanpotter already had a discussion on that: dart-lang/pub-dev#3277

You should try to work via JS interface: https://pub.dev/packages/js , but I never used it, so I cannot give you a promise that it will work.
But if it's too much effort right now, we can do this in a separate MR.

Regarding the crossOrigin flag, I also have no clue. It may solves itself when using JS interface 🤷

@Gustl22 Gustl22 enabled auto-merge (squash) September 21, 2022 22:14
@Gustl22
Copy link
Collaborator

Gustl22 commented Sep 22, 2022

@novikov-studio somehow, the linux implementation works on my Ubuntu machine. But on my WSL2 via windows it give the error, which is also in the CI workflow:

Error: 1; message=Internal data stream error.
flutter: Unexpected platform error: Error: 1; message=Internal data stream error.

Are we missing a package? I wonder if ALSA plays a role here.

@novikov-studio
Copy link
Contributor Author

setBalance uses GStreamer's audiopanorama plugin, which is in "GStreamer Good Plug-ins" package.
So, maybe gstreamer1-plugins-good is missing.

@Gustl22
Copy link
Collaborator

Gustl22 commented Sep 22, 2022

sorry, that's not the problem. It's also installed via the workflow. I'll try to figure it out tomorrow, if you can't reproduce it right now :)

@Gustl22
Copy link
Collaborator

Gustl22 commented Sep 22, 2022

FYI: @novikov-studio I also reworked the web implementation in this branch, but the cors flag is still needed.

@novikov-studio
Copy link
Contributor Author

novikov-studio commented Sep 22, 2022

Sorry, I never used WSL2, so I can't reproduce.

@Gustl22
Copy link
Collaborator

Gustl22 commented Sep 23, 2022

I now used autoaudiosink following this tutorial: https://gstreamer.freedesktop.org/documentation/tutorials/playback/custom-playbin-sinks.html?gi-language=c
I think ALSA is not available on WSL2 or Ubuntu-Servers on GH-Actions

@Gustl22 Gustl22 merged commit 5c0cb4e into bluefireteam:main Sep 23, 2022
@Gustl22 Gustl22 mentioned this pull request Sep 23, 2022
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spatial or Stereo Control for audio
3 participants