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

[Michael Niedermayer]

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 ?


> > 
> > 
> > > 
> > > > +
> > > > +/**
> > > > + * @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 ...

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have often repented speaking, but never of holding my tongue.
-- Xenocrates
-------------- 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/20140613/f244958c/attachment.asc>