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

[Luca Abeni]

Hi Stefano,

Stefano Sabatini wrote:
[...]
>>> Since the rawdec decoder uses the same input buffer to set the data
>>> buffer in the decoded frames, you can't free the input packet data and
>>> *then* access the decoded frame.
>>> (BTW I think there are other codecs with this property, I think at
>>> least the raw audio decoders, I'll check it better if the general idea
>>> I'm pushing will prove to be correct).
>>>
>>> In this case it's very difficult to free the packet only *after* the
>>> output frame has been accessed, since this requires a synchronization
>>> between the two threads which should be ideally independent. 
>> If you do not want to synchronise the two threads, the only solution is
>> to duplicate the buffer.
>  
>> Your solution ends up with never freeing the buffer, and I suspect
>> a race between the two threads, with the decoder decoding a frame
>> into a buffer before the second thread can process the previous
>> frame.
> 
> I can't understand here.
> 
> The packet data buffer will be freed when the output frame will be
> freed.

I might be wrong (I did not double-check the code), but I do not think
you need to free the output frame (the decoder will re-use it).


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


> I can't see how this can lead to some race condition.

Sorry, I misunderstood what you meant by "rawdec decoder uses
the same input buffer...".


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

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.

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


				Luca