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 > <mich...@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.
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
