Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[video_player] Add option to set video audio to mix mode in iOS #1174

Closed

Conversation

wwwdata
Copy link
Contributor

@wwwdata wwwdata commented Feb 7, 2019

When listening to music in iOS, the Video Playback would disable the music and only the audio from the Video is enabled. For most use cases that should be OK but when playing videos in the background that are silent, that is not a desirable behaviour.

I also noticed a lot of errors because the initializingCompleter was called even though it was already completed so I added a guard against that.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@wwwdata
Copy link
Contributor Author

wwwdata commented Feb 7, 2019

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Feb 7, 2019
@bparrishMines bparrishMines changed the title Add option to set video audio to mix mode in iOS [video_player] Add option to set video audio to mix mode in iOS Feb 22, 2019
@eopeter
Copy link

eopeter commented Jul 11, 2019

Any word on moving this pull request forward?

@wwwdata
Copy link
Contributor Author

wwwdata commented Jul 11, 2019

hm, I can fix the linter errors and rebase but I would appreciate some feedback from the maintainers if this is a good approach or what they think about it in general.

Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution, It looks good overall. I left some comments.

Also, I think we should try to combine this PR with #1755 so we can easily unify the public API.

@wwwdata
Copy link
Contributor Author

wwwdata commented Aug 5, 2019

Hi, thanks for the review. I will make the requested changes now and make the build green. I would suggest that I merge changes from #1755 (as one commit into this branch) and also set the correct mode for Android when the configuration option was provided to mix the audio. Is this ok for you @recastrodiaz ?

@recastrodiaz
Copy link
Contributor

Hi @wwwdata Feel free to merge the code in #1755, or even just copy paste the changes into you branch. (The other PR is basically a one liner change).

@wwwdata wwwdata force-pushed the audio-player-mix-option branch from 815ffa8 to ddd8718 Compare August 6, 2019 10:52
@wwwdata wwwdata requested a review from iskakaushik as a code owner August 6, 2019 10:52
@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no and removed cla: yes labels Aug 6, 2019
@wwwdata wwwdata force-pushed the audio-player-mix-option branch from ddd8718 to 762ff90 Compare August 6, 2019 11:43
@wwwdata
Copy link
Contributor Author

wwwdata commented Aug 6, 2019

@recastrodiaz pulled the one commit from you. I guess you have to tell the googlebot again that you consent, like explained in it's comment?

Android is now also using the option, tested in in emulator, worked fine for me. Now I will check that all builds get green.

@wwwdata wwwdata force-pushed the audio-player-mix-option branch from 762ff90 to fb4f307 Compare August 6, 2019 11:48
@recastrodiaz
Copy link
Contributor

@googlebot I consent

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot removed the cla: no label Aug 6, 2019
@wwwdata
Copy link
Contributor Author

wwwdata commented Dec 9, 2019

ok, @cbenhagen I will rebase and adopt this when I have time.

@sharpner sharpner force-pushed the audio-player-mix-option branch from 589fbe8 to d45c8ec Compare December 19, 2019 09:35
@wwwdata
Copy link
Contributor Author

wwwdata commented Dec 19, 2019

I adapted it to the new structure but I am not sure how to make the tests pass because I had to make a change in the interface package and in the player package and they depend on each other but the new one is not released... In general, this structure is very complicated in my opinion.

So the reason to split this up is just that the web plugin does not install the ios and android dependencies?

Feel free to change whatever is necessary in order to merge it.

@sanekyy
Copy link
Contributor

sanekyy commented Dec 19, 2019

Hello @wwwdata.

So the reason to split this up is just that the web plugin does not install the ios and android dependencies?

You can read about the reason here.

@crazedVic
Copy link

anyone that runs into this... like us for instance, we have an app that doesn't use sound but we have video backgrounds, calling play on VideoController was killing user's background music playing on ios.

fix is here for those desperate: https://github.com/crazedVic/plugins

