Re: [FFmpeg-devel] [PATCH]lavfi/avfiltergraph: Do not return ENOMEM if filter is missing

[Nicolas George]

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,

-- 
  Nicolas George


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH]lavfi/avfiltergraph: Do not return ENOMEM if filter is missing

[Mark Thompson]

On 16/06/18 13:39, Carl Eugen Hoyos wrote:
> 2018-02-05 3:05 GMT+01:00, James Almer :
>> 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 
>>> 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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH]lavfi/avfiltergraph: Do not return ENOMEM if filter is missing

[Carl Eugen Hoyos]

2018-02-05 3:05 GMT+01:00, James Almer :
> 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 
>> 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
From 491b6f071865f4639fbf2530cd6d1a2c9aa87cda Mon Sep 17 00:00:00 2001
From: Carl Eugen Hoyos 
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

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH]lavfi/avfiltergraph: Do not return ENOMEM if filter is missing

[James Almer]

On 2/5/2018 1:20 PM, Nicolas George wrote:
> James Almer (2018-02-05):
>> If it takes a parameter like the type of "something" you want to alloc
>> then yes, I'd support that policy as there's no way to prevent the
>> argument passed from being invalid, so NULL in that case will not mean
>> ENOMEM but EINVAL.
> 
> Ok.
> 
>> But if there's no parameters at all, or if the parameter is a size one,
>> then the failure can only be OOM.
> 
> I can think of a few cases where the failure could be something else.
> For example if the structure that is allocated contains a mutex, the
> error could come from pthread_mutex_init(), which can return other
> errors than ENOMEM.

We ignore pthread_* errors as the wrappers (w32, os2) can only return 0.
And afaik, things like this are why we many times provide both alloc()
and init() functions.

In any case, while i agree that returning an error value for future API
additions should be heavily encouraged if not enforced for most cases, i
insist that we should not outright forbid the addition of new simple
malloc-wrapping functions, like the kind used to allocate structs where
the size is not part of the ABI, as it's clear OOM will always be the
only case of failure. See most alloc functions in lavu (crypto modules,
packet/frame side data structs, etc).

> 
>>  In those cases I prefer the latter as
>> it's more versatile in some situations, like using the function itself
>> as an argument when calling another function.
> 
> That is true, but that also prevents from using the usual idiom:
> 
>   ret = ...;
>   if (ret < 0)
>   return ret; /* or goto fail */
> 
> It requires an extra "ret = AVERROR(ENOMEM)", plus the braces. I think
> this is more common than calling a function with the result without
> checking it.
> 
> In terms of API regularity and simplicity, I find the hardcoding of
> AVERROR(ENOMEM) in the call site very distasteful.

It has gotten a bit out of hand, true. That's why I'm with you that some
rules needs to be defined.

> 
> Regards,
> 
> 
> 
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH]lavfi/avfiltergraph: Do not return ENOMEM if filter is missing

[Nicolas George]

James Almer (2018-02-05):
> If it takes a parameter like the type of "something" you want to alloc
> then yes, I'd support that policy as there's no way to prevent the
> argument passed from being invalid, so NULL in that case will not mean
> ENOMEM but EINVAL.

Ok.

> But if there's no parameters at all, or if the parameter is a size one,
> then the failure can only be OOM.

I can think of a few cases where the failure could be something else.
For example if the structure that is allocated contains a mutex, the
error could come from pthread_mutex_init(), which can return other
errors than ENOMEM.

>   In those cases I prefer the latter as
> it's more versatile in some situations, like using the function itself
> as an argument when calling another function.

That is true, but that also prevents from using the usual idiom:

ret = ...;
if (ret < 0)
return ret; /* or goto fail */

It requires an extra "ret = AVERROR(ENOMEM)", plus the braces. I think
this is more common than calling a function with the result without
checking it.

In terms of API regularity and simplicity, I find the hardcoding of
AVERROR(ENOMEM) in the call site very distasteful.

