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

Add query fonction in ALC_SOFT_system_events unreleased extension #938

Conversation

MathiusD
Copy link
Contributor

@MathiusD MathiusD commented Nov 24, 2023

The purpose of this PR are allow to retrieve which events are supported and if events are fully supported or if some case isn't managed for some reason.

Here, since extension isn't release, I would like to propose this addition to allow consumer of this extension to known eventual limitation on runtime (Because backend don't implement some event for example)

For exemple only some backends provide system events:

  • pipewire -> Full support of extension
  • wasapi -> Full support of extension
  • pulseaudio -> Support of add and remove devices events only
  • coreaudio -> Support of default device change only

Here, for complete this goal, i propose following addition :

  • alcEventIsSupportedSOFT in ALC_SOFT_system_events extension

…LC_SOFT_system_events unreleased extension

The purpose of this addition (to my collection) are allow to retrieve which events are supported and if events are fully supported or if some case isn't managed for some reason

For exemple only some backends provide system events:
 * pipewire -> Full support of extension
 * wasapi -> Full support of extension
 * pulseaudio -> Support of add and remove devices events only
 * coreaudio -> Support of default device change only
@MathiusD MathiusD force-pushed the feature/add_alcEventIsSupportedSOFT_into_ALC_SOFT_system_events branch from 62b5dd2 to 80c6641 Compare November 24, 2023 13:38
@kcat
Copy link
Owner

kcat commented Nov 24, 2023

What does "partially supported" indicate? None of the backends seem to currently report it, what scenarios do you foresee it being used for?

include/AL/alext.h Outdated Show resolved Hide resolved
@MathiusD
Copy link
Contributor Author

MathiusD commented Nov 24, 2023

What does "partially supported" indicate? None of the backends seem to currently report it, what scenarios do you foresee it being used for?

In current implementation it's not very useful, but I've preferred to allow three states for events: full support, no support and partial support, so that libraries or backends implementing the extension can indicate that the event in question is only handled in certain cases. In my opinion, this is useful in the following cases:

  • Unfinished/Partial implementation
  • Limitations of the backend or a third-party library that don't allow you to track all events of the type
  • A known bug if the library or backend doesn't have time to fix it from one version to the next, but still wants to offer its library or backend in this partial support version.
  • If multiples backend are in used for control capture and/or playback devices and some backends provide full support of event A (named BF) and anothers also in use don't provide support of this events (named BN), during execution if this implementation don't allow to retrieve backend in use it's imo better to specify partial support are provided because when backend BF is in use support provided is full and when backend BN is in use no suport are provided

P.S. : If cases presented here seems are too edge case or have no place here, I originally intended to offer only a boolean return code (AL_TRUE and AL_FALSE).

@kcat
Copy link
Owner

kcat commented Nov 25, 2023

  • Unfinished/Partial implementation
  • Limitations of the backend or a third-party library that don't allow you to track all events of the type

This is what I'm not sure about. The events are pretty fine-grained and aren't that complicated; if it triggers the callback when a new device becomes available (or removed, etc) for a given device type, it's supported, if it doesn't, it's not. It's not like the query is for only the event type (where behavior can differ between playback and capture) or device type (where behavior can differ between added, removed, and default changed), it's querying both traits together. I don't see how it would partially support a given event for a given device type.

The only meaning I could see it having in that case is that some events of a given type may be missed (e.g. a new playback device is added, and the callback randomly may or may not fire for it). Which would be a pretty significant bug that I'd be hesitant to knowingly leave in, as it would make the event unreliable and the app should just treat it as unsupported in that case anyway.

But then again, maybe a future event type would make more sense to have a "partial support" response. Though without knowing what kind of event that would be, "partial support" may be too generic or ambiguous to indicate what the app should know about it and it would be better to wait for such events to be added to add appropriate query responses for it.

But maybe adding a new query response in the future could cause compatibility issues if they would apply to existing events too, so having a generic "partial" response may be good to have just in case. Or maybe some apps would be fine with events not always firing if they don't use it for anything critical.

I'm a bit torn. It probably wouldn't be a big deal to leave in as a "just in case", even if it never ends up used.

@MathiusD
Copy link
Contributor Author

MathiusD commented Nov 25, 2023

But then again, maybe a future event type would make more sense to have a "partial support" response. Though without knowing what kind of event that would be, "partial support" may be too generic or ambiguous to indicate what the app should know about it and it would be better to wait for such events to be added to add appropriate query responses for it.

But maybe adding a new query response in the future could cause compatibility issues if they would apply to existing events too, so having a generic "partial" response may be good to have just in case. Or maybe some apps would be fine with events not always firing if they don't use it for anything critical.

I'm a bit torn. It probably wouldn't be a big deal to leave in as a "just in case", even if it never ends up used.

In this case, a compromise could be as follows:
Add the following tokens only to the current extension (ALC_SOFT_system_events):

  • ALC_EVENT_SUPPORTED_SOFT
  • ALC_EVENT_NOT_SUPPORTED_SOFT

And define another extension (for example: ALC_SOFT_system_events_partial_support) dependent on ALC_SOFT_system_events adding the token:

  • ALC_EVENT_PARTIALLY_SUPPORTED_SOFT

In this way, extensions adding a new type of event to the callback that would need this token in addition of 2 primary tokens would depends on both extensions, while others would only need two primary tokens depends only on primary extension.

With this system any other return tokens could be added as required with another extension (and possibly dedicated tokens for some specific event support).
So, primary tokens as following meaning :

  • ALC_EVENT_SUPPORTED_SOFT should be specify event are fully supported
  • ALC_EVENT_NOT_SUPPORTED_SOFT should be specify event aren't supported.

Any new tokens should define their own meaning.

@kcat
Copy link
Owner

kcat commented Nov 25, 2023

In that case there's no point to having a separate ALC_SOFT_system_events_partial_support that adds ALC_EVENT_PARTIALLY_SUPPORTED_SOFT without also defining events that use it, and they can just be left out. The extension that adds such an event can add the "partially supported" or any other query response they need (multiple extensions can add/share the same tokens, giving them the same value), and the query response would need to be restricted to the events that are defined to potentially return it (so it can't be "backported" to have pre-existing queries return the new query response).

@MathiusD
Copy link
Contributor Author

MathiusD commented Nov 25, 2023

Indeed, adding an extension to define the return token without the associated events is not useful. ^^'

And yes, the backport of the new return type for an old event is absolutely unsuitable!

P.S.: I remove the partial support token when i get home

P.P.S.: Done

@kcat kcat merged commit c03603b into kcat:master Nov 26, 2023
8 checks passed
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.

2 participants