[FFmpeg-devel] [PATCH+RFC] AVFrame for audio
Justin Ruggles
justin.ruggles
Sat Nov 13 19:13:09 CET 2010
Michael Niedermayer wrote:
> On Mon, Nov 01, 2010 at 05:19:00PM -0400, Justin Ruggles wrote:
>> Michael Niedermayer wrote:
>>
>>> On Sat, Oct 30, 2010 at 08:06:42AM -0400, Justin Ruggles wrote:
>>>> Michael Niedermayer wrote:
>>>>
>>>>> On Thu, Oct 28, 2010 at 06:58:11PM -0400, Justin Ruggles wrote:
>>>>>> Michael Niedermayer wrote:
>>>>>>
>>>>>>> On Wed, Oct 27, 2010 at 10:13:10PM -0400, Justin Ruggles wrote:
>>>>>>>> Michael Niedermayer wrote:
>>>>>>>>
>>>>>>>>> On Tue, Oct 26, 2010 at 09:31:13PM -0400, Justin Ruggles wrote:
>>>>>>>>>> Michael Niedermayer wrote:
>>>>>>>>>>
>>>>>>>>>>> On Sun, Oct 17, 2010 at 05:22:54PM -0400, 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.
>>>>>>>>>>>>
>>>>>>>>>>>> -Justin
>>>>>>>>>>> [...]
>>>>>>>>>>>> int avcodec_default_get_buffer(AVCodecContext *s, AVFrame *pic){
>>>>>>>>>>>> int i;
>>>>>>>>>>>> int w= s->width;
>>>>>>>>>>>> int h= s->height;
>>>>>>>>>>>> + int is_video = (s->codec_type == AVMEDIA_TYPE_VIDEO);
>>>>>>>>>>>> InternalBuffer *buf;
>>>>>>>>>>>> int *picture_number;
>>>>>>>>>>>>
>>>>>>>>>>>> @@ -235,7 +258,8 @@ int avcodec_default_get_buffer(AVCodecContext *s, AVFrame *pic){
>>>>>>>>>>>> return -1;
>>>>>>>>>>>> }
>>>>>>>>>>>>
>>>>>>>>>>>> - if(av_image_check_size(w, h, 0, s))
>>>>>>>>>>>> + if(( is_video && av_image_check_size(w, h, 0, s)) ||
>>>>>>>>>>>> + (!is_video && audio_check_size(s->channels, pic->nb_samples, s->sample_fmt)))
>>>>>>>>>>>> return -1;
>>>>>>>>>>>>
>>>>>>>>>>>> if(s->internal_buffer==NULL){
>>>>>>>>>>>> @@ -253,17 +277,30 @@ 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)){
>>>>>>>>>>>> + if (buf->base[0]) {
>>>>>>>>>>>> + if (is_video && (buf->width != w || buf->height != h || buf->pix_fmt != s->pix_fmt)){
>>>>>>>>>>>> for(i=0; i<4; i++){
>>>>>>>>>>>> av_freep(&buf->base[i]);
>>>>>>>>>>>> buf->data[i]= NULL;
>>>>>>>>>>>> }
>>>>>>>>>>>> + } else if (!is_video && (buf->channels != s->channels ||
>>>>>>>>>>>> + buf->nb_samples != pic->nb_samples ||
>>>>>>>>>>>> + buf->sample_fmt != s->sample_fmt)) {
>>>>>>>>>>>> + if (buf->base[0] == s->user_buffer) {
>>>>>>>>>>>> + s->user_buffer_in_use = 0;
>>>>>>>>>>>> + buf->base[0] = NULL;
>>>>>>>>>>>> + } else {
>>>>>>>>>>>> + av_freep(&buf->base[0]);
>>>>>>>>>>>> + }
>>>>>>>>>>>> + buf->data[0] = NULL;
>>>>>>>>>>>> + }
>>>>>>>>>>>> }
>>>>>>>>>>>>
>>>>>>>>>>>> if(buf->base[0]){
>>>>>>>>>>>> pic->age= *picture_number - buf->last_pic_num;
>>>>>>>>>>>> buf->last_pic_num= *picture_number;
>>>>>>>>>>>> }else{
>>>>>>>>>>>> + if (is_video) {
>>>>>>>>>>>> int h_chroma_shift, v_chroma_shift;
>>>>>>>>>>>> int size[4] = {0};
>>>>>>>>>>>> int tmpsize;
>>>>>>>>>>>> @@ -327,6 +364,28 @@ int avcodec_default_get_buffer(AVCodecContext *s, AVFrame *pic){
>>>>>>>>>>>> buf->height = s->height;
>>>>>>>>>>>> buf->pix_fmt= s->pix_fmt;
>>>>>>>>>>>> pic->age= 256*256*256*64;
>>>>>>>>>>>> + } else { /* audio */
>>>>>>>>>>>> + int buf_size;
>>>>>>>>>>>> +
>>>>>>>>>>>> + buf->last_pic_num = -256*256*256*64;
>>>>>>>>>>>> +
>>>>>>>>>>>> + buf_size = pic->nb_samples * s->channels *
>>>>>>>>>>>> + (av_get_bits_per_sample_format(s->sample_fmt) / 8);
>>>>>>>>>>>> +
>>>>>>>>>>>> + if (s->user_buffer && !s->user_buffer_in_use && s->user_buffer_size >= buf_size) {
>>>>>>>>>>>> + buf->base[0] = s->user_buffer;
>>>>>>>>>>>> + s->user_buffer_in_use = 1;
>>>>>>>>>>>> + } else {
>>>>>>>>>>>> + buf->base[0] = av_mallocz(buf_size);
>>>>>>>>>>>> + if (!buf->base[0])
>>>>>>>>>>>> + return AVERROR(ENOMEM);
>>>>>>>>>>>> + }
>>>>>>>>>>>> +
>>>>>>>>>>>> + buf->data[0] = buf->base[0];
>>>>>>>>>>>> + buf->channels = s->channels;
>>>>>>>>>>>> + buf->nb_samples = pic->nb_samples;
>>>>>>>>>>>> + buf->sample_fmt = s->sample_fmt;
>>>>>>>>>>>> + }
>>>>>>>>>>>> }
>>>>>>>>>>>> pic->type= FF_BUFFER_TYPE_INTERNAL;
>>>>>>>>>>>>
>>>>>>>>>>>> @@ -360,9 +419,15 @@ void avcodec_default_release_buffer(AVCodecContext *s, AVFrame *pic){
>>>>>>>>>>>> }
>>>>>>>>>>>> assert(i < s->internal_buffer_count);
>>>>>>>>>>>> s->internal_buffer_count--;
>>>>>>>>>>>> + if (buf->base[0] == s->user_buffer) {
>>>>>>>>>>>> + assert(s->user_buffer_in_use);
>>>>>>>>>>>> + s->user_buffer_in_use = 0;
>>>>>>>>>>>> + buf->base[0] = NULL;
>>>>>>>>>>>> + } else {
>>>>>>>>>>>> last = &((InternalBuffer*)s->internal_buffer)[s->internal_buffer_count];
>>>>>>>>>>>>
>>>>>>>>>>>> FFSWAP(InternalBuffer, *buf, *last);
>>>>>>>>>>>> + }
>>>>>>>>>>>>
>>>>>>>>>>>> for(i=0; i<4; i++){
>>>>>>>>>>>> pic->data[i]=NULL;
>>>>>>>>>>> i dont see how this could work
>>>>>>>>>>> the buffer used and returned by the previous decode() is put in a que by the
>>>>>>>>>>> user app and user_buffer is set to a new buffer.
>>>>>>>>>>> also you appear to end up calling av_free()
>>>>>>>>>>> on user supplied buffers
>>>>>>>>>> Well, I meant to disallow that, but the documentation I put just says
>>>>>>>>>> the user cannot free or change the data while user_buffer_in_use is set.
>>>>>>>>>> I didn't consider the user replacing it with a new buffer. But at any
>>>>>>>>>> rate, if that should be allowed, things get more complicated. I'll need
>>>>>>>>>> to add a flag or something to indicate each user-supplied buffer. I'll
>>>>>>>>>> work on it.
>>>>>>>>> i think the API is too complex already and i dont see why it is so
>>>>>>>>> if user buffer is set get_buffer() should return it or fail, if its returned
>>>>>>>>> it should set user_buffer to NULL
>>>>>>>>> calling get_buffer() a second time if user_buffer was set should be disallowed
>>>>>>>>> release_buffer should do nothing
>>>>>>>>>
>>>>>>>>> if we ever have decoders that dont work with this then we need a AVCodec flag
>>>>>>>>> that indicates them. For this case get_buffer() would then ignore user_buffer
>>>>>>>>> and avcodec_decode() would copy to the provided user_buffer if any.
>>>>>>>>> (we do not need this currently though because we do not have such a decoder)
>>>>>>>>>
>>>>>>>>> maybe iam missing something but this seems simpler
>>>>>>>> Ok I did it a different way. New patch attached. I'm not 100% sure
>>>>>>>> about the way reget_buffer() is handled but it works. AVFrame.type
>>>>>>>> seems to be a mask, but I don't know if it was intended to be used that way.
>>>>>>>>
>>>>>>>> Cheers,
>>>>>>>> Justin
>>>>>>>>
>>>>>>>>
>>>>>>>> doc/APIchanges | 9 ++
>>>>>>>> libavcodec/avcodec.h | 100 ++++++++++++++++++++++++++++++--
>>>>>>>> libavcodec/pcm.c | 41 +++++++++++--
>>>>>>>> libavcodec/utils.c | 157 ++++++++++++++++++++++++++++++++++++++++++++-------
>>>>>>>> 4 files changed, 275 insertions(+), 32 deletions(-)
>>>>>>>> 39eb7fb791089c0822e3f87d3226b49131563a72 avcodec_decode_audio4.patch
>>>>>>>> diff --git a/doc/APIchanges b/doc/APIchanges
>>>>>>>> index 4155d32..a39d9fd 100644
>>>>>>>> --- a/doc/APIchanges
>>>>>>>> +++ b/doc/APIchanges
>>>>>>>> @@ -13,6 +13,15 @@ libavutil: 2009-03-08
>>>>>>>>
>>>>>>>> API changes, most recent first:
>>>>>>>>
>>>>>>>> +2010-XX-XX - rXXXXX - lavc 52.92.0 - AVFrame and avcodec_decode_audio
>>>>>>>> + Add nb_samples field to AVFrame.
>>>>>>>> + Add user_buffer, user_buffer_size, and user_buffer_in_use fields to AVCodecContext.
>>>>>>>> + Deprecate AVCODEC_MAX_AUDIO_FRAME_SIZE.
>>>>>>>> + Deprecate avcodec_decode_audio3() in favor of avcodec_decode_audio4().
>>>>>>>> + avcodec_decode_audio4() writes output samples to an AVFrame, which gives the
>>>>>>>> + audio decoders the ability to use get/release/reget_buffer() to get an
>>>>>>>> + output buffer.
>>>>>>>> +
>>>>>>>> 2010-10-10 - r25441 - lavfi 1.49.0 - AVFilterLink.time_base
>>>>>>>> Add time_base field to AVFilterLink.
>>>>>>>>
>>>>>>>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
>>>>>>>> index 4bddbaa..1aa1c8c 100644
>>>>>>>> --- a/libavcodec/avcodec.h
>>>>>>>> +++ b/libavcodec/avcodec.h
>>>>>>>> @@ -31,7 +31,7 @@
>>>>>>>> #include "libavutil/cpu.h"
>>>>>>>>
>>>>>>>> #define LIBAVCODEC_VERSION_MAJOR 52
>>>>>>>> -#define LIBAVCODEC_VERSION_MINOR 92
>>>>>>>> +#define LIBAVCODEC_VERSION_MINOR 93
>>>>>>>> #define LIBAVCODEC_VERSION_MICRO 0
>>>>>>>>
>>>>>>>> #define LIBAVCODEC_VERSION_INT AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
>>>>>>>> @@ -467,8 +467,10 @@ enum SampleFormat {
>>>>>>>> CH_FRONT_LEFT_OF_CENTER|CH_FRONT_RIGHT_OF_CENTER)
>>>>>>>> #define CH_LAYOUT_STEREO_DOWNMIX (CH_STEREO_LEFT|CH_STEREO_RIGHT)
>>>>>>>>
>>>>>>>> +#if FF_API_AUDIO_OLD
>>>>>>>> /* in bytes */
>>>>>>>> #define AVCODEC_MAX_AUDIO_FRAME_SIZE 192000 // 1 second of 48khz 32bit audio
>>>>>>>> +#endif
>>>>>>>>
>>>>>>>> /**
>>>>>>>> * Required number of additionally allocated bytes at the end of the input bitstream for decoding.
>>>>>>>> @@ -988,7 +990,13 @@ typedef struct AVPanScan{
>>>>>>>> * - decoding: Set by libavcodec\
>>>>>>>> */\
>>>>>>>> void *hwaccel_picture_private;\
>>>>>>>> -
>>>>>>>> +\
>>>>>>>> + /**\
>>>>>>>> + * number of audio samples (per channel) described by this frame\
>>>>>>>> + * - encoding: Set by user.\
>>>>>>>> + * - decoding: Set by libavcodec.\
>>>>>>>> + */\
>>>>>>>> + int nb_samples;\
>>>>>>>>
>>>>>>>> #define FF_QSCALE_TYPE_MPEG1 0
>>>>>>>> #define FF_QSCALE_TYPE_MPEG2 1
>>>>>>>> @@ -2744,6 +2752,33 @@ typedef struct AVCodecContext {
>>>>>>>> * - decoding: unused
>>>>>>>> */
>>>>>>>> int lpc_passes;
>>>>>>>> +
>>>>>>>> + /**
>>>>>>>> + * User-allocated audio decoder output buffer & buffer size
>>>>>>>> + * If user_buffer is non-NULL and is large enough,
>>>>>>>> + * avcodec_default_get_buffer() may user it as an internal buffer instead
>>>>>>>> + * of allocating its own. This only works with decoders that support
>>>>>>>> + * CODEC_CAP_DR1. If the decoder uses this buffer, it will set the value
>>>>>>>> + * to NULL.
>>>>>>>> + *
>>>>>>>> + * @note The user may not use this field when using avcodec_decode_audio3()
>>>>>>>> + * or avcodec_decode_audio2().
>>>>>>> I dont see why we need such special casing. *samples could be passed in the new
>>>>>>> api like it is in the old
>>>>>> The old API already has the samples buffer passed directly. Why should
>>>>>> the old API be changed to accept user_buffer as an alternative to
>>>>>> *samples? Do they have to match? If not, which takes priority? This
>>>>>> would require added documentation to an old API. That seems more
>>>>>> complexity than necessary when the ability to supply a direct buffer is
>>>>>> already there.
>>>>> you misunderstand
>>>>> why is the new api having it passed over AVCodecContext and the old over an
>>>>> function argument (in the sense why dont you change the new api to also take
>>>>> a argument for that?) maybe i miss something but this seems to be simpler
>>>> Ah, thanks for clarifying.
>>>>
>>>> The new API doesn't need it. The old API needs it in order to avoid
>>>> memcpy. In the new API, if the user wants a direct buffer, she can
>>>> override get/release_buffer().
>>> she can but that is more code and work
>> Ok. I'm somewhat against it, but I'm willing to implement it.
>
> can you elaborate on why you are against it?
> to me this looks nicer and more logic from the outsides users point of view
I would feel better about it if we have something in the documentation
saying that not passing a direct buffer is the preferred usage since it
will ensure that a large enough buffer will be allocated as long as
enough memory is available and the audio parameters are within a sane range.
>> my preference:
>>
>> int avcodec_decode_audio4(AVCodecContext *avctx, AVFrame *frame,
>> int *got_frame_ptr, AVPacket *avpkt,
>> void *samples, int samples_size);
>>
>> What do you think?
>
> i like it
Ok. I'll update my patch.
Thanks,
Justin
More information about the ffmpeg-devel
mailing list