| Message ID | 20171005142631.23899-1-daniel.kucera@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
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
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.
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,
> 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.
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. >
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,
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.
> > 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?
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);
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(-)