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

AudioContext.onerror #2580

Merged
merged 18 commits into from
Jul 10, 2024
Merged

AudioContext.onerror #2580

merged 18 commits into from
Jul 10, 2024

Conversation

hoch
Copy link
Member

@hoch hoch commented Mar 27, 2024

This PR resolves #2567.

  • Add the onerror event handler under the AudioContext interface.
  • Add a step to fire onerror event in the AudioContext constructor steps on the render thread.
  • Remove the non-normative notes on the notification being impossible.
  • Add a prose to specify the onerror case on a running AudioContext.

Preview | Diff

@hoch hoch changed the title [WIP] AudioContext.onerror AudioContext.onerror Mar 28, 2024
@hoch hoch requested a review from padenot March 28, 2024 18:04
@hoch
Copy link
Member Author

hoch commented Mar 28, 2024

@padenot Ready for the first pass. Let me know what you think!

@gsinafirooz
Copy link

Hi @padenot; I just wanted to bump this.

index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@marcoscaceres
Copy link
Contributor

@jernoble, could you take a look at this also by chance?

@hoch
Copy link
Member Author

hoch commented May 3, 2024

@padenot It looks like we resolved all discussions so far. Let me know if you have more questions!

@gsinafirooz
Copy link

@jernoble, could you take a look at this also by chance?

@jernoble, We'd like to hear your opinion :)

@hoch
Copy link
Member Author

hoch commented May 30, 2024

@jernoble @marcoscaceres Can any of you comment on this pull request? We've been waiting for three weeks, and would like to merge this PR so that we can make progress on the browser implementation.

The PR itself is not really controversial; we want to add an event handler for audio device/platform errors.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@jernoble
Copy link
Member

@jernoble, could you take a look at this also by chance?

@jernoble, We'd like to hear your opinion :)

As a comment about the general capability added here, I see no problem with exposing AudioContext error states. This PR appears to be narrowly around handling what happens when the AudioContext loses exclusive access to the current audio sink / device. That either because that device is no longer available permanently (a plug was pulled, a wireless device disappeared, etc.) or because that device is no longer available temporarily (another process / service received exclusive access to that device, etc.). And there's no way to distinguish between those two states here.

In the "permanent" error case, there's nothing for the application to do save choose a different sink. In the "temporary" error case, this is recoverable by simply un-suspending.

Is there some other mechanism available to distinguish these two states? If not, should the onerror handler take a custom Error object with a payload?

@hoch
Copy link
Member Author

hoch commented May 30, 2024

that device is no longer available temporarily (another process / service received exclusive access to that device, etc.). And there's no way to distinguish between those two states here.

This is somewhat related `interrupted' status from the AudioSession. Also I am a bit confused by the expression "no longer available" and "temporarily" in one sentence. We have to consider two cases separately.

  • If it's temporarily unavailable (as in interrupted) it is not an error. It's an intentional state change initiated by the platform.
  • If it's permanently unavailable, that's an error and should be reported by onerror.

@jernoble
Copy link
Member

that device is no longer available temporarily (another process / service received exclusive access to that device, etc.). And there's no way to distinguish between those two states here.

This is somewhat related `interrupted' status from the AudioSession. Also I am a bit confused by the expression "no longer available" and "temporarily" in one sentence. We have to consider two cases separately.

That was inelegantly phrased on my part. :)

I was referring to this new language:

  • The audio output from a running AudioContext unexpectedly interrupted or the device is disconnected.

What does "unexpectedly interrupted" mean here?

@hoch
Copy link
Member Author

hoch commented Jun 1, 2024

What does "unexpectedly interrupted" mean here?

Yes, now I can see why you used the word "interrupted". Perhaps I can intentionally move away from that language so we can reserve such case for AudioSession API. My example of "unexpectedly interrupted" cases would be any kind of audio infra malfunction reported by the operating system including (but not limited to):

  • A wireless device getting disconnected (e.g. too far away from the host machine)
  • An external audio device getting disconnected (e.g. USB cable plugged out)

IMO one significant distinction from the AudioSession is:

interrupted: the AudioSession is not playing sound, but can resume when it will get uninterrupted.

The interrupted status can be resumed automatically after the interruption (please correct me if I am mistaken). So I think this can be useful for situations like notification, phone call on mobile device, etc.

@hoch
Copy link
Member Author

hoch commented Jun 5, 2024

@marcoscaceres @jernoble If you don't have other outstanding concerns, can we land this PR?

Also if you would like to have deeper technical discussion on this topic, please consider joining our bi-weekly teleconference. :)

index.bs Show resolved Hide resolved
@hoch
Copy link
Member Author

hoch commented Jul 10, 2024

Thanks @padenot! Merging.

@hoch hoch merged commit 116612c into main Jul 10, 2024
1 check passed
Copy link

@GorbachovMaina1 GorbachovMaina1 left a comment

Choose a reason for hiding this comment

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

Tut mir leid aber ich kenne mich nicht aus das funktioniert ich bin bitte - [ ] weiterhelfen damit

@GorbachovMaina1
Copy link

Kannst du mir bitte auf Deutsch oder bulgarisch schreiben damit ich verstehe und was es dir ansonsten ist mir schwierig weil ich verstehe leider keine Englisch ich bedanke mich voraus

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.

Device-related error reporting via AudioContext.onerror
9 participants