diff mbox series

[FFmpeg-devel,1/3] lavu: add a template for refcounted objects.

Message ID 20200627151646.2593306-1-george@nsup.org
State New
Headers show
Series [FFmpeg-devel,1/3] lavu: add a template for refcounted objects.
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Nicolas George June 27, 2020, 3:16 p.m. UTC
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.

Comments

Anton Khirnov July 1, 2020, 3:05 p.m. UTC | #1
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.
Nicolas George July 1, 2020, 3:13 p.m. UTC | #2
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,
Anton Khirnov July 1, 2020, 4:11 p.m. UTC | #3
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.
Nicolas George July 1, 2020, 4:47 p.m. UTC | #4
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,
James Almer July 1, 2020, 6:20 p.m. UTC | #5
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.
Anton Khirnov July 1, 2020, 6:24 p.m. UTC | #6
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.
Nicolas George July 1, 2020, 6:28 p.m. UTC | #7
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,
Nicolas George July 1, 2020, 6:36 p.m. UTC | #8
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,
Lynne July 1, 2020, 6:41 p.m. UTC | #9
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.
Anton Khirnov July 3, 2020, 7:28 a.m. UTC | #10
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?
diff mbox series

Patch

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: