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

[Justin Ruggles]

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.
> 
> Cheers,
> Justin
> 

> @@ -253,9 +277,20 @@ int avcodec_default_get_buffer(AVCodecContext *s, AVFrame *pic){
>      picture_number= &(((InternalBuffer*)s->internal_buffer)[INTERNAL_BUFFER_SIZE]).last_pic_num; //FIXME ugly hack
>      (*picture_number)++;
>  
> -    if(buf->base[0] && (buf->width != w || buf->height != h || buf->pix_fmt != s->pix_fmt)){
> -        for(i=0; i<4; i++){
> +    if (buf->base[0] &&
> +        (is_video  && (buf->width   != w ||
> +                       buf->height  != h ||
> +                       buf->pix_fmt != s->pix_fmt)) ||
> +        (!is_video && (buf->channels   != s->channels     ||
> +                       buf->nb_samples != pic->nb_samples ||
> +                       buf->sample_fmt != s->sample_fmt))) {
> +        for(i=0; i<(is_video?4:1); i++){
> +            if (buf->base[i] && buf->base[i] == s->user_buffer) {
> +                s->user_buffer_in_use = 0;
> +                buf->base[i] = NULL;
> +            } else {
>              av_freep(&buf->base[i]);
> +            }
>              buf->data[i]= NULL;
>          }
>      }

I just realized this section is wrong since user_buffer is not supported
for video.  Ignore that part until the next patch.

-Justin