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

[Reimar Döffinger]

On Thu, Feb 25, 2010 at 01:36:06PM -0800, Jason Holt wrote:
> > Making it public and using it from anywhere else. I don't want a signal
> > handler
> > called from normal code.
> 
> I recall now why I added the exit_sighandler and EINTR code.  Without
> checking for EINTR, if you ^C mplayer while it's waiting for a datagram from
> the master, it doesn't die, since the signal got caught by the system.  So
> in that case, you have to send mplayer a second signal before it dies.  So
> that's why I'm calling the sighandler if we get EINTR.  I've added a comment
> to that effect.

The problem with this comment:
> +    // if you send mplayer a signal while it's waiting for a recvfrom, the
> +    // system intercepts the signal and recvfrom aborts with EINTR.
> +    if (n == -1 && errno == EINTR) {
> +        exit_sighandler(SIGINT); // actual signal number isn't available
> +    }

is that it's just nonsense.
After testing with this program:
#include <stdio.h>
#include <signal.h>
#include <errno.h>
#include <sys/types.h>
#include <sys/socket.h>

static void sighandler(int sig) {
  printf("got signal %i\n", sig);
}

int main(void) {
  int r;
  int s = socket(AF_INET, SOCK_STREAM, 0);
  listen(s, 0);
  signal(SIGINT, sighandler);
  siginterrupt(SIGINT, 1);
  r = accept(s, NULL, NULL);
  printf("returned %i %i\n", r, errno);
}

The correct comment would be:
// if you send mplayer a signal while it's waiting for a recvfrom,
// recvfrom aborts with EINTR and the signal handler is called.
// Now for the pure fun of it we call the signal handler once more
// with a bogus value, causing MPlayer to panic and just abort the
// whole program instead of cleaning up properly.
// Has the side effect that ^C "works" on some systems without having
// to do it the proper way e.g. by checking async_quit_request.
// However it's all quite pointless since a lot of kernels (Ubuntu 9.10
// at least) do not abort the syscall and do not return EINTR anyway
// even if a signal arrived.

> I'm happy to take it out if you prefer; it just means it's a little harder
> to kill mplayer when using -udp-slave.

On my system at least tests indicate it doesn't make any difference anyway,
not without additional changes.

> Sorry, I somehow imagined it was the variable you used for the return
> > value.
> > In this case it seems like long unfortunately is the right type :-(
> >
> 
> reverted

Doesn't matter, but I just noticed what's so confusing: the fcntl argument
is long, but the return value is int.
Since the flags must fit in both there is no "correct type" and either one works.

> @@ -94,6 +96,11 @@
>  
>  #include "input/input.h"
>  
> +#if !HAVE_WINSOCK2_H
> +#include <netinet/in.h>
> +#endif
> +

I can't quite see what this is needed for.

> +// globals for UDP sync
> +float udp_master_position = -1.0; // remember where the master is in the file
> +float udp_timing_tolerance = 0.02; // how far off is still considered equal

At least for udp_timing_tolerance I fail to see where it is used outside this
file or modified?

> +    static struct sockaddr_in servaddr, cliaddr;

Can't see why those are static.

> +    static char mesg[1000];

Since we shouldn't really need it to be that large, this
could be on stack as well.

> +        setsockopt(sockfd, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(struct timeval));

sizeof(tv) ?

> +    n = recvfrom(sockfd, mesg, 999, 0, (struct sockaddr *)&cliaddr, &len);

sizeof(mesg) - 1 ?

> +    while (-1 != recvfrom(sockfd, mesg, 999, 0, (struct sockaddr *)&cliaddr, &len)) {
> +        mesg[999] = 0;
> +        mesg[n] = 0;

I'm not really happy about the code duplication and the two different ways
of doing the 0-termination

> +            udp_master_position = master_position;

Making it a return value would probably make the main function a lot less confusing.
Or at least a pointer argument if the case where udp_master_position should not
change would be too messy with a return value.
Also this function is not used outside this file, so it should be static.

> +        // but if we've fallen behind a few seconds, we're "close behind"
> +        // and should run to catch up
> +        if ((my_position > udp_master_position - udp_seek_threshold) &&
> +                (my_position < udp_master_position + udp_timing_tolerance)) {

But that comment does not even describe what the code does!
It also triggers as "close behind" if we are only a slight bit ahead (well, by the
numbers, maybe not in reality).
There's also no explanation why the udp_timing_tolerance is only applied to
the lower check, but not to any of the udp_seek_threshold ones for example.
Probably it's a simplification assuming udp_timing_tolerance << udp_seek_threshold,
but that's at least not a documented assumption.
Or it is because the udp_timing_tolerance actually should be named udp_transfer_delay.

> +        // we're either right with the master, or close behind.  play
> +        // the frame that's currently ready
> +        if (my_position < udp_master_position + udp_timing_tolerance) {

And this _is_ checking the same thing as the condition on the next loop around.
Which leads me to the question why the code is not like

// try updating our info without waiting
exit = get_udp(0, &udp_master_position);

while (!exit) {
    // we are too far off, seek
    if (FFABS(mpctx->sh_video->pts - udp_master_position) > udp_seek_threshold) {
...
    }
    // time to display, master has (hopefully just) passed us
    if (udp_master_position + udp_delay > my_position)
        break;
    // wait for master/update information
    exit = get_udp(1, &udp_master_position);
}

I also have to add that it would be nicer if the exit wasn't hidden all the
way in get_udp but instead the information was passed all the way up to main().