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

[Michael Niedermayer]

On Sat, Nov 29, 2008 at 09:50:24PM +0100, Stefano Sabatini wrote:
> 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 mean the case identical to yours, just that all codecs always set the flag
even if its not correct.
What would break?

The problem simply is that i dont see where you ever free the packet when the
flag is set. It simply leaks ...


> 
> 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);

and where is the buffer then freed? your code is equivalent to
if (!dec_ctx->used_input_buffer)
    av_free_packet(pkt);

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081130/41c44015/attachment.pgp>