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

Justin Ruggles justin.ruggles
Sat Oct 16 22:12:26 CEST 2010


Michael Niedermayer wrote:

> On Fri, Oct 15, 2010 at 06:35:01PM -0400, Justin Ruggles wrote:
>> 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?
> 
> possible but this is seperate, lets not entangle this patch with too many
> other changes
> 
> 
>> If so, should it be user_buffer[4] and user_buffer_size[4]?
> 
> possible
> 
> 
> 
>> 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?
> 
> there are buffer hints (grep for hint in avcodec.h) that specify if a buffer
> will be reused/read/preserved/blah
> the user supplied buffer is likely just valid for this call and cannot be used
> for some cases of the hints. For what remains using the buffer on the first
> call only seems ok

I think I've implemented it in a way that will work even when the
various buffer hints are set.  This implementation will not use memcpy
in avcodec_decode_audio3() in the most common case of the decoder
supporting CODEC_CAP_DR1, only needing 1 buffer, and not needing a
buffer larger than AVCODEC_MAX_AUDIO_FRAME_SIZE.

One thing I'm unsure of is whether to truncate output if it is too large
for avcodec_decode_audio3() (which is done in this patch) or to return
an error instead.

Cheers,
Justin

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: avcodec_decode_audio4.patch
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101016/49a22555/attachment.txt>



More information about the ffmpeg-devel mailing list