[FFmpeg-devel] [PATCH] avcodec_decode_audio3 and multiple frames in a packet

[Sascha Sommer]

Hi,

On Samstag, 12. September 2009, Justin Ruggles wrote:
> Sascha Sommer wrote:
> > Hi,
> >
> > attached patch extends the documentation of avcodec_decode_audio3 a bit.
> > Previously, the case when multiple frames are stored in a single packet
> > was not documented very well. I described the current behaviour of
> > ffmpeg. That is:
> >                 avpkt.data += ret;
> >                 avpkt.size -= ret;
> > is done until the packet is fully decoded.
> > This also makes it possible to return 0 until the last frame in a packet
> > is decoded. This is currently done for wmapro.
> >
> > What really changes the API is the change of the return value so maybe
> > this should become avcodec_decode_audio4.
> > Previously a value of 0 meant that no frame was decoded.
> > I do not like this implication because
> >
> > 1. This can already be found out by checking *frame_size_ptr what should
> > be 0 then.
> > 2. Consider a hypothetical audio codec that stores two 4-bit frames in a
> >     8-bit packet. The description of the function says that decoding
> > stops after the first frame but what to return then?
> >     return 0 means that no frame has been decoded => wrong
> >     return 1 will abort the decoding after the first frame => wrong
> >
> >
> > Index: libavcodec/avcodec.h
> > ===================================================================
> > --- libavcodec/avcodec.h	(revision 19753)
> > +++ libavcodec/avcodec.h	(working copy)
> > @@ -3202,7 +3202,16 @@
> >  /**
> >   * Decodes the audio frame of size avpkt->size from avpkt->data into
> > samples. * Some decoders may support multiple frames in a single
> > AVPacket, such - * decoders would then just decode the first frame.
> > + * decoders would then just decode the first frame. If the same AVPacket
> > + * is passed in again, the second frame is returned and so on.
>
> What exactly is meant by "same AVPacket"?  If the pointer is the same,
> but the data inside it has changed is that the same packet, or

As the same AVPacket, I considered packets where the data did not change and 
the memory location of the data did not change. That is, the pointer could 
have been possibly incremented (this is mostly to not break existing 
decoders. They currently also work with reallocated packets). Originally the 
same packet really meant the same packet without any modifications.

> vice-versa?  If a "different" AVPacket is passed before the previous one
> has had all data consumed, what happens?
>

If a new AVPacket is passed before the previous one has had all data consumed 
decoding might break. This is also the case with the current API.

> > + * The AVPacket is fully decoded and the decoder is ready for the next
> > + * AVPacket when the number of returned bytes is equal to avpkt->size.
> > + *
> > + * If the AVPacket is not fully decoded yet, avpkt->data needs to be
> > + * incremented by the number of returned bytes and avpkt->size needs
> > + * to be decremented by the number of returned bytes before the function
> > + * is called again to get the next frame in the AVPacket.
>
> I don't agree with this change.  Requiring specific external changes to
> the state of a passed parameter based on the return value then assuming
> these changes in the next call of the function seems counter-intuitive
> and somewhat unstable.

I added this to explain what ffmpeg does. When I read the previous 
description, I did not know at all how a single AVPacket can output multiple 
frames.

>
> Also, the application may want to discard the rest of the packet after a
> partial decode.  How would the decoder know this?  Would this be
> disallowed?  Would it be allowed but be considered seeking and require
> an AVCodec.flush() before the next call?
>

Yes, in this proposal I would consider discarding the rest of the packet as 
seeking and therefore requiring an AVCodec.flush().

You are right however that assuming these changes in the next call of the 
function is not very nice.
I would prefere it if it was more clear that the rest of the packet is not 
independet from the first part of the packet as the decoding might not have 
stopped at a byte boundary etc.
Maybe it would be better to pass in the AVPacket only once and NULL afterwards 
to decode the rest of the packet? We could also return a special return code 
when a packet was not fully decoded?

Regards

Sascha