Giter Club home page Giter Club logo

Comments (36)

slouken avatar slouken commented on September 13, 2024 1

And of course a second thousand runs it didn't fail at all...

from sdl.

sezero avatar sezero commented on September 13, 2024

This seems to be an oddball? Doesn't seem to happen all the time, but it just did. @icculus may have an idea.

from sdl.

madebr avatar madebr commented on September 13, 2024

I think the audio testautomation has some bugs left from the SDL_bool change.

from sdl.

slouken avatar slouken commented on September 13, 2024

I think the audio testautomation has some bugs left from the SDL_bool change.

It's likely, but I did a quick scan through it and didn't see any issues. @sezero, can you add some additional logging to find out which SDL call is returning an unexpected result?

from sdl.

sezero avatar sezero commented on September 13, 2024

@sezero, can you add some additional logging to find out which SDL call is returning an unexpected result?

I would, but I don't know whether I can make it to fail: it seems to have happened very rarely -- only once.

from sdl.

slouken avatar slouken commented on September 13, 2024

@sezero, can you add some additional logging to find out which SDL call is returning an unexpected result?

I would, but I don't know whether I can make it to fail: it seems to have happened very rarely -- only once.

sdl2-compat consistently fails this test, so maybe this is a preexisting condition? I'll take a look at that.

from sdl.

slouken avatar slouken commented on September 13, 2024

sdl2-compat consistently fails this test, so maybe this is a preexisting condition? I'll take a look at that.

No, that's different, it's an off-by-one error that @icculus should really look at. Hmm, I'll step through the code again and see if I can see anywhere that could fail.

from sdl.

sezero avatar sezero commented on September 13, 2024

it's an off-by-one error that @icculus should really look at.

Most possibly: a <= or >= which should actually be < or > (or vice versa)

from sdl.

slouken avatar slouken commented on September 13, 2024

I don't see anywhere this can fail when using the same seed as the CI failure, other than legitimate allocation failures. This runs clean even with address sanitizer enabled. I'm bumping this to @icculus, maybe he has more ideas here?

from sdl.

sezero avatar sezero commented on September 13, 2024

As of commit 8ddb099, a lot of failures in CI..

from sdl.

slouken avatar slouken commented on September 13, 2024

As of commit 8ddb099, a lot of failures in CI..

Yup, it's the sdl2-compat failure, ported to SDL3. @icculus is going to take a look when he gets a chance. You can ignore that failure for purposes of merging PRs.

from sdl.

icculus avatar icculus commented on September 13, 2024

I'm going to poke this real fast just to unblock things. Hopefully it's easy.

from sdl.

icculus avatar icculus commented on September 13, 2024

I assume this is SDL_GetResamplerOutputFrames coming out one frame larger in some cases, which it happily fills in when you give it a larger buffer, but I don't really know how to fix it. @0x1F9F1, any idea?

It's not a floating point rounding issue, as it doesn't use floats for this.

from sdl.

icculus avatar icculus commented on September 13, 2024

Might be this:

    // output_frames = div_ceil(input_offset, resample_rate)
    Sint64 output_frames = (input_offset > 0) ? (((input_offset - 1) / resample_rate) + 1) : 0;

Taking out the +1 fixes it, but then we are one sample short on a later test.

Maybe "div_ceil" should round to nearest instead? I assume that's what the +1 is for, but I could be totally wrong.

from sdl.

madebr avatar madebr commented on September 13, 2024

Can we revert the audio_resampleLoss change on main?
CI is segfaulting in SDL_gpu at the moment and this issue is hiding that.

from sdl.

slouken avatar slouken commented on September 13, 2024

How about this?

from sdl.

icculus avatar icculus commented on September 13, 2024

Definitely fixes it for me.

from sdl.

sezero avatar sezero commented on September 13, 2024

Should 8ddb099 be reverted after that?

from sdl.

slouken avatar slouken commented on September 13, 2024

Should 8ddb099 be reverted after that?

No, we want to keep that test case.

from sdl.

slouken avatar slouken commented on September 13, 2024

Okay, I fixed the extra sample issue, but we still don't know why this failed once with -1 for @sezero.

from sdl.

slouken avatar slouken commented on September 13, 2024

I got another failure in CI in this same test:
https://github.com/slouken/SDL/actions/runs/10645992611/job/29512397443#step:25:46378

Of course I can't reproduce this locally, even using the same seed.

There's something fishy here, @icculus.

from sdl.

0x1F9F1 avatar 0x1F9F1 commented on September 13, 2024

