[FFmpeg-devel] udp: added option to ignore empty UDP packets

Submitted by Daniel Kucera on Oct. 5, 2017, 2:26 p.m.

Details

Message ID 20171005142631.23899-1-daniel.kucera@gmail.com
State New
Headers show

Commit Message

Daniel Kucera Oct. 5, 2017, 2:26 p.m.
Time to time some devices send UDP packets without payload.
ffmpeg previously exited on receiving such packet, this patch
adds an option to ignore such packets.

Signed-off-by: Daniel Kucera <daniel.kucera@gmail.com>
---
 libavformat/udp.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

Comments

Carl Eugen Hoyos Oct. 5, 2017, 8:18 p.m.
2017-10-05 16:26 GMT+02:00 Daniel Kucera <daniel.kucera@gmail.com>:
> Time to time some devices send UDP packets without payload.
> ffmpeg previously exited on receiving such packet

Just curious: Is it useful at all to exit on an empty packet?

Carl Eugen
Daniel Kucera Oct. 5, 2017, 10:06 p.m.
Dňa 5. 10. 2017 10:19 odpoludnia používateľ "Carl Eugen Hoyos" <
ceffmpeg@gmail.com> napísal:

2017-10-05 16:26 GMT+02:00 Daniel Kucera <daniel.kucera@gmail.com>:
> Time to time some devices send UDP packets without payload.
> ffmpeg previously exited on receiving such packet

Just curious: Is it useful at all to exit on an empty packet?

Carl Eugen


No, It's a bug and it's so deep that it would require fixing many modules
but I'm not able to do it and noone other cares. So I'm proposing this
workaround.

Daniel.
Nicolas George Oct. 6, 2017, 8:15 a.m.
Le quartidi 14 vendémiaire, an CCXXVI, Carl Eugen Hoyos a écrit :
> Just curious: Is it useful at all to exit on an empty packet?

It is not useful at all, it is a bug.

More specifically, when reading plain files, ret = 0 means EOF for plain
files, just because there is nothing more to read (but if the file
grows, more data is returned in a later read). This has been extended to
pipes and stream socket, and some devices, but not all. In particular,
ret = 0 means EOF neither for ttys nor for packet protocols.

In FFmpeg, we have AVERROR_EOF and use it for EOF. For consistency and
simplicity, it is better to have only one case, therefore ret = 0 should
not be used to mean EOF. Stream protocols should never return 0.

I agree that we have that mistake probably in a lot of places. Finding
and fixing all of them would be a huge work. Fortunately, it is not
absolutely necessary: fixing the cases that cause visible bugs for
actual users is enough.

And since empty packets are valid and can be used by applications (and
are actually used by protocols out there), the workaround of dropping
them is not acceptable.

Regards,
Daniel Kucera Oct. 6, 2017, 8:32 a.m.
> And since empty packets are valid and can be used by applications (and
> are actually used by protocols out there), the workaround of dropping
> them is not acceptable.
>

I'm not sure if you mean this patch is unacceptable but if so, I want
to note, that this patch is not the same as I submitted before: this
one adds cmdlne option to ignore empty packets and it doesn't ignore
them when not explicitly enabled.
Daniel Kucera Oct. 10, 2017, 5:41 a.m.
Waiting for review.

Dňa 6. 10. 2017 10:32 dopoludnia používateľ "Daniel Kučera" <
daniel.kucera@gmail.com> napísal:

> > And since empty packets are valid and can be used by applications (and
> > are actually used by protocols out there), the workaround of dropping
> > them is not acceptable.
> >
>
> I'm not sure if you mean this patch is unacceptable but if so, I want
> to note, that this patch is not the same as I submitted before: this
> one adds cmdlne option to ignore empty packets and it doesn't ignore
> them when not explicitly enabled.
>
>
> --
>
> S pozdravom / Best regards
> Daniel Kucera.
>
Nicolas George Oct. 12, 2017, 2:16 p.m.
Le quintidi 15 vendémiaire, an CCXXVI, Daniel Kučera a écrit :
> I'm not sure if you mean this patch is unacceptable but if so, I want
> to note, that this patch is not the same as I submitted before: this
> one adds cmdlne option to ignore empty packets and it doesn't ignore
> them when not explicitly enabled.

As for this particular solution, I think it exposes to the user the
innards of a work-around, this is not good at all.

