[FFmpeg-devel] [PATCH] decode.c: Handle 0-size packets in compat_decode

[wm4]

On Tue, 4 Jul 2017 00:03:19 +0200
Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:

> On 03.07.2017, at 21:07, wm4 <nfxjfg at googlemail.com> wrote:
> 
> > On Mon,  3 Jul 2017 20:57:21 +0200
> > Reimar Döffinger <Reimar.Doeffinger at 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.