Might be this:

    // output_frames = div_ceil(input_offset, resample_rate)
    Sint64 output_frames = (input_offset > 0) ? (((input_offset - 1) / resample_rate) + 1) : 0;

Taking out the +1 fixes it, but then we are one sample short on a later test.

Maybe "div_ceil" should round to nearest instead? I assume that's what the +1 is for, but I could be totally wrong.

Yeah, I think rounding to nearest instead of rounding up is the right move in terms of matching what the user would expect, since there will always be small precision errors from using fixed point arithmetic.

>>> resample_rate = (44100 << 32) // 48000
>>> (8820000 << 32) / resample_rate
9600000.000486568

I see you tried resample_rate / 2, but then switched to resample_rate * 3 / 4 instead. I guess that's also fine, though did using half not fix the issue?

Currently no idea about the random failures though. testautomation could do with being run with --log error, and calling SDL_GetError after a failure.

from sdl.

sezero avatar sezero commented on September 13, 2024

What is this really about ?

22/22 Test #21: testautomation-no-simd ...........***Failed  Error regular expression found in output. Regex=[Total: [0-9]+\.[0-9]+ Kb in [1-9][0-9]* allocations] 10.79 sec

95% tests passed, 1 tests failed out of 22

Total Test time (real) =  35.31 sec

The following tests FAILED:
	 21 - testautomation-no-simd (Failed)
Errors while running CTest
Output from these tests are in: D:/a/SDL/SDL/build/Testing/Temporary/LastTest.log
Use "--rerun-failed --output-on-failure" to re-run the failed cases verbosely.
Error: Process completed with exit code 8.

