[MPlayer-dev-eng] [PATCH] Network synchronized playback using UDP

[Reimar Döffinger]

On Wed, Feb 24, 2010 at 12:31:05PM -0800, Jason Holt wrote:
> @@ -766,7 +777,7 @@
>  static int crash_debug = 0;
>  #endif
>  
> -static void exit_sighandler(int x){
> +void exit_sighandler(int x){

No way.

> +	{"udp-slave", &udp_slave, CONF_TYPE_FLAG, 0, 0, 1, NULL},
> +	{"udp-master", &udp_master, CONF_TYPE_FLAG, 0, 0, 1, NULL},

And what happens with
-udp-slave -udp-master?
I admit it's less user-friendly, but a -udpmode with e.g. 0 == normal,
1 == master, 2 == slave would leave much less strange cases.

> +int udp_master = 0; 

trailing whitespace.

> +char *udp_ip = "127.0.0.1"; // where the master sends datagrams

Must be const.

> +void get_udp(int blocking)
> +{
> +    static int done_init_yet = 0;
> +    static int sockfd;
> +    static struct sockaddr_in servaddr, cliaddr;
> +
> +    int n;
> +    socklen_t len;
> +    char mesg[1000];
> +    char dummy[1000]; // for clearing the buffer

Since you are already using above static variables so it's impossible
to use more than one instance, why are you exactly putting the huge stuff
on the stack?

> +    long sock_flags;

Never ever use the long type. Just don't.

> +    if (!done_init_yet) {
> +        struct timeval tv;
> +
> +        done_init_yet = 1;
> +
> +        sockfd = socket(AF_INET, SOCK_DGRAM, 0);
> +
> +        memset(&servaddr, sizeof(servaddr), 0);
> +        servaddr.sin_family =      AF_INET;
> +        servaddr.sin_addr.s_addr = htonl(INADDR_ANY);
> +        servaddr.sin_port =        htons(udp_port);
> +        bind(sockfd, (struct sockaddr *)&servaddr, sizeof(servaddr));

We already have udp-related code in stream/stream_udp.c.
Given that completely broken network-related code always seems to be I'd
rather have not any of it more than once.

> +        tv.tv_sec = 30;
> +        setsockopt(sockfd, SOL_SOCKET, SO_RCVTIMEO, (struct timeval *) &tv, sizeof(struct timeval));

Why the cast??

> +    len = sizeof(cliaddr);
> +
> +    sock_flags = fcntl(sockfd, F_GETFL, 0);
> +    if (blocking) {
> +        fcntl(sockfd, F_SETFL, sock_flags & (~O_NONBLOCK));
> +    } else {
> +        fcntl(sockfd, F_SETFL, sock_flags | O_NONBLOCK);
> +    }
> +
> +    n = recvfrom(sockfd, mesg, 999, 0, (struct sockaddr *)&cliaddr, &len);

Assigning len and using it shouldn't be so far apart.
Also why is the fcnt stuff duplicated if only sock_flags change?
sock_flags = blocking ? sock_flags & ~O_NONBLOCK : sock_flags | O_NONBLOCK;


> +    if (n == -1 && errno == EINTR) {
> +        exit_sighandler(SIGINT); // actual signal number isn't available
> +    }

Not sure, but I suspect you completely misunderstood what EINTR means.
In particular this would end up calling the signal handler a second time
for a single signal.

> +    fcntl(sockfd, F_SETFL, sock_flags | O_NONBLOCK);
> +    // flush out any further messages so we don't get behind
> +    while (-1 != recvfrom(sockfd, mesg, 999, 0, (struct sockaddr *)&cliaddr, &len)) {

I think select is the generally accepted method, not using O_NONBLOCK trickery.
Though I admit I do my best not to get in contact with the mess that network programming is.

> +        if (strcmp(mesg, "bye") == 0 || errno == EINTR) {
> +            mp_msg(MSGT_CPLAYER, MSGL_INFO, MSGTR_MasterQuit);

At the very least the message is nonsense for EINTR.
In principle, not retrying will break SIGTRAP debugging helper code -
not that I consider it work much effort to avoid that.

> +        ip_valid = inet_aton(send_to_ip, &(socketinfo.sin_addr));

pointless ()

> +void udp_slave_sync(MPContext *mpctx) 
> +    // here's how this works: 
> +    // play frames with no delay until we catch up.    we keep an eye on
> +    // the master's position while we do so, but without blocking.    
> +    // so, udp_seek_threshold lets us judge whether we're so far off that 

Trailing whitespace and strange whitespace in the middle there.

> +        if ((my_position > udp_master_position - udp_seek_threshold) &&
> +                (my_position < udp_master_position + udp_timing_tolerance)) {
> +            closebehind = 1;
> +        } else {
> +            closebehind = 0;
> +        }
> +
> +        blocking = !closebehind;

Considering that closebehind does not seem to be used anywhere that is a
ridiculously complex way of doing it.

> +        // timing_error < 0 means we're behind the master
> +        timing_error = mpctx->sh_video->pts - udp_master_position;
> +        absolute_timing_error = (timing_error < 0) ? -timing_error: timing_error;
> +
> +        if (absolute_timing_error > udp_seek_threshold) {

This is not really consistent with the (my_position > udp_master_position - udp_seek_threshold)
condition used before. I suspect the previous condition is a shortcut, in which case I
can only say that I consider logic that is duplicated in slightly different
ways is what I consider "a maintainer's nightmare".

> +        if (my_position < udp_master_position + udp_timing_tolerance) {

I think you calculated that once before.