[FFmpeg-devel] [PATCH+RFC] AVFrame for audio
Justin Ruggles
justin.ruggles
Sat Oct 23 16:05:11 CEST 2010
Justin Ruggles wrote:
> 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.
Ha! that came out wrong. I don't want to waste my time by doing all
that BEFORE the new API is agreed upon. :)
-Justin
More information about the ffmpeg-devel
mailing list