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

[Jason Holt]

>
> > -static void exit_sighandler(int x){
> > +void exit_sighandler(int x){
>
> No way.
>

No idea what you're objecting to.


> > +     {"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.
>

-udp-slave -udp-master does just what it should, allowing people to
daisy-chain instead of broadcasting.


> > +int udp_master = 0;
>
> trailing whitespace.
>

done


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


> Must be const.
>

done


> > +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?
>

Made mesg static, removed dummy which was unused anyway.


>
> > +    long sock_flags;
>
> Never ever use the long type. Just don't.
>
>
man fcntl says:

        int fcntl(int fd, int cmd, long arg);

But I assume you know what you're talking about and changed it to int.

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.
>

I understand the sentiment, and did check there before writing my patch.
 I'd have to refactor that code to pull out the basic stuff I need, and then
I'd still have to do the setsockopt() stuff separately.  After refactoring,
the udp stuff probably shouldn't live in stream/ anymore.  I'm at 62
messages in the thread and 21 revisions of the patch, and coming up on the
thread's one year anniversary, so I'm a little reluctant to start
refactoring the streaming code.


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

removed


>
> > +    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;
>

Fixed len.  The ternary looks a little less readable to me, but changed it
anyway.


> > +    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.
>

I recall reading several long, conflicting threads about EINTR, like this
one (with Linus, even):

http://lkml.indiana.edu/hypermail/linux/kernel/0507.3/0004.html

Ultimately I decided to just abort in the unusual circumstance that it came
up, rather than pretending it doesn't exist.  You're correct that
exit_sighandler doesn't immediately exit on SIGINT, and thus might be called
twice if two recvfrom()s both returned EINTR, although that seems like an
unusual condition.

> +    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 you really care, I'll figure out how to change to select().


> > +        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.
>

errno == EINTR is unnecessary there due to while (-1 != recvfrom...)


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

done


> +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.
>

done


> > +        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.
>

This loop is tricky, so let me see if I understand your objection.  You'd
prefer to see this instead?

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

Or maybe even this?

        blocking = !((my_position > udp_master_position -
udp_seek_threshold) &&
                (my_position < udp_master_position + udp_timing_tolerance))

If that's what you're talking about, then the answer is that I did it to be
as clear as possible about what's going on.  We need to know if we're close
behind the master, and use nonblocking i/o if so.  Even if the compiler
didn't optimize away the verbosity, it adds a single int and a few cycles
per frame.  Reading over the code a year later, it was clearer to me than
the optimized alternatives would have been.

> +        // 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".
>

No, it's not a shortcut.  The first if() is because we need to be ready to
play the frame the master is about to tell us to play.  If we're a little
behind, we "run" to catch up.  If not, we wait for the master's message.
 Once we get the message, we normally discover that we're right where we
needed to be.  But sometimes we might discover that the master has seeked,
and that's what the second if() detects.

If you dig back in the thread, my original loop was (in my opinion) easier
to understand, but Uoti proposed the current logic, which took me a while to
understand but was more correct.  It's still hard for me to wrap my head
around, which is why the comment is as long as the loop itself, but each
time I go through it, I'm convinced that it's correct.  The astonishing
thing to me is how offhandedly Uoti wrote it.

> +        if (my_position < udp_master_position + udp_timing_tolerance) {
>
> I think you calculated that once before.
>

New value of udp_master_position.  The comment on get_udp() warns about that
update.

New patch attached.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mplayer-udp-patch-21.diff
Type: text/x-patch
Size: 17002 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20100224/3c572ee6/attachment.bin>