SDL3_net redesign #79

icculus · 2023-07-18T23:47:18Z

This is a complete redesign and rewrite of SDL_net for a 3.0 release. It has different goals and functionality than SDL2_net, of which I have retained no pieces.

See discussion in #77 for details.

Some things that are still pending before we can merge this:

A CMakeLists.txt. I'm just compiling by hand on the command line.
Wiring up to GitHub Actions.
Proper documentation in the header, so it can bridge to the wiki.
Testing on Windows and macOS, at a minimum.
SDLNet_GetLocalAddresses isn't implemented.
Example program(s).

icculus · 2023-08-14T00:00:48Z

Added a test program that does group voice chat over UDP. 374 lines of C code covers both client and server, so this is pretty nice, for just using SDL3_net and SDL3.

This let me fix a few mistakes and bugs in the proposed SDL3_net library.

It was also nice to flex the new SDL3 audio system; each person speaking just gets their own audio stream bound to the output device. You just queue audio on them when it comes in, and it mixes whatever's available. :)

madebr · 2023-08-14T00:05:05Z

Added a test program that does group voice chat over UDP. 374 lines of C code covers both client and server, so this is pretty nice, for just using SDL3_net and SDL3.

This looks great!
(After libsdl-org/SDL#5477, we can perhaps rename the test to SDL_zoom 😉)

madebr · 2023-08-14T01:01:18Z

I fixed the mingw build errors, expanded the cmake script a bit, and added minimum ci.
GitHub workflow result
Feel free to pick whatever you want.

icculus · 2023-08-14T03:53:51Z

Feel free to pick whatever you want.

I want it all. :)

It's pulled into this PR, thanks!

icculus · 2023-08-14T03:56:06Z

Should we move this into src/ and include/SDL3/ to match other libraries? Right now this all fits in one source file cleanly, but if I lose my mind and implement TLS 1.3 from scratch, I might use more source files. :)

sezero · 2023-08-14T10:41:09Z

Getting this error with gcc-4.4 or gcc-4.9:

$ gcc49 -I/home/sezero/SDL/include -O3 -Wall -W -std=gnu99 -c SDL_net.c
SDL_net.c: In function 'SDLNet_Init':
SDL_net.c:268:5: error: a label can only be part of a statement and a declaration is not a statement
     char *origerrstr = SDL_strdup(SDL_GetError());
     ^

Similar with clang-3.4:

SDL_net.c:268:5: error: expected expression
    char *origerrstr = SDL_strdup(SDL_GetError());
    ^
