[FFmpeg-devel] [PATCH+RFC] AVFrame for audio
Michael Niedermayer
michaelni
Thu Oct 28 17:23:00 CEST 2010
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
> >> [...]
> >>>> @@ -2763,7 +2813,7 @@ typedef struct AVCodec {
> >>>> int (*init)(AVCodecContext *);
> >>>> int (*encode)(AVCodecContext *, uint8_t *buf, int buf_size, void *data);
> >>>> int (*close)(AVCodecContext *);
> >>>> - int (*decode)(AVCodecContext *, void *outdata, int *outdata_size, AVPacket *avpkt);
> >>>> + int (*decode)(AVCodecContext *, void *outdata, int *got_output_ptr, AVPacket *avpkt);
> >>>> /**
> >>>> * Codec capabilities.
> >>>> * see CODEC_CAP_*
> >>> cosmetic
> >> yeah yeah. I do want to change it though. :) The size won't be needed
> >> by anything after the audio API is changed. And I still don't know why
> >> the video decoders set it to sizeof(AVFrame) or sizeof(AVPicture).
> >
> > The original idea probably was for ABI compatibility. That is if AVFrame grows
> > over the versions the user app has to know where it ends to know what fields
> > it can saftely read
> > setting it to sizeof of an internal struct is obviously not a good idea ...
>
> The user isn't supposed to use AVCodec.decode() directly right? And the
> video and subtitle decoding API just has got_*_ptr which is documented
> as being zero or non-zero. So would it be ok for the audio decoders to
> just set this to 0 or 1 in the new API? Changing all the video and
> subtitle decoders is obviously not necessary, but if we did do that at
> some point it would be less confusing.
the documentation can be changed to 0 vs. not 0
but imho that is seperate of this patch
>
> >
> >>> [...]
> >>>> 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
[...]
> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index ffd34ee..d95622b 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -114,6 +114,9 @@ typedef struct InternalBuffer{
> int linesize[4];
> int width, height;
> enum PixelFormat pix_fmt;
> + int channels;
> + int nb_samples;
> + enum SampleFormat sample_fmt;
> }InternalBuffer;
>
> #define INTERNAL_BUFFER_SIZE 32
[...]
> @@ -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){
> @@ -249,21 +273,50 @@ int avcodec_default_get_buffer(AVCodecContext *s, AVFrame *pic){
> );
> #endif
>
> + /* For audio, use AVCodecContext.user_buffer if it is non-NULL, large
> + enough to hold the frame data, and the decoder does not request
> + a reusable and/or preserved buffer. */
> + if (s->user_buffer && !is_video && ((pic->buffer_hints & FF_BUFFER_HINTS_VALID) &&
> + !(pic->buffer_hints & FF_BUFFER_HINTS_PRESERVE|FF_BUFFER_HINTS_REUSABLE))) {
> + int buf_size = pic->nb_samples * s->channels *
> + (av_get_bits_per_sample_format(s->sample_fmt) / 8);
> + if (s->user_buffer_size >= buf_size) {
> + pic->type = FF_BUFFER_TYPE_INTERNAL | FF_BUFFER_TYPE_USER;
> + pic->base[0] = pic->data[0] = s->user_buffer;
> + s->user_buffer = NULL;
> + pic->reordered_opaque = s->reordered_opaque;
> +
> + if (s->debug & FF_DEBUG_BUFFERS) {
> + av_log(s, AV_LOG_DEBUG, "default_get_buffer called on pic %p, "
> + "AVCodecContext.user_buffer used\n", pic);
> + }
> + return 0;
> + }
> + }
> +
i dont understand this code.
it looks to me like checking for alot of fatal error conditions but not failing
* user_buffer set for non audio
* mixing user buffers and some flags that make no sense for audio and i dont
see which decoder would use them
* the buffer being too small
I see no use case where not immedeatly failing would make any sense, also
this makes patch review much more difficult because i would have to make
sure these cases that appear nonsense to me dont lead to exploits a few lines
later. And it obviously increases code complexity at no obvious gain.
If iam missing some sense in these cases, please elaborately explain
[...]
> @@ -380,13 +451,26 @@ int avcodec_default_reget_buffer(AVCodecContext *s, AVFrame *pic){
>
> /* If no picture return a new buffer */
> if(pic->data[0] == NULL) {
> + int ret;
> /* We will copy from buffer, so must be readable */
> pic->buffer_hints |= FF_BUFFER_HINTS_READABLE;
> - return s->get_buffer(s, pic);
> + ret = s->get_buffer(s, pic);
> +
> + /* Don't allow user_buffer to be used */
> + if (!ret && pic->type == (FF_BUFFER_TYPE_INTERNAL | FF_BUFFER_TYPE_USER)) {
> + uint8_t *buf = pic->data[0];
> + assert(s->user_buffer == NULL);
> + ret = s->get_buffer(s, pic);
> + assert(ret || pic->type == FF_BUFFER_TYPE_INTERNAL);
> + /* restore user_buffer to indicate that it was not used */
> + s->user_buffer = buf;
> + }
> + return ret;
> }
>
> /* If internal buffer type return the same buffer */
> if(pic->type == FF_BUFFER_TYPE_INTERNAL) {
> + assert(!(pic->type & FF_BUFFER_TYPE_USER));
> pic->reordered_opaque= s->reordered_opaque;
> return 0;
> }
> @@ -399,11 +483,17 @@ int avcodec_default_reget_buffer(AVCodecContext *s, AVFrame *pic){
> pic->data[i] = pic->base[i] = NULL;
> pic->opaque = NULL;
> /* Allocate new frame */
> + assert(!s->user_buffer);
> if (s->get_buffer(s, pic))
> return -1;
> - /* Copy image data from old buffer to new buffer */
> + /* Copy frame data from old buffer to new buffer */
> + if (s->codec_type == AVMEDIA_TYPE_VIDEO) {
> av_picture_copy((AVPicture*)pic, (AVPicture*)&temp_pic, s->pix_fmt, s->width,
> s->height);
> + } else if (s->codec_type == AVMEDIA_TYPE_AUDIO) {
> + memcpy(pic->data[0], temp_pic.data[0], s->channels * pic->nb_samples *
> + (av_get_bits_per_sample_format(s->sample_fmt) / 8));
> + }
> s->release_buffer(s, &temp_pic); // Release old frame
> return 0;
> }
what does this code do?
what does reget_buffer() even mean for audio buffers ?
and what codec would use that?
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
The real ebay dictionary, page 2
"100% positive feedback" - "All either got their money back or didnt complain"
"Best seller ever, very honest" - "Seller refunded buyer after failed scam"
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101028/1878fe5a/attachment.pgp>
More information about the ffmpeg-devel
mailing list