[FFmpeg-devel] [PATCH] avformat/udp: Add a delay between packets for streaming to clients with short buffer

[Michael Niedermayer]

On Tue, Mar 08, 2016 at 11:27:45PM +0300, Pavel Nikiforov wrote:
> Nicolas George george at nsup.org wrote:
> 
> >>  libavformat/udp.c | 133 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
> >>  1 file changed, 129 insertions(+), 4 deletions(-)
> 
> >Missing documentation update.
> fixed.
> 
> >> -    while(1) {
> >> +    for(;;) {
> 
> >Stray change.
> Portability. You don't know how a compiler other than clang or gcc
> will optimize or not optimize "while (1) { }".
> 
> 
> >> +    if (!(h->flags & AVIO_FLAG_NONBLOCK)) {
> 
> >This looks suspicious: blocking/nonblocking is a property of the protocol as
> >seen by the calling application, it should not apply in a thread used
> >internally. See below.
> The thread should work as original non-thread code.
> 
> >> +        ret = sendto (s->udp_fd, buf, size, 0,
> >> +                      (struct sockaddr *) &s->dest_addr,
> 
> >Style: no spaces after function names and casts.
> It is not my code. This block from
> static int udp_write(URLContext *h, const uint8_t *buf, int size)
> 
> >> +        av_usleep(s->packet_gap);
> 
> >Not very reliable: if the task gets unscheduled for some time, it will be
> >added to the gap and never catch, possibly causing playback hiccups. A
> >system with a kind of rate control would probably be better.
> Want reliable delay ? Spin in a loop polling dogettimeofday() in
> kernel w/o task switch. Otherwise the only thing we have is usleep() .
> No more options available to make a delay in user space: all of them
> cause task switch with unpredicted return time after delay (preemtable
> kernel, you knows).
> 
> If all goes right and a system is not overloaded the gap between
> packets will be as set (and tcpdump with -ttt prove this). Also, we
> have a sendto() syscall (preemption again), the UDP socket buffer, a
> network device TX ring and many other things like "backpressure" on an
> ethernet switch port that will cause the delay be more than specified.
> 
> >> +    /*
> >> +      Create thread in case of:
> >> +      1. Input and circular_buffer_size is set
> >> +      2. Output and packet_gap and circular_buffer_size is set
> >> +    */
> 
> >UDP sockets can be read+write; in this case, two threads are needed.
> The UDP socket are mostly read or mostly write. The thread will handle
> the most part of socket communication, no need to make a thread for
> handle some packets.
> 
> Fixed version of the patch in attachment.

>  doc/protocols.texi |    3 +
>  libavformat/udp.c  |  136 +++++++++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 135 insertions(+), 4 deletions(-)
> 04ecb9352d570991947f4481df8f3b3fcdba13a4  udp.patch
>  libavformat/udp.c  | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  doc/protocols.texi |   3 +++
>  2 file changed, 135 insertions(+), 4 deletions(-)

applied with alot of fixes
i kept the code and most fixes in a seperate commit, felt more correct
to me as i changed alot

Thanks

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160525/bb2992ba/attachment.sig>