| Message ID | 20171017082930.12869-1-daniel.kucera@gmail.com |
|---|---|
| State | Accepted |
| Commit | 858db4b01fa2b55ee55056c033054ca54ac9b0fd |
| Headers | show |
On Tue, Oct 17, 2017 at 10:29:30AM +0200, Daniel Kucera wrote: > transfer_func variable passed to retry_transfer_wrapper > are h->prot->url_read and h->prot->url_write functions. > These need to return EOF or other error properly. > In case of returning >= 0, url_read/url_write is retried > until error is returned. > > Signed-off-by: Daniel Kucera <daniel.kucera@gmail.com> > --- > libavformat/avio.c | 6 ++++-- > libavformat/aviobuf.c | 20 ++++++++++++-------- > libavformat/cache.c | 4 ++-- > libavformat/concat.c | 9 +++++---- > libavformat/http.c | 5 ++++- > 5 files changed, 27 insertions(+), 17 deletions(-) Seems to work, no more comments from me thanks [...]
Le sextidi 26 vendémiaire, an CCXXVI, Michael Niedermayer a écrit :
> Seems to work, no more comments from me
I think this is correct too.
Thanks to Daniel for the efforts.
Regards,
On Thu, Oct 19, 2017 at 04:22:33PM +0200, Nicolas George wrote: > Le sextidi 26 vendémiaire, an CCXXVI, Michael Niedermayer a écrit : > > Seems to work, no more comments from me > > I think this is correct too. any reason why you dont apply it, if you think its correct ? [...]
L'octidi 28 vendémiaire, an CCXXVI, Michael Niedermayer a écrit :
> any reason why you dont apply it, if you think its correct ?
Good question. Just did it.
Regards,
On Tue, Oct 17, 2017 at 11:29 AM, Daniel Kucera <daniel.kucera@gmail.com> wrote: > transfer_func variable passed to retry_transfer_wrapper > are h->prot->url_read and h->prot->url_write functions. > These need to return EOF or other error properly. > In case of returning >= 0, url_read/url_write is retried > until error is returned. This seems to have broken seeking and files ending. FLAC was experienced yesterday but it seems like it's more general. This was reported on the mpv users' channel by sfan5, but I feel like this might be more general than related to just mpv. At first this was thought to be limited to HTTP, but later was found out to be applicable to local files as well. Quote: < wm4> reproduction is "play anything with lavf" < JEEB> oh really? and seek? < sfan5> yes < wm4> no, seek close to end of file and see if it ends correctly. if it freezes it's the bug < sfan5> sounds like two different bugs < sfan5> bug 1: files don't end correctly < sfan5> bug 2: seeking somewhere never finishes and spins the cpu < sfan5> 2 is what i was experiencing yesterday Best regards, Jan
2017-10-21 13:36 GMT+02:00 Jan Ekstrom <jeebjp@gmail.com>: > > This seems to have broken seeking and files ending. FLAC was > experienced yesterday but it seems like it's more general. This was > reported on the mpv users' channel by sfan5, but I feel like this > might be more general than related to just mpv. At first this was > thought to be limited to HTTP, but later was found out to be > applicable to local files as well. > Hi Jan, please post a test case, ideally as an issue into trac.
On 10/23/2017 5:25 AM, Daniel Kučera wrote: > 2017-10-21 13:36 GMT+02:00 Jan Ekstrom <jeebjp@gmail.com>: >> >> This seems to have broken seeking and files ending. FLAC was >> experienced yesterday but it seems like it's more general. This was >> reported on the mpv users' channel by sfan5, but I feel like this >> might be more general than related to just mpv. At first this was >> thought to be limited to HTTP, but later was found out to be >> applicable to local files as well. >> > > > Hi Jan, > > please post a test case, ideally as an issue into trac. https://trac.ffmpeg.org/ticket/6767
On Mon, Oct 23, 2017 at 3:22 PM, James Almer <jamrial@gmail.com> wrote: > On 10/23/2017 5:25 AM, Daniel Kučera wrote: >> 2017-10-21 13:36 GMT+02:00 Jan Ekstrom <jeebjp@gmail.com>: >>> >>> This seems to have broken seeking and files ending. FLAC was >>> experienced yesterday but it seems like it's more general. This was >>> reported on the mpv users' channel by sfan5, but I feel like this >>> might be more general than related to just mpv. At first this was >>> thought to be limited to HTTP, but later was found out to be >>> applicable to local files as well. >>> >> >> >> Hi Jan, >> >> please post a test case, ideally as an issue into trac. > > https://trac.ffmpeg.org/ticket/6767 This appears to be an API break, since those avio callbacks are basically public API. Previously ret 0 for EOF was perfectly valid (just like many OS I/O functions also act), and now it needs special casing. I had to update my own code as well for this breakage. We should fix the API break somehow. - Hendrik
Le duodi 2 brumaire, an CCXXVI, James Almer a écrit :
> https://trac.ffmpeg.org/ticket/6767
I do not see in this ticket anything that allows to reproduce the issue
without a huge amount of foreign code.
Regards,
On Tue, Oct 24, 2017 at 8:57 PM, Nicolas George <george@nsup.org> wrote: > Le duodi 2 brumaire, an CCXXVI, James Almer a écrit : >> https://trac.ffmpeg.org/ticket/6767 > > I do not see in this ticket anything that allows to reproduce the issue > without a huge amount of foreign code. > > Regards, Hi, The base idea was grasped rather quickly though, which seems to be that all API clients utilizing their own IO seem to now require an update to their code to return AVERROR_EOF if EOF was hit instead of being able to return the number of read bytes, as usual. See https://github.com/mpv-player/mpv/pull/5033/files , for example. This is a public API change, and I'm not sure if we were planning on this. I am not against the fact that zero is no longer implictly EOF, but this might have been worth a bit more notice as this does indeed break quite a few API clients as noted by nevcairiel as well. If it is considered that no further changes are required then I think we at the very least want to mention this in the change log as well as the doc/APIchanges file. Best regards, Jan Ekström
On Tue, Oct 24, 2017 at 9:18 PM, Jan Ekstrom <jeebjp@gmail.com> wrote: > This is a public API change, and I'm not sure if we were planning on > this. I am not against the fact that zero is no longer implictly EOF, > but this might have been worth a bit more notice as this does indeed > break quite a few API clients as noted by nevcairiel as well. If it is > considered that no further changes are required then I think we at the > very least want to mention this in the change log as well as the > doc/APIchanges file. > Also as another alternative we might put the new behavior under an option. Thus those clients that want this behavior can get it, and otherwise old clients don't just break. Then we can properly deprecate the option and change the default then. If this makes any darn sense. Because the final option is a revert and I don't think anyone here wants that. Best regards, Jan
Le tridi 3 brumaire, an CCXXVI, Jan Ekstrom a écrit : > The base idea was grasped rather quickly though, which seems to be > that all API clients utilizing their own IO seem to now require an > update to their code to return AVERROR_EOF if EOF was hit instead of > being able to return the number of read bytes, as usual. I had not considered the case of custom callbacks, indeed. I do not consider this a public API change because 0 was never documented as a valid return value for a read_packet callback, while AVERROR_EOF has been around for a long time. But my goal is not to break existing code on purpose. Fortunately, there is a rather simple workaround, I will try to implement it tomorrow. Still, lacking a simple test case, I will not be able to test it. Regards,
On Tue, Oct 24, 2017 at 10:17 PM, Nicolas George <george@nsup.org> wrote: > Le tridi 3 brumaire, an CCXXVI, Jan Ekstrom a écrit : > I do not consider this a public API change because 0 was never > documented as a valid return value for a read_packet callback, while > AVERROR_EOF has been around for a long time. > You might be very true, but funny enough I just checked the AVIO example doc/examples/avio_reading.c, and the read function there does not seem to utilize AVERROR_EOF either :) . > But my goal is not to break existing code on purpose. Fortunately, there > is a rather simple workaround, I will try to implement it tomorrow. > Still, lacking a simple test case, I will not be able to test it. > Just for your information, I decided to call people out a bit on the IRC channel to see what people would prefer, and it seems like James, BtbN and Martin voiced their opinion that they would like a flag that would let you enable the 0 != EOF behavior. Additionally, a warning would be given out if zero is returned and the flag is unset. Quote follows: > <+JEEB> can anyone else chime on the EOF thing, if we want to > either A) leave things as they are and just break old > clients with an API behavior change B) add an option for > the new behavior , or C) revert the 0 != EOF thing > <+JEEB> because while those people that are on IRC have most > likely adapted their stuff, we will have quite a bit of > breakage due to this :P > <+JEEB> I'd probably go for B but I'm not sure how many places I'd > have to poke to add the option > < jamrial> maybe a temporary option to recover the old behavior > should be added for the usual period of ~2 years, and a > notice about it being removed eventually added > < jamrial> basically, deprecating the old behavior and leaving a > compat mode for it > <+JEEB> yes > < jamrial> library users will nontheless have to update their code > to enable said option > < jamrial> then again to remove it two years from now :p > < jamrial> so dunno > < jamrial> is this change, unintended breakage aside, a good one? > because if it doesn't really bring any worthwile > benefit maybe we should just revert it > <+JEEB> the requirement for the change was zero-byte UDP packets > which seem to be valid. originally the code was added into > the UDP lavf module, but then it was requested to be moved > into lavf general > <+JEEB> which thus broke the API behavior > <@nevcairiel> jamrial: i have always questioned the usefulness of > the change, but apparently there is obscure crazy > things that might use it > <+JEEB> yes, most users don't find that stuff on their networks > <@nevcairiel> and re: flag, if anything it should be inverted, or > its still an API break =p > <+JEEB> well flag to have the NEW behavior > <+JEEB> so that the default behavior is the old one > <+JEEB> that's what I meant > < jamrial> yeah, probably > <@BtbN> It should behave as it used to, which is imo a bit broken, > with some flag somewhere to flip it to sane behaviour > <+JEEB> BtbN: agreed > <@BtbN> And it should throw a depercation warning if the flag is > unset > <@wbs> alternatively, only throw a warning if the flag is unset > _and_ you return 0? > <@wbs> then you can write code that uses AVERROR_EOF for older > lavf versions without any ifdefs for the new flag, while > still running without warnings on the new lavf as well Any opinions? Jan
On 10/24/2017 4:37 PM, Jan Ekstrom wrote: > On Tue, Oct 24, 2017 at 10:17 PM, Nicolas George <george@nsup.org> wrote: >> Le tridi 3 brumaire, an CCXXVI, Jan Ekstrom a écrit : >> I do not consider this a public API change because 0 was never >> documented as a valid return value for a read_packet callback, while >> AVERROR_EOF has been around for a long time. >> > > You might be very true, but funny enough I just checked the AVIO > example doc/examples/avio_reading.c, and the read function there does > not seem to utilize AVERROR_EOF either :) . > >> But my goal is not to break existing code on purpose. Fortunately, there >> is a rather simple workaround, I will try to implement it tomorrow. >> Still, lacking a simple test case, I will not be able to test it. >> > > Just for your information, I decided to call people out a bit on the > IRC channel to see what people would prefer, and it seems like James, > BtbN and Martin voiced their opinion that they would like a flag that > would let you enable the 0 != EOF behavior. Additionally, a warning > would be given out if zero is returned and the flag is unset. Maybe first ask what the workaround Nicolas mentioned is about? And preferably don't quote me on this subject. This is not my area of expertise and i simply gave a suggestion to keep the old behavior at least for a while, assuming this is an API break. And my suggestion was in fact the opposite of what everyone else agreed on in the end.
Le tridi 3 brumaire, an CCXXVI, Jan Ekstrom a écrit : > You might be very true, but funny enough I just checked the AVIO > example doc/examples/avio_reading.c, and the read function there does > not seem to utilize AVERROR_EOF either :) . The examples are not exempt from mistakes. > Just for your information, I decided to call people out a bit on the > IRC channel to see what people would prefer, and it seems like James, > BtbN and Martin voiced their opinion that they would like a flag that > would let you enable the 0 != EOF behavior. Additionally, a warning > would be given out if zero is returned and the flag is unset. Quote > follows: > Any opinions? These people have obviously not thought this through. As I said, I will try to find time to implement a correct workaround tomorrow. Regards,
On Tue, Oct 24, 2017 at 10:43 PM, James Almer <jamrial@gmail.com> wrote: > Maybe first ask what the workaround Nicolas mentioned is about? This was not meant to push anything on anyone. Just wanted to let him know what had been come up with elsewhere. > And preferably don't quote me on this subject. This is not my area of > expertise and i simply gave a suggestion to keep the old behavior at > least for a while, assuming this is an API break. And my suggestion was > in fact the opposite of what everyone else agreed on in the end. Sorry. It's just that I finally got people talking about it, which as you can see by this thread is not something happening as much here. Will not do again. And yes, but you seemed to agree at the end. That might have just been my misunderstanding. And yes, I am not an expert on this either. I just knew about the general breakage and the fact that no actions were yet taken regarding it, so I tried to start a discussion about alternatives of handling this. Jan
Le tridi 3 brumaire, an CCXXVI, Nicolas George a écrit : > As I said, I will try to find time to implement a correct workaround > tomorrow. Patch series posted. https://ffmpeg.org/pipermail/ffmpeg-devel/2017-October/218482.html https://patchwork.ffmpeg.org/patch/5686/ Please confirm if it fixes the issue with your application. Regards,
diff --git a/libavformat/avio.c b/libavformat/avio.c index 64248e098b..4718753b7b 100644 --- a/libavformat/avio.c +++ b/libavformat/avio.c @@ -391,8 +391,10 @@ static inline int retry_transfer_wrapper(URLContext *h, uint8_t *buf, } av_usleep(1000); } - } else if (ret < 1) - return (ret < 0 && ret != AVERROR_EOF) ? ret : len; + } else if (ret == AVERROR_EOF) + return (len > 0) ? len : AVERROR_EOF; + else if (ret < 0) + return ret; if (ret) { fast_retries = FFMAX(fast_retries, 2); wait_since = 0; diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c index 636cb46161..0d4eb051e1 100644 --- a/libavformat/aviobuf.c +++ b/libavformat/aviobuf.c @@ -572,13 +572,14 @@ static void fill_buffer(AVIOContext *s) if (s->read_packet) len = s->read_packet(s->opaque, dst, len); else - len = 0; - if (len <= 0) { + len = AVERROR_EOF; + if (len == AVERROR_EOF) { /* do not modify buffer if EOF reached so that a seek back can be done without rereading data */ s->eof_reached = 1; - if (len < 0) - s->error = len; + } else if (len < 0) { + s->eof_reached = 1; + s->error= len; } else { s->pos += len; s->buf_ptr = dst; @@ -646,13 +647,16 @@ int avio_read(AVIOContext *s, unsigned char *buf, int size) // bypass the buffer and read data directly into buf if(s->read_packet) len = s->read_packet(s->opaque, buf, size); - - if (len <= 0) { + else + len = AVERROR_EOF; + if (len == AVERROR_EOF) { /* do not modify buffer if EOF reached so that a seek back can be done without rereading data */ s->eof_reached = 1; - if(len<0) - s->error= len; + break; + } else if (len < 0) { + s->eof_reached = 1; + s->error= len; break; } else { s->pos += len; diff --git a/libavformat/cache.c b/libavformat/cache.c index 6aabca2e78..66bbbf54c9 100644 --- a/libavformat/cache.c +++ b/libavformat/cache.c @@ -201,7 +201,7 @@ static int cache_read(URLContext *h, unsigned char *buf, int size) } r = ffurl_read(c->inner, buf, size); - if (r == 0 && size>0) { + if (r == AVERROR_EOF && size>0) { c->is_true_eof = 1; av_assert0(c->end >= c->logical_pos); } @@ -263,7 +263,7 @@ resolve_eof: if (whence == SEEK_SET) size = FFMIN(sizeof(tmp), pos - c->logical_pos); ret = cache_read(h, tmp, size); - if (ret == 0 && whence == SEEK_END) { + if (ret == AVERROR_EOF && whence == SEEK_END) { av_assert0(c->is_true_eof); goto resolve_eof; } diff --git a/libavformat/concat.c b/libavformat/concat.c index 46b520fe80..19c83c309a 100644 --- a/libavformat/concat.c +++ b/libavformat/concat.c @@ -135,19 +135,20 @@ static int concat_read(URLContext *h, unsigned char *buf, int size) while (size > 0) { result = ffurl_read(nodes[i].uc, buf, size); - if (result < 0) - return total ? total : result; - if (!result) { + if (result == AVERROR_EOF) { if (i + 1 == data->length || ffurl_seek(nodes[++i].uc, 0, SEEK_SET) < 0) break; + result = 0; } + if (result < 0) + return total ? total : result; total += result; buf += result; size -= result; } data->current = i; - return total; + return total ? total : result; } static int64_t concat_seek(URLContext *h, int64_t pos, int whence) diff --git a/libavformat/http.c b/libavformat/http.c index 668cd51986..bd9148f45d 100644 --- a/libavformat/http.c +++ b/libavformat/http.c @@ -1296,8 +1296,11 @@ static int http_buf_read(URLContext *h, uint8_t *buf, int size) "Chunked encoding data size: %"PRIu64"'\n", s->chunksize); - if (!s->chunksize) + if (!s->chunksize) { + av_log(h, AV_LOG_DEBUG, "Last chunk received, closing conn\n"); + ffurl_closep(&s->hd); return 0; + } else if (s->chunksize == UINT64_MAX) { av_log(h, AV_LOG_ERROR, "Invalid chunk size %"PRIu64"\n", s->chunksize);
transfer_func variable passed to retry_transfer_wrapper are h->prot->url_read and h->prot->url_write functions. These need to return EOF or other error properly. In case of returning >= 0, url_read/url_write is retried until error is returned. Signed-off-by: Daniel Kucera <daniel.kucera@gmail.com> --- libavformat/avio.c | 6 ++++-- libavformat/aviobuf.c | 20 ++++++++++++-------- libavformat/cache.c | 4 ++-- libavformat/concat.c | 9 +++++---- libavformat/http.c | 5 ++++- 5 files changed, 27 insertions(+), 17 deletions(-)