@wwwdata
Copy link
Contributor Author

wwwdata commented Feb 14, 2020

@crazedVic if you want to take this topic over, go ahead and open a PR with your fixes. Link it here so we now that you took over then I gladly close my PR. For me, this is just a way too long ongoing process right now and I don't have time for this issue anymore.

@crazedVic
Copy link

@wwwdata To be honest, I think this oversight is a bug and something that Google should have fixed on their own instead of making someone trying to help (in this case, you) jump through all these hoops. If there's nobody on their team willing to take the time to fix their own product (or keep it updated, this is the only plugin that gives me a deprecation warning on android), then why should we waste our time? Google has literally billions of dollars at their disposal, they hire the best engineers, and then they let things like this remain unresolved for years? No way I am going to do their work for them. / endrant :).

On a less ranty note, your code saved my bacon yesterday. Client is super stoked that they can keep listening to music while using the app. So even though Google basically threw you to the curb, I super appreciate your efforts!

@ollyde
Copy link

ollyde commented Mar 15, 2020

Hey guys, any sign this will get fixed? Have several apps on the store with silent video content; getting lots of feedback from users saying it keeping pausing their music / podcasts.. not good..

@jbirori
Copy link

jbirori commented Mar 16, 2020

Is there a timeline of when this might be fixed and merged in?

@shovelmn12
Copy link

any news?

@iskakaushik iskakaushik removed their request for review April 28, 2020 16:34
@ollyde
Copy link

ollyde commented Jun 24, 2020

Is there any news on this? It's really annoying our users! The patch that was offered doesn't work anymore.

@obrunsmann
Copy link

It's very annoying... it is really not possible that for such fundamental problems after more than a year neither a solution is accepted nor other proposals are released... I like Flutter very much, but it simply destroys production use cases.

https://github.com/crazedVic/plugins worked for me.. thank you @crazedVic !

@ollyde
Copy link

ollyde commented Jul 16, 2020

@eseidelGoogle

@w3ggy
Copy link

w3ggy commented Aug 4, 2020

Still not merged

@cyanglaz
Copy link
Contributor

cyanglaz commented Aug 4, 2020

@wwwdata Sorry for the delay. The PR looks good. If you could please rebase and I can land it.

@shovelmn12
Copy link

@cyanglaz Thanks lots of us are waiting for this :)

@tvolkert
Copy link
Contributor

tvolkert commented Aug 7, 2020

if you want to take this topic over, go ahead and open a PR with your fixes. Link it here so we now that you took over then I gladly close my PR. For me, this is just a way too long ongoing process right now and I don't have time for this issue anymore.

@wwwdata I understand your frustration. Can you let @cyanglaz know if you're planning on rebasing so he can land, or if you'd prefer someone else to do so?

@wwwdata
Copy link
Contributor Author

wwwdata commented Aug 7, 2020

Hi. I can rebase it again. But this weekend I have no time. I will do it next week. Maybe Monday/Tuesday. Would be great to finally merge this in 😂

@tvolkert
Copy link
Contributor

tvolkert commented Aug 7, 2020

Sounds good - thank you so much for your patience! 🙂

@cyanglaz
Copy link
Contributor

cyanglaz commented Aug 7, 2020

@wwwdata Thank you! Feel free to @ me after the rebase. I will work on landing it!

@wwwdata
Copy link
Contributor Author

wwwdata commented Aug 11, 2020

Hi, I did the work now but unfortunately, I lost access to the repo I originally opened the pull request from because I don't work for this company anymore. I created another PR with my changes here. Sorry for the inconvenience: #2922

@tvolkert
Copy link
Contributor

No worries! Thanks for sticking with it - I'll close this now and move discussion over to #2922

@tvolkert tvolkert closed this Aug 11, 2020
@tvolkert
Copy link
Contributor

FYI, this landed in #2939. THANK YOU @wwwdata!! 🙏

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.