This seems all very reasonable.
Most of these suggestions are easily fixed, but I am struggling to find
the best approach to avoiding those global function pointers.
The approach of storing these in some kind of context, as mentioned in
the referenced message looks like the best solution. Am I right in
thinking that this has not been actually built yet? I couldn't find any
references in the code.
Assuming that I'm right and nothing of the sort exists yet, would it be
sensible to:
- create an AVUtilContext struct, which contains the
allocation/deallocation functions, as well as a "user data" pointer
- add those same members at the start of the relevant existing contexts
(i.e. AVCodecContext), so that we can cast those structs to
ACUtilContext.
- change the relevant buffer allocation funtions to take the structure
as pointer
Please let me know if I am missing anything here, i.e. a global context
which already exists, or an alternative strategy.
With regards,
Martijn Otto
On Sat, 2021-03-06 at 15:39 +0100, Nicolas George wrote:
> Martijn Otto (12021-03-05):
> > Hello all,
> >
> > I have made some changes to get custom allocation functions in
> > ffmpeg.
> > This is useful to me, as the software I work with easily runs into
> > congestion on memory allocations in certain cases.
> >
> > I hope it is useful to others as well. It would be nice if this
> > could
> > be part of ffmpeg proper. If you have specific issues with my
> > implementation, please let me know.
>
> Thanks for the contribution. It may be useful, but it cannot be
> accepted
> as is for several reasons, see below.
>
> > From 802b4aecb77c8a35eb6641aa8dd6d27bbcda1fe2 Mon Sep 17 00:00:00
> > 2001
> > From: Martijn Otto <ffm...@martijnotto.nl>
> > Date: Thu, 4 Mar 2021 17:30:15 +0100
> > Subject: [PATCH] Add custom memory allocation routines.
> >
> > This feature is a useful optimization strategy. It allows the
> > implementation of working with a memory pool for better
> > performance.
> > ---
> > libavformat/hlsenc.c | 1 +
> > libavutil/buffer.c | 41 +++++++++++++++++++++++++++++++++++-----
> > -
> > libavutil/buffer.h | 23 +++++++++++++++++++++++
> > 3 files changed, 59 insertions(+), 6 deletions(-)
> >
> > diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
> > index 7d97ce1789..c8e4281e7b 100644
> > --- a/libavformat/hlsenc.c
> > +++ b/libavformat/hlsenc.c
> > @@ -927,6 +927,7 @@ static int hls_mux_init(AVFormatContext *s,
> > VariantStream *vs)
> > } else {
> > ret = hlsenc_io_open(s, &vs->out, vs-
> > >base_output_dirname, &options);
> > }
> > + avio_flush(oc->pb);
>
> This is an unrelated change, it needs to be submitted separately.
>
> > av_dict_free(&options);
> > }
> > if (ret < 0) {
> > diff --git a/libavutil/buffer.c b/libavutil/buffer.c
> > index 3204f11b68..bd86b38524 100644
> > --- a/libavutil/buffer.c
> > +++ b/libavutil/buffer.c
> > @@ -59,23 +59,52 @@ AVBufferRef *av_buffer_create(uint8_t *data,
> > int size,
> > return ref;
> > }
> >
> > +PFNBufferAlloc externalAllocFunc = NULL;
> > +PFNBufferDealloc externalDeallocFunc = NULL;
>
> We do not use camelCase for identifiers.
>
> These variables are local to the file and should be declared as such.
>
> More importantly: we are trying to get rid of mutable global state.
> This
> adds to it.
>
> See this:
> https://ffmpeg.org/pipermail/ffmpeg-devel/2020-December/274168.html
> for my attempt at making global state local.
>
> > +
> > +void av_set_buffer_alloc_free_funcs( PFNBufferAlloc externalAlloc,
> > PFNBufferDealloc externalDealloc )
> > +{
> > + externalAllocFunc = externalAlloc;
> > + externalDeallocFunc = externalDealloc;
> > +}
> > +
> > void av_buffer_default_free(void *opaque, uint8_t *data)
> > {
> > av_free(data);
> > }
> > +void av_buffer_external_free(void *opaque, uint8_t *data)
>
> Missing new line.
>
> This function is not declared. Does it even compile?
>
> If it is not exported, it should be static and not in the av_
> namespace.
>
> > +{
> > + if (externalDeallocFunc != NULL)
> > + externalDeallocFunc(data);
> > +}
> >
> > AVBufferRef *av_buffer_alloc(int size)
> > {
> > AVBufferRef *ret = NULL;
> > uint8_t *data = NULL;
> >
> > - data = av_malloc(size);
> > - if (!data)
> > - return NULL;
> > + //Give priority to the external allocation function to give
> > the application a chance to manage it's own buffer allocations.
> > + if (externalAllocFunc != NULL)
> > + data = externalAllocFunc(size);
> >
> > - ret = av_buffer_create(data, size, av_buffer_default_free,
> > NULL, 0);
> > - if (!ret)
> > - av_freep(&data);
> > + if (data) {
> > + //Create a buffer and tell it to free it's data using the
> > external free function. We've used the external
> > + //allocator for allocation, so we need to use external
> > deallocator for deallocation.
> > + ret = av_buffer_create(data, size,
> > av_buffer_external_free, NULL, 0);
> > + if (!ret)
> > + av_buffer_external_free(NULL, data);
> > + } else {
> > + //The external allocation function may return NULL for
> > other reasons than out of memory, so
> > + //if it did we will fall back to our own allocation
> > function.
>
> Random fallbacks are wrong. The allocation function failed, period.
>
> > + data = av_malloc(size);
> > + if (!data)
> > + return NULL; //We're out of memory after all.
> > +
> > + //We've created the buffer data ourselves so we can use
> > our own free function.
> > + ret = av_buffer_create(data, size, av_buffer_default_free,
> > NULL, 0);
> > + if (!ret)
> > + av_freep(&data);
> > + }
> >
> > return ret;
> > }
> > diff --git a/libavutil/buffer.h b/libavutil/buffer.h
> > index fd4e381efa..3efe585d56 100644
> > --- a/libavutil/buffer.h
> > +++ b/libavutil/buffer.h
> > @@ -93,6 +93,29 @@ typedef struct AVBufferRef {
> > int size;
> > } AVBufferRef;
> >
> > +#if defined(_WIN32)
> > + #define FFAPI __stdcall
> > +#else
> > + #define FFAPI
> > +#endif
>
> We never needed this. Why change now?
>
> > +
> > +typedef void* (FFAPI *PFNBufferAlloc)( int size );
> > +typedef void (FFAPI *PFNBufferDealloc)( void* buffer );
>
> Public types should be in the AV namespace. We do not declare
> function
> types usually anyway.
>
> > +/**
> > + * Set allocation functions that can be used to externally manage
> > buffer allocations.
> > + * During regular playback buffers are continuously being
> > allocated and deallocated. In high performance
> > + * applications this becomes a problem. When multiple files are
> > playing at the same time on different threads
> > + * these allocations interlock with eachother causing performance
> > loss due to reduced paralellism.
> > + * To remedy this these applications may set these
> > allocation/deallocation functions which it can use to prevent
> > + * this behaviour. It could for example implement a pool allocator
> > from which it will source the buffers.
>
> Please wrap at <80 characters.
>
> Also, if you want to solve the problem with parallelism, you will
> need
> to pass an extra parameter to the function.
>
> > + *
> > + * @param externalAlloc The function that will be called when a
> > new buffer is required. This function can return
> > + * NULL if it does not take care of
> > allocating buffers of the provided size. In this case FFMPeg will
> > + * fall back to it's own allocation
> > function.
> > + * @param externalDealloc The function that will be called when a
> > buffer is to be deallocated.
> > + */
> > +void av_set_buffer_alloc_free_funcs( PFNBufferAlloc externalAlloc,
> > PFNBufferDealloc externalDealloc );
>
> Unusual spacing.
>
> > +
> > /**
> > * Allocate an AVBuffer of the given size using av_malloc().
> > *
>
> Regards,
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
