-
-
Notifications
You must be signed in to change notification settings - Fork 163
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
Add gstreamer based audioplayers plugin #277
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just some styling changes to stay consistent with the rest of flutter-pi.
Ideally the existing gstreamer player could be modified to support audio and then this plugin could just use that. But I think this is fine for now |
This is my
|
@ardera I applied comments and clang format using your config |
f65fc14
to
c733a49
Compare
I applied second round of comments. |
@DoumanAsh btw, are you applying the suggestions directly? Because I'm writing them that way so you can just click "commit suggestion" |
Yeah, I didn't notice at first but even still github UI is not really convenient to do it as some changes require consideration (lik e renaming of function etc). |
Co-authored-by: Hannes Winkler <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, I think after these comments it should be fine
Ah okay, yeah then it's probably better to only use suggestions when you can apply them directly |
Co-authored-by: Hannes Winkler <[email protected]>
Co-authored-by: Hannes Winkler <[email protected]>
Co-authored-by: Hannes Winkler <[email protected]>
Co-authored-by: Hannes Winkler <[email protected]>
Co-authored-by: Hannes Winkler <[email protected]>
Co-authored-by: Hannes Winkler <[email protected]>
@ardera |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Ah wait, forgot one thing. meta-flutter depends on the name of the build arg being |
Oh I see... it is a bit of pain, should we make them completely independent features then? |
@DoumanAsh Last comments, I swear :p |
@ardera I reworked to make them independent. But please take a look yourself and test it too maybe to be sure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Could we also have a simpler/light weight plugin based on miniaudio since it's a header only library and can cross compile very well. I've already written a very small poc plugin for flutter-elinux |
@Taha-Firoz A 3.68MB header-only library :) |
Choice of gstreamer is really mostly because existing audioplayers plugin use it, not to mention flutter-pi also rely on it for video. |
😅 I think it'll get pretty small after compilation, it contains a lot of code to make it cross compile to almost every platform(ios, windows, android etc.), but I'm not sure |
This a bit update implementation that we used in our machines.
I have a bit of concern on how dispose is supposed to work, but otherwise it is complete
It unites video and audio into single option as both depend on gstreamer
CC @laynor