Re: [FFmpeg-devel] [PATCH 2/3] swscale/utils: correctly return from sws_init_single_context

[Michael Niedermayer <michael@niedermayer.cc>]

[-- Attachment #1.1: Type: text/plain, Size: 3105 bytes --]

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@niedermayer.cc> wrote:
> > but i dont feel like i fully understand the issue here so maybe iam missing
> > the goal of this patchset somewhat
> 
> So, to summarize the main problem:
> 
> 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.
> 
> 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)
> 
> 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.
> 
> 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.
> 
> 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.
> 
> > * 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
> 
> 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.
> 
> > 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
> > 
> 

> 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

[...]
-- 
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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".