On Wed, 14 Jan 2015 18:06:41 +0100
Michael Niedermayer <michae...@gmx.at> wrote:
> On Wed, Jan 14, 2015 at 05:58:04PM +0100, wm4 wrote:
> > On Wed, 14 Jan 2015 17:23:08 +0100
> > Michael Niedermayer <michae...@gmx.at> wrote:
> >
> > > This can be extended easily to skip the buffer growing for codecs which do
> > > not need it.
> > >
> > > Signed-off-by: Michael Niedermayer <michae...@gmx.at>
> > > ---
> > > libavcodec/avcodec.h | 8 +++++++-
> > > libavcodec/utils.c | 12 ++++++++++--
> > > 2 files changed, 17 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > > index 99467bb..ec95194 100644
> > > --- a/libavcodec/avcodec.h
> > > +++ b/libavcodec/avcodec.h
> > > @@ -4128,9 +4128,15 @@ int avcodec_decode_audio4(AVCodecContext *avctx,
> > > AVFrame *frame,
> > > * Some decoders may support multiple frames in a single AVPacket, such
> > > * decoders would then just decode the first frame.
> > > *
> > > - * @warning The input buffer must be FF_INPUT_BUFFER_PADDING_SIZE larger
> > > than
> > > + * @warning If you use non AVBuffer based AVPackets, then the input
> > > buffer must
> > > + * be FF_INPUT_BUFFER_PADDING_SIZE larger than
> > > * the actual read bytes because some optimized bitstream readers read
> > > 32 or 64
> > > * bits at once and could read over the end.
> > > + * If you do use AVBuffer based AVPackets and the allocated space as
> > > indicated
> > > + * by the AVBuffer does not include FF_INPUT_BUFFER_PADDING_SIZE padding
> > > then
> > > + * many decoders will reallocate the buffer.
> > > + * Most AVPacket as created by the FFmpeg APIs will already include the
> > > needed
> > > + * padding.
> > > *
> > > * @warning The end of the input buffer buf should be set to 0 to ensure
> > > that
> > > * no overreading happens for damaged MPEG streams.
> > > diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> > > index 1ec5cae..eaa0431 100644
> > > --- a/libavcodec/utils.c
> > > +++ b/libavcodec/utils.c
> > > @@ -2332,7 +2332,8 @@ int attribute_align_arg
> > > avcodec_decode_video2(AVCodecContext *avctx, AVFrame *pi
> > > AVCodecInternal *avci = avctx->internal;
> > > int ret;
> > > // copy to ensure we do not change avpkt
> > > - AVPacket tmp = *avpkt;
> > > + AVPacket tmp;
> > > + int did_split = 0;
> > >
> > > if (!avctx->codec)
> > > return AVERROR(EINVAL);
> > > @@ -2347,8 +2348,15 @@ int attribute_align_arg
> > > avcodec_decode_video2(AVCodecContext *avctx, AVFrame *pi
> > >
> > > av_frame_unref(picture);
> > >
> > > + if (avpkt->buf && avpkt->size && avpkt->buf->size - avpkt->size <
> > > FF_INPUT_BUFFER_PADDING_SIZE) {
> > > + if ((ret = av_grow_packet(avpkt, FF_INPUT_BUFFER_PADDING_SIZE))
> > > < 0)
> > > + goto fail;
> > > + tmp.size -= FF_INPUT_BUFFER_PADDING_SIZE;
> > > + }
> > > + tmp = *avpkt;
Staring at this again... tmp.size is set, and then the entire tmp
variable is overwitten with *avpkt?
> > > +
> >
> > This assumes that AVBuffer is actually always padded. Is this the case?
> > The user can create custom AVBuffers, with custom allocation and custom
> > free function.
>
> You mean the user could allocate a sufficicently large buffer but
> set the AVBuffer size incorrectly, yes this is hypothetically possible
> and would lead to a unneeded reallocation.
> What do you suggest ?
I overlooked that the AVBuffer uses the padded size, while AVPacket
uses the data size. So my comment is invalid, and the code should work.
Though I think AVPacket.data doesn't necessarily have to point at
AVBuffer start - and maybe not into the AVBuffer at all?
Also, do you have any future plans for this? As of now, the only change
in behavior is when the user sets up his own AVBuffers.
>
> >
> > > if ((avctx->codec->capabilities & CODEC_CAP_DELAY) || avpkt->size ||
> > > (avctx->active_thread_type & FF_THREAD_FRAME)) {
> > > - int did_split = av_packet_split_side_data(&tmp);
> > > + did_split = av_packet_split_side_data(&tmp);
> > > ret = apply_param_change(avctx, &tmp);
> > > if (ret < 0) {
> > > av_log(avctx, AV_LOG_ERROR, "Error applying parameter
> > > changes.\n");
> >
> > What's the point of the did_split change?
>
> not to leave it uninitialized on the "goto fail" path, which could be
> bad
OK.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
