[FFmpeg-devel] [PATCH] avcodec: add AVCODEC_REQUIRED_INPUT_BUFFER_PADDING_SIZE, split FF_INPUT_BUFFER_PADDING_SIZE

[wm4]

On Fri, 13 Jun 2014 03:32:59 +0200
Michael Niedermayer <michaelni at gmx.at> wrote:

> On Fri, Jun 13, 2014 at 02:49:01AM +0200, wm4 wrote:
> > On Thu, 12 Jun 2014 23:06:13 +0200
> > Michael Niedermayer <michaelni at gmx.at> wrote:
> > 
> > > TODO bump minor version, update APIChanges
> > > 
> > > Signed-off-by: Michael Niedermayer <michaelni at gmx.at>
> > > ---
> > >  libavcodec/avcodec.h |   11 ++++++++++-
> > >  1 file changed, 10 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > > index 68b1f26..ca66f3c 100644
> > > --- a/libavcodec/avcodec.h
> > > +++ b/libavcodec/avcodec.h
> > > @@ -608,13 +608,22 @@ typedef struct AVCodecDescriptor {
> > >  
> > >  /**
> > >   * @ingroup lavc_decoding
> > > + * The amount of padding that is allocated at the end of the input bitstream by
> > > + * libavformat and other libs, this affects all buffers which may reasonably be
> > > + * directly used as avcodec input buffers.
> > > + * The value of this symbol may be increased without major version bump.
> > > + */
> > > +#define FF_INPUT_BUFFER_PADDING_SIZE 32
> > 
> > Why is there something about libavformat and "other libs" in libavcodec?
> 
> it has to be somewhere, and it is about buffers that are input into
> libavcodec.
> also existing API requires it to be included when libavcodec/avcodec.h
> is
> 
> 
> > 
> > The name of this define is more informative than these 4 lines of help
> > text.
> 
> what do you suggest instead ?
> 
> 
> > 
> > > +
> > > +/**
> > > + * @ingroup lavc_decoding
> > >   * Required number of additionally allocated bytes at the end of the input bitstream for decoding.
> > >   * This is mainly needed because some optimized bitstream readers read
> > >   * 32 or 64 bit at once and could read over the end.<br>
> > >   * Note: If the first 23 bits of the additional bytes are not 0, then damaged
> > >   * MPEG bitstreams could cause overread and segfault.
> > >   */
> > > -#define FF_INPUT_BUFFER_PADDING_SIZE 32
> > > +#define AVCODEC_REQUIRED_INPUT_BUFFER_PADDING_SIZE 16
> > 
> > I don't understand the difference between these two.
> 
> Each lib has its own requirement for padding
> 
> to be able to pass through input buffers from libavcodec over
> output AVFrames into libavfilter, it is neccessary that the input
> to libavcodec has been padded sufficiently for both libavfilter and
> libavcodec. This case can occurs with raw video for example

I think it can occur _only_ with raw video/audio, simply because these
are the only "decoders" where the input data can be passed through as
image data unchanged. Adding so much confusion just for the raw video
demuxer doesn't seem like a good idea. Even more confusing: the padding
is actually not needed for demuxing raw video, but it is needed when
trying to pass the demuxed AVFrames to (some?) libavfilter filters.
This is pretty insane IMHO.

> 
> AVCODEC_REQUIRED_INPUT_BUFFER_PADDING_SIZE represents the amount of
> padding that is required for input to libavcodec
> 
> FF_INPUT_BUFFER_PADDING_SIZE represents the amount that we pad.
> Using this we can pass the output from libavcodec to other libs like
> libavfilter in the case where (due to rawvideo) the input has been
> passed through
> 
> I hope above is understandable

Yes, but still confusing. At least it could be a global constant, maybe
defined in libavutil/mem.h? Actually, why does not just av_malloc add
the padding? It also ensures alignment, so why not. (It will waste some
memory, but you do that for alignment too, with every allocation, even
strings.)

Declaring that input buffers must be allocated with av_malloc because
ffmpeg is so damn "special" is probably easier to understand for API
users. For advanced users, enumerating the memory buffer requirements
somewhere else might be useful too.

> [...]
> 
> PS: iam not sure you even need 16byte padding for libavcodec, possibly
> less will be fine
> 

Ideally it wouldn't require any padding, but changing that now is
probably near impossible.