| Message ID | 20250304231254.10588-3-dan.scally@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Paging Paul and Stefan on this one ;-) Then I hope we can merge this series soon ... The main C55 tuning files do not impact any potential regression - and I'd rather see additional platforms get their core tuning parameters in so we can build up the tuning platforms to generate for all supported targets sometime. Quoting Daniel Scally (2025-03-04 23:12:53) > The Linear distributor attempts to distribute pixels into sectors > of even size. Where the image width doesn't allow this it attempts > to create all but one sector of full size, and allows the caller > to choose to have the stunted sector at the front or the back. The > implicit assumption is that > > domain == (sector_size * (n_sectors - 1)) + remainder > > which does not necessarily hold true. For example with 32 sectors and > a domain of 648 the calculated sector size will be 21 pixels, which > leads to 31 * 21 = 651 which is larger than the domain size. > > Correct the issue by checking if there's more one stunted sector to > be filled. If there is, rather than following the remainder hint for > distribution place one of the stunted sectors at the front and one at > the back. > > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > --- > utils/tuning/libtuning/gradient.py | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/utils/tuning/libtuning/gradient.py b/utils/tuning/libtuning/gradient.py > index b643f502..bb2edef2 100644 > --- a/utils/tuning/libtuning/gradient.py > +++ b/utils/tuning/libtuning/gradient.py > @@ -55,17 +55,23 @@ class Linear(Gradient): > > size = math.ceil(size) > rem = domain % size > - output_sectors = [int(size)] * (sectors - 1) > + n_full_sectors = math.floor(domain / size) > + output_sectors = [int(size)] * n_full_sectors > > if self.remainder == lt.Remainder.Float: > size = domain / sectors > output_sectors = [size] * sectors > - elif self.remainder == lt.Remainder.DistributeFront: > + elif ((sectors - n_full_sectors) == 1): > + if self.remainder == lt.Remainder.DistributeFront: > + output_sectors.append(int(rem)) > + elif self.remainder == lt.Remainder.DistributeBack: > + output_sectors.insert(0, int(rem)) > + else: > + raise ValueError > + else: > + rem = rem / 2 > output_sectors.append(int(rem)) > - elif self.remainder == lt.Remainder.DistributeBack: > output_sectors.insert(0, int(rem)) > - else: > - raise ValueError > > return output_sectors > > -- > 2.34.1 >
Quoting Daniel Scally (2025-03-05 08:12:53) > The Linear distributor attempts to distribute pixels into sectors > of even size. Where the image width doesn't allow this it attempts > to create all but one sector of full size, and allows the caller > to choose to have the stunted sector at the front or the back. The > implicit assumption is that > > domain == (sector_size * (n_sectors - 1)) + remainder > > which does not necessarily hold true. For example with 32 sectors and > a domain of 648 the calculated sector size will be 21 pixels, which > leads to 31 * 21 = 651 which is larger than the domain size. Doesn't the equation hold true if sector_size is 20? (20 * 31) + 28 = 648 (Which means the calculated sector size is wrong) > > Correct the issue by checking if there's more one stunted sector to > be filled. If there is, rather than following the remainder hint for > distribution place one of the stunted sectors at the front and one at > the back. So this would get you 9 + (21 * 30) + 9 = 648 instead. Is that better than the above? > > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> The math below checks out with what you describe. Thanks, Paul > --- > utils/tuning/libtuning/gradient.py | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/utils/tuning/libtuning/gradient.py b/utils/tuning/libtuning/gradient.py > index b643f502..bb2edef2 100644 > --- a/utils/tuning/libtuning/gradient.py > +++ b/utils/tuning/libtuning/gradient.py > @@ -55,17 +55,23 @@ class Linear(Gradient): > > size = math.ceil(size) > rem = domain % size > - output_sectors = [int(size)] * (sectors - 1) > + n_full_sectors = math.floor(domain / size) > + output_sectors = [int(size)] * n_full_sectors > > if self.remainder == lt.Remainder.Float: > size = domain / sectors > output_sectors = [size] * sectors > - elif self.remainder == lt.Remainder.DistributeFront: > + elif ((sectors - n_full_sectors) == 1): > + if self.remainder == lt.Remainder.DistributeFront: > + output_sectors.append(int(rem)) > + elif self.remainder == lt.Remainder.DistributeBack: > + output_sectors.insert(0, int(rem)) > + else: > + raise ValueError > + else: > + rem = rem / 2 > output_sectors.append(int(rem)) > - elif self.remainder == lt.Remainder.DistributeBack: > output_sectors.insert(0, int(rem)) > - else: > - raise ValueError > > return output_sectors > > -- > 2.34.1 >
Hi Dan, Thank you for the patch. On Tue, Mar 04, 2025 at 11:12:53PM +0000, Daniel Scally wrote: > The Linear distributor attempts to distribute pixels into sectors > of even size. Where the image width doesn't allow this it attempts > to create all but one sector of full size, and allows the caller > to choose to have the stunted sector at the front or the back. The > implicit assumption is that > > domain == (sector_size * (n_sectors - 1)) + remainder > > which does not necessarily hold true. For example with 32 sectors and > a domain of 648 the calculated sector size will be 21 pixels, which > leads to 31 * 21 = 651 which is larger than the domain size. > > Correct the issue by checking if there's more one stunted sector to > be filled. If there is, rather than following the remainder hint for > distribution place one of the stunted sectors at the front and one at > the back. > > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > --- > utils/tuning/libtuning/gradient.py | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/utils/tuning/libtuning/gradient.py b/utils/tuning/libtuning/gradient.py > index b643f502..bb2edef2 100644 > --- a/utils/tuning/libtuning/gradient.py > +++ b/utils/tuning/libtuning/gradient.py > @@ -55,17 +55,23 @@ class Linear(Gradient): > > size = math.ceil(size) > rem = domain % size > - output_sectors = [int(size)] * (sectors - 1) > + n_full_sectors = math.floor(domain / size) > + output_sectors = [int(size)] * n_full_sectors > > if self.remainder == lt.Remainder.Float: > size = domain / sectors > output_sectors = [size] * sectors > - elif self.remainder == lt.Remainder.DistributeFront: > + elif ((sectors - n_full_sectors) == 1): > + if self.remainder == lt.Remainder.DistributeFront: > + output_sectors.append(int(rem)) > + elif self.remainder == lt.Remainder.DistributeBack: > + output_sectors.insert(0, int(rem)) > + else: > + raise ValueError > + else: > + rem = rem / 2 > output_sectors.append(int(rem)) > - elif self.remainder == lt.Remainder.DistributeBack: > output_sectors.insert(0, int(rem)) > - else: > - raise ValueError This looks relly convoluted. Could you simplify the whole function ? > > return output_sectors >
Hi Paul On 22/07/2025 12:40, Paul Elder wrote: > Quoting Daniel Scally (2025-03-05 08:12:53) >> The Linear distributor attempts to distribute pixels into sectors >> of even size. Where the image width doesn't allow this it attempts >> to create all but one sector of full size, and allows the caller >> to choose to have the stunted sector at the front or the back. The >> implicit assumption is that >> >> domain == (sector_size * (n_sectors - 1)) + remainder >> >> which does not necessarily hold true. For example with 32 sectors and >> a domain of 648 the calculated sector size will be 21 pixels, which >> leads to 31 * 21 = 651 which is larger than the domain size. > Doesn't the equation hold true if sector_size is 20? (20 * 31) + 28 = 648 > > (Which means the calculated sector size is wrong) At the moment you'd end up with 628 as the last sector would be calculated as 8 instead of 28 - the current implementation doesn't allow for the unequal sector to be larger...but perhaps that's a better change than splitting the runt into two? > >> Correct the issue by checking if there's more one stunted sector to >> be filled. If there is, rather than following the remainder hint for >> distribution place one of the stunted sectors at the front and one at >> the back. > So this would get you 9 + (21 * 30) + 9 = 648 instead. Is that better than the > above? I think it is...unless I'm misremembering I think the effect of a single larger sector would be that more of the more central pixels would be included in that "slice", would presumably be brighter and therefore would have slightly less shading applied than the equivalent slice on the other side of the image...but the effect is probably really really minimal. I can generate some LSC tables with both methodologies and see how they're different? Thanks Dan > >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > The math below checks out with what you describe. > > > Thanks, > > Paul > >> --- >> utils/tuning/libtuning/gradient.py | 16 +++++++++++----- >> 1 file changed, 11 insertions(+), 5 deletions(-) >> >> diff --git a/utils/tuning/libtuning/gradient.py b/utils/tuning/libtuning/gradient.py >> index b643f502..bb2edef2 100644 >> --- a/utils/tuning/libtuning/gradient.py >> +++ b/utils/tuning/libtuning/gradient.py >> @@ -55,17 +55,23 @@ class Linear(Gradient): >> >> size = math.ceil(size) >> rem = domain % size >> - output_sectors = [int(size)] * (sectors - 1) >> + n_full_sectors = math.floor(domain / size) >> + output_sectors = [int(size)] * n_full_sectors >> >> if self.remainder == lt.Remainder.Float: >> size = domain / sectors >> output_sectors = [size] * sectors >> - elif self.remainder == lt.Remainder.DistributeFront: >> + elif ((sectors - n_full_sectors) == 1): >> + if self.remainder == lt.Remainder.DistributeFront: >> + output_sectors.append(int(rem)) >> + elif self.remainder == lt.Remainder.DistributeBack: >> + output_sectors.insert(0, int(rem)) >> + else: >> + raise ValueError >> + else: >> + rem = rem / 2 >> output_sectors.append(int(rem)) >> - elif self.remainder == lt.Remainder.DistributeBack: >> output_sectors.insert(0, int(rem)) >> - else: >> - raise ValueError >> >> return output_sectors >> >> -- >> 2.34.1 >>
Quoting Dan Scally (2025-07-24 06:28:04) > Hi Paul > > On 22/07/2025 12:40, Paul Elder wrote: > > Quoting Daniel Scally (2025-03-05 08:12:53) > >> The Linear distributor attempts to distribute pixels into sectors > >> of even size. Where the image width doesn't allow this it attempts > >> to create all but one sector of full size, and allows the caller > >> to choose to have the stunted sector at the front or the back. The > >> implicit assumption is that > >> > >> domain == (sector_size * (n_sectors - 1)) + remainder > >> > >> which does not necessarily hold true. For example with 32 sectors and > >> a domain of 648 the calculated sector size will be 21 pixels, which > >> leads to 31 * 21 = 651 which is larger than the domain size. > > Doesn't the equation hold true if sector_size is 20? (20 * 31) + 28 = 648 > > > > (Which means the calculated sector size is wrong) > > > At the moment you'd end up with 628 as the last sector would be calculated as 8 instead of 28 - the > current implementation doesn't allow for the unequal sector to be larger...but perhaps that's a Oh ok I see what you mean. That was indeed my assumption when I wrote it... > better change than splitting the runt into two? > > > > >> Correct the issue by checking if there's more one stunted sector to > >> be filled. If there is, rather than following the remainder hint for > >> distribution place one of the stunted sectors at the front and one at > >> the back. > > So this would get you 9 + (21 * 30) + 9 = 648 instead. Is that better than the > > above? > > I think it is...unless I'm misremembering I think the effect of a single larger sector would be that > more of the more central pixels would be included in that "slice", would presumably be brighter and > therefore would have slightly less shading applied than the equivalent slice on the other side of > the image...but the effect is probably really really minimal. I can generate some LSC tables with > both methodologies and see how they're different? I had a brain fart. Smaller sectors on the edges would give you more granularity, and for lsc that's what you want. So of course it's better. I guess we can worry about it later when other things start using linear distributions that want the opposite effect. Ok, I'm convinced: Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > > Thanks > > Dan > > > > >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > > The math below checks out with what you describe. > > > > > > Thanks, > > > > Paul > > > >> --- > >> utils/tuning/libtuning/gradient.py | 16 +++++++++++----- > >> 1 file changed, 11 insertions(+), 5 deletions(-) > >> > >> diff --git a/utils/tuning/libtuning/gradient.py b/utils/tuning/libtuning/gradient.py > >> index b643f502..bb2edef2 100644 > >> --- a/utils/tuning/libtuning/gradient.py > >> +++ b/utils/tuning/libtuning/gradient.py > >> @@ -55,17 +55,23 @@ class Linear(Gradient): > >> > >> size = math.ceil(size) > >> rem = domain % size > >> - output_sectors = [int(size)] * (sectors - 1) > >> + n_full_sectors = math.floor(domain / size) > >> + output_sectors = [int(size)] * n_full_sectors > >> > >> if self.remainder == lt.Remainder.Float: > >> size = domain / sectors > >> output_sectors = [size] * sectors > >> - elif self.remainder == lt.Remainder.DistributeFront: > >> + elif ((sectors - n_full_sectors) == 1): > >> + if self.remainder == lt.Remainder.DistributeFront: > >> + output_sectors.append(int(rem)) > >> + elif self.remainder == lt.Remainder.DistributeBack: > >> + output_sectors.insert(0, int(rem)) > >> + else: > >> + raise ValueError > >> + else: > >> + rem = rem / 2 > >> output_sectors.append(int(rem)) > >> - elif self.remainder == lt.Remainder.DistributeBack: > >> output_sectors.insert(0, int(rem)) > >> - else: > >> - raise ValueError > >> > >> return output_sectors > >> > >> -- > >> 2.34.1 > >>
Hi Laurent, Paul On 23/07/2025 19:37, Laurent Pinchart wrote: > Hi Dan, > > Thank you for the patch. > > On Tue, Mar 04, 2025 at 11:12:53PM +0000, Daniel Scally wrote: >> The Linear distributor attempts to distribute pixels into sectors >> of even size. Where the image width doesn't allow this it attempts >> to create all but one sector of full size, and allows the caller >> to choose to have the stunted sector at the front or the back. The >> implicit assumption is that >> >> domain == (sector_size * (n_sectors - 1)) + remainder >> >> which does not necessarily hold true. For example with 32 sectors and >> a domain of 648 the calculated sector size will be 21 pixels, which >> leads to 31 * 21 = 651 which is larger than the domain size. >> >> Correct the issue by checking if there's more one stunted sector to >> be filled. If there is, rather than following the remainder hint for >> distribution place one of the stunted sectors at the front and one at >> the back. >> >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> >> --- >> utils/tuning/libtuning/gradient.py | 16 +++++++++++----- >> 1 file changed, 11 insertions(+), 5 deletions(-) >> >> diff --git a/utils/tuning/libtuning/gradient.py b/utils/tuning/libtuning/gradient.py >> index b643f502..bb2edef2 100644 >> --- a/utils/tuning/libtuning/gradient.py >> +++ b/utils/tuning/libtuning/gradient.py >> @@ -55,17 +55,23 @@ class Linear(Gradient): >> >> size = math.ceil(size) >> rem = domain % size >> - output_sectors = [int(size)] * (sectors - 1) >> + n_full_sectors = math.floor(domain / size) >> + output_sectors = [int(size)] * n_full_sectors >> >> if self.remainder == lt.Remainder.Float: >> size = domain / sectors >> output_sectors = [size] * sectors >> - elif self.remainder == lt.Remainder.DistributeFront: >> + elif ((sectors - n_full_sectors) == 1): >> + if self.remainder == lt.Remainder.DistributeFront: >> + output_sectors.append(int(rem)) >> + elif self.remainder == lt.Remainder.DistributeBack: >> + output_sectors.insert(0, int(rem)) >> + else: >> + raise ValueError >> + else: >> + rem = rem / 2 >> output_sectors.append(int(rem)) >> - elif self.remainder == lt.Remainder.DistributeBack: >> output_sectors.insert(0, int(rem)) >> - else: >> - raise ValueError > This looks relly convoluted. Could you simplify the whole function ? I tried to simplify and in testing my simplified solution I found a whole bunch of situations in which neither the old nor new version worked...for example with a domain of 1174 and 64 sectors the current method of calculating the "normal" sector size with roundup(domain / sectors) gives you 61 sectors of 19 and a remainder of 15 to divide across 3 sectors...which seems to me quite undesirable. I wonder if it's not better to change the function to calculate the "normal" sector size with rounddown(domain / sectors) and then distribute the remainder (which in the above example would be 4) by incrementally assigning 1 to each sector starting from either the front or back of the array, or the outside inwards, or the centre outwards...That would leave us with four sectors of 19 and 60 of 18 - I think that's a lot better than 61 sectors of 19 and 3 sectors of 5. Or possibly those parameters are so unrealistic that accounting for them is daft, and we should instead define limits for the parameters that lets us simplify things... Thoughts? Dan > >> >> return output_sectors >>
On Fri, Jul 25, 2025 at 08:57:06AM +0100, Daniel Scally wrote: > Hi Laurent, Paul > > On 23/07/2025 19:37, Laurent Pinchart wrote: > > Hi Dan, > > > > Thank you for the patch. > > > > On Tue, Mar 04, 2025 at 11:12:53PM +0000, Daniel Scally wrote: > >> The Linear distributor attempts to distribute pixels into sectors > >> of even size. Where the image width doesn't allow this it attempts > >> to create all but one sector of full size, and allows the caller > >> to choose to have the stunted sector at the front or the back. The > >> implicit assumption is that > >> > >> domain == (sector_size * (n_sectors - 1)) + remainder > >> > >> which does not necessarily hold true. For example with 32 sectors and > >> a domain of 648 the calculated sector size will be 21 pixels, which > >> leads to 31 * 21 = 651 which is larger than the domain size. > >> > >> Correct the issue by checking if there's more one stunted sector to > >> be filled. If there is, rather than following the remainder hint for > >> distribution place one of the stunted sectors at the front and one at > >> the back. > >> > >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > >> --- > >> utils/tuning/libtuning/gradient.py | 16 +++++++++++----- > >> 1 file changed, 11 insertions(+), 5 deletions(-) > >> > >> diff --git a/utils/tuning/libtuning/gradient.py b/utils/tuning/libtuning/gradient.py > >> index b643f502..bb2edef2 100644 > >> --- a/utils/tuning/libtuning/gradient.py > >> +++ b/utils/tuning/libtuning/gradient.py > >> @@ -55,17 +55,23 @@ class Linear(Gradient): > >> > >> size = math.ceil(size) > >> rem = domain % size > >> - output_sectors = [int(size)] * (sectors - 1) > >> + n_full_sectors = math.floor(domain / size) > >> + output_sectors = [int(size)] * n_full_sectors > >> > >> if self.remainder == lt.Remainder.Float: > >> size = domain / sectors > >> output_sectors = [size] * sectors > >> - elif self.remainder == lt.Remainder.DistributeFront: > >> + elif ((sectors - n_full_sectors) == 1): > >> + if self.remainder == lt.Remainder.DistributeFront: > >> + output_sectors.append(int(rem)) > >> + elif self.remainder == lt.Remainder.DistributeBack: > >> + output_sectors.insert(0, int(rem)) > >> + else: > >> + raise ValueError > >> + else: > >> + rem = rem / 2 > >> output_sectors.append(int(rem)) > >> - elif self.remainder == lt.Remainder.DistributeBack: > >> output_sectors.insert(0, int(rem)) > >> - else: > >> - raise ValueError > > > > This looks relly convoluted. Could you simplify the whole function ? > > I tried to simplify and in testing my simplified solution I found a whole bunch of situations in > which neither the old nor new version worked...for example with a domain of 1174 and 64 sectors the > current method of calculating the "normal" sector size with roundup(domain / sectors) gives you 61 > sectors of 19 and a remainder of 15 to divide across 3 sectors...which seems to me quite > undesirable. I wonder if it's not better to change the function to calculate the "normal" sector > size with rounddown(domain / sectors) and then distribute the remainder (which in the above example > would be 4) by incrementally assigning 1 to each sector starting from either the front or back of > the array, or the outside inwards, or the centre outwards...That would leave us with four sectors of > 19 and 60 of 18 - I think that's a lot better than 61 sectors of 19 and 3 sectors of 5. I had a similar idea, based on the gut feeling that distributing the error could be better. I would possibly try to distribute it evenly. Paul, why did we add support for different distributions ? We only use DistributeFront as far as I can see. > Or possibly those parameters are so unrealistic that accounting for them is daft, and we should > instead define limits for the parameters that lets us simplify things... > > Thoughts? > > >> > >> return output_sectors > >>
diff --git a/utils/tuning/libtuning/gradient.py b/utils/tuning/libtuning/gradient.py index b643f502..bb2edef2 100644 --- a/utils/tuning/libtuning/gradient.py +++ b/utils/tuning/libtuning/gradient.py @@ -55,17 +55,23 @@ class Linear(Gradient): size = math.ceil(size) rem = domain % size - output_sectors = [int(size)] * (sectors - 1) + n_full_sectors = math.floor(domain / size) + output_sectors = [int(size)] * n_full_sectors if self.remainder == lt.Remainder.Float: size = domain / sectors output_sectors = [size] * sectors - elif self.remainder == lt.Remainder.DistributeFront: + elif ((sectors - n_full_sectors) == 1): + if self.remainder == lt.Remainder.DistributeFront: + output_sectors.append(int(rem)) + elif self.remainder == lt.Remainder.DistributeBack: + output_sectors.insert(0, int(rem)) + else: + raise ValueError + else: + rem = rem / 2 output_sectors.append(int(rem)) - elif self.remainder == lt.Remainder.DistributeBack: output_sectors.insert(0, int(rem)) - else: - raise ValueError return output_sectors
The Linear distributor attempts to distribute pixels into sectors of even size. Where the image width doesn't allow this it attempts to create all but one sector of full size, and allows the caller to choose to have the stunted sector at the front or the back. The implicit assumption is that domain == (sector_size * (n_sectors - 1)) + remainder which does not necessarily hold true. For example with 32 sectors and a domain of 648 the calculated sector size will be 21 pixels, which leads to 31 * 21 = 651 which is larger than the domain size. Correct the issue by checking if there's more one stunted sector to be filled. If there is, rather than following the remainder hint for distribution place one of the stunted sectors at the front and one at the back. Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> --- utils/tuning/libtuning/gradient.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)