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

[Stefano Sabatini]

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.

Thanks, regards.
-- 
FFmpeg = Friendly & Freak Mega Proud Earthshaking Game
-------------- next part --------------
A non-text attachment was scrubbed...
Name: add-use-input-buffer-cap-02.patch
Type: text/x-diff
Size: 1133 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081007/b1271041/attachment.patch>