(Origin: https://github.com/sezero/SDL/actions/runs/10654652204/job/29531282628)

from sdl.

madebr avatar madebr commented on September 13, 2024

What is this really about ?

The audio_resampleLoss test failed and didn't clean up its buffers.
And now CTest fails because there are leaks.
Since the return code is still !=0, it would fail anyhow.

from sdl.

icculus avatar icculus commented on September 13, 2024

Of course I can't reproduce this locally, even using the same seed.

Does the CI not use SIMD or something? Trying to decide what would cause the difference.

from sdl.

sezero avatar sezero commented on September 13, 2024

What is this really about ?

The audio_resampleLoss test failed and didn't clean up its buffers. And now CTest fails because there are leaks. Since the return code is still !=0, it would fail anyhow.

What is the regex complaint about?

from sdl.

madebr avatar madebr commented on September 13, 2024

What is the regex complaint about?

The key part is in [1-9][0-9]* allocations, which means >= 1.
The stdout/stderr of the process contains the text Total: 18750.00 Kb in 1 allocations.

from sdl.

slouken avatar slouken commented on September 13, 2024

Does the CI not use SIMD or something? Trying to decide what would cause the difference.

It doesn't, in the cases where it's failing. But it doesn't fail all the time, most of the time the test passes in CI. I tested here with the exact same environment and command line and wasn't able to reproduce it either.

from sdl.

icculus avatar icculus commented on September 13, 2024

Gonna run this in a loop for awhile to see if it ever blows up on repeat.

while `./test/testautomation --seed WT96XZFERX63KY38 --filter audio_resampleLoss` ; do echo ok ; done

(I think that's right...?)

from sdl.

madebr avatar madebr commented on September 13, 2024

On Linux, I cannot get it to fail.
But on Wine, it fails when I run it in a loop.
--log error does not print anything extra.

Running with --log error does not print extra information.

command:

SDL_AUDIO_DRIVER=dummy SDL_VIDEO_DRIVER=dummy SDL_CPU_FEATURE_MASK=-all  wine test/testautomation.exe --filter audio_resampleLoss  --log error --seed WT96XZFERX63KY38
with this patch:
diff --git a/include/SDL3/SDL_test_harness.h b/include/SDL3/SDL_test_harness.h
index 302388904..54dc2dc82 100644
--- a/include/SDL3/SDL_test_harness.h
+++ b/include/SDL3/SDL_test_harness.h
@@ -63,7 +63,7 @@ extern "C" {
 #define TEST_RESULT_SETUP_FAILURE       4
 
 /* !< Function pointer to a test case setup function (run before every test) */
-typedef void (*SDLTest_TestCaseSetUpFp)(void *arg);
+typedef void (*SDLTest_TestCaseSetUpFp)(void **arg);
 
 /* !< Function pointer to a test case function */
 typedef int (*SDLTest_TestCaseFp)(void *arg);
diff --git a/src/test/SDL_test_harness.c b/src/test/SDL_test_harness.c
index 1347bbf54..f1fc68f92 100644
--- a/src/test/SDL_test_harness.c
+++ b/src/test/SDL_test_harness.c
@@ -237,6 +237,7 @@ static int SDLTest_RunTest(SDLTest_TestSuiteReference *testSuite, const SDLTest_
     int testCaseResult = 0;
     int testResult = 0;
     int fuzzerCount;
+    void *data = NULL;
 
     if (!testSuite || !testCase || !testSuite->name || !testCase->name) {
         SDLTest_LogError("Setup failure: testSuite or testCase references NULL");
@@ -259,7 +260,7 @@ static int SDLTest_RunTest(SDLTest_TestSuiteReference *testSuite, const SDLTest_
 
     /* Maybe run suite initializer function */
     if (testSuite->testSetUp) {
-        testSuite->testSetUp(0x0);
+        testSuite->testSetUp(&data);
         if (SDLTest_AssertSummaryToTestResult() == TEST_RESULT_FAILED) {
             SDLTest_LogError(SDLTEST_FINAL_RESULT_FORMAT, "Suite Setup", testSuite->name, COLOR_RED "Failed" COLOR_END);
             return TEST_RESULT_SETUP_FAILURE;
@@ -267,7 +268,7 @@ static int SDLTest_RunTest(SDLTest_TestSuiteReference *testSuite, const SDLTest_
     }
 
     /* Run test case function */
-    testCaseResult = testCase->testCase(0x0);
+    testCaseResult = testCase->testCase(data);
 
     /* Convert test execution result into harness result */
     if (testCaseResult == TEST_SKIPPED) {
@@ -286,7 +287,7 @@ static int SDLTest_RunTest(SDLTest_TestSuiteReference *testSuite, const SDLTest_
 
     /* Maybe run suite cleanup function (ignore failed asserts) */
     if (testSuite->testTearDown) {
-        testSuite->testTearDown(0x0);
+        testSuite->testTearDown(data);
     }
 
     /* Cancel timeout timer */
diff --git a/test/testautomation.c b/test/testautomation.c
index 6480feb70..764a34518 100644
--- a/test/testautomation.c
+++ b/test/testautomation.c
@@ -165,8 +165,15 @@ int main(int argc, char *argv[])
         SDL_RenderClear(renderer);
     }
 
-    /* Call Harness */
-    result = SDLTest_RunSuites(testSuites, userRunSeed, userExecKey, filter, testIterations, randomOrder);
+    for (;;) {
+        /* Call Harness */
+        result = SDLTest_RunSuites(testSuites, userRunSeed, userExecKey, filter, testIterations, randomOrder);
+        if (result != 0) {
+            SDL_Log("FAIL!");
+            break;
+        }
+        SDL_Quit();
+    }
 
     /* Empty event queue */
     done = 0;
diff --git a/test/testautomation_audio.c b/test/testautomation_audio.c
index 9a9e726f5..967c313a5 100644
--- a/test/testautomation_audio.c
+++ b/test/testautomation_audio.c
@@ -15,12 +15,17 @@
 #include <SDL3/SDL_test.h>
 #include "testautomation_suites.h"
 
+typedef struct {
+    SDL_bool init_audio;
+} TestautomationAudioData;
+
 /* ================= Test Case Implementation ================== */
 
 /* Fixture */
 
-static void audioSetUp(void *arg)
+static void audioSetUp(void **arg)
 {
+    TestautomationAudioData *userdata;
     /* Start SDL audio subsystem */
     SDL_bool ret = SDL_InitSubSystem(SDL_INIT_AUDIO);
     SDLTest_AssertPass("Call to SDL_InitSubSystem(SDL_INIT_AUDIO)");
@@ -28,13 +33,24 @@ static void audioSetUp(void *arg)
     if (!ret) {
         SDLTest_LogError("%s", SDL_GetError());
     }
+    userdata = SDL_calloc(1, sizeof(TestautomationAudioData));
+    if (userdata) {
+        userdata->init_audio = ret;
+    }
+    *arg = userdata;
 }
 
 static void audioTearDown(void *arg)
 {
+    TestautomationAudioData* userdata = arg;
     /* Remove a possibly created file from SDL disk writer audio driver; ignore errors */
     (void)remove("sdlaudio.raw");
 
+    if (userdata && userdata->init_audio) {
+        SDL_QuitSubSystem(SDL_INIT_AUDIO);
+        SDLTest_AssertPass("Call to SDL_QuitSubSystem()");
+        SDL_free(userdata);
+    }
     SDLTest_AssertPass("Cleanup of test files completed");
 }
 
diff --git a/test/testautomation_blit.c b/test/testautomation_blit.c
index f13d9abf9..8ea58604f 100644
--- a/test/testautomation_blit.c
+++ b/test/testautomation_blit.c
@@ -40,7 +40,7 @@ Uint32 getRandomUint32() {
 /*
  * Resets PRNG state to initialize tests using PRNG
  */
-void blitSetUp(void *arg) {
+void blitSetUp(void **arg) {
     rngState[0] = 1;
     rngState[1] = 2;
 }
diff --git a/test/testautomation_iostream.c b/test/testautomation_iostream.c
index 23ac85061..3cff4aae5 100644
--- a/test/testautomation_iostream.c
+++ b/test/testautomation_iostream.c
@@ -32,7 +32,7 @@ static const char IOStreamAlphabetString[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
 
 /* Fixture */
 
-static void IOStreamSetUp(void *arg)
+static void IOStreamSetUp(void **arg)
 {
     size_t fileLen;
     FILE *handle;
diff --git a/test/testautomation_render.c b/test/testautomation_render.c
index 809a7ae91..230b5d3c7 100644
--- a/test/testautomation_render.c
+++ b/test/testautomation_render.c
@@ -44,7 +44,7 @@ static SDL_bool hasDrawColor(void);
 /**
  * Create software renderer for tests
  */
-static void InitCreateRenderer(void *arg)
+static void InitCreateRenderer(void **arg)
 {
     int width = 320, height = 240;
     const char *renderer_name = NULL;
diff --git a/test/testautomation_subsystems.c b/test/testautomation_subsystems.c
index 626c202e0..ab92bcaa8 100644
--- a/test/testautomation_subsystems.c
+++ b/test/testautomation_subsystems.c
@@ -9,7 +9,7 @@
 
 /* Fixture */
 
-static void subsystemsSetUp(void *arg)
+static void subsystemsSetUp(void **arg)
 {
     /* Reset each one of the SDL subsystems */
     /* CHECKME: can we use SDL_Quit here, or this will break the flow of tests? */
diff --git a/test/testautomation_surface.c b/test/testautomation_surface.c
index de1ce38a1..c10dc259e 100644
--- a/test/testautomation_surface.c
+++ b/test/testautomation_surface.c
@@ -41,7 +41,7 @@ static SDL_Surface *testSurface = NULL;
 /* Fixture */
 
 /* Create a 32-bit writable surface for blitting tests */
-static void surfaceSetUp(void *arg)
+static void surfaceSetUp(void **arg)
 {
     int result;
     SDL_BlendMode blendMode = SDL_BLENDMODE_NONE;
diff --git a/test/testautomation_timer.c b/test/testautomation_timer.c
index d04979efa..f8dc7138e 100644
--- a/test/testautomation_timer.c
+++ b/test/testautomation_timer.c
@@ -20,7 +20,7 @@ static int g_timerCallbackCalled = 0;
 
 /* Fixture */
 
-static void timerSetUp(void *arg)
+static void timerSetUp(void **arg)
 {
     /* Start SDL timer subsystem */
     SDL_bool ret = SDL_InitSubSystem(SDL_INIT_TIMER);

from sdl.

icculus avatar icculus commented on September 13, 2024

Guessing the common denominator here is Visual Studio, then.

from sdl.

madebr avatar madebr commented on September 13, 2024

Guessing the common denominator here is Visual Studio, then.

This was built with mingw64. So I'd say Windows.

I just got this error:

 09/01/24 18:39:55: Assert 'Call to convert_audio_chunks(stream, buf_in, 19200000, buf_out, 4410000)': Passed
ERROR: 09/01/24 18:39:55: Assert 'Expected output length to be 4410000, got 1494488.': Failed

from sdl.

slouken avatar slouken commented on September 13, 2024

I ran it 1000 times and it failed 3 times. I'm setting a breakpoint to catch the error.

from sdl.

sezero avatar sezero commented on September 13, 2024

And of course a second thousand runs it didn't fail at all...

it's mocking us :))

from sdl.

slouken avatar slouken commented on September 13, 2024

Okay, I think I see what's going on here. When we write a very small number of bytes into the stream (12) we don't get any bytes out, because it's within the number of padding bytes we keep in the stream and we're not flushing it yet. The test assumes this means end of stream, and returns a short count, which is a test failure.

I'm still looking at the best fix, but the good news is that this appears to be a test issue, not an SDL issue.

from sdl.

slouken avatar slouken commented on September 13, 2024

Fixed, AND I figured out why re-running with the same seed wouldn't let us reproduce the issue. :)

from sdl.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.