| Message ID | 20180418121003.25240-1-h.leppkes@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 18 Apr 2018 14:10:03 +0200 Hendrik Leppkes <h.leppkes@gmail.com> wrote: > --- > libavformat/tls_schannel.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavformat/tls_schannel.c b/libavformat/tls_schannel.c > index 9a6e0c92e3..3986b88667 100644 > --- a/libavformat/tls_schannel.c > +++ b/libavformat/tls_schannel.c > @@ -515,7 +515,7 @@ cleanup: > if (ret == 0 && !c->connection_closed) > ret = AVERROR(EAGAIN); > > - return ret < 0 ? ret : 0; > + return ret < 0 ? ret : AVERROR_EOF; > } > > static int tls_write(URLContext *h, const uint8_t *buf, int len) Maybe we should consider reverting this whole avio EOF change for the release? So many breakages...
On Wed, Apr 18, 2018 at 3:10 PM, Hendrik Leppkes <h.leppkes@gmail.com> wrote: > --- > libavformat/tls_schannel.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavformat/tls_schannel.c b/libavformat/tls_schannel.c > index 9a6e0c92e3..3986b88667 100644 > --- a/libavformat/tls_schannel.c > +++ b/libavformat/tls_schannel.c > @@ -515,7 +515,7 @@ cleanup: > if (ret == 0 && !c->connection_closed) > ret = AVERROR(EAGAIN); > > - return ret < 0 ? ret : 0; > + return ret < 0 ? ret : AVERROR_EOF; > } > > static int tls_write(URLContext *h, const uint8_t *buf, int len) > -- > 2.16.1.windows.4 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel I think I was hit by this before. LGTM. Jan
On Wed, Apr 18, 2018 at 7:53 PM, wm4 <nfxjfg@googlemail.com> wrote: > > Maybe we should consider reverting this whole avio EOF change for the > release? So many breakages... While I have kind of felt like this for a while now, I am just not sure if we can find all the things where with or without mention things have been poked to fix issues with it, which would now have to all be reverted. Also we already angered API users once with this change (albeit I'm not sure if it ever ended up in release yet?), so I'm not sure if we want to do it again. Of course as I mentioned if this was never in any release then that is kind of not relevant. Jan
On 4/18/2018 3:34 PM, Jan Ekström wrote: > On Wed, Apr 18, 2018 at 7:53 PM, wm4 <nfxjfg@googlemail.com> wrote: >> >> Maybe we should consider reverting this whole avio EOF change for the >> release? So many breakages... > > While I have kind of felt like this for a while now, I am just not > sure if we can find all the things where with or without mention > things have been poked to fix issues with it, which would now have to > all be reverted. Also we already angered API users once with this > change (albeit I'm not sure if it ever ended up in release yet?), so > I'm not sure if we want to do it again. Of course as I mentioned if > this was never in any release then that is kind of not relevant. I think it was not in any release, so if we revert anything, we do it /now/. ffmpeg 4.0 has been delayed enough as is. Mind, quite a few downstream projects use git head, but afaik there was a commit that added temporary backwards compat code so most of such users shouldn't have noticed anything.
On Wed, 18 Apr 2018 15:46:55 -0300 James Almer <jamrial@gmail.com> wrote: > On 4/18/2018 3:34 PM, Jan Ekström wrote: > > On Wed, Apr 18, 2018 at 7:53 PM, wm4 <nfxjfg@googlemail.com> wrote: > >> > >> Maybe we should consider reverting this whole avio EOF change for the > >> release? So many breakages... > > > > While I have kind of felt like this for a while now, I am just not > > sure if we can find all the things where with or without mention > > things have been poked to fix issues with it, which would now have to > > all be reverted. Also we already angered API users once with this > > change (albeit I'm not sure if it ever ended up in release yet?), so > > I'm not sure if we want to do it again. Of course as I mentioned if > > this was never in any release then that is kind of not relevant. > > I think it was not in any release, so if we revert anything, we do it > /now/. ffmpeg 4.0 has been delayed enough as is. Uh, good point I guess. Plus a revert could cause other regressions in FFmpeg. JEEB makes a good point about how we might need to find or change EOF fixes that were made after the initial commits that changed the behavior. > Mind, quite a few downstream projects use git head, but afaik there was > a commit that added temporary backwards compat code so most of such > users shouldn't have noticed anything. This is only a problem if any project explicitly relies on getting AVERROR_EOF, and ignores 0. User code is compatible with older ffmpeg most likely will consider both 0 and AVERROR_EOF to be eof. So I don't expect too many problems here, although a revert _could_ break API users. I still would love if we could make 0 mean EOF again, with the only change that the aviobuf read loop exits on 0 instead of retrying. This would not be very disruptive and is unlikely to break anything. Even if code was changed to return AVERROR_EOF instead of 0, the change would not break it. The only incompatibility is with code that either implements avio and was changed to return 0 in cases that mean "retry" (like the FFmpeg UDP code), or application code which was changed to ignore 0 returns and check _only_ for AVERROR_EOF (unlikely). I doubt I want to send a patch though, because I'd only stir up more shit.
Jan Ekström (2018-04-18): > While I have kind of felt like this for a while now, I am just not > sure if we can find all the things where with or without mention > things have been poked to fix issues with it, which would now have to > all be reverted. Also we already angered API users once with this > change (albeit I'm not sure if it ever ended up in release yet?), so > I'm not sure if we want to do it again. Of course as I mentioned if > this was never in any release then that is kind of not relevant. Might I ask how you propose to address the issue that required this change in the first place? The convention ret=0 for EOF does not work for packtized packets. We had complaints about issues caused by that. You cannot revert a solution that works unless you have one that work better in view. Regards,
On Wed, 18 Apr 2018 22:50:19 +0200 Nicolas George <george@nsup.org> wrote: > Jan Ekström (2018-04-18): > > While I have kind of felt like this for a while now, I am just not > > sure if we can find all the things where with or without mention > > things have been poked to fix issues with it, which would now have to > > all be reverted. Also we already angered API users once with this > > change (albeit I'm not sure if it ever ended up in release yet?), so > > I'm not sure if we want to do it again. Of course as I mentioned if > > this was never in any release then that is kind of not relevant. > > Might I ask how you propose to address the issue that required this > change in the first place? > > The convention ret=0 for EOF does not work for packtized packets. We had > complaints about issues caused by that. You cannot revert a solution > that works unless you have one that work better in view. There was a simple patch that would have solved that issue locally in the UDP code. This whole EOF thing was a non-issue and would not have required breaking so much other code.
wm4 (2018-04-18): > There was a simple patch that would have solved that issue locally in > the UDP code. This is not true. The patch would have fixed the author's use case but would have broken all applications that rely on empty UDP packets. Since it is about public API, we cannot assume anything about what applications do. A solution is only acceptable if it works for all cases and do not break existing applications.
On Wed, 18 Apr 2018 23:01:00 +0200 Nicolas George <george@nsup.org> wrote: > wm4 (2018-04-18): > > There was a simple patch that would have solved that issue locally in > > the UDP code. > > This is not true. The patch would have fixed the author's use case but > would have broken all applications that rely on empty UDP packets. Since > it is about public API, we cannot assume anything about what > applications do. A solution is only acceptable if it works for all cases > and do not break existing applications. > Which applications (in context of FFmpeg) need to detect empty UDP packets? Which applications would be broken by a trivial patch in the UDP code? From what I remember, the problem was that empty packets were recognized as EOF, and this issue was fixed with a trivial patch (which was rejected). On the other hand, the current code still gives you no way to detect empty UDP packets, because the retry_wrapper thing in the avio generic code basically drops them. A solution that might have worked better (if empty packets really need to be detected) would have been returning a special error code, that applications could choose to interpret.
wm4 (2018-04-18): > Which applications (in context of FFmpeg) need to detect empty UDP > packets? Which applications would be broken by a trivial patch in the > UDP code? One last time: this is public API: any existing application can rely on it. Including unpublished applications. This patch breaks them. This is the reason it was rejected then, this is the reason is is unacceptable now.
On Wed, 18 Apr 2018 23:21:30 +0200 Nicolas George <george@nsup.org> wrote: > wm4 (2018-04-18): > > Which applications (in context of FFmpeg) need to detect empty UDP > > packets? Which applications would be broken by a trivial patch in the > > UDP code? > > One last time: this is public API: any existing application can rely on > it. Including unpublished applications. This patch breaks them. This is > the reason it was rejected then, this is the reason is is unacceptable > now. Like I said, this didn't even work. And the EOF behavior was changed anyway, how is that not an API break? That change also broke a whole lot of other things, so I can't quite understand your argument here. In fact, you should have rejected the EOF change patch which you pushed yourself. Wasn't this completely unacceptable? By the way, I'm trying to understand your arguments. But it's not easy because you never explain them and you just respond with general platitudes. I can't read your mind.
wm4 (2018-04-18): > Like I said, this didn't even work. And the EOF behavior was changed > anyway, how is that not an API break? That change also broke a whole > lot of other things, so I can't quite understand your argument here. In > fact, you should have rejected the EOF change patch which you pushed > yourself. Wasn't this completely unacceptable? > > By the way, I'm trying to understand your arguments. But it's not easy > because you never explain them and you just respond with general > platitudes. I can't read your mind. This is not true, but I will not discuss it further with somebody who has repeatedly badmouthed me. Regards,
On Wed, 18 Apr 2018 23:35:11 +0200 Nicolas George <george@nsup.org> wrote: > wm4 (2018-04-18): > > Like I said, this didn't even work. And the EOF behavior was changed > > anyway, how is that not an API break? That change also broke a whole > > lot of other things, so I can't quite understand your argument here. In > > fact, you should have rejected the EOF change patch which you pushed > > yourself. Wasn't this completely unacceptable? > > > > By the way, I'm trying to understand your arguments. But it's not easy > > because you never explain them and you just respond with general > > platitudes. I can't read your mind. > > This is not true, but I will not discuss it further with somebody who > has repeatedly badmouthed me. Even if that should be the case (which is debatable), you made definitely insults against me, yet I'm ready to attempt to discuss things on a technical level with you. But you're rejecting even that. I guess I'll propose a patch to revert these changes, then.
diff --git a/libavformat/tls_schannel.c b/libavformat/tls_schannel.c index 9a6e0c92e3..3986b88667 100644 --- a/libavformat/tls_schannel.c +++ b/libavformat/tls_schannel.c @@ -515,7 +515,7 @@ cleanup: if (ret == 0 && !c->connection_closed) ret = AVERROR(EAGAIN); - return ret < 0 ? ret : 0; + return ret < 0 ? ret : AVERROR_EOF; } static int tls_write(URLContext *h, const uint8_t *buf, int len)