The good approach is to fix the bugs. You do not need to fix all of
them, just fix the one or two instances that break things for you: in
your use case, there is one line of code in the whole project that gets
ret=0 and thinks it means EOF: find it and fix it, and you should be
good. Other instance, if any, can be fixed as needed.

Regards,
wm4 Oct. 12, 2017, 2:25 p.m.
On Thu, 12 Oct 2017 16:16:31 +0200
Nicolas George <george@nsup.org> wrote:

> Le quintidi 15 vendémiaire, an CCXXVI, Daniel Kučera a écrit :
> > I'm not sure if you mean this patch is unacceptable but if so, I want
> > to note, that this patch is not the same as I submitted before: this
> > one adds cmdlne option to ignore empty packets and it doesn't ignore
> > them when not explicitly enabled.  
> 
> As for this particular solution, I think it exposes to the user the
> innards of a work-around, this is not good at all.
> 
> The good approach is to fix the bugs. You do not need to fix all of
> them, just fix the one or two instances that break things for you: in
> your use case, there is one line of code in the whole project that gets
> ret=0 and thinks it means EOF: find it and fix it, and you should be
> good. Other instance, if any, can be fixed as needed.

You can't force an occasional contributor to fix deep issues to fix a
minor bug, especially not with such a smug attitude.

You need to provide some leeway.
Daniel Kucera Oct. 13, 2017, 8:18 a.m.
>
> The good approach is to fix the bugs. You do not need to fix all of
> them, just fix the one or two instances that break things for you: in
> your use case, there is one line of code in the whole project that gets
> ret=0 and thinks it means EOF: find it and fix it, and you should be
> good. Other instance, if any, can be fixed as needed.
>

I tried this approach but when I changed that one critical line, 10
other things got broken according to fate. Even if I fixed them and
posted patches here in list, I was sugested to thorougouhly test all
other protocol modules to make sure it doesn't break anything else but
that's not possible (at least I'm not able to).

So what other option I have than trying to submit a workaround?

Patch hide | download patch | download mbox

diff --git a/libavformat/udp.c b/libavformat/udp.c
index 3835f989c4..15455dbef5 100644
--- a/libavformat/udp.c
+++ b/libavformat/udp.c
@@ -88,6 +88,7 @@  typedef struct UDPContext {
     int is_broadcast;
     int local_port;
     int reuse_socket;
+    int ignore_empty;
     int overrun_nonfatal;
     struct sockaddr_storage dest_addr;
     int dest_addr_len;
@@ -137,6 +138,7 @@  static const AVOption options[] = {
     { "timeout",        "set raise error timeout (only in read mode)",     OFFSET(timeout),        AV_OPT_TYPE_INT,    { .i64 = 0 },      0, INT_MAX, D },
     { "sources",        "Source list",                                     OFFSET(sources),        AV_OPT_TYPE_STRING, { .str = NULL },               .flags = D|E },
     { "block",          "Block list",                                      OFFSET(block),          AV_OPT_TYPE_STRING, { .str = NULL },               .flags = D|E },
+    { "ignore_empty",   "ignore empty UDP packets",                        OFFSET(ignore_empty),   AV_OPT_TYPE_BOOL,   { .i64 = 0 },      0, 1,       D|E },
     { NULL }
 };
 
@@ -786,6 +788,9 @@  static int udp_open(URLContext *h, const char *uri, int flags)
                                   FF_ARRAY_ELEMS(exclude_sources)))
                 goto fail;
         }
+        if (av_find_info_tag(buf, sizeof(buf), "ignore_empty", p)) {
+            s->ignore_empty = strtol(buf, NULL, 10);
+        }
         if (!is_output && av_find_info_tag(buf, sizeof(buf), "timeout", p))
             s->timeout = strtol(buf, NULL, 10);
         if (is_output && av_find_info_tag(buf, sizeof(buf), "broadcast", p))
@@ -1039,7 +1044,12 @@  static int udp_read(URLContext *h, uint8_t *buf, int size)
                 av_fifo_generic_read(s->fifo, buf, avail, NULL);
                 av_fifo_drain(s->fifo, AV_RL32(tmp) - avail);
                 pthread_mutex_unlock(&s->mutex);
-                return avail;
+                if (s->ignore_empty && avail == 0){
+                  av_log(h, AV_LOG_DEBUG, "Ignoring empty UDP packet\n");
+                  continue;
+                } else {
+                  return avail;
+                }
             } else if(s->circular_buffer_error){
                 int err = s->circular_buffer_error;
                 pthread_mutex_unlock(&s->mutex);