[FFmpeg-devel,1/2] lavfi: check links properties after configuring them.

For now, check the image size.
Inspired by a patch from Paul B Mahol.

Invalid sizes would be detected later by allocation failures,
detecting problems earlier is cleaner.

Signed-off-by: Nicolas George <george@nsup.org>
---
 libavfilter/avfiltergraph.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

On 11/1/17, Nicolas George <george@nsup.org> wrote:
> For now, check the image size.
> Inspired by a patch from Paul B Mahol.
>
> Invalid sizes would be detected later by allocation failures,
> detecting problems earlier is cleaner.
>
> Signed-off-by: Nicolas George <george@nsup.org>
> ---
>  libavfilter/avfiltergraph.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)

LGTM

On Wed, Nov 01, 2017 at 09:39:52PM +0100, Nicolas George wrote:
> For now, check the image size.
> Inspired by a patch from Paul B Mahol.
> 
> Invalid sizes would be detected later by allocation failures,
> detecting problems earlier is cleaner.
> 
> Signed-off-by: Nicolas George <george@nsup.org>
> ---
>  libavfilter/avfiltergraph.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/libavfilter/avfiltergraph.c b/libavfilter/avfiltergraph.c
> index 69cf26896d..a009e0a760 100644
> --- a/libavfilter/avfiltergraph.c
> +++ b/libavfilter/avfiltergraph.c
> @@ -28,6 +28,7 @@
>  #include "libavutil/avstring.h"
>  #include "libavutil/bprint.h"
>  #include "libavutil/channel_layout.h"
> +#include "libavutil/imgutils.h"
>  #include "libavutil/internal.h"
>  #include "libavutil/opt.h"
>  #include "libavutil/pixdesc.h"
> @@ -263,6 +264,27 @@ static int graph_config_links(AVFilterGraph *graph, AVClass *log_ctx)
>      return 0;
>  }
>  
> +static int graph_check_links(AVFilterGraph *graph, AVClass *log_ctx)
> +{
> +    AVFilterContext *f;
> +    AVFilterLink *l;

> +    unsigned i, j;

nb_filters and nb_outputs are signed so the counters better be the same
type.

I think we already had that discussion but I'd rather have the counters
signed so the compiler can exploit the undefined property of signed
overflow to assume it will never happen.

[...]

Le primidi 11 brumaire, an CCXXVI, Clement Boesch a écrit :
> nb_filters and nb_outputs are signed so the counters better be the same
> type.

I re-checked, I think you are mistaken.

> I think we already had that discussion but I'd rather have the counters
> signed so the compiler can exploit the undefined property of signed
> overflow to assume it will never happen.

On the other hand, it forces it to make code that can handle negative as
well, even though the value are positive. I really think it is better to
inform the compiler of the intent, and the intent is that the numbers
are positive, i.e. unsigned.

Regards,

On Wed, Nov 01, 2017 at 11:01:22PM +0100, Nicolas George wrote:
> Le primidi 11 brumaire, an CCXXVI, Clement Boesch a écrit :
> > nb_filters and nb_outputs are signed so the counters better be the same
> > type.
> 
> I re-checked, I think you are mistaken.
> 

My bad, you're right, I was looking at the wrong header with the same
variable names. Dismiss my comment.

> > I think we already had that discussion but I'd rather have the counters
> > signed so the compiler can exploit the undefined property of signed
> > overflow to assume it will never happen.
> 
> On the other hand, it forces it to make code that can handle negative as
> well, even though the value are positive. I really think it is better to
> inform the compiler of the intent, and the intent is that the numbers
> are positive, i.e. unsigned.

I'd say that a counter is unlikely to require a sign vs unsigned
optimization (and if it does and we know the counter is positive only, we
can explicit it with bit shifts etc). OTOH, the compiler will always have
to assume an overflow can happen if the initial counter value comes from
another variable, and you can't do much to hint it about it. Assuming a
loop overflow has IMO much more impact of what the compiler can do in the
general logic of the loop. But that's pure speculation from me.

Le primidi 11 brumaire, an CCXXVI, Clement Boesch a écrit :
> My bad, you're right, I was looking at the wrong header with the same
> variable names. Dismiss my comment.

Ok, series pushed.

> I'd say that a counter is unlikely to require a sign vs unsigned
> optimization (and if it does and we know the counter is positive only, we
> can explicit it with bit shifts etc). OTOH, the compiler will always have
> to assume an overflow can happen if the initial counter value comes from
> another variable, and you can't do much to hint it about it. Assuming a
> loop overflow has IMO much more impact of what the compiler can do in the
> general logic of the loop. But that's pure speculation from me.

I find the conclusion that lying to the compiler would help it make the
code faster it hard to swallow.

I think the real logic is this: unsigned, since they have a much simpler
semantic, have been optimized efficiently by compilers for a long time.
Recently, compilers have learned to use undefined behaviours to optimize
signed further, almost as much as unsigned. But not better.

Regards,