Regards,

-- 
  Nicolas George


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH]lavfi/avfiltergraph: Do not return ENOMEM if filter is missing

[James Almer]

On 2/5/2018 12:22 PM, Nicolas George wrote:
> James Almer (2018-02-04):
>> -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.
> 
> Sounds reasonable.
> 
>> That being said, based on the above the return value of
>> avfilter_graph_alloc_filter() should have never been an AVFilterContext,
>> but an int instead.
>> Maybe a new function that returns a proper AVERROR value should be
>> added. AVERROR(ENOMEM), AVERROR(EINVAL), AVERROR_FILTER_NOT_FOUND, or
>> whatever corresponds depending on the type of failure.
> 
> I agree.
> 
> In fact, would you support me if I propose to add the following policy:?
> 
>   Any new function av_something_alloc() should have an API:
> 
>   ret = av_something_alloc(, params);
> 
>   and not:
> 
>   ptr = av_something_alloc(params);
> 
> even if it can, for the time being, only return AVERROR(ENOMEM).
> 
> Regards,

If it takes a parameter like the type of "something" you want to alloc
then yes, I'd support that policy as there's no way to prevent the
argument passed from being invalid, so NULL in that case will not mean
ENOMEM but EINVAL.
But if there's no parameters at all, or if the parameter is a size one,
then the failure can only be OOM. In those cases I prefer the latter as
it's more versatile in some situations, like using the function itself
as an argument when calling another function.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH]lavfi/avfiltergraph: Do not return ENOMEM if filter is missing

[Nicolas George]

James Almer (2018-02-04):
> -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.

Sounds reasonable.

> That being said, based on the above the return value of
> avfilter_graph_alloc_filter() should have never been an AVFilterContext,
> but an int instead.
> Maybe a new function that returns a proper AVERROR value should be
> added. AVERROR(ENOMEM), AVERROR(EINVAL), AVERROR_FILTER_NOT_FOUND, or
> whatever corresponds depending on the type of failure.

I agree.

In fact, would you support me if I propose to add the following policy:?

Any new function av_something_alloc() should have an API:

ret = av_something_alloc(, params);

and not:

ptr = av_something_alloc(params);

even if it can, for the time being, only return AVERROR(ENOMEM).

Regards,

-- 
  Nicolas George


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH]lavfi/avfiltergraph: Do not return ENOMEM if filter is missing

[James Almer]

On 2/4/2018 11:05 PM, James Almer wrote:
> 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 
>> 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.
> 
> That being said, based on the above the return value of
> avfilter_graph_alloc_filter() should have never been an AVFilterContext,
> but an int instead.
> Maybe a new function that returns a proper AVERROR value should be
> added. AVERROR(ENOMEM), AVERROR(EINVAL), AVERROR_FILTER_NOT_FOUND, or
> whatever corresponds depending on the type of failure.

Of course, assuming the actual reason of failure in
avfilter_graph_alloc_filter is important only in
avfilter_graph_create_filter and not really in any other situation, then
an internal ff_graph_alloc_filter function could be added instead of a
new public function for the above purpose.

> 
>>  
>>  ret = avfilter_init_str(*filt_ctx, args);
>>  if (ret < 0)
>> -- 1.7.10.4
>>
>>
>>
>> ___
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
> 

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH]lavfi/avfiltergraph: Do not return ENOMEM if filter is missing

[James Almer]

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 
> 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.

That being said, based on the above the return value of
avfilter_graph_alloc_filter() should have never been an AVFilterContext,
but an int instead.
Maybe a new function that returns a proper AVERROR value should be
added. AVERROR(ENOMEM), AVERROR(EINVAL), AVERROR_FILTER_NOT_FOUND, or
whatever corresponds depending on the type of failure.

>  
>  ret = avfilter_init_str(*filt_ctx, args);
>  if (ret < 0)
> -- 1.7.10.4
> 
> 
> 
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel