-
Notifications
You must be signed in to change notification settings - Fork 1.1k
RaspiVid is now capable to listen on a certain TCP port and wait for … #359
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
Conversation
Not tested, but looks sane. Not a network sockets expert though, so cannot
comment further.
…On 2 December 2016 at 14:21, maroviher ***@***.***> wrote:
…an incoming connection. The same functionality as
raspivid -o - | nc -k -l -p 5001
but without nc invokation and without pipe read/write overhead. It reduces
cpu usage and video latency. Added help message for an output to a network.
------------------------------
You can view, comment on, or merge this pull request online at:
#359
Commit Summary
- RaspiVid is now capable to listen on a certain TCP port and wait for
an incoming connection. The same functionality as
File Changes
- *M* host_applications/linux/apps/raspicam/RaspiVid.c
<https://github.com/raspberrypi/userland/pull/359/files#diff-0> (111)
Patch Links:
- https://github.com/raspberrypi/userland/pull/359.patch
- https://github.com/raspberrypi/userland/pull/359.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#359>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADqrHXcDrgcnKVgF4RFRtyi9sYZQfAcHks5rEClSgaJpZM4LCmUJ>
.
|
I've just taken a cursory look myself (I don't have the hardware to test this), but everything looks correct and secure. One general comment though, instead of adding 'l' to the beginning of stuff to specify listening, I would suggest using a separate switch, as that's more consistent with both netcat, and quite a few other tools. |
I would distinguish between '-o' (for write to a regular file or stdout) and some new switch for writing to a network, but we have already tcp:// and udp:// prefixes for '-o', so I thought a ltcp:// could be a minimal change to an already existing switch. |
My main complaint is that it's not intuitive (ltcp:// looks like a different protocol from tcp://, but it isn't), and it's inconsistent with most other tools. Take a look at nc, or iperf, or other tools that can listen or connect to a remote listener. Essentially all of them use a switch to determine whether to run as a server or client, because it makes things clearer, and it's a whole lot safer to parse a switch than to parse a free-form argument. |
Out of interest, what does this change actually do over and above the tcp
option?
…On 5 December 2016 at 12:48, Austin S. Hemmelgarn ***@***.***> wrote:
My main complaint is that it's not intuitive (ltcp:// looks like a
different protocol from tcp://, but it isn't), and it's inconsistent with
most other tools. Take a look at nc, or iperf, or other tools that can
listen or connect to a remote listener. Essentially all of them use a
switch to determine whether to run as a server or client, because it makes
things clearer, and it's a whole lot safer to parse a switch than to parse
a free-form argument.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#359 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADqrHfGmg-ahs2Q37frPDa7acdk6e5mLks5rFAg0gaJpZM4LCmUJ>
.
|
@JamesH65 @Ferroin |
I think a separate command line option for listen mode is more consistent with how netcat and similar tools work, so that would be my preference. |
Inclined to agree - this is actually waiting on input, which isn't really
appropriate for a output switch.
…On 5 December 2016 at 15:20, popcornmix ***@***.***> wrote:
I think a separate command line option for listen mode is more consistent
with how netcat and similar tools work, so that would be my preference.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#359 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADqrHf623bOdcg17Z8nGPRtV6xU4OO0Zks5rFCvTgaJpZM4LCmUJ>
.
|
@popcornmix Howewer, It would not be backwards compatible to the current version, where '-o' can be used with tcp:// and udp:// prefixes. Where one must say, you do not have any chance to learn about this prefixes in the current version, because nothing about that is in the usage text. I have come to it by studying of the source code. |
{ CommandOutput, "-output", "o", "Output filename <filename> (to write to stdout, use '-o -')", 1 }, | ||
{ CommandOutput, "-output", "o", "Output filename <filename> (to write to stdout, use '-o -').\n" | ||
"\t\t Connect to a remote Host (e.g. tcp://192.168.1.2:1234, udp://192.168.1.2:1234)\n" | ||
"\t\t Listen on a TCP port and wait for an incoming connection (e.g. ltcp://0.0.0.0:1234, ltcp://192.168.1.1:1234)", 1 }, |
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.
LTCP seems like an unfortunate choice, since it is an existing (albeit draft, and now forgotten) protocol:
https://tools.ietf.org/html/draft-bhandarkar-ltcp-00
A separate option to enable this might (as others have said) be better.
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.
I agree, see my previous post.
} | ||
network = network_listen = 1; | ||
struct sockaddr_in serv_addr, cli_addr; | ||
bzero((char *) &serv_addr, sizeof(serv_addr)); |
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.
memset is preferred over bzero:
http://pubs.opengroup.org/onlinepubs/009695399/functions/bzero.html
int sockListen = socket(AF_INET, SOCK_STREAM, 0); | ||
if (sockListen < 0) | ||
{ | ||
fprintf(stderr, "error %d creating socket\n", errno); |
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.
It would be helpful to also convert errno to a string with strerror().
sfd = accept(sockListen, (struct sockaddr *) &cli_addr, &clilen); | ||
if (sfd < 0) | ||
{ | ||
close(sockListen); |
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.
You can move close() to the error handling at the end.
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.
No, I can not, because at the end is not only an error handling.
|
||
serv_addr.sin_family = AF_INET; | ||
serv_addr.sin_port = htons(serv_addr.sin_port); | ||
int true = 1; |
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.
Using "true" as a variable name seems brave. It probably won't compile under C99.
if (sfd < 0) | ||
{ | ||
close(sockListen); | ||
fprintf(stderr, "error %d on accept\n", errno); |
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.
Would be nicer to report strerror(errno).
sfd = socket(AF_INET, socktype, 0); | ||
if (sfd < 0) | ||
{ | ||
perror("socket"); |
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.
Error handling here?
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 not my, but an already signed-off code, on 24 august 2015, see git log.
|
||
if (connect(sfd, (struct sockaddr *) &saddr, sizeof(struct sockaddr_in)) < 0) | ||
{ | ||
perror("connect"); |
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.
Error handling?
Strictly speaking this could also just return EINTR and require retrying on a signal. There's a rather ugly gcc macro to hide this.
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, each system call can return EINTR. Should we add EINTR handling around each system call? What macro is that?
The same, highlighted code is already signed-off
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.
TEMP_FAILURE_RETRY() is the macro. It's defined in unistd.h if __USE_GNU is defined, which I think we are defining already (via _GNU_SOURCE).
Using it will stop this being easily portable to other glibc versions (e.g. musl) so maybe this should be done in a followup patch?
saddr.sin_port = htons(port); | ||
inet_aton(filename, &saddr.sin_addr); | ||
fprintf(stderr, "Client connected from %s:%"SCNu16"\n", inet_ntoa(cli_addr.sin_addr), ntohs(cli_addr.sin_port)); | ||
close(sockListen); //do not listen on a given port anymore |
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.
Could combine all error handling in one place.
{ | ||
perror("socket"); | ||
} | ||
network = network_listen = 1; |
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.
Could we use bool here instead? We're using other C99 features in this 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, we can, the int type was already there,
As we only support tcp for listening, and the IP address is unused, then only the port is valid. |
The IP address is typically used to bind the listening port to one specific network interface on a multi-homed system, so I'd say it is used. The code appears to read it and populate serv_addr.sin_addr.s_addr with it. |
|
||
unsigned char* chp = (unsigned char*) &serv_addr.sin_addr.s_addr; | ||
if (5 != sscanf(filename + sizeof(ltcpSrch) - 1, | ||
"%"SCNu8".%"SCNu8".%"SCNu8".%"SCNu8":%"SCNu16, chp + 0, chp + 1, chp + 2, |
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.
We already do the equivalent for normal network connections using inet_aton at 1123. That's the normal call used for converting host names to addresses. Any reason we need to roll our own here?
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.
I've tried to change the existing code as little as is going.
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.
I was implying that inet_aton (as used at 1123) is the normal way to handle network addresses, rather than rolling your own sscanf line.
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.
sscanf on that place also checks the correctness of a string.
A current version of raspivid will just crash if you type:
raspivid -o tcp://0.0.0.0
Segmentation fault (core dumped)
Okay, how about: Is there much value in supporting both writing to a file and sending over a network (I'm thinking not much, but others may have different ideas). |
I'm good with that.
Simultaneously? I'd agree not, and it complicates the code quite a lot. |
Yes specifying both |
I would also add the listening mode for UDP, to do that I have to use sendto instead of fdopen->fwrite. That's why I thought, if I change so much, it could not be a big deal to use a regular file write and network write simultaneously. Some people can find such feature useful, e.g. live observation over network and simultaneously write to a file on a PI. Some examples one more time: #wait for an incoming tcp connection on all interfaces. #wait for an incoming tcp connection on network interface #wait for an incoming udp packet on all interfaces #start sending UDP packets to a remote IPv4 host 195.12.1.2:1234 #initiate a TCP connection to a remote IPv4 host 195.12.1.2:1234 #initiate a TCP connection to a remote IPv4 host 195.12.1.2:1234 What do you think? |
No interest in network listening features? |
I have reservations about adding more big new features to Raspivid/still. They were meant as demo code and relatively simple examples of how to communicate with the camera. They're morphing into huge things which have so many bells and whistles that trying to grasp the basics is becoming harder. While network listening is just modifying the setup that is relatively simple to comprehend. Adding in output to multiple locations puts in yet more stuff to the buffer callbacks, and those are already busy with all the code for inline motion vector, circular buffer, and saving time stamps. |
Good points. Might have the chance to look at this next year. Some thinking
needed I suspect. Might be worth creating a library so we can have many
small apps that provide the sort of facilities mentioned here but have the
same underlying 'camera' base.
…On 16 December 2016 at 11:18, 6by9 ***@***.***> wrote:
I have reservations about adding more big new features to Raspivid/still.
They were meant as demo code and relatively simple examples of how to
communicate with the camera. They're morphing into huge things which have
so many bells and whistles that trying to grasp the basics is becoming
harder.
While network listening is just modifying the setup that is relatively
simple to comprehend. Adding in output to multiple locations puts in yet
more stuff to the buffer callbacks, and those are already busy with all the
code for inline motion vector, circular buffer, and saving time stamps.
If raspivid is to get extra features like this, I think we need to sort
really simple example apps first (hello_pi type level but using MMAL), and
then refactor the buffer callback to break out the various options and make
it more readable. These aren't tasks I'm asking you to do, but would
benefit from being done.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#359 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADqrHZW384UB5JgpX6Da5EFhsvBXowNBks5rInOagaJpZM4LCmUJ>
.
|
In general I will comment on two things about this:
|
My current position is that I'd happily accept the current patch to add listening if it used either Simultaneous streaming and local saving I'll currently push back on due to the significant changes it requires. Part of my reservation is that you really then need to be passing the buffers to separate threads to have the network send and file save happening in parallel, so another big jump in complexity. Your example of
is curious. What are you considering as a UDP connect? Such a thing does not exist as UDP is a connection-less protocol. |
Sorry, "UDP connect" is poorly expressed. UDP is a connection-less protocol. But sometimes it does matter which of counterparts sends the first packet, for a communication to be possible. Imagine following: That is what I meant with: #wait for an incoming udp packet on all interfaces But OK, if you want to keep raspivid simple, I would just add a new -l switch for TCP listening only and add some more error handling (-EINTR). Try to run with a camera attached :) |
I know the use case you're meaning - effectively you're auto-configuring the remote IP address/port based on whoever last sent data to your port. It does require a chunk more changes that make the code harder to understand. There are things happening in the new year that should allow resourcing of tidying stuff up, so at that point adding UDP "listen" in a tidy manner is feasible. Error handling on your patch would be good. |
8a9789d
to
85477ad
Compare
Can you squash these two commits now? I'm happy with the code overall - many thanks for your efforts. |
…port given (raspivid -o tcp://1.1.1.1). Added listening mode for tcp (e.g. raspivid -l -o tcp://192.168.0.1:5001)
All commits were squashed in a one single commit. |
@6by9 The github GUI now allows us to squash and merge or rebase all of the commits attached to a PR. For more complicated scenarios you still have to resort to the command line, but this is a step forward. |
@maroviher Thank you. Any other reviewers want to chime in? |
Ping @luked99 and @JamesH65. Do you want to review again, or should @popcornmix merge as is? |
I'm OK with it. |
@popcornmix: happy to merge this? Luke seems to be elsewhere at the moment, but both James and I are happy. |
firmware: arm_loader: Populate kaslr_seed dt entry See: #694 firmware: raspivid: listen on TCP port for incoming connection See: raspberrypi/userland#359
firmware: arm_loader: Populate kaslr_seed dt entry See: raspberrypi/firmware#694 firmware: raspivid: listen on TCP port for incoming connection See: raspberrypi/userland#359
- firmware: Add usb/net support from next branch - firmware: Fix usb/net boot issue See: Hexxeh/rpi-firmware#134 - firmware: Redo CEC code cleanup Parts 1-10 - firmware: arm_loader: Populate kaslr_seed dt entry See: #694 - firmware: raspivid: listen on TCP port for incoming connection See: raspberrypi/userland#359 - firmware: dispmanx: Return failure when dispmanx_resource_create fails to allocate image - firmware: display_server: Avoid overwriting a host allocated resource when an allocation fails See: #723 - firmware: vce: Fix unsafe access without lock - firmware: vce: Remove unwanted vce_release_semaphore when obtain failed - firmware: hdmi: Use hdmi drive when any hdmi modes are supported See: https://www.raspberrypi.org/forums/viewtopic.php?f=66&t=169879 - firmware: di_adv: Fix for green artefacts regression See: http://forum.kodi.tv/showthread.php?tid=304573 - firmware: di_adv: Avoid artefacts at bottom of video for YUV420 See: http://forum.kodi.tv/showthread.php?tid=304814 - firmware: raspistill: Added SIGUSR2 signal to capture and exit immediately See: raspberrypi/userland#368 - firmware: dispmanx: Protect a null element access - firmware: leds: Provide a way of changing LED assignments for CM3 - firmware: leds: Prevent re-initialisation - firmware: arm_display: Fix limit of aspect ratio of two to one See: https://www.raspberrypi.org/forums/viewtopic.php?f=28&t=5851&start=475#p1101545 - firmware: raspicam: Fixed dummy error: SIGUSR2 should capture and exit even if verbose is false See: raspberrypi/userland#372 - firmware: leds: Allow controlled re-initialisation - firmware: platform: Always bit-bash the SMPS and GPIO expander - firmware: i2c_gpio: Remove pointless latch get/put - firmware: platform: Remove unused/incorrect CEC_OSD_NAME define - firmware: Fixup CEC code cleanup 8: fixed hdmi state machine clk See: #732 - firmware: CEC code cleanup 11: cec_release_logical_addr - firmware: CEC code cleanup 12: CEC init @ HPD firmware: IL image_encode: Add BGR888 support - firmware: FXL6408 expander: allow readback of output state - firmware: IL Video_splitter: Handle stereoscopic into buffers See: waveform80/picamera#342 - firmware: IL Image_encode: Correct list of supported formats See: #733 - firmware: i2c_gpio: Disable logging - firmware: Camplus annotate: hold back lines until annotated See: #701 - firmware: FXL6408/GPIOman: Support config of termination via dt-blob - firmware: GPIO expander: Add API to reconfigure pins - firmware: GPIOMAN: Add API to reconfigure pins - firmware: Mailbox service: Add command to reconfigure GPIO setup - firmware: FXL6408: Return success code on reading status of an output - firmware: GPIO expander: Add set/get_config functions to dummy driver
firmware: arm_loader: Populate kaslr_seed dt entry See: raspberrypi#694 firmware: raspivid: listen on TCP port for incoming connection See: raspberrypi/userland#359
…an incoming connection. The same functionality as
raspivid -o - | nc -k -l -p 5001
but without nc invokation and without pipe read/write overhead. It reduces cpu usage and video latency. Added help message for an output to a network.