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

Safari on iOS17 does not play audio after first touch #759

Closed
mlabbe opened this issue Oct 26, 2023 · 8 comments
Closed

Safari on iOS17 does not play audio after first touch #759

mlabbe opened this issue Oct 26, 2023 · 8 comments
Labels

Comments

@mlabbe
Copy link
Contributor

mlabbe commented Oct 26, 2023

This bug is a request for discussion with a link to a simple fix.

Admittedly, iOS Safari and audio "autoplay" are a bit of a dark art. Any number of things can prevent a page from playing.

In #270 , a code patch was merged to resume the audio context on click, touchstart and touchend. In my testing, this works on Chrome and Safari desktop, but fails on iOS mobile unless I use an actual mouse click via trackpad or similar.

Removing touchstart from miniaudio.unlock_event_types makes it work, as the criteria for autoplay is met only when the first finger is removed from the screen. I have done that in this commit: frogtoss@efd5432 .

The current best practice is to have a "tap to start" button on a webpage, to get the necessary event that allows for the audio context to resume. For me, this raises a point: what works for resuming audio is going to change over time, as browsers update. Ideally, miniaudio would offer an event or promise to hook to when the audio is sucessfully resumed to start the actual app, rather than monitor a set of events on that "tap to start" button that attempt to ape what Miniaudio is doing.

Would a code patch to hook into successful audio context resumption be of interest, and if so, what would this code patch look like in your eyes?

@mackron
Copy link
Owner

mackron commented Oct 27, 2023

Thanks. The web situation is an annoying one for miniaudio because the two just don't quite align with their usage and API design, so indeed that event thing is a bit of a hack to try and get them as close as possible. I'm honestly not sure how to approach your suggestion, but just to clarify, were you perhaps suggesting something like a notification that's posted by miniaudio to the application, similar to the stop notification, except for when the Web Audio context starts playing for real? If that's the general idea of what you had in mind, maybe something in ma_device_notification? Maybe another item in the ma_device_notification_type enum?

typedef enum
{
    ma_device_notification_type_started,
    ma_device_notification_type_stopped,
    ma_device_notification_type_rerouted,
    ma_device_notification_type_interruption_began,
    ma_device_notification_type_interruption_ended
} ma_device_notification_type;

Indeed, there's a "started" notification in there already which might be appropriate?

Just with your patch you linked to - so it's just a matter of removing touchstart? I think I understand the situation there, and I think that change is fine. I find it hard to believe that on other browsers anyone would complain that it doesn't start until after you've lifted your finger.

@mlabbe
Copy link
Contributor Author

mlabbe commented Oct 27, 2023

Removing touchstart is indeed the way to go. I also tested this on an Android phone and the results were the same -- touchstart was a problem.

There is possible minor downstream impact on web software that is waiting on touch down events to begin main + audio processing, where the processing starts before the finger is lifted, creating a short, silent start or an unflushed buffer of sounds. I would list this minor issue under breaking changes but move forward with removing the touchstart event from the list because it causes miniaudio to silently fail on mobile iOS.

I'll investigate the notification system and reply further on that point.

@mlabbe
Copy link
Contributor Author

mlabbe commented Oct 27, 2023

It seems ma_device_notification_type_started currently fires on device initialization. At this point, the device is explicitly suspended (to use the web standard's definition).

For the sake of any future readers, I'll point out that ma_device_notification_type_started does not fire on unlock, currently.

There are a couple reasons why we might want a separate unlock event:

  1. An "autoplay" unlock is a one-time only event, even if a device is stopped, suspended or restarted multiple times.

  2. The audioContext resume() returns a promise to when the resume finished, which only happens when the unlock is successful, and finished. Both Chrome desktop and iOS 17 Safari are correctly executing this promise in my test code.

The notification system seems to be a good approach, however. Given these facts, what are your thoughts on creating a separate unlock notification versus using the started one?

@mackron
Copy link
Owner

mackron commented Oct 28, 2023

Thanks. I've pushed a change to the dev branch to remove touchstart. I put a comment in CHANGES.md to mention a slight change in behaviour.

You make good points regarding the notification thing and I fully agree with everything you said. I think a ma_device_notification_type_unlocked (or whatever you think might be a better name) is a good solution which I'd be happy to support.

@mlabbe
Copy link
Contributor Author

mlabbe commented Oct 29, 2023

Small update -- I've now tested the touchstart removal fix on four Android phones running Chrome ranging from 7 to 12, and it succeeded at all of them.

I am going to attempt a ma_device_notification_type_unlocked PR soon. It would be very helpful if I knew how to run the miniaudio splitter into the separate .c & .h files before I started, since that's the version my codebase uses, and I would use that for testing.

@mackron
Copy link
Owner

mackron commented Oct 30, 2023

I haven't open sourced the splitter yet because it uses some other code of mine that I have no plans on releasing. I need to replace all of that code, but won't be happening any time soon.

@mackron
Copy link
Owner

mackron commented Nov 3, 2023

I've released version 0.11.19 and have updated the split version with it. I don't usually allow it, but if you like you can do your PR against the split version and I'll manually merge it into the main file.

@mackron mackron added the bug label Nov 3, 2023
@mlabbe
Copy link
Contributor Author

mlabbe commented Nov 6, 2023

Thanks for the offer. I'd like to be able to offer a tested PR, so I may just convert my project. I should have some time this week to take a crack at a PR. I'll base it on 0.11.19.

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

No branches or pull requests

2 participants