[FFmpeg-devel] [PATCH+RFC] AVFrame for audio
Justin Ruggles
justin.ruggles
Sat Oct 16 22:33:28 CEST 2010
Justin Ruggles wrote:
> 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.
new patch attached.
-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/553acf56/attachment.asc>
More information about the ffmpeg-devel
mailing list