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

[Jason Holt]

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

Sounds like my mistake.  I think exit_sighandler was catching the signal but
not dying, leading me to believe that the signal wasn't getting caught at
all.  Removed the EINTR stuff.


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

Spurious.  Removed.


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

Right you are.  Originally this was going to be user-configurable, which is
why it was global.  Changed to a #define.


> > +    static struct sockaddr_in servaddr, cliaddr;
>
>
Can't see why those are static.
>

fixed.


> > +    static char mesg[1000];
>
> Since we shouldn't really need it to be that large, this
> could be on stack as well.
>

Changed to 100.


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

fixed


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

fixed


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

fixed


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

fixed


> 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 agree, that's much better.  Fixed.


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

fixed

updated patch attached.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mplayer-udp-patch-25.diff
Type: text/x-patch
Size: 15458 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20100303/6ebe83f0/attachment.bin>