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

[Michael Niedermayer]

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 flagin 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.

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

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

No great genius has ever existed without some touch of madness. -- Aristotle
-------------- 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/20081126/99f39c66/attachment.pgp>