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

[wm4]

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

> On Fri, Jun 13, 2014 at 03:08:11PM +0200, wm4 wrote:
> > On Fri, 13 Jun 2014 14:44:59 +0200
> > Michael Niedermayer <michaelni at gmx.at> wrote:
> > 
> > > On Fri, Jun 13, 2014 at 12:41:30PM +0200, wm4 wrote:
> > > > 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 ?
> > > 
> > > no reply here ?
> > > you are more interrested in the complaint than the solution ?
> > 
> > No, I'm rejecting the very concept of the solution.
> 
> my question was not about a specific solution but about the problem(s)
> you talk about in general
> what do you suggest instead to be done to solve them ...

Well, as I said, the unrealistic solution is to make lavc just accept
unpadded and unaligned buffers. This could probably be done by either
adjusting the bitstream readers (I don't know much about them), or by
copying the input buffer if it's not needed.

Note that we could very well check whether the input buffer is
correctly aligned, and choose a faster code path is that the case. For
padding, we could add a packet flag, and the user could set that packet
flag to signal that padding is present.

Both could lead to accidental slowdown (but how much? surely not much),
but unlike the current situation, the user couldn't accidentally add
crashes or security issues by overlooking subtle/unusual/unexpected API
requirements.

My goal would be to reduce violations of the principle of least
astonishment in the API.

> 
> > 
> > I don't think it's a good idea to make these weird corner cases explicit
> > (such as the possibility that the packet buffer might be passed as
> > AVFrame to libavfilter).
> 
> i fully agree
> 
> 
> > Rather, it's better to pretend this corner
> > case doesn't exist by making the amount of required padding always the
> > same.
> 
> yes, and, the change of FF_INPUT_BUFFER_PADDING_SIZE to 32 is
> a step in this direction. The actual requirement can only be raised
> with the next ABI bump though

Well yes, with the problem at hand, just bumping that constant is the
"quick fix", simply because it was already broken. I'd still argue for
an ABI bump though, because as is applications linked against newer
libavcodec might still cause reads beyond allocated memory objects.

> 
> > Make it explicit that the user has to pad with 0, instead of
> > writing something weird about what bits must be set when using mpeg, and
> > that nobody really understands in its full consequences. This just
> > leads to users guessing wrong.
> 
> thats a bit trickier, and i think getting rid of the zero padding
> requirement might be the nicer solution than unconditionally requiring
> it
> 

Certainly!