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

[Justin Ruggles]

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?

-Justin