libsdl-org / SDL

Simple Directmedia Layer
https://libsdl.org
zlib License
9.84k stars 1.82k forks source link

SDL_ANDROID_BLOCK_ON_PAUSE should be removed, instead please fix SDL_WaitEvent #3193

Closed SDLBugzilla closed 3 months ago

SDLBugzilla commented 3 years ago

This bug report was migrated from our old Bugzilla tracker.

Reported in version: 2.0.9 Reported for operating system, platform: Android (All), x86_64

Comments on the original bug report:

On 2019-03-07 12:45:08 +0000, Ellie wrote:

SDL_ANDROID_BLOCK_ON_PAUSE somewhat looks like a hack around the fact that SDL_WaitEvent wastes battery due to not being implemented in a smart way. This hack breaks the semantics of SDL_PollEvent in surprising and not-cross-platform ways.

I suggest the following changes to fix this:

  • remove SDL_ANDROID_BLOCK_ON_PAUSE / default it to 0
  • SDL_WaitEvent should be fixed at least on Android to actually block

Apps can then choose to use SDL_WaitEvent if in the background if they want this blocking behavior. This solves: 1. the weird unexpected API semantics difference, 2. that this cannot be chosen at runtime without recompiling SDL2, 3. that there's this weird duplication of functionality which SDL_WaitEvent already should provide

On 2019-04-02 07:20:50 +0000, Sylvain wrote:

SDL_WaitEvent is actually doing SDL_PollEvent + SDL_Delay:

https://hg.libsdl.org/SDL/file/d5b4d374a312/src/events/SDL_events.c#l697

On 2019-04-02 09:20:20 +0000, Ellie wrote:

Yes and that makes SDL_WaitEvent somewhat useless in terms of battery saving, even though at a first glance it would be the obvious choice! So fixing it to be truly blocking would make it the obvious usable choice and make that on pause "feature" completely unnecessary

On 2019-04-17 02:53:42 +0000, Sam Lantinga wrote:

SDL_WaitEvent() can't block because events come from multiple sources that can't be aggregated in a cross-platform way.

On 2019-04-17 07:00:33 +0000, Ellie wrote:

Isn't the point of SDL2 to solve these issues between different platforms? If that currently doesn't work with various internal sources, then it sounds a little like that should be refactored.

Also it would already help a lot if at least it worked properly on mobile platforms, if getting this to work on all platforms is too much work.

I can tell you, I have lurked in freenode/irc and even familiar SDL2 users are always surprised that SDL_WaitEvent is close to a busy loop rather than a proper block-until-event. It's really not expected behavior, and it would IMHO in the long term make much more sense to see how to fix it rather than add hacks like SDL_ANDROID_BLOCK_ON_PAUSE to work around this - at least if we're talking purely from a sane API design angle.

Of course if this is way too complicated to be feasible then maybe it's not worth it. But I can't judge that, I'm not familiar with the affected code

On 2019-04-23 03:51:41 +0000, Sam Lantinga wrote:

Yes, it is too complicated to be feasible, in my opinion. However, I'll leave this bug open in case someone wants to contribute a patch they feel is valuable.

icculus commented 2 years ago

I still think a fully-waitable SDL is possible in the most common cases, even if we have to fall into busy-looping to cover some scenarios, so I'm tossing this into the SDL3 milestone to consider.

superfury commented 9 months ago

