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

[Justin Ruggles]

Justin Ruggles wrote:

> Michael Niedermayer wrote:
> 
>> On Sat, Oct 16, 2010 at 04:12:26PM -0400, Justin Ruggles wrote:
>>> 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.
>> I think its better to tell the user straight through an error that there is a
>> problem instead of generating output that contains randomly truncated packets
> 
> Ok. New patch.

So does this look ok as far as the buffering and API and such?  If so I
will start going through each audio decoder to change to the new API.  I
just don't want to waste my time with that until it's agreed that this
approach is ok.

-Justin