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

[wm4]

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.

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). Rather, it's better to pretend this corner
case doesn't exist by making the amount of required padding always the
same.  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.

> 
> > > 
> > > 
> > > > 
> > > > > +
> > > > > +/**
> > > > > + * @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.
> 
> yes, some need it, some will be fine without.
> 
> 
> 
> > 
> > > 
> > > 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.
> 
> that statement is a bit misleading
> There are tradeoffs between complexity, speed and padding requirements
> complexity also affects maitainability and bugs.
> the existing choice is toward fast, simple and easy to maintain.
> 
> adding support to "no padding" now would be about the same amount
> of work than what it would have been 10 years ago.
> Though now we would have side by side comparissions of what effect
> it has and the work would not be spread over 10 years
> besides its certainly not impossible
> it just needs someone to sit down and do it, and 95% of the decoders
> can be handled with some generic solution like the existing checked
> bitstream reader, the number of real world speed relevant decoders
> is not so large.
> The real question is would any applications benefit from removing the
> padding requirement and is there anyone who wants to put some coding
> work behind the words ...
> 
> [...]

See my other reply - the assumption of implicit padding is probably far
too widespread. If we agree that an effort should be made to eliminate
all padding, that would be another story.