| Message ID | CAB0OVGoUd7XGC7F40k5K2JbtPrek-b7BnnD3OAfvuuhag4ovZw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On 16/06/18 13:39, Carl Eugen Hoyos wrote: > 2018-02-05 3:05 GMT+01:00, James Almer <jamrial@gmail.com>: >> On 2/4/2018 10:33 PM, Carl Eugen Hoyos wrote: >>> Hi! >>> >>> OOM is unlikely as a failure for avfilter_graph_alloc_filter(), the >>> patch avoids a surprising error if a filter was not found. >>> >>> Please comment, Carl Eugen >>> >>> >>> 0001-lavfi-avfiltergraph-Do-not-return-ENOMEM-if-filterch.patch >>> >>> >>> From 6587726a5e96570bb54e49ccf0b7fd6d94b929c8 Mon Sep 17 00:00:00 2001 >>> From: Carl Eugen Hoyos <ceffmpeg@gmail.com> >>> Date: Mon, 5 Feb 2018 01:43:08 +0100 >>> Subject: [PATCH] lavfi/avfiltergraph: Do not return ENOMEM if filterchain >>> init failed. >>> >>> A more likely reason is that the filter was not found. >>> --- >>> libavfilter/avfiltergraph.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/libavfilter/avfiltergraph.c b/libavfilter/avfiltergraph.c >>> index 4cc6892..7ccd895 100644 >>> --- a/libavfilter/avfiltergraph.c >>> +++ b/libavfilter/avfiltergraph.c >>> @@ -147,7 +147,7 @@ int avfilter_graph_create_filter(AVFilterContext >>> **filt_ctx, const AVFilter *fil >>> >>> *filt_ctx = avfilter_graph_alloc_filter(graph_ctx, filt, name); >>> if (!*filt_ctx) >>> - return AVERROR(ENOMEM); >>> + return -1; >> >> -1 is not acceptable for a public function that states it returns an >> AVERROR value on failure. >> If the issue is that avfilter_graph_alloc_filter() may fail because of >> both OOM and an invalid value for filter, then I'm more inclined to use >> AVERROR_UNKNOWN here instead. > > New patch attached. > > Please comment, Carl Eugen There is AVERROR_FILTER_NOT_FOUND - while it isn't currently returned from lavfi, it does sound exactly right for this. I'd prefer for it to use a function which can return an error code and propagate that (IMO the return-a-pointer style should only be used for functions which can only fail by OOM), but given the circumstances what you've written there looks fine. (An alternative would be to check beforehand whether the filter name exists? Not sure if that's really any better.) Thanks, - Mark
Mark Thompson (2018-06-17): > There is AVERROR_FILTER_NOT_FOUND - while it isn't currently returned > from lavfi, it does sound exactly right for this. It is not exactly right for this, because this can be ENOMEM too. Furthermore, this error code is really wrong, because this function does not search for a filter, it takes it as an argument, and the argument should not be NULL. Therefore, in this particular instance, the correct fix would be to replace the initial check in ff_filter_alloc() by an assert. > (An alternative would be to check beforehand whether the filter name > exists? Not sure if that's really any better.) It is not a filter name, it is a filter that was already checked. Calling avfilter_graph_create_filter() with a NULL filter makes no sense and is not allowed by the documented API. Adding a check before is necessary. Regards,
From 491b6f071865f4639fbf2530cd6d1a2c9aa87cda Mon Sep 17 00:00:00 2001 From: Carl Eugen Hoyos <ceffmpeg@gmail.com> Date: Sat, 16 Jun 2018 14:37:55 +0200 Subject: [PATCH] lavfi/avfiltergraph: Do not return ENOMEM if filterchain init failed. A more likely reason is that the filter was not found. Improves ticket #7001. --- libavfilter/avfiltergraph.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libavfilter/avfiltergraph.c b/libavfilter/avfiltergraph.c index a149f8f..b1c88e7 100644 --- a/libavfilter/avfiltergraph.c +++ b/libavfilter/avfiltergraph.c @@ -147,7 +147,7 @@ int avfilter_graph_create_filter(AVFilterContext **filt_ctx, const AVFilter *fil *filt_ctx = avfilter_graph_alloc_filter(graph_ctx, filt, name); if (!*filt_ctx) - return AVERROR(ENOMEM); + return AVERROR_UNKNOWN; ret = avfilter_init_str(*filt_ctx, args); if (ret < 0) -- 1.7.10.4