[FFmpeg-devel] [PATCH] avcodec: Allow user applications to provide non padded input buffers

Michael Niedermayer michaelni at gmx.at
Wed Jan 14 21:19:24 CET 2015


On Wed, Jan 14, 2015 at 07:22:48PM +0100, wm4 wrote:
> On Wed, 14 Jan 2015 18:06:41 +0100
> Michael Niedermayer <michaelni at 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 <michaelni at 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 <michaelni at 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?

yes, thats broken, i had tested this but simplified it afterwards
quite a bit without restesting, ill fix this


> 
> > > > +
> > > 
> > > 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

hmm, i guess this is true


> - and maybe not into the AVBuffer at all?

that would be a very odd thing

but if preferred i can just add a field to AVCodecContext in which
the user can specify the allocated size of the next packet, this
would avoid all ambiguities though it would be more code


> 
> 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.

I had no immedeate plans to do more work on this due to too many other
things i wanted to work on but the logical next step would be to
mark decoders which do not need the paddding as such and to change
decoders which are important like h264 to not need the padding if
that turns out to be not too ugly when implemented

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have never wished to cater to the crowd; for what I know they do not
approve, and what they approve I do not know. -- Epicurus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150114/1b202b00/attachment.asc>


More information about the ffmpeg-devel mailing list