| Message ID | 20200627151646.2593306-1-george@nsup.org |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/3] lavu: add a template for refcounted objects. | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
Quoting Nicolas George (2020-06-27 17:16:44) > Signed-off-by: Nicolas George <george@nsup.org> > --- > libavutil/avrefcount_template.h | 140 ++++++++++++++++++++++++++++++++ > tests/ref/fate/source | 1 + > 2 files changed, 141 insertions(+) > create mode 100644 libavutil/avrefcount_template.h > > > I will need to refcount something soon. Recently, the need to stop > abusing AVBuffer for all refcounting was mentioned on the list. So here > is an attempt at isolating the refcounting itself. > > This is not the final verion, I will first work on the "something" to > make sure it suits the needs. But it is a first version. > > Anton, I would appreciate if you had a look at this and told me if there > is something you strongly dislike about before I have piled too much > efforts over it. Why a template? It seems simpler to add a struct like typedef struct AVRefcount { atomic_uint refcount; void *opaque; void (*free)(void *opaque); } AVRefcount; and then embed it in everything that wants to be refcounted. All just normal structs and functions, no layers of macros.
Anton Khirnov (12020-07-01): > Why a template? It seems simpler to add a struct like > typedef struct AVRefcount { > atomic_uint refcount; > void *opaque; > void (*free)(void *opaque); > } AVRefcount; > and then embed it in everything that wants to be refcounted. All just > normal structs and functions, no layers of macros. Because what you propose requires an extra layer of pointers, which is both annoying when reading and writing the code and inefficient when running it. Also because what you propose has void pointers, and therefore will not benefit from the compiler checking the types at compilation time. Regards,
Quoting Nicolas George (2020-07-01 17:13:22) > Anton Khirnov (12020-07-01): > > Why a template? It seems simpler to add a struct like > > typedef struct AVRefcount { > > atomic_uint refcount; > > void *opaque; > > void (*free)(void *opaque); > > } AVRefcount; > > and then embed it in everything that wants to be refcounted. All just > > normal structs and functions, no layers of macros. > > Because what you propose requires an extra layer of pointers, which is > both annoying when reading and writing the code instead of #define AVRC_TYPE AVBuffer #define AVRC_PREFIX prefix #define AVRC_FIELD refcount you'd have av_refcount_init(&buf->refcount, buf, buffer_free); Does not look more annoying to me, to the contrary macro templates require more effort to understand or debug. > and inefficient when running it. Why? I do not see how an extra pointer dereference could possibly matter here. > > Also because what you propose has void pointers, and therefore will not > benefit from the compiler checking the types at compilation time. In my experience, providing the wrong opaque pointer has never been a common source of bugs (and is typically easily noticeable if it does happen), so I see that as an acceptable tradeoff.
Anton Khirnov (12020-07-01): > instead of > #define AVRC_TYPE AVBuffer > #define AVRC_PREFIX prefix > #define AVRC_FIELD refcount > you'd have > av_refcount_init(&buf->refcount, buf, buffer_free); > Does not look more annoying to me, to the contrary macro templates > require more effort to understand or debug. The annoying part is to have to dereference buf->opaque each time we need to use it. It clutters all the code that use a structure. It is completely unacceptable to me. > Why? I do not see how an extra pointer dereference could possibly matter here. It does not matter much, but in tights loops it can. > In my experience, providing the wrong opaque pointer has never been a > common source of bugs (and is typically easily noticeable if it does > happen), so I see that as an acceptable tradeoff. Acceptable, certainly. But I prefer static type safety whenever possible, especially when it comes with the other benefits. Regards,
On 7/1/2020 12:05 PM, Anton Khirnov wrote: > Quoting Nicolas George (2020-06-27 17:16:44) >> Signed-off-by: Nicolas George <george@nsup.org> >> --- >> libavutil/avrefcount_template.h | 140 ++++++++++++++++++++++++++++++++ >> tests/ref/fate/source | 1 + >> 2 files changed, 141 insertions(+) >> create mode 100644 libavutil/avrefcount_template.h >> >> >> I will need to refcount something soon. Recently, the need to stop >> abusing AVBuffer for all refcounting was mentioned on the list. So here >> is an attempt at isolating the refcounting itself. >> >> This is not the final verion, I will first work on the "something" to >> make sure it suits the needs. But it is a first version. >> >> Anton, I would appreciate if you had a look at this and told me if there >> is something you strongly dislike about before I have piled too much >> efforts over it. > > Why a template? It seems simpler to add a struct like > typedef struct AVRefcount { > atomic_uint refcount; > void *opaque; > void (*free)(void *opaque); > } AVRefcount; > and then embed it in everything that wants to be refcounted. All just > normal structs and functions, no layers of macros. I very much prefer this approach, being clean looking, public, and free of macros from unguarded headers, but it needs to be either easily extensible or well defined since day 1. For example, it could have more callbacks to be triggered by specific actions, like when creating new references, to workaround the current constrains of AVBufferRef.
Quoting Nicolas George (2020-07-01 18:47:15) > Anton Khirnov (12020-07-01): > > instead of > > #define AVRC_TYPE AVBuffer > > #define AVRC_PREFIX prefix > > #define AVRC_FIELD refcount > > you'd have > > av_refcount_init(&buf->refcount, buf, buffer_free); > > Does not look more annoying to me, to the contrary macro templates > > require more effort to understand or debug. > > The annoying part is to have to dereference buf->opaque each time we > need to use it. It clutters all the code that use a structure. It is > completely unacceptable to me. You do not dereference buf->opaque. You pass it to the free callback when the refcount reaches zero, that is the only way it should ever be used. > > > Why? I do not see how an extra pointer dereference could possibly matter here. > > It does not matter much, but in tights loops it can. If you constantly alloc and free objects in tight loops then you have way bigger problems than a single pointer dereference. This is really a red herring. Beyond that, I suppose this is a personal preference difference. Some more opinions from other people would be welcome.
James Almer (12020-07-01): > I very much prefer this approach, being clean looking, public, and free > of macros from unguarded headers Please do not forget the major drawback: This version makes the creation of the refcounted structure simpler, and every single use more complex. One creation, many uses. Please remember this, it is easy to forget. Regards,
Anton Khirnov (12020-07-01): > You do not dereference buf->opaque. You pass it to the free callback > when the refcount reaches zero, that is the only way it should ever be > used. You are talking about the implementation of the refcounted structure. I was talking about when we use the refcounted structure. Your version makes every single use more complex, because of the indirection. This is something to know about code I write with new APIs: I always make efforts to make the API as simple to use as possible, even if it means making the implementation more complex. Because the implementation is done only once, but using the API will happen many times. And this is exactly what happens with your proposal: a refcounted structure defined with it would be more annoying to use because of the indirection, and I will not have it. If you have a proposal that makes using the refcounted structure as simple and clean as: av_foobar_ref(AVFoobar *f); av_foobar_unrefp(AVFoobar **f); then I will listen. But if you have type pruning or indirection, then my proposal is superior. Regards,
Jul 1, 2020, 19:20 by jamrial@gmail.com: > On 7/1/2020 12:05 PM, Anton Khirnov wrote: > >> Quoting Nicolas George (2020-06-27 17:16:44) >> >>> Signed-off-by: Nicolas George <george@nsup.org> >>> --- >>> libavutil/avrefcount_template.h | 140 ++++++++++++++++++++++++++++++++ >>> tests/ref/fate/source | 1 + >>> 2 files changed, 141 insertions(+) >>> create mode 100644 libavutil/avrefcount_template.h >>> >>> >>> I will need to refcount something soon. Recently, the need to stop >>> abusing AVBuffer for all refcounting was mentioned on the list. So here >>> is an attempt at isolating the refcounting itself. >>> >>> This is not the final verion, I will first work on the "something" to >>> make sure it suits the needs. But it is a first version. >>> >>> Anton, I would appreciate if you had a look at this and told me if there >>> is something you strongly dislike about before I have piled too much >>> efforts over it. >>> >> >> Why a template? It seems simpler to add a struct like >> typedef struct AVRefcount { >> atomic_uint refcount; >> void *opaque; >> void (*free)(void *opaque); >> } AVRefcount; >> and then embed it in everything that wants to be refcounted. All just >> normal structs and functions, no layers of macros. >> > > I very much prefer this approach, being clean looking, public, and free > of macros from unguarded headers, but it needs to be either easily > extensible or well defined since day 1. > For example, it could have more callbacks to be triggered by specific > actions, like when creating new references, to workaround the current > constrains of AVBufferRef. > Same.
Quoting James Almer (2020-07-01 20:20:32) > On 7/1/2020 12:05 PM, Anton Khirnov wrote: > > Quoting Nicolas George (2020-06-27 17:16:44) > >> Signed-off-by: Nicolas George <george@nsup.org> > >> --- > >> libavutil/avrefcount_template.h | 140 ++++++++++++++++++++++++++++++++ > >> tests/ref/fate/source | 1 + > >> 2 files changed, 141 insertions(+) > >> create mode 100644 libavutil/avrefcount_template.h > >> > >> > >> I will need to refcount something soon. Recently, the need to stop > >> abusing AVBuffer for all refcounting was mentioned on the list. So here > >> is an attempt at isolating the refcounting itself. > >> > >> This is not the final verion, I will first work on the "something" to > >> make sure it suits the needs. But it is a first version. > >> > >> Anton, I would appreciate if you had a look at this and told me if there > >> is something you strongly dislike about before I have piled too much > >> efforts over it. > > > > Why a template? It seems simpler to add a struct like > > typedef struct AVRefcount { > > atomic_uint refcount; > > void *opaque; > > void (*free)(void *opaque); > > } AVRefcount; > > and then embed it in everything that wants to be refcounted. All just > > normal structs and functions, no layers of macros. > > I very much prefer this approach, being clean looking, public, and free > of macros from unguarded headers, but it needs to be either easily > extensible or well defined since day 1. > For example, it could have more callbacks to be triggered by specific > actions, like when creating new references, to workaround the current > constrains of AVBufferRef. Why would that be needed in the refcount object itself? I imagined that would be handled in higher-level APIs that use AVRefcout. Or do you see something that cannot be done at higher levels?
Hi all! On 2020-07-01 17:05 +0200, Anton Khirnov wrote: > Quoting Nicolas George (2020-06-27 17:16:44) > > Signed-off-by: Nicolas George <george@nsup.org> > > --- > > libavutil/avrefcount_template.h | 140 ++++++++++++++++++++++++++++++++ > > tests/ref/fate/source | 1 + > > 2 files changed, 141 insertions(+) > > create mode 100644 libavutil/avrefcount_template.h > > > > > > I will need to refcount something soon. Recently, the need to stop > > abusing AVBuffer for all refcounting was mentioned on the list. So here > > is an attempt at isolating the refcounting itself. > > > > This is not the final verion, I will first work on the "something" to > > make sure it suits the needs. But it is a first version. > > > > Anton, I would appreciate if you had a look at this and told me if there > > is something you strongly dislike about before I have piled too much > > efforts over it. > > Why a template? It seems simpler to add a struct like > typedef struct AVRefcount { > atomic_uint refcount; > void *opaque; > void (*free)(void *opaque); > } AVRefcount; > and then embed it in everything that wants to be refcounted. All just > normal structs and functions, no layers of macros. Maybe we need to be more precise about the goal. Or maybe we need to find a common goal. So my first question in this direction is: What do we want? Do we want to make reference counting available for internal and FFmpeg-external use? If we want to generalize for internal use I think Nicolas' proposal has advantages. Not because of performance, but because it makes the definition of ref-counted types easier and more uniformly manageable. If we want to export ref counting, so lavu users can use it for their own types too, I would tend to what Anton proposed. I can go more into detail about the why, but I think we need to find the wanted direction first. Best regards, Alexander
diff --git a/libavutil/avrefcount_template.h b/libavutil/avrefcount_template.h new file mode 100644 index 0000000000..8d0a37370c --- /dev/null +++ b/libavutil/avrefcount_template.h @@ -0,0 +1,140 @@ +/* + * This file is part of FFmpeg. + * + * FFmpeg is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * FFmpeg is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with FFmpeg; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +/** + * Create a set of functions to implement lockless thread-safe reference + * counting for a structure type. + * + * To use this template, you must first define a few macros to describe the + * type and select the name of the functions and their properties. Then you + * include this template as a header file. And finally you use the functions + * that it provides. + * + * Before including this header, you must define the following macros: + * + * AVRC_TYPE the name of the type that will be refcounted. + * Example: #define AVRC_TYPE AVBuffer + * + * AVRC_PREFIX the prefix for all the related functions. + * Example: #define AVRC_PREFIX ff_buffer_ + * + * AVRC_FIELD the field in AVRC_TYPE holding the reference count, + * must be atomic_uint as defined in stdatomic.h. + * + * You can define the following macros, otherwise sane defaults are used: + * + * AVRC_SCOPE the scope for the defined functions, + * defaults to static inline. + * Example: #define AVRC_SCOPE static + * + * AVRC_INIT_REF_COUNT the initial reference count, defaults to 1. + * + * AVRC_FREE the function or macro to call when the reference count drops + * to 0, defaults to AVRC_PREFIX_free(). + * + * After the template has been included, the following functions will be + * defined, with the appropriate prefix: + * + * void PREFIX_init(TYPE *rc); + * + * Init the reference count field of a structure, to be used in functions + * creating new instances. + * + * TYPE *PREFIX_ref(TYPE *rc); + * + * Increase the reference count by one. Returns rc itself for convenience. + * + * void PREFIX_unrefp(TYPE **rc); + * + * Decrease the reference count by one and set *rc to NULL. + * If the reference count drops to 0, call AVRC_FREE(). + * + * unsigned PREFIX_get_ref_count(TYPE *rc); + * + * Return the current number of references. + * + */ + +#include <stdatomic.h> + +#ifndef AVRC_TYPE +#error AVRC_TYPE must be defined. +#endif + +#ifndef AVRC_PREFIX +#error AVRC_PREFIX must be defined. +#endif + +#ifndef AVRC_FIELD +#error AVRC_FIELD must be defined. +#endif + +#ifndef AVRC_SCOPE +#define AVRC_SCOPE static inline +#endif + +#ifndef AVRC_INIT_REF_COUNT +#define AVRC_INIT_REF_COUNT 1 +#endif + +#ifndef AVRC_FREE +#define AVRC_FREE FFRC_PREFIXED(free) +#endif + +#define FFRC_CALL(macro, args) macro args +#define FFRC_CONCAT(prefix, suffix) prefix##suffix +#define FFRC_PREFIXED(suffix) FFRC_CALL(FFRC_CONCAT, (AVRC_PREFIX, suffix)) + +AVRC_SCOPE void +FFRC_PREFIXED(init_ref_count)(AVRC_TYPE *rc) +{ + atomic_init(&rc->AVRC_FIELD, AVRC_INIT_REF_COUNT); +} + +AVRC_SCOPE AVRC_TYPE * +FFRC_PREFIXED(ref)(AVRC_TYPE *rc) +{ + atomic_fetch_add_explicit(&rc->AVRC_FIELD, 1, memory_order_relaxed); + return rc; +} + +AVRC_SCOPE void +FFRC_PREFIXED(unrefp)(AVRC_TYPE **rc) +{ + if (atomic_fetch_sub_explicit(&(*rc)->AVRC_FIELD, 1, memory_order_acq_rel) == 1) { + AVRC_FREE(*rc); + *rc = NULL; + } +} + +AVRC_SCOPE unsigned +FFRC_PREFIXED(get_ref_count)(AVRC_TYPE *rc) +{ + return atomic_load(&rc->AVRC_FIELD); +} + +#undef FFRC_CALL +#undef FFRC_CONCAT +#undef FFRC_PREFIXED + +#undef AVRC_TYPE +#undef AVRC_PREFIX +#undef AVRC_FIELD +#undef AVRC_SCOPE +#undef AVRC_INIT_REF_COUNT +#undef AVRC_FREE diff --git a/tests/ref/fate/source b/tests/ref/fate/source index c64bc05241..32954fd828 100644 --- a/tests/ref/fate/source +++ b/tests/ref/fate/source @@ -20,5 +20,6 @@ Headers without standard inclusion guards: compat/djgpp/math.h compat/float/float.h compat/float/limits.h +libavutil/avrefcount_template.h Use of av_clip() where av_clip_uintp2() could be used: Use of av_clip() where av_clip_intp2() could be used:
Signed-off-by: Nicolas George <george@nsup.org> --- libavutil/avrefcount_template.h | 140 ++++++++++++++++++++++++++++++++ tests/ref/fate/source | 1 + 2 files changed, 141 insertions(+) create mode 100644 libavutil/avrefcount_template.h I will need to refcount something soon. Recently, the need to stop abusing AVBuffer for all refcounting was mentioned on the list. So here is an attempt at isolating the refcounting itself. This is not the final verion, I will first work on the "something" to make sure it suits the needs. But it is a first version. Anton, I would appreciate if you had a look at this and told me if there is something you strongly dislike about before I have piled too much efforts over it.