fix: AudioContext::resume reanimates the failed Oboe stream due to the change in AudioPlayer::resume - #1252
fix: AudioContext::resume reanimates the failed Oboe stream due to the change in AudioPlayer::resume #1252basiav wants to merge 18 commits into
Conversation
WPT non-regression comparisonPASS — no regressions · 0 improved section(s) · overall 2717 → 2717 (0) Unchanged sections (28)
Baseline: Workflow run · this comment is updated on every push. |
closetcaiman
changed the title
closetcaiman
added
fix
| if (error != oboe::Result::ErrorDisconnected || driverMutex_ == nullptr) { | ||
| // error != oboe::Result::ErrorDisconnected condition is deleted to handle more cases of errors | ||
| if (driverMutex_ == nullptr) { |
There was a problem hiding this comment.
Removing this guard seems really dangerous to me. Consider this: should all types of errors result in stream rebuild retry? What about some non-recoverable errors, won't that create an infinite loop? Maybe it is better to choose action in response to specific errors - oboe::Result is not that big of an enum.
There was a problem hiding this comment.
Fair point. I've modified this to include 4 common errors: ErrorDisconnected, ErrorTimeout, ErrorInternal, ErrorNoService, 2 of them mentioned in the issue. We are not sure what ErrorTimeout, ErrorInternal and ErrorNoService rebuild will turn out in, it depends on the situation in question (especially hardware aspects) and it is not completely clear how to proceed. An infinite loop is possible, however:
- I've let only
ErrorDisconnectedtry to rebuild the stream once, if it fails thenAudioContext::onStreamFailis called. - I've let
ErrorTimeout,ErrorInternalandErrorNoServiceonly to callAudioContext::onStreamFail, so that there is the aspect of invokingonerror(absent in a regular stream rebuild), so we are letting some possible logic in (mainly of waiting, e.g. 2 seconds beforeAudioContext::resume). This can help reduce the risk of an infinite loop, which is however possible.
…logic from resume
mdydek
left a comment
mdydek
left a comment
There was a problem hiding this comment.
add also mention in the docs about onerror
| JSI_PROPERTY_SETTER_IMPL(AudioContextHostObject, onerror) { | ||
| auto audioContext = std::static_pointer_cast<AudioContext>(context_); | ||
|
|
||
| if (!value.isObject() || !value.getObject(runtime).isFunction(runtime)) { | ||
| audioContext->setOnError(nullptr); | ||
| return; | ||
| } | ||
|
|
||
| auto jsFunc = std::make_shared<jsi::Function>(value.getObject(runtime).getFunction(runtime)); | ||
|
|
||
| audioContext->setOnError([jsFunc, invoker = callInvoker_, rt = &runtime]() { | ||
| invoker->invokeAsync([jsFunc, rt]() { jsFunc->call(*rt); }); | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
even if it looked simple, it can introduce some errors, such as datarace when swaping the function mid oboe's onError, reference cycle due to strong ptr and that it can be destroyed on different thread. Utilize existing AudioEventHandlerRegistry and compare how events are done using it and implement the onerror this way.
0db88c5 to
d43fedc
Compare
Co-authored-by: Michał Dydek <dydmichal@gmail.com>
3 participants
Closes #1230
Introduced changes
AudioPlayer::onErrorAfterCloseAudioPlayer::onErrorAfterClose:ErrorDisconnected,ErrorTimeout,ErrorNoService,ErrorInternal.ErrorDisconnected, there is one try of the stream rebuild, if it failsAudioContext::OnStreamFailis called.ErrorTimeout,ErrorNoServiceandErrorInternalonStreamFailis called.AudioContext::onStreamFailAudioPlayer::cleanupprocedure, setsisInitialized_to false, invokesonerrordefined in JS.Checklist