| Message ID | 20231113153234.8812-2-ffmpeg@haasn.xyz |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/3] swscale: don't assign range converters for float | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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 [...]
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.
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 [...]
On Tue, 14 Nov 2023 23:52:50 +0100 Michael Niedermayer <michael@niedermayer.cc> wrote: > 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. I gave this a try, but doing it internally is very tricky for a number of reasons and does not present obvious advantages over requiring the user to free+reinit if they wish to change range. So, the best long-term solution here would be to simply remove srcRange/dstRange from the signature of sws_setColorspaceDetails. vf_scale is the only current user of this API inside ffmpeg itself, and after the YUVJ removal series this call is no longer needed at all. (All range setup happens at filter init time with filter range negotiation) I think merging this series as-is represents the best short-term fix to the existing fundamental issues with this design. But if you want to rewrite all of swscale init code to allow graceful re-init on range change, be my guest.
Hi On Wed, Nov 22, 2023 at 01:45:05PM +0100, Niklas Haas wrote: > On Tue, 14 Nov 2023 23:52:50 +0100 Michael Niedermayer <michael@niedermayer.cc> wrote: > > 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. > > I gave this a try, but doing it internally is very tricky for a number > of reasons ok > and does not present obvious advantages over requiring the > user to free+reinit if they wish to change range. So, the best long-term > solution here would be to simply remove srcRange/dstRange from the > signature of sws_setColorspaceDetails. this doesnt feel right logic should be: 1. alloc struct 2. set all details for everything 3. init 4+ use n free or some API with convert between 2 frames and have a automatically cached and managed context where all details are either in the metadata of the frames itself or given to the function The whole idea of adjusting some details which could affect the required codepath without init is fishy unless everything can be adjusted that way and its the normal way of initing things So IMHO first lets figure out how this should be in the long run (moving to a clean API and clean implemenattion) and then find out how to move towards that in small steps that achieves teh short term goals quickly I dont like trying to achieve the short term goal with messy code and the long term unrelated. Because i have to maintain this and so i will not agree to something that moves us away from a clean long term result That said, if you must change the API, change the API, that i do not mind thx [...]
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; } }
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(+)