| 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 | expand |
| 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 |
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,
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
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,
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". >
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.
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
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,
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 --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)
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavfilter/vsrc_testsrc.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)