diff mbox series

[FFmpeg-devel,16/16] avfilter/vsrc_testsrc: Deduplicate options

Message ID 20210104002816.2321974-16-andreas.rheinhardt@gmail.com
State Accepted
Commit 482aeda8bfe641239c901261a646ab07275da50b
Headers show
Series [FFmpeg-devel,01/16] avcodec/g723_1: Deduplicate arrays
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Andreas Rheinhardt Jan. 4, 2021, 12:28 a.m. UTC
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavfilter/vsrc_testsrc.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

Comments

Nicolas George Jan. 4, 2021, 7:16 p.m. UTC | #1
Hi.

Andreas Rheinhardt (12021-01-04):
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavfilter/vsrc_testsrc.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)

IIRC, options tables cannot be de-duplicated, because of some
enumerating code. Or am I confusing with something else?

Regards,
Andreas Rheinhardt Jan. 4, 2021, 7:22 p.m. UTC | #2
Nicolas George:
> Hi.
> 
> Andreas Rheinhardt (12021-01-04):
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>> ---
>>  libavfilter/vsrc_testsrc.c | 15 +++++----------
>>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> IIRC, options tables cannot be de-duplicated, because of some
> enumerating code. Or am I confusing with something else?
> 

What enumerating code? It is actually commonplace that options are
shared (you can find examples in libavfilter by using 'git grep -e
define --and -e options'); pointing into other options and thereby
reusing only a part of other options is not common, but I don't really
see why it shouldn't work.

- Andreas
Nicolas George Jan. 4, 2021, 7:29 p.m. UTC | #3
Andreas Rheinhardt (12021-01-04):
> What enumerating code? It is actually commonplace that options are
> shared (you can find examples in libavfilter by using 'git grep -e
> define --and -e options'); pointing into other options and thereby
> reusing only a part of other options is not common, but I don't really
> see why it shouldn't work.

Using #define to de-duplicate the source is fine, of course.

But IIRC pointing to the same array, i.e. de-duplicating in the binary,
will lead to the code that enumerate options to loop in some way. I do
not remember the details, and I cannot find a commit that talks about
it, sorry. Maybe somebody here remembers better?

Regards,
James Almer Jan. 4, 2021, 7:35 p.m. UTC | #4
On 1/4/2021 4:29 PM, Nicolas George wrote:
> Andreas Rheinhardt (12021-01-04):
>> What enumerating code? It is actually commonplace that options are
>> shared (you can find examples in libavfilter by using 'git grep -e
>> define --and -e options'); pointing into other options and thereby
>> reusing only a part of other options is not common, but I don't really
>> see why it shouldn't work.
> 
> Using #define to de-duplicate the source is fine, of course.
> 
> But IIRC pointing to the same array, i.e. de-duplicating in the binary,
> will lead to the code that enumerate options to loop in some way. I do
> not remember the details, and I cannot find a commit that talks about
> it, sorry. Maybe somebody here remembers better?
> 
> Regards,

I recall issues with modules sharing a common AVClass, but different 
AVClasses sharing a common array of AVOptions seems to be ok.
See rawdec.h in libavformat, and how ff_raw_options is used in several 
different demuxer AVClasses.

> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Andreas Rheinhardt Jan. 4, 2021, 7:39 p.m. UTC | #5
Nicolas George:
> Andreas Rheinhardt (12021-01-04):
>> What enumerating code? It is actually commonplace that options are
>> shared (you can find examples in libavfilter by using 'git grep -e
>> define --and -e options'); pointing into other options and thereby
>> reusing only a part of other options is not common, but I don't really
>> see why it shouldn't work.
> 
> Using #define to de-duplicate the source is fine, of course.
> 
> But IIRC pointing to the same array, i.e. de-duplicating in the binary,
> will lead to the code that enumerate options to loop in some way. I do
> not remember the details, and I cannot find a commit that talks about
> it, sorry. Maybe somebody here remembers better?
> 
> Regards,
> 
The AVOpt API simply deals with arrays of AVOptions that are terminated
by a { NULL } entry. A pointer that points into such an array still
points into a { NULL } terminated list of options. It is really the same
as with ordinary strings*. Using ffmpeg -h filter=allyuv still shows the
same options.
(Is it possible that you are confusing this with AVFilterPads? Several
of the AVFilterPad arrays are also the same and I have not yet checked
whether they can be safely deduplicated or not.)

- Andreas

*: With the difference that the compiler is not allowed to deduplicate
on its own whereas this is legal for string literals.
Andreas Rheinhardt Jan. 4, 2021, 7:46 p.m. UTC | #6
Nicolas George:
> Andreas Rheinhardt (12021-01-04):
>> What enumerating code? It is actually commonplace that options are
>> shared (you can find examples in libavfilter by using 'git grep -e
>> define --and -e options'); pointing into other options and thereby
>> reusing only a part of other options is not common, but I don't really
>> see why it shouldn't work.
> 
> Using #define to de-duplicate the source is fine, of course.
> 
The examples that this command shows you do not only de-duplicate in the
source (like it is done here in vsrc_testsrc.c), but really in the
binary. And it works (for it not to work the AVOpt API would need to be
modified to e.g. store the addresses of all the AVOption arrays that it
has received so far and compare whether a AVOption * actually points
into an array already known; and of course it would have to error
out/abort for it to matter).

> But IIRC pointing to the same array, i.e. de-duplicating in the binary,
> will lead to the code that enumerate options to loop in some way. I do
> not remember the details, and I cannot find a commit that talks about
> it, sorry. Maybe somebody here remembers better?

- Andreas
Nicolas George Jan. 4, 2021, 8:34 p.m. UTC | #7
Andreas Rheinhardt (12021-01-04):
> The examples that this command shows you do not only de-duplicate in the
> source (like it is done here in vsrc_testsrc.c), but really in the
> binary. And it works (for it not to work the AVOpt API would need to be
> modified to e.g. store the addresses of all the AVOption arrays that it
> has received so far and compare whether a AVOption * actually points
> into an array already known; and of course it would have to error
> out/abort for it to matter).

It seems I speaking of another issue. Thanks James for clarifying.

No objection, then. Sorry.

Regards,
Anton Khirnov Jan. 17, 2021, 3:29 p.m. UTC | #8
Quoting James Almer (2021-01-04 20:35:30)
> On 1/4/2021 4:29 PM, Nicolas George wrote:
> > Andreas Rheinhardt (12021-01-04):
> >> What enumerating code? It is actually commonplace that options are
> >> shared (you can find examples in libavfilter by using 'git grep -e
> >> define --and -e options'); pointing into other options and thereby
> >> reusing only a part of other options is not common, but I don't really
> >> see why it shouldn't work.
> > 
> > Using #define to de-duplicate the source is fine, of course.
> > 
> > But IIRC pointing to the same array, i.e. de-duplicating in the binary,
> > will lead to the code that enumerate options to loop in some way. I do
> > not remember the details, and I cannot find a commit that talks about
> > it, sorry. Maybe somebody here remembers better?
> > 
> > Regards,
> 
> I recall issues with modules sharing a common AVClass, but different 
> AVClasses sharing a common array of AVOptions seems to be ok.
> See rawdec.h in libavformat, and how ff_raw_options is used in several 
> different demuxer AVClasses.

child_class_next() implementation for AVFormatContext and others assumes
a one-to-one mapping between objects and their AVClasses. But that is
now deprecated and there should be no issues when it's removed.
diff mbox series

Patch

diff --git a/libavfilter/vsrc_testsrc.c b/libavfilter/vsrc_testsrc.c
index cf9fa4b2b2..7001f9ba16 100644
--- a/libavfilter/vsrc_testsrc.c
+++ b/libavfilter/vsrc_testsrc.c
@@ -99,6 +99,9 @@  typedef struct TestSourceContext {
 
 #define COMMON_OPTIONS SIZE_OPTIONS COMMON_OPTIONS_NOSIZE
 
+#define NOSIZE_OPTIONS_OFFSET 2
+/* Filters using COMMON_OPTIONS_NOSIZE also use the following options
+ * via &options[NOSIZE_OPTIONS_OFFSET]. So don't break it. */
 static const AVOption options[] = {
     COMMON_OPTIONS
     { NULL }
@@ -1653,11 +1656,7 @@  AVFilter ff_vsrc_smptehdbars = {
 
 #if CONFIG_ALLYUV_FILTER
 
-static const AVOption allyuv_options[] = {
-    COMMON_OPTIONS_NOSIZE
-    { NULL }
-};
-
+#define allyuv_options &options[NOSIZE_OPTIONS_OFFSET]
 AVFILTER_DEFINE_CLASS(allyuv);
 
 static void allyuv_fill_picture(AVFilterContext *ctx, AVFrame *frame)
@@ -1734,11 +1733,7 @@  AVFilter ff_vsrc_allyuv = {
 
 #if CONFIG_ALLRGB_FILTER
 
-static const AVOption allrgb_options[] = {
-    COMMON_OPTIONS_NOSIZE
-    { NULL }
-};
-
+#define allrgb_options &options[NOSIZE_OPTIONS_OFFSET]
 AVFILTER_DEFINE_CLASS(allrgb);
 
 static void allrgb_fill_picture(AVFilterContext *ctx, AVFrame *frame)