[FFmpeg-devel] [PATCH 1/2] pixdesc: Improve scoring for opaque/unknown pixel formats

[wm4]

On Thu, 6 Jul 2017 22:59:24 +0100
Mark Thompson <sw at jkqxz.net> wrote:

> Hardware pixel formats do not tell you anything about their actual
> contents, but should still score higher than formats with completely
> unknown properties, which in turn should score higher than invalid
> formats.
> 
> Do not return an AVERROR code as a score.
> 
> Fixes a hang in libavfilter where format negotiation gets stuck in a
> loop because AV_PIX_FMT_NONE scores more highly than all other
> possibilities.
> ---
> The hang in libavfilter happens when trying to choose a pixfmt for output from a list of software formats when a hardware format is the input (the hwdownload filter does this).  The matching begins with AV_PIX_FMT_NONE as an invalid value and compares against each possibility in turn, but unfortunately it scores -1 when considered as a conversion from an opaque hardware format, higher than the AVERROR(EINVAL) (== -22 on Linux) scored for all of the real formats.  Hence the format selection code chooses AV_PIX_FMT_NONE and thinks it is making forward progress, but actually hasn't and therefore gets stuck in an infinite loop.
> 
> 
>  libavutil/pixdesc.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/libavutil/pixdesc.c b/libavutil/pixdesc.c
> index 46a7eff06d..35b63f43c6 100644
> --- a/libavutil/pixdesc.c
> +++ b/libavutil/pixdesc.c
> @@ -2512,7 +2512,11 @@ static int get_pix_fmt_score(enum AVPixelFormat dst_pix_fmt,
>      int score = INT_MAX - 1;
>  
>      if (dst_pix_fmt >= AV_PIX_FMT_NB || dst_pix_fmt <= AV_PIX_FMT_NONE)
> -        return ~0;
> +        return -2;
> +
> +    if ((src_desc->flags & AV_PIX_FMT_FLAG_HWACCEL) ||
> +        (dst_desc->flags & AV_PIX_FMT_FLAG_HWACCEL))
> +        return 0;
>  
>      /* compute loss */
>      *lossp = loss = 0;
> @@ -2521,9 +2525,9 @@ static int get_pix_fmt_score(enum AVPixelFormat dst_pix_fmt,
>          return INT_MAX;
>  
>      if ((ret = get_pix_fmt_depth(&src_min_depth, &src_max_depth, src_pix_fmt)) < 0)
> -        return ret;
> +        return -1;
>      if ((ret = get_pix_fmt_depth(&dst_min_depth, &dst_max_depth, dst_pix_fmt)) < 0)
> -        return ret;
> +        return -1;
>  
>      src_color = get_color_type(src_desc);
>      dst_color = get_color_type(dst_desc);

I don't think it makes any sense to prefer one hw format over an
another without additional information, but I guess that also means
returning a random one doesn't do any harms.

Wouldn't it be better to fix the lavfi code, though?