On Mon, Nov 13, 2023 at 04:32:33PM +0100, Niklas Haas wrote:
> From: Niklas Haas <git@haasn.dev>
> 
> Before cedf589, this function would return early return on RGB and float
> formats, as well as when range was equal. While this commit
> intentionally removed the early return for same-range YUV conversions,
> it missed that RGB and float formats that have an unscaled converter
> should always early return, no matter what the source range was set to.
> 
> Fixes: cedf589c09c567b72bf4c1a58db53d94622567e1
> ---
>  libswscale/utils.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/libswscale/utils.c b/libswscale/utils.c
> index ec822ff5d9..7ce86f83ea 100644
> --- a/libswscale/utils.c
> +++ b/libswscale/utils.c
> @@ -1733,6 +1733,9 @@ static av_cold int sws_init_single_context(SwsContext *c, SwsFilter *srcFilter,
>                  av_log(c, AV_LOG_INFO,
>                         "unscaled %s -> %s special converter is available\n",
>                         av_get_pix_fmt_name(srcFormat), av_get_pix_fmt_name(dstFormat));
> +
> +            if (isAnyRGB(dstFormat) || isFloat(srcFormat) || isFloat(dstFormat))
> +                return 0;

this and the other patch result in difficult to understand code.

if i look back to 6.0 the 2 cases had unconditional "return 0"

the code before basically

if(set of conditions for "alphablend")
    convert_unscaled = alphablend
    return 0

if(set of conditions for "special converter")
    convert_unscaled = set special converter
    return 0


now they conditionally return

if(set of conditions)
    convert_unscaled = alphablend
    if set of more conditions
        return 0

if(set of conditions)
    convert_unscaled = set special converter
    if set of more conditions
        return 0

I think this could add some burden to whoever eventually has to
clean this up

i may be missing something but i wonder if not either
* convert_unscaled should only be set when used
OR
* if these are set "always" if not alphablend and convert_unscaled should be
  two seperate fields. But i have not at all looked at what consequences that
  would have so maybe that has issues


Also if some range convert should not be used/set for some cases then
the check should maybe be where the range convert is setup not far away
from it. I mean a check close to the related code is easier to understand

but i dont feel like i fully understand the issue here so maybe iam missing
the goal of this patchset somewhat

thx


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Good people do not need laws to tell them to act responsibly, while bad
people will find a way around the laws. -- Plato