SDL_net.c:272:9: error: use of undeclared identifier 'origerrstr'
    if (origerrstr) {
        ^
SDL_net.c:273:28: error: use of undeclared identifier 'origerrstr'
        SDL_SetError("%s", origerrstr);
                           ^
SDL_net.c:274:18: error: use of undeclared identifier 'origerrstr'
        SDL_free(origerrstr);
                 ^
4 errors generated.

Moving origerrstr to top fixes it for me:

diff --git a/SDL_net.c b/SDL_net.c
index 488073f..cd2c510 100644
--- a/SDL_net.c
+++ b/SDL_net.c
@@ -228,6 +228,8 @@ static void DestroyAddress(SDLNet_Address *addr)
 
 int SDLNet_Init(void)
 {
+    char *origerrstr;
+
     #ifdef __WINDOWS__
     WSADATA data;
     if (WSAStartup(MAKEWORD(1, 1), &data) != 0) {
@@ -265,7 +267,7 @@ int SDLNet_Init(void)
     return 0;  // good to go.
 
 failed:
-    char *origerrstr = SDL_strdup(SDL_GetError());
+    origerrstr = SDL_strdup(SDL_GetError());
 
     SDLNet_Quit();

madebr · 2023-08-14T13:29:48Z

Should we move this into src/ and include/SDL3/ to match other libraries? Right now this all fits in one source file cleanly, but if I lose my mind and implement TLS 1.3 from scratch, I might use more source files. :)

Probably. I did the minimum to get it building on mingw and ci.
Once you're finished, we can give it the "usual treatment".

icculus · 2023-08-14T15:39:54Z

Getting this error with gcc-4.4 or gcc-4.9:

I pulled your fix in, thanks!

SDL_net.c

DanielGibson · 2023-08-15T19:17:21Z

I think supporting blocking reads (with a timeout) might make sense, especially if you're doing networking (or at least reading) in a separate thread (=> after a failed read you don't have much to do, so you'd either busy-loop on SDLNet_ReadStreamSocket()/SDLNet_ReceiveDatagram() or add sleep()s that increase latency in case a network package arrives in the meantime after all).

Kinda related, probably SDLNet_WaitForServerIncoming() should have a timeout parameter - in fact, I guess all SDLNET_Wait*() functions should have a timeout parameter, if the underlying APIs support it at all.

icculus · 2023-08-16T12:41:03Z

We probably should add a poll()-style interface, to check multiple sockets at once (but they can still remain non-blocking).

DNS resolution waits on a condition variable internal, so we can definitely add a timeout on that.

DanielGibson · 2023-08-16T12:52:14Z

Yeah, explicit poll might be even better, otherwise it'd probably use poll under the hood (but with only one socket at a time) anyway..

Though for convenience you could still offer *WithTimeout() variants of the read/receive functions, and for the *WaitFor*() functions I'd argue that (if technically possible) they should always have a timeout that could take -1 for "no timeout/system standard timeout" - even if it's redundant with an explicit poll-style function, it's much easier to use, especially if you only have one socket.

DanielGibson · 2023-08-16T16:53:27Z

BTW, just noticed, SDLNet_WaitForServerIncoming() is a confusing name (IMHO), maybe something like SDLNet_WaitForConnectingClient() or WaitForClientConnection() would be better?

icculus · 2023-09-11T05:32:43Z

BTW, just noticed, SDLNet_WaitForServerIncoming() is a confusing name (IMHO), maybe something like SDLNet_WaitForConnectingClient() or WaitForClientConnection() would be better?

I removed SDLNet_WaitForServerIncoming completely, there's now SDLNet_WaitUntilInputAvailable which can take an array of sockets and sleep the thread until any of them have new input (which in the case of a listen socket, means there's a new connection to accept). This is implemented with poll(). If there's pending data to send, it'll wake up when we can write more and then go right back to poll().

All the Wait functions now accept a timeout (0 to not block at all, -1 for infinite timeout, milliseconds otherwise), and are named better, I think.

icculus · 2023-09-11T05:38:08Z

(Also, this compiles but wasn't tested yet because it's 1:30am. I'll do it in the morning.)

DanielGibson · 2023-09-11T13:38:20Z

By the way, you may wanna look at https://gist.github.com/DanielGibson/bf6bd299c50c1ac1aff4cd063472cbe4 and maybe steal some of its code (it's public domain, though of course I wouldn't mind credit if you end up using bigger parts of it), it documents several differences between Winsock and BSD sockets and has a crossplatform strerror() implementation (at least for the error number relevant for sockets) and also one for gai_strerror()

icculus · 2023-09-24T04:49:10Z

and has a crossplatform strerror() implementation (at least for the error number relevant for sockets) and also one for gai_strerror()

I ended up wrapping error string management in a few functions in 786aaa7 ... in the Windows case, it eventually boils down to a FormatMessage() win32 API call that generates a string in the end-user's language. The WinSock docs say you can FormatMessage any error code that you would get from WSAGetLastError, which covers both the things you'd want from strerror(errno) and gai_strerror().

Some of this will come back. But for now: clean slate!

…ocking. This way the error string is set correctly.

This is generally useful for "are these two addresses the same thing?" but also suitable for using as a compare callback for SDL_qsort.

These might come in with IPPROTO_TCP and then get used for a UDP packet, so it gets upset when it sees a socktype of SOCK_DGRAM with TCP. It can figure it out for now, at least for IPv4 and IPv6.

Please don't use this for anything serious. :)

It only causes the program to fail to start, as it's the only DNS lookup, so it becomes a pain to get the example running at all at high packet-loss rates, when what you _really_ want to see here is what it sounds like when packets go missing.

Apparently some common routers will drop packets that are larger than 1278 bytes...gave myself a little wiggle room here.

icculus · 2023-09-27T16:27:28Z

Compiler warnings are fixed, and voipchat picks a more router-friendly maximum packet size now. What else is still pending?

src/SDL_net.c

…ot `char`. Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>

.github/workflows/main.yml

Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>

src/SDL_net.c

sezero · 2023-09-28T07:35:13Z

We should do something about the following? Appears in SDL_image CI logs too (probably in others' as well.)

fatal: ambiguous argument 'HEAD~..': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'

icculus · 2023-09-28T12:43:38Z

I assumed this is the wikiheaders script trying to figure out the current revision (but I haven't looked into why it fails).

madebr · 2023-09-28T13:30:03Z

I've fixed these warnings in the Perl script of SDL, but have not replicated it to all satellite libraries.

libsdl-org/SDL@e85206f

Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>

icculus · 2023-09-28T14:59:19Z

Ok, _WIN32_WINNT nonsense should be resolved, and the perl script will get updated later with every other satellite library...are we ready to set this live?!

madebr · 2023-09-28T17:04:14Z

Ok, _WIN32_WINNT nonsense should be resolved, and the perl script will get updated later with every other satellite library...are we ready to set this live?!

The question should be: "is the world ready for this library?" 😉

There is something wrong with the man pages. Nothing gets installed.

edit: Never mind I did not know the perl error was fatal on Windows.

slouken · 2023-09-28T17:07:37Z

The question should be: "is the world ready for this library?" 😉

And also, as a game developer, why do I want to use this library?

icculus · 2023-09-28T21:11:30Z

One small step for SDL_net, but just wait until I lose my mind and implement all the bonkers bonus features, later. :)

slouken · 2023-09-28T22:28:34Z

As long as the bonkers bonus features include support for encrypted websockets... ;)

madebr · 2023-09-29T03:38:39Z

Ok, _WIN32_WINNT nonsense should be resolved, and the perl script will get updated later with every other satellite library...are we ready to set this live?!

All SDL3 satellite libraries now have an updated perl script.

madebr · 2023-09-29T05:14:16Z

I fixed and verified the VisualC project builds binaries.
It emits the following warnings:

Severity	Code	Description	Project	File	Line	Suppression State
Warning	C4267	'function': conversion from 'size_t' to 'int', possible loss of data	SDL3_net	C:\Users\maarten\source\repos\SDL_net\src\SDL_net.c	1308	
Warning	C6387	'_Param_(5)' could be '0':  this does not adhere to the specification for the function 'WSARecv'. 	SDL3_net	C:\Users\maarten\source\repos\SDL_net\src\SDL_net.c	30	
Warning	C26451	Arithmetic overflow: Using operator '+' on a 4 byte value and then casting the result to a 8 byte value. Cast the value to the wider type before calling operator '+' to avoid overflow (io.2).	SDL3_net	C:\Users\maarten\source\repos\SDL_net\src\SDL_net.c	146	
Warning	C6011	Dereferencing NULL pointer 'addr'. 	SDL3_net	C:\Users\maarten\source\repos\SDL_net\src\SDL_net.c	190	
Warning	C6011	Dereferencing NULL pointer 'sdlneta'. 	SDL3_net	C:\Users\maarten\source\repos\SDL_net\src\SDL_net.c	562	
Warning	C6011	Dereferencing NULL pointer 'sdlnetb'. 	SDL3_net	C:\Users\maarten\source\repos\SDL_net\src\SDL_net.c	563	
Warning	C6011	Dereferencing NULL pointer 'a'. 	SDL3_net	C:\Users\maarten\source\repos\SDL_net\src\SDL_net.c	570	
Warning	C6011	Dereferencing NULL pointer 'b'. 	SDL3_net	C:\Users\maarten\source\repos\SDL_net\src\SDL_net.c	570	
Warning	C26451	Arithmetic overflow: Using operator '+' on a 4 byte value and then casting the result to a 8 byte value. Cast the value to the wider type before calling operator '+' to avoid overflow (io.2).	SDL3_net	C:\Users\maarten\source\repos\SDL_net\src\SDL_net.c	652	
Warning	C26451	Arithmetic overflow: Using operator '+' on a 4 byte value and then casting the result to a 8 byte value. Cast the value to the wider type before calling operator '+' to avoid overflow (io.2).	SDL3_net	C:\Users\maarten\source\repos\SDL_net\src\SDL_net.c	718	
Warning	C26451	Arithmetic overflow: Using operator '-' on a 4 byte value and then casting the result to a 8 byte value. Cast the value to the wider type before calling operator '-' to avoid overflow (io.2).	SDL3_net	C:\Users\maarten\source\repos\SDL_net\src\SDL_net.c	1064	
Warning	C6011	Dereferencing NULL pointer 'dgram'. 	SDL3_net	C:\Users\maarten\source\repos\SDL_net\src\SDL_net.c	1329	
Warning	C6011	Dereferencing NULL pointer 'a'. 	SDL3_net	C:\Users\maarten\source\repos\SDL_net\src\SDL_net.c	1447	
Warning	C6385	Reading invalid data from 'sock->latest_recv_addrs':  the readable size is '512' bytes, but 'sock->latest_recv_addrs_idx' bytes may be read.	SDL3_net	C:\Users\maarten\source\repos\SDL_net\src\SDL_net.c	1493	
Warning	C6386	Buffer overrun while writing to 'sock->latest_recv_addrs':  the writable size is '512' bytes, but 'sock->latest_recv_addrs_idx++' bytes might be written.	SDL3_net	C:\Users\maarten\source\repos\SDL_net\src\SDL_net.c	1494	
Warning	D9002	ignoring unknown option '/arch:SSE'	SDL3_net	C:\Users\maarten\source\repos\SDL_net\VisualC\cl	1	
Warning	C4267	'function': conversion from 'size_t' to 'int', possible loss of data	SDL3_net	C:\Users\maarten\source\repos\SDL_net\src\SDL_net.c	22	
Warning	C4267	'=': conversion from 'size_t' to 'ULONG', possible loss of data	SDL3_net	C:\Users\maarten\source\repos\SDL_net\src\SDL_net.c	28	
Warning	C4267	'function': conversion from 'size_t' to 'socklen_t', possible loss of data	SDL3_net	C:\Users\maarten\source\repos\SDL_net\src\SDL_net.c	201	
Warning	C4244	'function': conversion from 'const Uint64' to 'Sint32', possible loss of data	SDL3_net	C:\Users\maarten\source\repos\SDL_net\src\SDL_net.c	511	
Warning	C4267	'function': conversion from 'size_t' to 'int', possible loss of data	SDL3_net	C:\Users\maarten\source\repos\SDL_net\src\SDL_net.c	843	
Warning	C4267	'function': conversion from 'size_t' to 'int', possible loss of data	SDL3_net	C:\Users\maarten\source\repos\SDL_net\src\SDL_net.c	936	
Warning	C4267	'function': conversion from 'size_t' to 'int', possible loss of data	SDL3_net	C:\Users\maarten\source\repos\SDL_net\src\SDL_net.c	1286

Also, is this expected?

>simple-http-get.exe libsdl.org
INFO: Looking up libsdl.org ...
INFO: libsdl.org is 192.241.223.99



Failed to read from socket: The system detected an invalid pointer address in attempting to use a pointer argument in a call.

icculus · 2023-09-30T04:58:25Z

We'll mess with the last few Windows details (and encrypted WebSockets, lol) later. For now, I'm merging!

sezero reviewed Aug 14, 2023

View reviewed changes

SDL_net.c Outdated Show resolved Hide resolved

sezero mentioned this pull request Aug 21, 2023

Use const IPaddress * in TCP open functions #81

Closed

sezero mentioned this pull request Sep 10, 2023

Also detect UWP as windows #82

Merged

icculus added 11 commits September 24, 2023 03:02

Delete literally everything.

48535ac

Some of this will come back. But for now: clean slate!

First shot at an SDL3_net API and implementation! WIP!

b595e79

Corrected SDLNet_Address::status comment.

bd4b97a

SDLNet_WaitForResolution should call SDLNet_GetAddressStatus after bl…

3dadbda

…ocking. This way the error string is set correctly.

SDLNet_RefAddress should return the address, for convenience.

7f5e542

Added SDLNet_CompareAddresses.

b359745

This is generally useful for "are these two addresses the same thing?" but also suitable for using as a compare callback for SDL_qsort.

MakeAddrWithPort: don't specify protocol.

b4f722e

These might come in with IPPROTO_TCP and then get used for a UDP packet, so it gets upset when it sees a socktype of SOCK_DGRAM with TCP. It can figure it out for now, at least for IPv4 and IPv6.

SDLNet_GetMaxDatagramSize() was removed, take it out of the headers.

43cd801

Forgot to increment references on cached addresses in ReceiveDatagram.

f25a03c

Added an example VoIP group chat program.

3c57166

Please don't use this for anything serious. :)

voipchat: don't simulate DNS failure.

d2b84e2

It only causes the program to fail to start, as it's the only DNS lookup, so it becomes a pain to get the example running at all at high packet-loss rates, when what you _really_ want to see here is what it sounds like when packets go missing.

madebr force-pushed the sdlnet3-redesign branch from 7e51776 to a3e8f14 Compare September 26, 2023 23:55

icculus added 2 commits September 27, 2023 12:12

voipchat: Clamp datagram payload to 1200 instead of 1500.

cc307c2

Apparently some common routers will drop packets that are larger than 1278 bytes...gave myself a little wiggle room here.

Fixed signed/unsigned comparison warnings.

cbe7b71

madebr reviewed Sep 27, 2023

View reviewed changes

src/SDL_net.c Outdated Show resolved Hide resolved

src/SDL_net.c Outdated Show resolved Hide resolved

icculus and others added 3 commits September 27, 2023 12:57

WinSock write implementation should take a const void * buffer, n…

8c9e027

…ot `char`. Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>

Try to only define _WIN32_WINNT if necessary.

386d5b1

WinSock recvfrom needs a char * buffer, not void *.

65b16bf

madebr reviewed Sep 27, 2023

View reviewed changes

.github/workflows/main.yml Outdated Show resolved Hide resolved

cmake: Enable warnings-as-errors.

c02e699

Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>

madebr reviewed Sep 27, 2023

View reviewed changes

src/SDL_net.c Outdated Show resolved Hide resolved

icculus and others added 2 commits September 28, 2023 10:48

.wikiheaders-options: fix wikisubdir to point to "SDL3_net"

f26495d

Move _WIN32_WINNT check to top of SDL_net.c

fc265ae

Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>

cmake: pass revision to wikiheaders.pl to avoid executing git

91a086d

VisualC: fix MSVC project + tests

9e3b6d7

icculus added 2 commits September 30, 2023 00:24

SDL_net.h: Let wikiheaders reformat the API documentation comments.

0df43d7

Fixed some Visual Studio compiler warnings.

1fb54e4

icculus merged commit b78aa63 into libsdl-org:main Sep 30, 2023
5 checks passed

icculus deleted the sdlnet3-redesign branch September 30, 2023 04:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SDL3_net redesign #79

SDL3_net redesign #79

icculus commented Jul 18, 2023 •

edited

icculus commented Aug 14, 2023

madebr commented Aug 14, 2023

madebr commented Aug 14, 2023 •

edited

icculus commented Aug 14, 2023

icculus commented Aug 14, 2023

sezero commented Aug 14, 2023 •

edited

madebr commented Aug 14, 2023

icculus commented Aug 14, 2023

DanielGibson commented Aug 15, 2023 •

edited

icculus commented Aug 16, 2023

DanielGibson commented Aug 16, 2023 •

edited

DanielGibson commented Aug 16, 2023 •

edited

icculus commented Sep 11, 2023 •

edited

icculus commented Sep 11, 2023

DanielGibson commented Sep 11, 2023 •

edited

icculus commented Sep 24, 2023

icculus commented Sep 27, 2023

sezero commented Sep 28, 2023

icculus commented Sep 28, 2023

madebr commented Sep 28, 2023 •

edited

icculus commented Sep 28, 2023

madebr commented Sep 28, 2023 •

edited

slouken commented Sep 28, 2023

icculus commented Sep 28, 2023

slouken commented Sep 28, 2023

madebr commented Sep 29, 2023

madebr commented Sep 29, 2023

icculus commented Sep 30, 2023

SDL3_net redesign #79

SDL3_net redesign #79

Conversation

icculus commented Jul 18, 2023 • edited

icculus commented Aug 14, 2023

madebr commented Aug 14, 2023

madebr commented Aug 14, 2023 • edited

icculus commented Aug 14, 2023

icculus commented Aug 14, 2023

sezero commented Aug 14, 2023 • edited

madebr commented Aug 14, 2023

icculus commented Aug 14, 2023

DanielGibson commented Aug 15, 2023 • edited

icculus commented Aug 16, 2023

DanielGibson commented Aug 16, 2023 • edited

DanielGibson commented Aug 16, 2023 • edited

icculus commented Sep 11, 2023 • edited

icculus commented Sep 11, 2023

DanielGibson commented Sep 11, 2023 • edited

icculus commented Sep 24, 2023

icculus commented Sep 27, 2023

sezero commented Sep 28, 2023

icculus commented Sep 28, 2023

madebr commented Sep 28, 2023 • edited

icculus commented Sep 28, 2023

madebr commented Sep 28, 2023 • edited

slouken commented Sep 28, 2023

icculus commented Sep 28, 2023

slouken commented Sep 28, 2023

madebr commented Sep 29, 2023

madebr commented Sep 29, 2023

icculus commented Sep 30, 2023

icculus commented Jul 18, 2023 •

edited

madebr commented Aug 14, 2023 •

edited

sezero commented Aug 14, 2023 •

edited

DanielGibson commented Aug 15, 2023 •

edited

DanielGibson commented Aug 16, 2023 •

edited

DanielGibson commented Aug 16, 2023 •

edited

icculus commented Sep 11, 2023 •

edited

DanielGibson commented Sep 11, 2023 •

edited

madebr commented Sep 28, 2023 •

edited

madebr commented Sep 28, 2023 •

edited