| Message ID | 20170703185721.9520-1-Reimar.Doeffinger@gmx.de |
|---|---|
| State | New |
| Headers | show |
On Mon, 3 Jul 2017 20:57:21 +0200 Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote: > The old API did that just fine, and if we provide > a compatibility layer it should at least be compatible. > For the test-case (feeding AVParser output directly to > decoder, failing to discard 0-size packets) just discarding > 0-size packets at the start works, but not sure if in some > cases we might want to pass them on to e.g. allow retrieving > additional pending frames. > --- > libavcodec/decode.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/libavcodec/decode.c b/libavcodec/decode.c > index 052f93d82f..c63090f137 100644 > --- a/libavcodec/decode.c > +++ b/libavcodec/decode.c > @@ -842,6 +842,7 @@ static int compat_decode(AVCodecContext *avctx, AVFrame *frame, > avci->compat_decode = 1; > > if (avci->compat_decode_partial_size > 0 && > + pkt->size && > avci->compat_decode_partial_size != pkt->size) { > av_log(avctx, AV_LOG_ERROR, > "Got unexpected packet size after a partial decode\n"); > @@ -902,7 +903,10 @@ finish: > ret = FFMIN(avci->compat_decode_consumed, pkt->size); > } > avci->compat_decode_consumed = 0; > - avci->compat_decode_partial_size = (ret >= 0) ? pkt->size - ret : 0; > + // for compatibility with old API behaviour handle > + // 0-size specially > + if (pkt->size) > + avci->compat_decode_partial_size = (ret >= 0) ? pkt->size - ret : 0; > > return ret; > } I thought it was decided that 0-sized packets are always disallowed for encoding? Also, 0-sized packets are drain packets even with the old API, and thus could never properly work. You just relied in implicit restart behavior (i.e. leaving the drained state by feeding a new non-0-sized packet), which also didn't work with all decoders.
On 03.07.2017, at 21:07, wm4 <nfxjfg@googlemail.com> wrote: > On Mon, 3 Jul 2017 20:57:21 +0200 > Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote: > >> The old API did that just fine, and if we provide >> a compatibility layer it should at least be compatible. >> For the test-case (feeding AVParser output directly to >> decoder, failing to discard 0-size packets) just discarding >> 0-size packets at the start works, but not sure if in some >> cases we might want to pass them on to e.g. allow retrieving >> additional pending frames. >> --- >> libavcodec/decode.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/libavcodec/decode.c b/libavcodec/decode.c >> index 052f93d82f..c63090f137 100644 >> --- a/libavcodec/decode.c >> +++ b/libavcodec/decode.c >> @@ -842,6 +842,7 @@ static int compat_decode(AVCodecContext *avctx, AVFrame *frame, >> avci->compat_decode = 1; >> >> if (avci->compat_decode_partial_size > 0 && >> + pkt->size && >> avci->compat_decode_partial_size != pkt->size) { >> av_log(avctx, AV_LOG_ERROR, >> "Got unexpected packet size after a partial decode\n"); >> @@ -902,7 +903,10 @@ finish: >> ret = FFMIN(avci->compat_decode_consumed, pkt->size); >> } >> avci->compat_decode_consumed = 0; >> - avci->compat_decode_partial_size = (ret >= 0) ? pkt->size - ret : 0; >> + // for compatibility with old API behaviour handle >> + // 0-size specially >> + if (pkt->size) >> + avci->compat_decode_partial_size = (ret >= 0) ? pkt->size - ret : 0; >> >> return ret; >> } > > I thought it was decided that 0-sized packets are always disallowed for > encoding? This function is for decoding, I have not checked encoding side... > Also, 0-sized packets are drain packets even with the old > API, and thus could never properly work. You just relied in implicit > restart behavior (i.e. leaving the drained state by feeding a new > non-0-sized packet), which also didn't work with all decoders. I don't know, and I don't really care, I just noticed that we have a huge compatibility wrapper function that isn't even compatible, which is a bit ridiculous. So I proposed to make it at least more compatible. I don't mind changing it to be simpler and just discard them right at the start, if you think that makes more sense. Though even if there's just one decoder where 0-sized packets had some working practical use it might be better to pass them through... Note that I don't disagree that this was probably a bug that 0-sized packets got in (though I didn't re-check the parser API), just saying I do not see much reason to not be backwards-compatible. I know for sure that the ac3 and mp3 audio codecs at least had no issues with 0-size packets before.
On Tue, 4 Jul 2017 00:03:19 +0200 Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote: > On 03.07.2017, at 21:07, wm4 <nfxjfg@googlemail.com> wrote: > > > On Mon, 3 Jul 2017 20:57:21 +0200 > > Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote: > > > >> The old API did that just fine, and if we provide > >> a compatibility layer it should at least be compatible. > >> For the test-case (feeding AVParser output directly to > >> decoder, failing to discard 0-size packets) just discarding > >> 0-size packets at the start works, but not sure if in some > >> cases we might want to pass them on to e.g. allow retrieving > >> additional pending frames. > >> --- > >> libavcodec/decode.c | 6 +++++- > >> 1 file changed, 5 insertions(+), 1 deletion(-) > >> > >> diff --git a/libavcodec/decode.c b/libavcodec/decode.c > >> index 052f93d82f..c63090f137 100644 > >> --- a/libavcodec/decode.c > >> +++ b/libavcodec/decode.c > >> @@ -842,6 +842,7 @@ static int compat_decode(AVCodecContext *avctx, AVFrame *frame, > >> avci->compat_decode = 1; > >> > >> if (avci->compat_decode_partial_size > 0 && > >> + pkt->size && > >> avci->compat_decode_partial_size != pkt->size) { > >> av_log(avctx, AV_LOG_ERROR, > >> "Got unexpected packet size after a partial decode\n"); > >> @@ -902,7 +903,10 @@ finish: > >> ret = FFMIN(avci->compat_decode_consumed, pkt->size); > >> } > >> avci->compat_decode_consumed = 0; > >> - avci->compat_decode_partial_size = (ret >= 0) ? pkt->size - ret : 0; > >> + // for compatibility with old API behaviour handle > >> + // 0-size specially > >> + if (pkt->size) > >> + avci->compat_decode_partial_size = (ret >= 0) ? pkt->size - ret : 0; > >> > >> return ret; > >> } > > > > I thought it was decided that 0-sized packets are always disallowed for > > encoding? > > This function is for decoding, I have not checked encoding side... Typo. I meant decoding. I was probably thinking about how encoding sometimes uses 0-sized packets with extradata, and then wrote "encoding" instead of "decoding". Sorry. > > Also, 0-sized packets are drain packets even with the old > > API, and thus could never properly work. You just relied in implicit > > restart behavior (i.e. leaving the drained state by feeding a new > > non-0-sized packet), which also didn't work with all decoders. > > I don't know, and I don't really care, I just noticed that we have a huge compatibility wrapper function that isn't even compatible, which is a bit ridiculous. > So I proposed to make it at least more compatible. > I don't mind changing it to be simpler and just discard them right at the start, if you think that makes more sense. Though even if there's just one decoder where 0-sized packets had some working practical use it might be better to pass them through... Well no, there are multiple situations where 0-sized packets are treated as end-of-stream, so we decided that they are simply not allowed. > Note that I don't disagree that this was probably a bug that 0-sized packets got in (though I didn't re-check the parser API), just saying I do not see much reason to not be backwards-compatible. > I know for sure that the ac3 and mp3 audio codecs at least had no issues with 0-size packets before. Other decoders crashed or didn't recover. (I faintly remember something about windows audio...) Not really comfortable with the current patch. Why does it even touch the compat_decode_partial_size handling path? If you really want this, then I think you should go all the way and implement the implicit drain-and-flush behavior that the old API exposed with most decoders. (Meaning you can drain the decoder, and then it's implicitly flushed, so that sending new packets will restart decoding.) Either that, or skip empty packets on the API user side. If a libavformat demuxer or libavcodec parser is producing empty packets, these should probably be fixed.
On 04.07.2017, at 10:42, wm4 <nfxjfg@googlemail.com> wrote: > Not really comfortable with the current patch. Why does it even touch > the compat_decode_partial_size handling path? Because that is the code that gets utterly confused by 0-size packets and breaks everything. For ac3 what happens is that after the first 0-sized packets all following packets fail to decode. Probably that means the whole partial_size thing is broken and not robust, but I haven't been able to really understand it. > If you really want this, then I think you should go all the way and > implement the implicit drain-and-flush behavior that the old API > exposed with most decoders. (Meaning you can drain the decoder, and > then it's implicitly flushed, so that sending new packets will restart > decoding.) Yeah, I just don't feel particularly confident implementing that, since this compat wrapper has all kinds of weird code and no comments... > Either that, or skip empty packets on the API user side. If a > libavformat demuxer or libavcodec parser is producing empty packets, > these should probably be fixed. Skipping it on user side is of course what I did, but it doesn't provide compatibility. Any distribution e.g. updating FFmpeg more often than MPlayer will break.
On Tue, 4 Jul 2017 20:20:38 +0200 Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote: > On 04.07.2017, at 10:42, wm4 <nfxjfg@googlemail.com> wrote: > > Not really comfortable with the current patch. Why does it even touch > > the compat_decode_partial_size handling path? > > Because that is the code that gets utterly confused by 0-size packets and breaks everything. > For ac3 what happens is that after the first 0-sized packets all following packets fail to decode. 0-sized packets do not exist in AC3 in particular and have no meaning. > Probably that means the whole partial_size thing is broken and not robust, but I haven't been able to really understand it. This is for the mechanism where you passed a packet to the decode API, and it only decoded a few bytes of it, and you had to feed the rest of the packet to the decoder again next time to get all audio. (Plus confusing special cases about some decoders consuming 0 bytes in the first call, or consuming more bytes than what was provided as input...) The new API doesn't have that, and will just require multiple receive calls in those cases. > > If you really want this, then I think you should go all the way and > > implement the implicit drain-and-flush behavior that the old API > > exposed with most decoders. (Meaning you can drain the decoder, and > > then it's implicitly flushed, so that sending new packets will restart > > decoding.) > > Yeah, I just don't feel particularly confident implementing that, since this compat wrapper has all kinds of weird code and no comments... Shouldn't be too hard if you understand the old API. The new API is well-documented. > > Either that, or skip empty packets on the API user side. If a > > libavformat demuxer or libavcodec parser is producing empty packets, > > these should probably be fixed. > > Skipping it on user side is of course what I did, but it doesn't provide compatibility. > Any distribution e.g. updating FFmpeg more often than MPlayer will break. Well, it was broken before anyway. Decoding 0 sized packets usually will start draining the decoder, and sending new data after that (without flush) resulted in arbitrary behavior.
On Wed, Jul 05, 2017 at 09:52:55AM +0200, wm4 wrote: > On Tue, 4 Jul 2017 20:20:38 +0200 > Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote: > > > On 04.07.2017, at 10:42, wm4 <nfxjfg@googlemail.com> wrote: > > > Not really comfortable with the current patch. Why does it even touch > > > the compat_decode_partial_size handling path? > > > > Because that is the code that gets utterly confused by 0-size packets and breaks everything. > > For ac3 what happens is that after the first 0-sized packets all following packets fail to decode. > > 0-sized packets do not exist in AC3 in particular and have no meaning. > > > Probably that means the whole partial_size thing is broken and not robust, but I haven't been able to really understand it. > > This is for the mechanism where you passed a packet to the decode API, > and it only decoded a few bytes of it, and you had to feed the rest of > the packet to the decoder again next time to get all audio. (Plus > confusing special cases about some decoders consuming 0 bytes in the > first call, or > consuming more bytes than what was provided as input...) That would be a bug and such bugs where fixed when they where reported. > > The new API doesn't have that, and will just require multiple receive > calls in those cases. [...]
diff --git a/libavcodec/decode.c b/libavcodec/decode.c index 052f93d82f..c63090f137 100644 --- a/libavcodec/decode.c +++ b/libavcodec/decode.c @@ -842,6 +842,7 @@ static int compat_decode(AVCodecContext *avctx, AVFrame *frame, avci->compat_decode = 1; if (avci->compat_decode_partial_size > 0 && + pkt->size && avci->compat_decode_partial_size != pkt->size) { av_log(avctx, AV_LOG_ERROR, "Got unexpected packet size after a partial decode\n"); @@ -902,7 +903,10 @@ finish: ret = FFMIN(avci->compat_decode_consumed, pkt->size); } avci->compat_decode_consumed = 0; - avci->compat_decode_partial_size = (ret >= 0) ? pkt->size - ret : 0; + // for compatibility with old API behaviour handle + // 0-size specially + if (pkt->size) + avci->compat_decode_partial_size = (ret >= 0) ? pkt->size - ret : 0; return ret; }