| Message ID | 20241014113647.9711-3-anton@khirnov.net |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/4] fftools/ffmpeg_demux: use proper logging contexts everywhere | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
On Mon, Oct 14, 2024 at 01:36:46PM +0200, Anton Khirnov wrote: > --- > fftools/opt_common.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/fftools/opt_common.c b/fftools/opt_common.c > index 021ed75272..34da2cee7d 100644 > --- a/fftools/opt_common.c > +++ b/fftools/opt_common.c > @@ -808,7 +808,6 @@ int show_filters(void *optctx, const char *opt, const char *arg) > printf("Filters:\n" > " T.. = Timeline support\n" > " .S. = Slice threading\n" > - " ..C = Command support\n" > " A = Audio input/output\n" > " V = Video input/output\n" > " N = Dynamic number and/or type of input/output\n" > @@ -833,10 +832,9 @@ int show_filters(void *optctx, const char *opt, const char *arg) > ( i && (filter->flags & AVFILTER_FLAG_DYNAMIC_OUTPUTS))) ? 'N' : '|'; > } > *descr_cur = 0; > - printf(" %c%c%c %-17s %-10s %s\n", > + printf(" %c%c %-17s %-10s %s\n", > filter->flags & AVFILTER_FLAG_SUPPORT_TIMELINE ? 'T' : '.', > filter->flags & AVFILTER_FLAG_SLICE_THREADS ? 'S' : '.', > - filter->process_command ? 'C' : '.', > filter->name, descr, filter->description); > } > #else The commit message is not describing this change accurately Its not just "not accessing a priavte field", it removes information from the printed filter list Thx [...]
On 2024-10-14 17:52 +0200, Michael Niedermayer wrote: > On Mon, Oct 14, 2024 at 01:36:46PM +0200, Anton Khirnov wrote: > > --- > > fftools/opt_common.c | 4 +--- > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > diff --git a/fftools/opt_common.c b/fftools/opt_common.c > > index 021ed75272..34da2cee7d 100644 > > --- a/fftools/opt_common.c > > +++ b/fftools/opt_common.c > > @@ -808,7 +808,6 @@ int show_filters(void *optctx, const char *opt, const char *arg) > > printf("Filters:\n" > > " T.. = Timeline support\n" > > " .S. = Slice threading\n" > > - " ..C = Command support\n" > > " A = Audio input/output\n" > > " V = Video input/output\n" > > " N = Dynamic number and/or type of input/output\n" > > @@ -833,10 +832,9 @@ int show_filters(void *optctx, const char *opt, const char *arg) > > ( i && (filter->flags & AVFILTER_FLAG_DYNAMIC_OUTPUTS))) ? 'N' : '|'; > > } > > *descr_cur = 0; > > - printf(" %c%c%c %-17s %-10s %s\n", > > + printf(" %c%c %-17s %-10s %s\n", > > filter->flags & AVFILTER_FLAG_SUPPORT_TIMELINE ? 'T' : '.', > > filter->flags & AVFILTER_FLAG_SLICE_THREADS ? 'S' : '.', > > - filter->process_command ? 'C' : '.', > > filter->name, descr, filter->description); > > } > > #else > > The commit message is not describing this change accurately > > Its not just "not accessing a priavte field", it removes information > from the printed filter list Learning question: How can we see this is a private field? This information was useful. How can we bring it back if we decide it's not OK too test if this field is NULL? Alexander
Alexander Strasser via ffmpeg-devel (12024-10-14):
> Learning question: How can we see this is a private field?
I suppose “revert everything and start again properly designing things
without loss of feature” will not be appreciated?
Regards,
Quoting Alexander Strasser via ffmpeg-devel (2024-10-14 18:27:24) > On 2024-10-14 17:52 +0200, Michael Niedermayer wrote: > > On Mon, Oct 14, 2024 at 01:36:46PM +0200, Anton Khirnov wrote: > > > --- > > > fftools/opt_common.c | 4 +--- > > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > > > diff --git a/fftools/opt_common.c b/fftools/opt_common.c > > > index 021ed75272..34da2cee7d 100644 > > > --- a/fftools/opt_common.c > > > +++ b/fftools/opt_common.c > > > @@ -808,7 +808,6 @@ int show_filters(void *optctx, const char *opt, const char *arg) > > > printf("Filters:\n" > > > " T.. = Timeline support\n" > > > " .S. = Slice threading\n" > > > - " ..C = Command support\n" > > > " A = Audio input/output\n" > > > " V = Video input/output\n" > > > " N = Dynamic number and/or type of input/output\n" > > > @@ -833,10 +832,9 @@ int show_filters(void *optctx, const char *opt, const char *arg) > > > ( i && (filter->flags & AVFILTER_FLAG_DYNAMIC_OUTPUTS))) ? 'N' : '|'; > > > } > > > *descr_cur = 0; > > > - printf(" %c%c%c %-17s %-10s %s\n", > > > + printf(" %c%c %-17s %-10s %s\n", > > > filter->flags & AVFILTER_FLAG_SUPPORT_TIMELINE ? 'T' : '.', > > > filter->flags & AVFILTER_FLAG_SLICE_THREADS ? 'S' : '.', > > > - filter->process_command ? 'C' : '.', > > > filter->name, descr, filter->description); > > > } > > > #else > > > > The commit message is not describing this change accurately > > > > Its not just "not accessing a priavte field", it removes information > > from the printed filter list > > Learning question: How can we see this is a private field? It is documented in the header.
On 2024-10-14 19:22 +0200, Anton Khirnov wrote: > Quoting Alexander Strasser via ffmpeg-devel (2024-10-14 18:27:24) > > On 2024-10-14 17:52 +0200, Michael Niedermayer wrote: > > > On Mon, Oct 14, 2024 at 01:36:46PM +0200, Anton Khirnov wrote: > > > > --- > > > > fftools/opt_common.c | 4 +--- > > > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > > > > > diff --git a/fftools/opt_common.c b/fftools/opt_common.c > > > > index 021ed75272..34da2cee7d 100644 > > > > --- a/fftools/opt_common.c > > > > +++ b/fftools/opt_common.c > > > > @@ -808,7 +808,6 @@ int show_filters(void *optctx, const char *opt, const char *arg) > > > > printf("Filters:\n" > > > > " T.. = Timeline support\n" > > > > " .S. = Slice threading\n" > > > > - " ..C = Command support\n" > > > > " A = Audio input/output\n" > > > > " V = Video input/output\n" > > > > " N = Dynamic number and/or type of input/output\n" > > > > @@ -833,10 +832,9 @@ int show_filters(void *optctx, const char *opt, const char *arg) > > > > ( i && (filter->flags & AVFILTER_FLAG_DYNAMIC_OUTPUTS))) ? 'N' : '|'; > > > > } > > > > *descr_cur = 0; > > > > - printf(" %c%c%c %-17s %-10s %s\n", > > > > + printf(" %c%c %-17s %-10s %s\n", > > > > filter->flags & AVFILTER_FLAG_SUPPORT_TIMELINE ? 'T' : '.', > > > > filter->flags & AVFILTER_FLAG_SLICE_THREADS ? 'S' : '.', > > > > - filter->process_command ? 'C' : '.', > > > > filter->name, descr, filter->description); > > > > } > > > > #else > > > > > > The commit message is not describing this change accurately > > > > > > Its not just "not accessing a priavte field", it removes information > > > from the printed filter list > > > > Learning question: How can we see this is a private field? > > It is documented in the header. I figured as much but couldn't find a hint in in avfilter.h You changed it in a previous patch of this series or am I reading it wrong? Alexander
On 10/14/2024 1:36 PM, Nicolas George wrote: > Alexander Strasser via ffmpeg-devel (12024-10-14): >> Learning question: How can we see this is a private field? > > I suppose “revert everything and start again properly designing things > without loss of feature” will not be appreciated? No, it wont. Also, this field is effectively documented as not being part of the public API in avfilter.h, so an alternative is needed.
Quoting Alexander Strasser via ffmpeg-devel (2024-10-14 22:21:38) > > It is documented in the header. > > I figured as much but couldn't find a hint in in avfilter.h > > You changed it in a previous patch of this series or am I reading it wrong? I don't follow, I changed what?
On 10/14/2024 1:27 PM, Alexander Strasser via ffmpeg-devel wrote: > On 2024-10-14 17:52 +0200, Michael Niedermayer wrote: >> On Mon, Oct 14, 2024 at 01:36:46PM +0200, Anton Khirnov wrote: >>> --- >>> fftools/opt_common.c | 4 +--- >>> 1 file changed, 1 insertion(+), 3 deletions(-) >>> >>> diff --git a/fftools/opt_common.c b/fftools/opt_common.c >>> index 021ed75272..34da2cee7d 100644 >>> --- a/fftools/opt_common.c >>> +++ b/fftools/opt_common.c >>> @@ -808,7 +808,6 @@ int show_filters(void *optctx, const char *opt, const char *arg) >>> printf("Filters:\n" >>> " T.. = Timeline support\n" >>> " .S. = Slice threading\n" >>> - " ..C = Command support\n" >>> " A = Audio input/output\n" >>> " V = Video input/output\n" >>> " N = Dynamic number and/or type of input/output\n" >>> @@ -833,10 +832,9 @@ int show_filters(void *optctx, const char *opt, const char *arg) >>> ( i && (filter->flags & AVFILTER_FLAG_DYNAMIC_OUTPUTS))) ? 'N' : '|'; >>> } >>> *descr_cur = 0; >>> - printf(" %c%c%c %-17s %-10s %s\n", >>> + printf(" %c%c %-17s %-10s %s\n", >>> filter->flags & AVFILTER_FLAG_SUPPORT_TIMELINE ? 'T' : '.', >>> filter->flags & AVFILTER_FLAG_SLICE_THREADS ? 'S' : '.', >>> - filter->process_command ? 'C' : '.', >>> filter->name, descr, filter->description); >>> } >>> #else >> >> The commit message is not describing this change accurately >> >> Its not just "not accessing a priavte field", it removes information >> from the printed filter list > > Learning question: How can we see this is a private field? > > This information was useful. How can we bring it back if we decide it's > not OK too test if this field is NULL? A new flag AVFILTER_FLAG_SUPPORT_COMMANDS that the user can check to ensure a call to avfilter_process_command() will not return ENOSYS could be added. And of course, to print the C in here.
On 2024-10-14 22:35 +0200, Anton Khirnov wrote: > Quoting Alexander Strasser via ffmpeg-devel (2024-10-14 22:21:38) > > > It is documented in the header. > > > > I figured as much but couldn't find a hint in in avfilter.h > > > > You changed it in a previous patch of this series or am I reading it wrong? > > I don't follow, I changed what? No, I don't mean you did sth. I'm just too stupid to read where in avfilter.h it is documented to be private. So I'm asking what is written where that indicates this. Alexander
James Almer (12024-10-14): > A new flag AVFILTER_FLAG_SUPPORT_COMMANDS that the user can check to ensure > a call to avfilter_process_command() will not return ENOSYS could be added. So you mean to have the same information twice in the source code of the filter: once as the process_command callback, once as the flag. Having the same information twice in the source code is a recipe for bugs: unavoidably somebody will update one and not the other. The correct way of doing this would be to add a function in libavfilter.
Quoting Alexander Strasser via ffmpeg-devel (2024-10-15 07:34:26) > On 2024-10-14 22:35 +0200, Anton Khirnov wrote: > > Quoting Alexander Strasser via ffmpeg-devel (2024-10-14 22:21:38) > > > > It is documented in the header. > > > > > > I figured as much but couldn't find a hint in in avfilter.h > > > > > > You changed it in a previous patch of this series or am I reading it wrong? > > > > I don't follow, I changed what? > > No, I don't mean you did sth. > > I'm just too stupid to read where in avfilter.h it is documented > to be private. > > So I'm asking what is written where that indicates this. Here: http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavfilter/avfilter.h;h=b88b31a834094f15d3159b017438499f1809b82f;hb=HEAD#l247 Of course experience shows that such warnings do not really work, as users ignore or fail to notice them. Which is why we have been switching to an approach that outright removes private information from public headers, but that first requires to eliminate its use.
Anton Khirnov (12024-10-15):
> headers, but that first requires to eliminate its use.
Not an excuse to remove the feature it was used for at the same time,
especially without warning about it.
On 15 Oct 2024, at 10:30, Anton Khirnov wrote: > Quoting Alexander Strasser via ffmpeg-devel (2024-10-15 07:34:26) >> On 2024-10-14 22:35 +0200, Anton Khirnov wrote: >>> Quoting Alexander Strasser via ffmpeg-devel (2024-10-14 22:21:38) >>>>> It is documented in the header. >>>> >>>> I figured as much but couldn't find a hint in in avfilter.h >>>> >>>> You changed it in a previous patch of this series or am I reading it wrong? >>> >>> I don't follow, I changed what? >> >> No, I don't mean you did sth. >> >> I'm just too stupid to read where in avfilter.h it is documented >> to be private. >> >> So I'm asking what is written where that indicates this. > > Here: > http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavfilter/avfilter.h;h=b88b31a834094f15d3159b017438499f1809b82f;hb=HEAD#l247 > > Of course experience shows that such warnings do not really work, as > users ignore or fail to notice them. Yes because approaches like „everything below this line“ falls apart horribly in Doxygen as it is completely lost there. Same for IntelliSense-like things in editors. > Which is why we have been switching > to an approach that outright removes private information from public > headers, but that first requires to eliminate its use. > > -- > 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-request@ffmpeg.org with subject "unsubscribe".
Quoting James Almer (2024-10-14 22:48:12) > On 10/14/2024 1:27 PM, Alexander Strasser via ffmpeg-devel wrote: > > On 2024-10-14 17:52 +0200, Michael Niedermayer wrote: > >> On Mon, Oct 14, 2024 at 01:36:46PM +0200, Anton Khirnov wrote: > >>> --- > >>> fftools/opt_common.c | 4 +--- > >>> 1 file changed, 1 insertion(+), 3 deletions(-) > >>> > >>> diff --git a/fftools/opt_common.c b/fftools/opt_common.c > >>> index 021ed75272..34da2cee7d 100644 > >>> --- a/fftools/opt_common.c > >>> +++ b/fftools/opt_common.c > >>> @@ -808,7 +808,6 @@ int show_filters(void *optctx, const char *opt, const char *arg) > >>> printf("Filters:\n" > >>> " T.. = Timeline support\n" > >>> " .S. = Slice threading\n" > >>> - " ..C = Command support\n" > >>> " A = Audio input/output\n" > >>> " V = Video input/output\n" > >>> " N = Dynamic number and/or type of input/output\n" > >>> @@ -833,10 +832,9 @@ int show_filters(void *optctx, const char *opt, const char *arg) > >>> ( i && (filter->flags & AVFILTER_FLAG_DYNAMIC_OUTPUTS))) ? 'N' : '|'; > >>> } > >>> *descr_cur = 0; > >>> - printf(" %c%c%c %-17s %-10s %s\n", > >>> + printf(" %c%c %-17s %-10s %s\n", > >>> filter->flags & AVFILTER_FLAG_SUPPORT_TIMELINE ? 'T' : '.', > >>> filter->flags & AVFILTER_FLAG_SLICE_THREADS ? 'S' : '.', > >>> - filter->process_command ? 'C' : '.', > >>> filter->name, descr, filter->description); > >>> } > >>> #else > >> > >> The commit message is not describing this change accurately > >> > >> Its not just "not accessing a priavte field", it removes information > >> from the printed filter list > > > > Learning question: How can we see this is a private field? > > > > This information was useful. How can we bring it back if we decide it's > > not OK too test if this field is NULL? > > A new flag AVFILTER_FLAG_SUPPORT_COMMANDS that the user can check to > ensure a call to avfilter_process_command() will not return ENOSYS could > be added. And of course, to print the C in here. avfilter_process_command() may or may not return ENOSYS whether that flag is set or not, so I don't see what exactly would it be useful for. Same holds for printing the capability in fftools - just what is the user expected to do with that information?
On 10/15/2024 6:57 AM, Anton Khirnov wrote: > Quoting James Almer (2024-10-14 22:48:12) >> On 10/14/2024 1:27 PM, Alexander Strasser via ffmpeg-devel wrote: >>> On 2024-10-14 17:52 +0200, Michael Niedermayer wrote: >>>> On Mon, Oct 14, 2024 at 01:36:46PM +0200, Anton Khirnov wrote: >>>>> --- >>>>> fftools/opt_common.c | 4 +--- >>>>> 1 file changed, 1 insertion(+), 3 deletions(-) >>>>> >>>>> diff --git a/fftools/opt_common.c b/fftools/opt_common.c >>>>> index 021ed75272..34da2cee7d 100644 >>>>> --- a/fftools/opt_common.c >>>>> +++ b/fftools/opt_common.c >>>>> @@ -808,7 +808,6 @@ int show_filters(void *optctx, const char *opt, const char *arg) >>>>> printf("Filters:\n" >>>>> " T.. = Timeline support\n" >>>>> " .S. = Slice threading\n" >>>>> - " ..C = Command support\n" >>>>> " A = Audio input/output\n" >>>>> " V = Video input/output\n" >>>>> " N = Dynamic number and/or type of input/output\n" >>>>> @@ -833,10 +832,9 @@ int show_filters(void *optctx, const char *opt, const char *arg) >>>>> ( i && (filter->flags & AVFILTER_FLAG_DYNAMIC_OUTPUTS))) ? 'N' : '|'; >>>>> } >>>>> *descr_cur = 0; >>>>> - printf(" %c%c%c %-17s %-10s %s\n", >>>>> + printf(" %c%c %-17s %-10s %s\n", >>>>> filter->flags & AVFILTER_FLAG_SUPPORT_TIMELINE ? 'T' : '.', >>>>> filter->flags & AVFILTER_FLAG_SLICE_THREADS ? 'S' : '.', >>>>> - filter->process_command ? 'C' : '.', >>>>> filter->name, descr, filter->description); >>>>> } >>>>> #else >>>> >>>> The commit message is not describing this change accurately >>>> >>>> Its not just "not accessing a priavte field", it removes information >>>> from the printed filter list >>> >>> Learning question: How can we see this is a private field? >>> >>> This information was useful. How can we bring it back if we decide it's >>> not OK too test if this field is NULL? >> >> A new flag AVFILTER_FLAG_SUPPORT_COMMANDS that the user can check to >> ensure a call to avfilter_process_command() will not return ENOSYS could >> be added. And of course, to print the C in here. > > avfilter_process_command() may or may not return ENOSYS whether that > flag is set or not, so I don't see what exactly would it be useful for. I see, although I wouldn't expect ENOSYS for any other case than "unsupported". If a command fails, imo it would be EINVAL, or maybe ENOMEM. > Same holds for printing the capability in fftools - just what is the > user expected to do with that information? Know they can use the send command interrupt for a given filter without having it unconditionally fail, i guess. But for that matter, is the list of supported commands available anywhere for the user? Is there an API to query them, or is it expected to be only in the documentation? If there's no API, then maybe printing this is pointless.
Quoting James Almer (2024-10-15 14:54:08) > On 10/15/2024 6:57 AM, Anton Khirnov wrote: > > avfilter_process_command() may or may not return ENOSYS whether that > > flag is set or not, so I don't see what exactly would it be useful for. > > I see, although I wouldn't expect ENOSYS for any other case than > "unsupported". If a command fails, imo it would be EINVAL, or maybe ENOMEM. Correct, but * some commands work on all filters; for now it's just "ping", but we can add more in the future; * the "enable" command works on all filters flagged with AVFILTER_FLAG_SUPPORT_TIMELINE; again, we may add more such "semi-generic" commands in the future; * filters that do implement the process_command() callback will still return ENOSYS when you send them a command they do not support. So checking for the existence of process_command() does not really tell you anything specific, and the same would hold for the hypothetical flag that would replace it. > > > Same holds for printing the capability in fftools - just what is the > > user expected to do with that information? > > Know they can use the send command interrupt for a given filter without > having it unconditionally fail, i guess. But for that matter, is the > list of supported commands available anywhere for the user? Is there an > API to query them, or is it expected to be only in the documentation? > If there's no API, then maybe printing this is pointless. There is no such API, which I agree would be useful - but if it did exist we would again not need the flag.
On 2024-10-15 10:37 +0200, epirat07@gmail.com wrote: > > > On 15 Oct 2024, at 10:30, Anton Khirnov wrote: > > > Quoting Alexander Strasser via ffmpeg-devel (2024-10-15 07:34:26) > >> On 2024-10-14 22:35 +0200, Anton Khirnov wrote: > >>> Quoting Alexander Strasser via ffmpeg-devel (2024-10-14 22:21:38) > >>>>> It is documented in the header. > >>>> > >>>> I figured as much but couldn't find a hint in in avfilter.h > >>>> > >>>> You changed it in a previous patch of this series or am I reading it wrong? > >>> > >>> I don't follow, I changed what? > >> > >> No, I don't mean you did sth. > >> > >> I'm just too stupid to read where in avfilter.h it is documented > >> to be private. > >> > >> So I'm asking what is written where that indicates this. > > > > Here: > > http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavfilter/avfilter.h;h=b88b31a834094f15d3159b017438499f1809b82f;hb=HEAD#l247 > > > > Of course experience shows that such warnings do not really work, as > > users ignore or fail to notice them. > > Yes because approaches like „everything below this line“ > falls apart horribly in Doxygen as it is completely lost there. > > Same for IntelliSense-like things in editors. Thanks Anton for pointing to the exact line. Now it all makes sense. I was looking at the header and the AVFilter struct multiple times. First at the beginning of the struct, then searching and later out of paranoia paging up and down. The thing is, I didn't expect nor see that comment as it was just somewhere in between and looking like documentation of some other field. Then I went to the HTML rendered by Doxygen and like epirat07 pointed out, of course couldn't find any hint either... > > Which is why we have been switching > > to an approach that outright removes private information from public > > headers, but that first requires to eliminate its use. Sure, I agree to that direction. Still that functionality is useful and while it can be improved I would hope for a solution that doesn't drop the feature now and hopes for it to comeback at some later point. Alexander
On 2024-10-15 15:09 +0200, Anton Khirnov wrote: > Quoting James Almer (2024-10-15 14:54:08) > > On 10/15/2024 6:57 AM, Anton Khirnov wrote: > > > avfilter_process_command() may or may not return ENOSYS whether that > > > flag is set or not, so I don't see what exactly would it be useful for. > > > > I see, although I wouldn't expect ENOSYS for any other case than > > "unsupported". If a command fails, imo it would be EINVAL, or maybe ENOMEM. > > Correct, but > * some commands work on all filters; for now it's just "ping", but we > can add more in the future; > * the "enable" command works on all filters flagged with > AVFILTER_FLAG_SUPPORT_TIMELINE; again, we may add more such > "semi-generic" commands in the future; > * filters that do implement the process_command() callback will still > return ENOSYS when you send them a command they do not support. > > So checking for the existence of process_command() does not really tell > you anything specific, and the same would hold for the hypothetical flag > that would replace it. > > > > > > Same holds for printing the capability in fftools - just what is the > > > user expected to do with that information? > > > > Know they can use the send command interrupt for a given filter without > > having it unconditionally fail, i guess. But for that matter, is the > > list of supported commands available anywhere for the user? Is there an > > API to query them, or is it expected to be only in the documentation? > > If there's no API, then maybe printing this is pointless. > > There is no such API, which I agree would be useful - but if it did > exist we would again not need the flag. For me that flag is more about if some filter brings commands of its own. Generic stuff that we will add should not be indicated with the C flag. Though adding more flags for the generic stuff like we have with T now should be fine for some time to come before it gets unwieldy. Listing and querying of filter caps has much room for improvement, but as long as no one has particular interest in doing that, at least I don't see it as very important right now. Alexander
Quoting Alexander Strasser via ffmpeg-devel (2024-10-15 21:05:54)
> Still that functionality is useful
How is it useful? It gives you no actionable information.
Quoting Alexander Strasser via ffmpeg-devel (2024-10-15 21:16:28) > On 2024-10-15 15:09 +0200, Anton Khirnov wrote: > > Quoting James Almer (2024-10-15 14:54:08) > > > On 10/15/2024 6:57 AM, Anton Khirnov wrote: > > > > avfilter_process_command() may or may not return ENOSYS whether that > > > > flag is set or not, so I don't see what exactly would it be useful for. > > > > > > I see, although I wouldn't expect ENOSYS for any other case than > > > "unsupported". If a command fails, imo it would be EINVAL, or maybe ENOMEM. > > > > Correct, but > > * some commands work on all filters; for now it's just "ping", but we > > can add more in the future; > > * the "enable" command works on all filters flagged with > > AVFILTER_FLAG_SUPPORT_TIMELINE; again, we may add more such > > "semi-generic" commands in the future; > > * filters that do implement the process_command() callback will still > > return ENOSYS when you send them a command they do not support. > > > > So checking for the existence of process_command() does not really tell > > you anything specific, and the same would hold for the hypothetical flag > > that would replace it. > > > > > > > > > Same holds for printing the capability in fftools - just what is the > > > > user expected to do with that information? > > > > > > Know they can use the send command interrupt for a given filter without > > > having it unconditionally fail, i guess. But for that matter, is the > > > list of supported commands available anywhere for the user? Is there an > > > API to query them, or is it expected to be only in the documentation? > > > If there's no API, then maybe printing this is pointless. > > > > There is no such API, which I agree would be useful - but if it did > > exist we would again not need the flag. > > For me that flag is more about if some filter brings commands of its own. Most filters implementing the process_command() callback just use the generic implementation - ff_filter_process_command(). So what information does that flag actually give you?
Anton Khirnov (12024-10-15): > Most filters implementing the process_command() callback just use > the generic implementation - ff_filter_process_command(). So what > information does that flag actually give you? It gives the information that the filter is ready to deal with what ff_filter_process_command() does, i.e. change the value of options on the fly. Note: commands are not a part I am familiar with, I had to check and it took me less than 30 seconds. You could have done the same.
diff --git a/fftools/opt_common.c b/fftools/opt_common.c index 021ed75272..34da2cee7d 100644 --- a/fftools/opt_common.c +++ b/fftools/opt_common.c @@ -808,7 +808,6 @@ int show_filters(void *optctx, const char *opt, const char *arg) printf("Filters:\n" " T.. = Timeline support\n" " .S. = Slice threading\n" - " ..C = Command support\n" " A = Audio input/output\n" " V = Video input/output\n" " N = Dynamic number and/or type of input/output\n" @@ -833,10 +832,9 @@ int show_filters(void *optctx, const char *opt, const char *arg) ( i && (filter->flags & AVFILTER_FLAG_DYNAMIC_OUTPUTS))) ? 'N' : '|'; } *descr_cur = 0; - printf(" %c%c%c %-17s %-10s %s\n", + printf(" %c%c %-17s %-10s %s\n", filter->flags & AVFILTER_FLAG_SUPPORT_TIMELINE ? 'T' : '.', filter->flags & AVFILTER_FLAG_SLICE_THREADS ? 'S' : '.', - filter->process_command ? 'C' : '.', filter->name, descr, filter->description); } #else