| Message ID | 8ef60c68-b260-83a2-8765-87737879e0b3@jkqxz.net |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, Jul 06, 2017 at 10:59:24PM +0100, Mark Thompson 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; this breaks ffmpegs choose_pixel_fmt() src_desc being NULL > > /* compute loss */ > *lossp = loss = 0; shouldnt this be set on a 0 return ? i also wonder if src and dst being identical should score different than if not if either is AV_PIX_FMT_FLAG_HWACCEL [...]
On Thu, 6 Jul 2017 22:59:24 +0100 Mark Thompson <sw@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?
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);