From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <ffmpeg-devel-bounces@ffmpeg.org>
Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100])
	by master.gitmailbox.com (Postfix) with ESMTP id C16944825E
	for <ffmpegdev@gitmailbox.com>; Tue, 14 Nov 2023 22:53:00 +0000 (UTC)
Received: from [127.0.1.1] (localhost [127.0.0.1])
	by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 1A04668CD74;
	Wed, 15 Nov 2023 00:52:59 +0200 (EET)
Received: from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net
 [217.70.183.196])
 by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id C040068CAC6
 for <ffmpeg-devel@ffmpeg.org>; Wed, 15 Nov 2023 00:52:52 +0200 (EET)
Received: by mail.gandi.net (Postfix) with ESMTPSA id B5CCFE0003
 for <ffmpeg-devel@ffmpeg.org>; Tue, 14 Nov 2023 22:52:51 +0000 (UTC)
Date: Tue, 14 Nov 2023 23:52:50 +0100
From: Michael Niedermayer <michael@niedermayer.cc>
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Message-ID: <20231114225250.GM3543730@pb2>
References: <20231113153234.8812-1-ffmpeg@haasn.xyz>
 <20231113153234.8812-2-ffmpeg@haasn.xyz>
 <20231113183008.GK3543730@pb2> <20231114141437.GB16959@haasn.xyz>
MIME-Version: 1.0
In-Reply-To: <20231114141437.GB16959@haasn.xyz>
X-GND-Sasl: michael@niedermayer.cc
Subject: Re: [FFmpeg-devel] [PATCH 2/3] swscale/utils: correctly return from
 sws_init_single_context
X-BeenThere: ffmpeg-devel@ffmpeg.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: FFmpeg development discussions and patches <ffmpeg-devel.ffmpeg.org>
List-Unsubscribe: <https://ffmpeg.org/mailman/options/ffmpeg-devel>,
 <mailto:ffmpeg-devel-request@ffmpeg.org?subject=unsubscribe>
List-Archive: <https://ffmpeg.org/pipermail/ffmpeg-devel>
List-Post: <mailto:ffmpeg-devel@ffmpeg.org>
List-Help: <mailto:ffmpeg-devel-request@ffmpeg.org?subject=help>
List-Subscribe: <https://ffmpeg.org/mailman/listinfo/ffmpeg-devel>,
 <mailto:ffmpeg-devel-request@ffmpeg.org?subject=subscribe>
Reply-To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Content-Type: multipart/mixed; boundary="===============2957345106153542301=="
Errors-To: ffmpeg-devel-bounces@ffmpeg.org
Sender: "ffmpeg-devel" <ffmpeg-devel-bounces@ffmpeg.org>
Archived-At: <https://master.gitmailbox.com/ffmpegdev/20231114225250.GM3543730@pb2/>
List-Archive: <https://master.gitmailbox.com/ffmpegdev/>
List-Post: <mailto:ffmpegdev@gitmailbox.com>


--===============2957345106153542301==
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="YNe5xdZPOSbiflA1"
Content-Disposition: inline


--YNe5xdZPOSbiflA1
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Hi

On Tue, Nov 14, 2023 at 02:14:37PM +0100, Niklas Haas wrote:
> On Mon, 13 Nov 2023 19:30:08 +0100 Michael Niedermayer <michael@niedermay=
er.cc> wrote:
> > but i dont feel like i fully understand the issue here so maybe iam mis=
sing
> > the goal of this patchset somewhat
>=20
> So, to summarize the main problem:
>=20
> 1. sws_init_single_context() previously hard-coded decisions based on
>    c->srcRange and c->dstRange. This is fundamentally broken, as
>    srcRange/dstRange can change at any time with
>    sws_setColorspaceDetails.
>=20
> 2. To fix this, this function was made to not early-return, and instead
>    run the rest of the init code just in case range conversion is needed
>    later. (With the check for whether or not the special converter can
>    be used being moved to the callsite instead of the setup site)
>=20
> 3. This caused problems for non-YUV inputs, because previously these
>    would always early return, but now they run the rest of the code,
>    which triggers at least one assertion for float32 formats.
>=20
> 4. To fix this, this commit restores the early-return for non-YUV,
>    preserving the status quo of existing behavior w.r.t not hitting the
>    rest of the init function.
>=20
> 5. Separately, this commit fixes an error in previous condition (2) at
>    the callsite, which relied on c->lumConvertRange being unset when no
>    range conversion is needed. However, that condition did not match the
>    condition used in the setup check before.
>=20
> > * convert_unscaled should only be set when used
> > OR
> > * if these are set "always" if not alphablend and convert_unscaled shou=
ld be
> >   two seperate fields. But i have not at all looked at what consequence=
s that
> >   would have so maybe that has issues
>=20
> convert_unscaled cannot be set only when used because we don't yet know
> if it will be used or not. There is also no advantage I see to splitting
> the fields, as they have basically the same logic attached to them
> - being dependent only on whether or not range conversion is needed.
>=20
> > 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 understa=
nd
> >=20
>=20

> One alternative that would make this possible would be to re-run whole
> context init from sws_setColorspaceDetails, if the srcRange/dstRange
> change.

would this result in overall cleaner code or do you see some problems
with this ?

Given the messi-ness that the always setting results in i would maybe
suggest to explore this and see if this is cleaner.

Its conceptually not wrong that if parameters change that init should
be redone.

thx

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

Rewriting code that is poorly written but fully understood is good.
Rewriting code that one doesnt understand is a sign that one is less smart
than the original author, trying to rewrite it will not make it better.

--YNe5xdZPOSbiflA1
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iF0EABEIAB0WIQSf8hKLFH72cwut8TNhHseHBAsPqwUCZVP6PgAKCRBhHseHBAsP
q+4pAJoDpWwzGsbca2ytEaVc+VULrK0xjwCfWNXQW+m6i2gTb1ue9qpAQDatM6M=
=ONeO
-----END PGP SIGNATURE-----

--YNe5xdZPOSbiflA1--

--===============2957345106153542301==
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

--===============2957345106153542301==--