[FFmpeg-devel] [PATCH] Add a CODEC_CAP_USE_INPUT_BUFFER capabilities flag

[Stefano Sabatini]

On date Wednesday 2008-11-26 12:22:36 +0100, Michael Niedermayer encoded:
> On Tue, Oct 07, 2008 at 10:29:00PM +0200, Stefano Sabatini wrote:
> > On date Tuesday 2008-10-07 00:44:38 +0200, Luca Abeni encoded:
> > > Hi Stefano,
> > > 
> > > Stefano Sabatini wrote:
> > [...]
> > > > Also in the two threads scenario the first thread, after the call to
> > > > av_free_packet (which in this case won't free the input packet data,
> > > > since that buffer is set in the output frame), simply will pass the
> > > > output frame to some queue with locked access accessed by the second
> > > > thread (similar to what happens in ffplay).
> > > 
> > > Note that ffplay calls av_dup_packet() (exactly for this reason, I
> > > think). I suspect this function is half-broken, but it should be
> > > needed (AFAIK) when passing AVPackets between different threads.
> > 
> > The packets are duped in packet_queue_put() in both ffplay versions,
> > so the data which comes out from the queue is safe, and may be used
> > independently from the source (but need to be freed anyway). 
> > 
> > [...]
> > > >>> Making the decoding process aware of the fact that the packet data has
> > > >>> not to be freed seems to me like a simpler solution.
> > > >> But I do not see anything similar in your patch :)
> > > > 
> > > > The packet destruct callback is set to av_destruct_packet_nofree, so
> > > > the input packet data is not freed.
> > > 
> > > In this sentence, you were not talking about freeing or not freeing the
> > > buffer, you were talking about making the decoding process aware...
> > > 
> > > Anyway, I had a better look at the code, and I suspect the problem is
> > > due to a (IMHO) misuse of the API. In my understanding (I hope people
> > > will correct me if I am wrong), this is the correct usage pattern:
> > > 1) Read a packet
> > > 2) Decode the packet
> > > 3) Do something with the decoded data
> > > 4) Free the packet
> > > 5) Do not use the data anymore
> > 
> > Once we have a duped packet we don't need anymore to worry about the
> > next av_read_frame() read.
> > 
> > What I would like to be able to do is to decode the data buffer,
> > eventually free it if it makes sense, discard it and do whatever I
> > want with the output frame.
> > 
> > I don't want to know when I have to free the input data when I'm
> > dealing with output packet.
> > 
> > In order to achieve this we can use this technique:
> > if we know that the decoder uses the input data to set the output
> > frame, we don't free the input data, other we did it.
> > 
> > This way we can avoid memory leaks or worse crashes due to an access
> > to an already freed data.
> > 
> > In order to be able to adopt this technique we have to know if the
> > decoder used uses the input buffer to store the output data, otherwise
> > we have to do a specific check on the codec id, which is what I would
> > like to avoid introducing the capability flag.
> > 
> > Said that, I read again the libavformat code and got to the conclusion
> > that the second solution which I proposed (implemented in libavformat)
> > was bloody wrong, this is indeed a problem of libavcodec and has more
> > to do with the buffer data than with the use of AVPacket.
> > 
> > The following snippet that I used like an example:
> > 
> >     if (is->video_st->codec->codec->capabilities & CODEC_CAP_USE_INPUT_BUFFER)
> >         pkt->destruct = av_destruct_packet_nofree;
> >     av_free_packet(pkt);
> > 
> > simply makes sure that the data buffer (still used in the output
> > frame) has not to be freed.
> > 
> > > If you want to do something different (using the decoded data after
> > > freeing the packet, using multiple threads, etc...) you need to duplicate
> > > the buffer, or to use some similar technique.
> > 
> > This is already performed in both ffplay and ffplay-libavfilter-soc, since
> > packets are duplicated when they're inserted in the queue.
> > 
> > > At least, this is my understanding of how the libav* API should be used
> > > (and I think ffmpeg.c does this. ffplay.c uses av_dup_packet()).
> > >
> > > In any case, I am happily leaving this discussion to people who
> > > understand the libav* internal better than me :)
> > 
> > Your review has been useful anyway, I got forced to read code which I
> > didn't know/remember, at least now I know for sure that the
> > libavformat solution was plain wrong.
> > 
> > If the maitainers agree that this feature is needed as I think and the
> > implementation is correct then I'll search for other codecs with the
> > same property.
> 
> what you write makes no sense
> 
> there really are 2 options
> If your code works (which i doubt) you can just assume every codec uses its
> input buffer and thus wont need a flag in AVCodec telling you if the codec
> actually does, not to mention that this can change on a frame by frame basis
> for codecs mixing raw and compressed data.

Yes the frame by frame consideration is perfectly valid but I'm not
sure about what you mean implementation-level.

I'd say we could set a flag in the decoder context telling if the last
issued frame used the input buffer.

So the check may look like this:

    if (dec_ctx->used_input_buffer)
        pkt->destruct = av_destruct_packet_nofree;
    av_free_packet(pkt);

> The second choice is a CODEC_FLAG that disables this use, for this though
> id like to see at least 2-3 applications that would actually benefit from
> this as it adds some extra code into a few decoders and causes a slowdown
> if the flag is set incorrectly

I'm perfectly satisfied with one (or both?) of these solutions,
let me know which do you prefer.

Regards.
-- 
FFmpeg = Foolish and Free MultiPurpose EnGine