[AUDIO_WORKLET] Fix emscripten_audio_worklet_post_function_sig #23108
Conversation
mighty1231
force-pushed
the
audio-worklet-post-sig
branch
from
7f4c6b9 to
261e509
Compare
|
I see test job 'test-browser-chrome-wasm64-4gb' fails with |
|
test_offset_converter_pthread is a known flaky test |
| @@ -21,7 +21,7 @@ void MessageReceivedInAudioWorkletThread(int a, int b) { | |||
| printf("MessageReceivedInAudioWorkletThread: a=%d, b=%d\n", a, b); | |||
| assert(emscripten_current_thread_is_audio_worklet()); | |||
| assert(a == 42 && b == 9000); | |||
| emscripten_audio_worklet_post_function_viii(EMSCRIPTEN_AUDIO_MAIN_THREAD, MessageReceivedOnMainThread, /*d=*/1, /*e=*/2, /*f=*/3); | |||
| emscripten_audio_worklet_post_function_sig(EMSCRIPTEN_AUDIO_MAIN_THREAD, (void *)MessageReceivedOnMainThread, "iii", /*d=*/1, /*e=*/2, /*f=*/3); | |||
There was a problem hiding this comment.
Should we not be tests both the _viii form and the _sig form?
So IIUC that emscripten_audio_worklet_post_function_sig simply does not work today and is not tested?
(That makes me wonder if its really needed as part of the API?)
There was a problem hiding this comment.
emscripten_audio_worklet_post_function_sig is not currently tested in the test suite, and it does not work properly at the moment. So that I changed existing test case. I could enhance the test, but I’d like to hear your thoughts on whether this version of test case is truly necessary. What do you think?
There are 2 threads for test: main and audio_worklet_thread
before this PR
main-->audio_worklet_thread, calling_viiaudio_worklet_thread-->main, calling_viii
after this PR
main-->audio_worklet_thread, calling_viiaudio_worklet_thread-->main, calling_sig
possible test case (necessary?, covering (both direction) * (_viii)
main-->audio_worklet_thread, calling_viiaudio_worklet_thread-->main, calling_sigmain-->audio_worklet_thread, calling_sigaudio_worklet_thread-->main, calling_viii
There was a problem hiding this comment.
As long as at least one of the _vxxx functions is tested that seems reasonable. Having coverage for all difference _vxxx functions seems even better.
There was a problem hiding this comment.
I enhanced the test case with a new commit, referring other test case - test/wasm_worker/post_function.c. It now covers all difference _vxxx functions.
|
Can you mention the PR description the name of the function ( |
sbc100
changed the title
sbc100
changed the title
I changed the PR description. Previously, I set PR description as same as my commit message. Now, PR description differs to my commit message. Is it OK? +It fixes emscripten_audio_worklet_post_function_sig, which was previously not worked, and changes existing test to check it. |
How about changing void magic_test_function(void) {
int result = EM_ASM_INT({
function report(x) {
out(x);
fetch('http://localhost:8888?stdout=' + encodeURIComponent(x));
}
- report('magic_test_function: input=' + $0);
- var converted = wasmOffsetConverter.getName($0);
- report('magic_test_function: converted=' + converted);
- return converted == 'magic_test_function';
+ try {
+ report('magic_test_function: input=' + $0);
+ var converted = wasmOffsetConverter.getName($0);
+ report('magic_test_function: converted=' + converted);
+ return converted == 'magic_test_function';
+ } catch (e) {
+ report((e?.message ?? '(unknown message)') + ' ' + (e?.stack ?? '(unknown stack)'));
+ return false;
+ }
}, get_pc());
assert(result);
} |
|
Now test (edited) It comes from Instead changing the js code, node serves emscripten_set_timeout_loop: (cb, msecs, userData) => {
function tick() {
var t = _emscripten_get_now();
var n = t + msecs;
{{{ runtimeKeepalivePop() }}}
callUserCallback(() => {
if ({{{ makeDynCall('idp', 'cb') }}}(t, userData)) {
// Save a little bit of code space: modern browsers should treat
// negative setTimeout as timeout of 0
// (https://stackoverflow.com/questions/8430966/is-calling-settimeout-with-a-negative-delay-ok) 😇
{{{ runtimeKeepalivePush() }}}
setTimeout(tick, n - _emscripten_get_now()); // Math.max(n - _emscripten_get_now(), 0) 😅
}
});
}
{{{ runtimeKeepalivePush() }}}
return setTimeout(tick, 0);
},Changing file @node_pthreads
@no_wasm2js('https://github.com/WebAssembly/binaryen/issues/5991')
def test_pthread_wait64_notify(self):
+ self.node_args += ['--disable-warning=TimeoutNegativeWarning']
self.do_run_in_out_file_test('atomic/test_wait64_notify.c') |
mighty1231
force-pushed
the
audio-worklet-post-sig
branch
from
2386ed7 to
f854ac0
Compare
| emscripten_audio_worklet_post_function_vdd(EMSCRIPTEN_AUDIO_MAIN_THREAD, vdd, 2.5, 3.5); | ||
| emscripten_audio_worklet_post_function_vddd(EMSCRIPTEN_AUDIO_MAIN_THREAD, vddd, 4.5, 5.5, 6.5); | ||
| emscripten_audio_worklet_post_function_sig(EMSCRIPTEN_AUDIO_MAIN_THREAD, viiiiiidddddd, "iiiiiidddddd", 10, 11, 12, 13, 14, 15, 16.5, 17.5, 18.5, 19.5, 20.5, 21.5); | ||
| emscripten_audio_worklet_post_function_v(EMSCRIPTEN_AUDIO_MAIN_THREAD, sided_test_finished); |
There was a problem hiding this comment.
Why not use a different function here? e.g. do_exit? Then you don't need the testingAudioWorklet global maybe?
There was a problem hiding this comment.
- On resolving conflict with merged PR [test] AudioWorklet tests now correctly run and exit #23695, I used
emscripten_force_exit(0);as the PR used, instead ofdo_exit. - Originally,
testingAudioWorkletis on the purpose to checkemscripten_current_thread_is_audio_worklet()behaves correctly on each callbacks. I changed the name of it toonAudioWorkletThread, for legibility.
There was a problem hiding this comment.
Sure, it just seems a little odd that this callback has two completely different behaviors depending on the thread. Why not have two different callbacks, one which gets called on the worklet and triggers the second set of callbacks, and one which run on the main thread call on_exit which just does the exiting?
| } else { // for the second call, finishing checks for audio worklet -> main | ||
| #ifdef REPORT_RESULT | ||
| REPORT_RESULT(success); | ||
| #endif |
There was a problem hiding this comment.
Can we just access success == XX here and return zero?
There was a problem hiding this comment.
During resolving conflict, I changed emscripten_force_exit(0);, and added assertion to check callbackCount with 16=8 for main->awthread + 8 for awthread->main
mighty1231
force-pushed
the
audio-worklet-post-sig
branch
2 times, most recently
from
ed135ce to
d6a88c2
Compare
|
|
If you are able to help debug the flakiness that would be amazing, but that should part of a separate PR |
mighty1231
force-pushed
the
audio-worklet-post-sig
branch
from
d6a88c2 to
83a866f
Compare
|
Is Is |
|
Just to say nice work, I think every function of the AW API now has code coverage with this addition and #23659. |
|
The codesize test I believe just needs to be rebaselines. I'll that now. |
|
Rebaselined: fc15522 |
It fixes emscripten_audio_worklet_post_function_sig, which was previously not worked, and enhances test to cover all emscripten_audio_worklet_post_function_*. In order to fix, it changes the old term `readAsmConstArgs` to the new term `readEmAsmArgs`, due to conflict of emscripten-core#16449 and emscripten-core#18218 Background history: 1. emscripten-core#16449 proposed, introducing AudioWorklet earlier 2. emscripten-core#18218 proposed, renaming readAsmConstArgs to readEmAsmArgs 3. emscripten-core#18218 merged earlier 4. emscripten-core#16449 merged later, using the old term `readAsmConstArgs` enhance the test for cover all emscripten_audio_worklet_post_function_*
mighty1231
force-pushed
the
audio-worklet-post-sig
branch
from
83a866f to
be59a9d
Compare
It fixes emscripten_audio_worklet_post_function_sig, which was previously not worked, and enhances test to cover all
emscripten_audio_worklet_post_function_*.
In order to fix, it changes the old term
readAsmConstArgsto the new termreadEmAsmArgs, due to conflict of #16449 and #18218Background history:
runEmAsmFunction. NFC #18218 proposed, renaming readAsmConstArgs to readEmAsmArgsrunEmAsmFunction. NFC #18218 merged earlierreadAsmConstArgs