-
Notifications
You must be signed in to change notification settings - Fork 866
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
C examples update #276
C examples update #276
Conversation
examples/make-test-c.sh
Outdated
@@ -0,0 +1,9 @@ | |||
#!/bin/bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just a Makefile?
It might be probably even better if this uses the installed version of SRT. For example, instruct the user that they should do:
- Configure with installation directory of your choice, may be also inside this directory, e.g. "installed":
configure --prefix=installed
- make && make install
- Compile this pointing the directory in SRTDIR variable:
make SRTDIR=../installed
- Makefile will contain something like:
FLAGS=$(shell PKG_CONFIG_PATH=$(SRTDIR)/lib/pkgconfig pkg-config --cflags --libs srt openssl)
test-c-server: test-c-server.c
test-c-client: test-c-client.c
%:%.c
$(CC) -o $@ $^ $(FLAGS) -lstdc++
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ethouris ,
I know about your opinion. And you're right!
However, I think on this examples as a "guide". This it's the reason for a very simple script. So, I suggest to commit in this form (at time).
Regarding the "install" I suggest to support two ways in the "configure": Option 1 it's the "make install". And the Option 2 it's "make package", that generates a target directory with "target/lib", "target/include" and some examples. Why this? For use it for cross-compilation without installing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because this can also demonstrate to the users how they can write their applications. In order to have a directory "target" with installed things, it's enough that you simply do configure --prefix=target
. Although I think there could be a script that would first compile the SRT library into a directory local to examples
and then continues with compiling and linking against it...
} | ||
|
||
printf("srt setsockflag\n"); | ||
srt_setsockflag(ss, SRTO_SENDER, &yes, sizeof yes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mind how the user would like to call this application, and whether the stream would be sent from client to server or the other way around. Might be that it would be specified by options, and if not (for simplification) it's rather better that we have a kinda "stream server", so server is sending, client is reading. Important to inform the user that this flag is only needed to communicate with SRT with version earlier than 1.3.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The user doesn't need to call this application. It does "dummy" work. This it's only an "example" code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but you know that users will next try to compile it and see if this works...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Me too! 😄
BTW - after my changes with reshaping the project's directories, there's |
Come on, I just did this together with some other work. During this "reshaping" I created the examples directory, moved this file to it, and then figured out that it's a complete messup and doesn't work, so I added some fixes. Sorry for that. But still, we need an example with connect and with accept, and without this PR we wouldn't have the latter. |
Hello? We still need this. |
Hi @ethouris , Yes! I'm very busy. I'll review soon. |
9d8f115
to
cecfb2c
Compare
Deleted make-test-c.sh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Updated with the latest master
- Replaced deprecated
srt_socket(AF_INET, SOCK_DGRAM, 0)
withsrt_create_socket()
. - Fixed
sin_family
not being set insockaddr_in
. - Deleted
make-test-c.sh
if favor of including the samples to CMakeLists.txt. - Fixed other examples (updated
logging
->srt_logging
namespace).
This it's an upgrade from original PR #21
Take into account that the file
examples/make-test-c.sh
assumes the structure ofexamples/include/
andexamples/lib/
described in #265.Regards.