Re: [FFmpeg-devel] [PATCH] avformat/udp: Add a delay between packets for streaming to clients with short buffer
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 *) >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
signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] avformat/udp: Add a delay between packets for streaming to clients with short buffer
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 *) >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.
udp.patch
Description: Binary data
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avformat/udp: Add a delay between packets for streaming to clients with short buffer
Le primidi 11 ventôse, an CCXXIV, Pavel Nikiforov a écrit :
> Hello !
>
> This patch enables background sending of UDP packets with specified delay.
> When sending packets without a delay some devices with small RX buffer
> ( MAG200 STB, for example) will drop tail packets in burst causing
> decoding errors.
>
> It needs to specify "fifo_size" with "packet_gap" .
>
> The output url will looks like udp://xxx:yyy?fifo_size= size>_gap=
>
> Patch attached.
Thanks for the patch. Comments below.
> libavformat/udp.c | 133
> --
> 1 file changed, 129 insertions(+), 4 deletions(-)
Missing documentation update.
>
> diff --git a/libavformat/udp.c b/libavformat/udp.c
> index ea80e52..ff676f4 100644
> --- a/libavformat/udp.c
> +++ b/libavformat/udp.c
> @@ -92,6 +92,7 @@ typedef struct UDPContext {
> int circular_buffer_size;
> AVFifoBuffer *fifo;
> int circular_buffer_error;
> +int packet_gap; /* delay between transmitted packets */
> #if HAVE_PTHREAD_CANCEL
> pthread_t circular_buffer_thread;
> pthread_mutex_t mutex;
> @@ -112,6 +113,7 @@ typedef struct UDPContext {
> #define E AV_OPT_FLAG_ENCODING_PARAM
> static const AVOption options[] = {
> { "buffer_size","System data size (in bytes)",
> OFFSET(buffer_size),AV_OPT_TYPE_INT,{ .i64 = -1 },-1, INT_MAX,
> .flags = D|E },
> +{ "packet_gap", "Delay between packets, in usec",
> OFFSET(packet_gap), AV_OPT_TYPE_INT,{ .i64 = 0 }, 0, INT_MAX,
> .flags = E },
As pointed by Michael, this should use AV_OPT_TYPE_DURATION.
> { "localport", "Local port",
> OFFSET(local_port), AV_OPT_TYPE_INT,{ .i64 = -1 },-1, INT_MAX,
> D|E },
> { "local_port", "Local port",
> OFFSET(local_port), AV_OPT_TYPE_INT,{ .i64 = -1 },-1, INT_MAX,
> .flags = D|E },
> { "localaddr", "Local address",
> OFFSET(localaddr), AV_OPT_TYPE_STRING, { .str = NULL },
> .flags = D|E },
> @@ -486,7 +488,7 @@ static int udp_get_file_handle(URLContext *h)
> }
>
> #if HAVE_PTHREAD_CANCEL
> -static void *circular_buffer_task( void *_URLContext)
> +static void *circular_buffer_task_rx( void *_URLContext)
> {
> URLContext *h = _URLContext;
> UDPContext *s = h->priv_data;
> @@ -499,7 +501,7 @@ static void *circular_buffer_task( void *_URLContext)
> s->circular_buffer_error = AVERROR(EIO);
> goto end;
> }
> -while(1) {
> +for(;;) {
Stray change.
> int len;
>
> pthread_mutex_unlock(>mutex);
> @@ -542,6 +544,81 @@ end:
> pthread_mutex_unlock(>mutex);
> return NULL;
> }
> +
> +static void do_udp_write(void *arg, void *buf, int size) {
> +URLContext *h = arg;
> +UDPContext *s = h->priv_data;
> +
> +int ret;
> +
> +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.
> +ret = ff_network_wait_fd(s->udp_fd, 1);
> +if (ret < 0) {
> +s->circular_buffer_error=ret;
Please keep in line with surrounding coding style. In this instance: spaces
around operators.
> +return;
> +}
> +}
> +
> +if (!s->is_connected) {
> +ret = sendto (s->udp_fd, buf, size, 0,
> + (struct sockaddr *) >dest_addr,
Style: no spaces after function names and casts.
> + s->dest_addr_len);
> +} else
> +ret = send(s->udp_fd, buf, size, 0);
> +
> +s->circular_buffer_error=ret;
> +}
> +
> +static void *circular_buffer_task_tx( void *_URLContext)
> +{
> +URLContext *h = _URLContext;
> +UDPContext *s = h->priv_data;
> +int old_cancelstate;
> +
> +pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, _cancelstate);
> +
> +for(;;) {
> +int len;
> +uint8_t tmp[4];
> +
> +pthread_setcancelstate(PTHREAD_CANCEL_ENABLE, _cancelstate);
> +
> +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.
> +
> +pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, _cancelstate);
> +
> +pthread_mutex_lock(>mutex);
> +
> +len=av_fifo_size(s->fifo);
> +
> +while (len<4) {
> +if (pthread_cond_wait(>cond, >mutex) < 0) {
> +goto end;
> +}
> +len=av_fifo_size(s->fifo);
> +}
> +
> +av_fifo_generic_peek(s->fifo, tmp, 4, NULL);
> +len=AV_RL32(tmp);
> +
> +if (len>0 && av_fifo_size(s->fifo)>=len) {
len + 4
> +
Re: [FFmpeg-devel] [PATCH] avformat/udp: Add a delay between packets for streaming to clients with short buffer
On Mon, Feb 29, 2016 at 10:40:02PM +0300, Pavel Nikiforov wrote:
> Hello !
>
> This patch enables background sending of UDP packets with specified delay.
> When sending packets without a delay some devices with small RX buffer
> ( MAG200 STB, for example) will drop tail packets in burst causing
> decoding errors.
>
> It needs to specify "fifo_size" with "packet_gap" .
>
> The output url will looks like udp://xxx:yyy?fifo_size= size>_gap=
>
> Patch attached.
> udp.c | 133
> --
> 1 file changed, 129 insertions(+), 4 deletions(-)
> 80b57f176b5492070b2ed4853472de61b7d9ab7f udp.patch
> libavformat/udp.c | 133
> --
> 1 file changed, 129 insertions(+), 4 deletions(-)
>
> diff --git a/libavformat/udp.c b/libavformat/udp.c
> index ea80e52..ff676f4 100644
> --- a/libavformat/udp.c
> +++ b/libavformat/udp.c
> @@ -92,6 +92,7 @@ typedef struct UDPContext {
> int circular_buffer_size;
> AVFifoBuffer *fifo;
> int circular_buffer_error;
> +int packet_gap; /* delay between transmitted packets */
> #if HAVE_PTHREAD_CANCEL
> pthread_t circular_buffer_thread;
> pthread_mutex_t mutex;
> @@ -112,6 +113,7 @@ typedef struct UDPContext {
> #define E AV_OPT_FLAG_ENCODING_PARAM
> static const AVOption options[] = {
> { "buffer_size","System data size (in bytes)",
> OFFSET(buffer_size),AV_OPT_TYPE_INT,{ .i64 = -1 },-1, INT_MAX,
> .flags = D|E },
> +{ "packet_gap", "Delay between packets, in usec",
> OFFSET(packet_gap), AV_OPT_TYPE_INT,{ .i64 = 0 }, 0, INT_MAX,
> .flags = E },
units should be seconds so that SI suffixes like for usecs work
also independant of this patch but supporting to transmit packets
at at their PCR/SCR/transmit time should be interresting
> { "localport", "Local port",
> OFFSET(local_port), AV_OPT_TYPE_INT,{ .i64 = -1 },-1, INT_MAX,
> D|E },
> { "local_port", "Local port",
> OFFSET(local_port), AV_OPT_TYPE_INT,{ .i64 = -1 },-1, INT_MAX,
> .flags = D|E },
> { "localaddr", "Local address",
> OFFSET(localaddr), AV_OPT_TYPE_STRING, { .str = NULL },
> .flags = D|E },
> @@ -486,7 +488,7 @@ static int udp_get_file_handle(URLContext *h)
> }
>
> #if HAVE_PTHREAD_CANCEL
> -static void *circular_buffer_task( void *_URLContext)
> +static void *circular_buffer_task_rx( void *_URLContext)
> {
> URLContext *h = _URLContext;
> UDPContext *s = h->priv_data;
renaming things should be in a seperate patch
anyway, real review left to someone who knows th network stuff better
like nicolas
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
He who knows, does not speak. He who speaks, does not know. -- Lao Tsu
signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] avformat/udp: Add a delay between packets for streaming to clients with short buffer
Hello ! This patch enables background sending of UDP packets with specified delay. When sending packets without a delay some devices with small RX buffer ( MAG200 STB, for example) will drop tail packets in burst causing decoding errors. It needs to specify "fifo_size" with "packet_gap" . The output url will looks like udp://xxx:yyy?fifo_size=_gap= Patch attached. udp.patch Description: Binary data ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
