[FFmpeg-devel] [PATCH+RFC] AVFrame for audio
Michael Niedermayer
michaelni
Sun Oct 17 10:42:46 CEST 2010
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
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
When you are offended at any man's fault, turn to yourself and study your
own failings. Then you will forget your anger. -- Epictetus
-------------- 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/20101017/6d323c70/attachment.pgp>
More information about the ffmpeg-devel
mailing list