Re: [FFmpeg-devel] [PATCH 1/3] lavu: add a template for refcounted objects.
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
> > ---
> > 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
___
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".
Re: [FFmpeg-devel] [PATCH 1/3] lavu: add a template for refcounted objects.
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
> >> ---
> >> 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?
--
Anton Khirnov
___
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".
Re: [FFmpeg-devel] [PATCH 1/3] lavu: add a template for refcounted objects.
Jul 1, 2020, 19:20 by jamr...@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
>>> ---
>>> 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.
___
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".
Re: [FFmpeg-devel] [PATCH 1/3] lavu: add a template for refcounted objects.
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, -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH 1/3] lavu: add a template for refcounted objects.
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 signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH 1/3] lavu: add a template for refcounted objects.
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. -- Anton Khirnov ___ 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".
Re: [FFmpeg-devel] [PATCH 1/3] lavu: add a template for refcounted objects.
On 7/1/2020 12:05 PM, Anton Khirnov wrote:
> Quoting Nicolas George (2020-06-27 17:16:44)
>> Signed-off-by: Nicolas George
>> ---
>> 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.
___
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".
Re: [FFmpeg-devel] [PATCH 1/3] lavu: add a template for refcounted objects.
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, -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH 1/3] lavu: add a template for refcounted objects.
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
___
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".
Re: [FFmpeg-devel] [PATCH 1/3] lavu: add a template for refcounted objects.
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,
--
Nicolas George
signature.asc
Description: PGP signature
___
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".
Re: [FFmpeg-devel] [PATCH 1/3] lavu: add a template for refcounted objects.
Quoting Nicolas George (2020-06-27 17:16:44)
> Signed-off-by: Nicolas George
> ---
> 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
___
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".
