[FFmpeg-devel] [PATCH+RFC] AVFrame for audio

[Justin Ruggles]

Justin Ruggles wrote:

> Michael Niedermayer wrote:
> 
>> On Wed, Oct 13, 2010 at 07:52:12PM -0400, Justin Ruggles wrote:
>>> Michael Niedermayer wrote:
>>>
>>>> On Wed, Oct 06, 2010 at 11:05:26AM -0400, Justin Ruggles wrote:
>>>>> Michael Niedermayer wrote:
>>>>>
>>>>>> On Tue, Oct 05, 2010 at 04:55:12PM -0400, Justin Ruggles wrote:
>>>>>>> Michael Niedermayer wrote:
>>>>>>>
>>>>>>>> On Wed, Sep 29, 2010 at 09:20:04PM -0400, Justin Ruggles wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> Peter Ross wrote:
>>>>>>>>>
>>>>>>>>>> On Thu, Sep 02, 2010 at 07:11:37PM -0400, Justin Ruggles wrote:
>>>>>>>>>>> Hi,
>>>> [...]
>>>>>>>> [...]
>>>>>>>>> @@ -644,29 +677,49 @@
>>>>>>>>>  }
>>>>>>>>>  #endif
>>>>>>>>>  
>>>>>>>>> +#if LIBAVCODEC_VERSION_MAJOR < 53
>>>>>>>>>  int attribute_align_arg avcodec_decode_audio3(AVCodecContext *avctx, int16_t *samples,
>>>>>>>>>                           int *frame_size_ptr,
>>>>>>>>>                           AVPacket *avpkt)
>>>>>>>>>  {
>>>>>>>>> +    AVFrame frame;
>>>>>>>>> +    int ret, got_frame = 0;
>>>>>>>>> +
>>>>>>>>> +    avcodec_get_frame_defaults(&frame);
>>>>>>>>> +
>>>>>>>>> +    ret = avcodec_decode_audio4(avctx, &frame, &got_frame, avpkt);
>>>>>>>>> +
>>>>>>>>> +    if (ret >= 0 && got_frame) {
>>>>>>>>> +        *frame_size_ptr = frame.nb_samples * avctx->channels *
>>>>>>>>> +                          (av_get_bits_per_sample_format(avctx->sample_fmt) / 8);
>>>>>>>>> +
>>>>>>>>> +        /* ensure data will fit in the output buffer */
>>>>>>>>> +        if (*frame_size_ptr > AVCODEC_MAX_AUDIO_FRAME_SIZE) {
>>>>>>>>> +            av_log(avctx, AV_LOG_WARNING, "avcodec_decode_audio3 samples "
>>>>>>>>> +                   "truncated to AVCODEC_MAX_AUDIO_FRAME_SIZE\n");
>>>>>>>>> +            *frame_size_ptr = AVCODEC_MAX_AUDIO_FRAME_SIZE;
>>>>>>>>> +        }
>>>>>>>>> +
>>>>>>>>> +        memcpy(samples, frame.data[0], *frame_size_ptr);
>>>>>>>>  the default get_buffer() should return the appropriate
>>>>>>>> buffer for this case.
>>>>>>> I'm sorry, I don't understand your comment.
>>>>>> i mean (non functional psseudocode below to explain the idea)
>>>>>> avcodec_decode_audio3(){
>>>>>>     avctx->foobar= samples;
>>>>>>     ret = avcodec_decode_audio4(avctx, &frame, &got_frame, avpkt);
>>>>>>     ...
>>>>>>     assert(samples == frame.data[0]);
>>>>>>
>>>>>> -----
>>>>>> default_get_buffer(){
>>>>>>     if(avctx->foobar)
>>>>>>         return avctx->foobar;
>>>>>>
>>>>>>
>>>>>> (and yes this cannot work for all theoretical decoders)
>>>>> I think I get it.  So avctx->foobar would be an optional user-supplied
>>>>> buffer (avctx->user_buffer?) that default_get_buffer() would return if
>>>>> it is non-NULL, right?
>>>> yes
>>>>
>>>>
>>>>> The problem I see is this:
>>>>> avcodec_decode_audio3() would use avcodec_decode_audio4().
>>>>> avcodec_decode_audio4() allocates as large a buffer as is needed through
>>>>> get_buffer(), but the avcodec_decode_audio3() API only requires the
>>>>> user-supplied buffer to be AVCODEC_MAX_AUDIO_FRAME_SIZE.  Couldn't this
>>>>> lead to the decoder writing past the end of a user-supplied buffer if it
>>>>> isn't large enough?  I guess we could also add a field
>>>>> avctx->user_buffer_size?
>>>> yes, of course
>>>> it was just a rough idea ...
>>> I'm running into some questions trying to implement the rough idea.  The
>>> only way I can see this working smoothly is if avcodec_decode_audio3()
>>> always sets get/release_buffer to default.  Also, either all audio
>>> decoders will have to support CODEC_CAP_DR1 (i.e. they always use
>>> get/release_buffer) or there needs to be a fallback that will memcpy
>>> into the user buffer if CODEC_CAP_DR1 is not supported.
>> old API decoders surely dont need to copy with old API.
>> old API decoders surely dont need to copy with new API if the api can provide
>> a buffer to the decoder (this can be through function argument like its done
>> currently)
>> new API decoders surely dont need to copy with new API because otherwise the
>> API sucks and needs more work
>> whats left is new API decoders and used with old API and for this get_buffer()
>> should return the user supplied buffer if its large enough and fail if its not
>> large enough.
>> The case where the user overrides get_buffer() and supplies a user specified
>> buffer which its own code doesnt use is a case that id consider user error.
> 
> I think I might have been misinterpreting the API.  For video decoders,
> what does it mean as far as buffer allocation when CODEC_CAP_DR1 is not set?

So I think I have this worked out and I don't see how we can avoid a
memcpy with the old API when CODEC_CAP_DR1 is not set.  There would be
no other way to get the data into the correct output buffer.

other questions:

1. Should AVCodecContext.user_buffer be supported for video decoders?
If so, should it be user_buffer[4] and user_buffer_size[4]?

2. avcodec_default_get_buffer() supports allocating multiple internal
buffers.  How should that be handled if the buffer is supplied by the
user?  Don't support multiple buffers?  Use the user-supplied buffer
just for the first one?

Thanks,
Justin