The main issue why the define for SDL_ANDROID_BLOCK_ON_PAUSE is there is because this very issue. It's because a backgrounded app (think music player or the like, or even messaging apps, games that need to continue backgrounded etc.) need to continue running (can't block) in the background. And SDL_PollEvent itself is blocking in that case(when the define isn't used to override), preventing the app from properly running in the background (blocking the main loop) if said define isn't used (also affecting audio processing for audio players by blocking as well, and messing up game timings). Right now, the default value of the setting is the SDL2 legacy one (for app compatibility).

My app itself only uses SDL_PollEvent, but if that's blocking it can't run and time properly anymore (not to mention generated audio being muted), as one of the core message handling is blocking, which it should never be by default.

In fact, isn't blocking/pausing the responsibility if the app itself? Isn't that the whole reason events like 'WILL/DID'ENTER'FORE/BACKGROUND' events (on some platforms minimized events and related) exist in the first place? If an app needs to save battery so bad, simply add larger SDL_Delay calls in the app itself when backgrounded status is detected (in fact, my app already does this)? The only issue remaining might be those audio calls in the main Blocking/NonBlocking routines? Thus the solution would be the NonBlocking routine and letting the app itself decide to render silent samples if it wants to (again, based on background status). In fact, in my own app it's simply configurable by the user of the app in it's settings how it should handle audio.

So the two (SDL_WaitEvent and SDL's Blocking/NonBlocking) serve different purposes. One is for polling and should never block (in fact, SDL_PollEvent blocking is more a compatibility with older programs), while the other (SDL_WaitEvent) is requesting to block (and is expected by apps).

slouken commented 9 months ago

Yeah, we should revisit this for SDL3. @1bsyl, with SDL's lifecycle improvements, do we still need this?

superfury commented 9 months ago

In fact, thinking about it, it should be like this:

Basically it's like the overrides are both (new audio playing backgrounded and nonblocking event behaviour) used, with WaitEvent busywaiting itself (that should never be done in SDL_PollEvent).

1bsyl commented 9 months ago

btw there is SDL_HINT_ANDROID_BLOCK_ON_PAUSE_PAUSEAUDIO so that it can still play audio in background (it simply doesn't pause the audio in bg).

Blocking on pause seems to me a good default for a game...

superfury commented 9 months ago

btw there is SDL_HINT_ANDROID_BLOCK_ON_PAUSE_PAUSEAUDIO so that it can still play audio in background (it simply doesn't pause the audio in bg).

Blocking on pause seems to me a good default for a game...

Yeah, I know. Both options combine into a bigger effect though? They are handled in the same functions as far as I remember (the Blocking and NonBlocking ones).

The main point is that them being Blocking or Muting (losing GL context for audio rendering actually) by default is just a compatibility option for older Android apps. None of the other OS builds of SDL2 do this, for example. In fact, doing that on other OSes (like Windows) would break apps on such platforms afaik (since they don't expect it to happen), again messing up timings and audio. Android is the only platform doing this from what I know.

As I said, it should be the app's own decision to mute audio (or continue rendering something in the background) or pause itself (based on the DID/WILL events or window minimize status or window having focus etc.) if the developer wants to, not SDL forcing one or the other (mute, blocking, or both)? The decision and optional usage of the old way (blocking the thread calling and losing audio capability for rendering) is a compatibility case here. Perhaps default to the new way (NonBlocking behaviour and continue audio playback and letting the app mute by rendering silence samples itself if the dev intends to, or delaying in the app itself itself by calling delay functions from the app instead of SDL3 itself doing so) for SDL3 would be the way? That would improve both cases, as apps will need to be changed for SDL3 anyways?

Just remember to document it in the migration guide for SDL2/SDL 1.x and function documentation (for SDL_PollEvent, SDL_WaitEvent and perhaps SDL events in general).

slouken commented 9 months ago

I agree with @superfury, let's move the logic for this into the application for SDL3.

1bsyl commented 9 months ago

I think this may be confusing and people are going start reporting issues and/or event not seeing their issues.

but we can try and that would be ?

src/video/android/SDL_androidvideo.c

-    block_on_pause = SDL_GetHintBoolean(SDL_HINT_ANDROID_BLOCK_ON_PAUSE, SDL_TRUE);
+    block_on_pause = SDL_GetHintBoolean(SDL_HINT_ANDROID_BLOCK_ON_PAUSE, SDL_FALSE);

-    videodata->pauseAudio = SDL_GetHintBoolean(SDL_HINT_ANDROID_BLOCK_ON_PAUSE_PAUSEAUDIO, SDL_TRUE);
+    videodata->pauseAudio = SDL_GetHintBoolean(SDL_HINT_ANDROID_BLOCK_ON_PAUSE_PAUSEAUDIO, SDL_FALSE);

Also, "pause audio thread" feature is not exported as a public API. here, the thread is locked (SDL_LockMutex(device->lock);), not sure how different is from putting silence.

Turning the non-blocking into a blocking loop, from application point of view, isn't similar as putting a SDL_Delay(). the blocking loop is done with a SDL_WaitSemaphore(). // sem_wait(). this also makes sure it's synchronized with the java SurfaceView states.

superfury commented 7 months ago

Well, people shouldn't be getting issues if they follow the official SDL3 migration guides and documentation (as SDL3 isn't even officially released yet). They're starting with a fresh SDL3 project after all (or using the migration guide anyways to migrate, if done properly).

And ofc the migration guide and SDL3 functions' wiki being updated on these changes is required for that.

If people will need to migrate to SDL3, they would need to check for these kinds of things anyways (and should check the migration guide). The same for new SDL3 projects, as those are clean and won't be directly affected by this (assuming the documentation is up-to-date on this behaviour).

superfury commented 6 months ago

Any progress on this change (by @1bsyl )? I don't see the change yet in the prerelease of SDL 3.1.1? If there's any time to change this (also for future compatibility with other platforms), it should be before SDL3 is actually released?

1bsyl commented 6 months ago

not from me, not sure what SDL3 behavior change is expected here ? I haven't re-read this, but from me last message it could be simply changing the default values of the 2 hints: SDL_HINT_ANDROID_BLOCK_ON_PAUSE SDL_HINT_ANDROID_BLOCK_ON_PAUSE_PAUSEAUDIO

to be "false" by default.

superfury commented 6 months ago

not from me, not sure what SDL3 behavior change is expected here ? I haven't re-read this, but from me last message it could be simply changing the default values of the 2 hints: SDL_HINT_ANDROID_BLOCK_ON_PAUSE SDL_HINT_ANDROID_BLOCK_ON_PAUSE_PAUSEAUDIO

to be "false" by default.

Yes. That what it effectively boils down to the solution of the issue (of course with changing the documention wiki and header documention to reflect that, as well as the SDL3 migration guide noting the change).

1bsyl commented 6 months ago

Not really sure with want this. Most app usually doesn't play music in background nor try to render in background or eat 100% in bg. Currently, it's seamlessly working when you move an app from desktop to mobile. I wouldn't make the platform flimsy with this. One can set the hint if needed, in very rare occasion I guess.

superfury commented 6 months ago

Not really sure with want this. Most app usually doesn't play music in background nor try to render in background or eat 100% in bg. Currently, it's seamlessly working when you move an app from desktop to mobile. I wouldn't make the platform flimsy with this. One can set the hint if needed, in very rare occasion I guess.

Not exactly seamlessly though. If on Windows you minimize the app (rough equivalent of Android behaviour) the app won't block the thread handling the events (thus everything the app does continues, just no SDL3 events happen). But Android otoh does block the calling thread, so compared to the other platforms it's the one that's behaving different? So it's already Android vs other platforms that is incompatible, which is why this override exists in the first place (Windows for example doesn't block, so apps will merrily run when minimized. But Android apps will start to block the main thread when the app is ported over straight from Windows (probably Linux as well, don't know about other platforms)).

slime73 commented 6 months ago

I think Android is closest to iOS – although it does have differences. On iOS if you minimize an app, the OS pauses the entire app aside from a few special cases for running small low-power things in the background. As a user of an iPhone this is pretty desirable, but I can't really speak for Android users.

superfury commented 6 months ago

You reacted before I could finish editing. Anyways (continuing my previous post)...

Basically it's Android behaviour (blocking) vs other platforms (except IOS perhaps, based on the shared foreground/background event handling) that don't block that's part of the main issue. And like you said, changing it because of porting behaviour is incompatible no matter what default you pick (either you'd change behaviour on Android or you'd change behaviour on all other platforms). Although a sensible default would be the majority I'd think (thus all other platforms, with it not blocking)? Or you could choose to leave it as-is (the same default as SDL2), maintaining compatibility with all existing software, but requiring that the SDL_ANDROID_BLOCK_ON_PAUSE would never be removed for any reason (because that would break all apps that use it or require compatibility with other platforms that don't have such a blocking behaviour (or like my app choose to handle things themselves, silencing audio if configured to do so in it's settings etc.)).

Though the downside this is that SDL_ANDROID_BLOCK_ON_PAUSE can never be removed, or changed, because doing either will break compatibility with existing apps no matter what (other platforms (except iOS) or Android/iOS platforms becoming broken).

Also, what about apps like music players on iOS/Android built with SDL2/3? Don't they require background not to be blocking as well? Assuming that anything like that exists of course.

Though besides music players there's also other kinds of apps like messaging features etc. that can't run if the blocking of the main thread is enabled (think like WhatsApp and those kind of apps etc.). Or even games with in-game messaging.

slouken commented 3 months ago

After discussion, we're going to change SDL_ANDROID_BLOCK_ON_PAUSE to default to 0, and look to see if we can do something smarter in WaitEvent, which has a system specific wait call now.

slouken commented 3 months ago

I've reviewed the blocking behavior on Android, and made it more closely match iOS. SDL_ANDROID_BLOCK_ON_PAUSE is still the default, and if it's on you need to handle the life cycle messages in an event watch callback. If you turn it off, you'll continue to run in the background, using low CPU. Audio will follow this behavior, pausing if the rest of the app does, continuing to play if the app continues in the background.

slouken commented 3 months ago

The latest commit also makes it so waiting on events doesn't spin on Android.

superfury commented 3 months ago

The latest commit also makes it so waiting on events doesn't spin on Android.

But does it allow audio rendering while the app is in the background? It's fine that the threads keep running, but in my app's case it also has a media player. It has an option inside it to enable playing audio backgrounded (or to mute it optionally when backgrounded). The audio thread is also required to clear buffers (shouldn't cause any problems I think, other than audio getting lost once the rendering buffers are full. If not, the app would hang waiting for more room, which causes a wait loop, but I'm pretty positive that wasn't blocking like that).

Anyways, my app can and does in fact keep rendering sound when backgrounded on Android (or any OS for that matter), just rendering zeroed samples when instructed to mute outputted samples. The same kind pf thing with recording devices detected (muting if to mute during backgrounded operation). The main thread as it keeps (the x86 emulator) going just keeps rendering samples in realtime speeds (or as close possible) according to emulated hardware converted to SDL's reported samplerate (w/ low-pass filter before resampling). The main app startup routine will set the SDL2/3 BLOCK_ON_PAUSE to 0. It does the same for PAUSEAUDIO if it's defined (not anymore on latest SDL3?).

So basically what you're saying is that PAUSEAUDIO now follows BLOCK_ON_PAUSE essentially? So background sound rendering still works with this (legacy SDL2) setup code? Or is it forced muted by SDL3 now (breaking my app's functionality)?

I can also imagine some games or apps might want to play sound this way (like, for example playing a sound for an event happening to a game, like a player message arriving notification while backgrounded or some other notification).

Also, apparently this isn't a weird thing (a toggle for backgrounded sound mute/playback) to be implemented by the app itself (after a quick Google search I saw posts saying that many games have a toggle (like my app) to mute or play audio in the background), so it's not just my app but a more common thing that games use, probably SDL games as well.

slouken commented 3 months ago

So basically what you're saying is that PAUSEAUDIO now follows BLOCK_ON_PAUSE essentially? So background sound rendering still works with this (legacy SDL2) setup code?

Yep!