| Message ID | 20241025130300.227817-1-umang.jain@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series | media: dw100: Rectify rounding error | expand |
Hi Umang, Thanks for the patch. Nice catch. On 10/25/24 3:03 PM, Umang Jain wrote: > From: Stefan Klug <stefan.klug@ideasonboard.com> > > The current scaler code fails in many cases which can be validated > by the following python script: > > ``` > error_count = 0 > valid_count = 0 > > def check_scaling_error(src_w, dst_w, add_point_five): > global error_count > global valid_count > > qscale = (dst_w << 16) // src_w > > if (add_point_five): > delta = 1 << 15; # 0.5 in Q16.16 > else: > delta = 0 > > scaled_w = ((((src_w << 16) + delta) * qscale) >> 32) > if dst_w != scaled_w: > print(f'scale_error: src_w: {src_w} | dst_w:{dst_w} | scaled_w:{scaled_w}') > error_count += 1 > else: > valid_count += 1 > > print(f'==== Test without delta=0.5 ====\n') > for i in range(1000, 1920): > check_scaling_error(1920, i, False) > print(f'Error: {error_count} | Valid: {valid_count}\n\n') > > error_count = 0 > print(f'==== Test with delta=0.5 ====') > for i in range(1000, 1920): > check_scaling_error(1920, i, True) > print(f'Error: {error_count} | Valid: {valid_count}\n\n') > ``` > > Excerpt of the output is retrieved: > ``` > ==== Test without delta=0.5 ==== > ... > ... > scale_error: src_w: 1920 | dst_w:1915 | scaled_w:1914 > scale_error: src_w: 1920 | dst_w:1916 | scaled_w:1915 > scale_error: src_w: 1920 | dst_w:1917 | scaled_w:1916 > scale_error: src_w: 1920 | dst_w:1918 | scaled_w:1917 > scale_error: src_w: 1920 | dst_w:1919 | scaled_w:1918 > Error: 859 | Valid: 61 > > ==== Test with delta=0.5 ==== > Error: 0 | Valid: 981 > ``` > Hence, fixing the scaling rounding error by adding 0.5 to the > frame dimensions before applying the scale. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > drivers/media/platform/nxp/dw100/dw100.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/nxp/dw100/dw100.c b/drivers/media/platform/nxp/dw100/dw100.c > index 42712ccff754..541706f0aec4 100644 > --- a/drivers/media/platform/nxp/dw100/dw100.c > +++ b/drivers/media/platform/nxp/dw100/dw100.c > @@ -984,6 +984,7 @@ static int dw100_s_selection(struct file *file, void *fh, > u32 qscalex, qscaley, qscale; > int x, y, w, h; > unsigned int wframe, hframe; > + uint32_t zero_point_five; > > if (sel->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) > return -EINVAL; > @@ -1032,8 +1033,9 @@ static int dw100_s_selection(struct file *file, void *fh, > } > } > > - w = (u32)((((u64)wframe << 16) * qscale) >> 32); > - h = (u32)((((u64)hframe << 16) * qscale) >> 32); > + zero_point_five = 1 << 15; > + w = (u32)(((((u64)wframe << 16)+zero_point_five) * qscale) >> 32); > + h = (u32)(((((u64)hframe << 16)+zero_point_five) * qscale) >> 32); > x = x + (sel->r.width - w) / 2; > y = y + (sel->r.height - h) / 2; > x = min(wframe - w, (unsigned int)max(0, x)); Reviewed-by: Xavier Roumegue <xavier.roumegue@oss.nxp.com>
Hi Umang, Stefan, Thank you for the patch. On Fri, Oct 25, 2024 at 06:33:00PM +0530, Umang Jain wrote: > From: Stefan Klug <stefan.klug@ideasonboard.com> > > The current scaler code fails in many cases which can be validated > by the following python script: > > ``` > error_count = 0 > valid_count = 0 > > def check_scaling_error(src_w, dst_w, add_point_five): > global error_count > global valid_count > > qscale = (dst_w << 16) // src_w > > if (add_point_five): > delta = 1 << 15; # 0.5 in Q16.16 > else: > delta = 0 > > scaled_w = ((((src_w << 16) + delta) * qscale) >> 32) > if dst_w != scaled_w: > print(f'scale_error: src_w: {src_w} | dst_w:{dst_w} | scaled_w:{scaled_w}') > error_count += 1 > else: > valid_count += 1 > > print(f'==== Test without delta=0.5 ====\n') > for i in range(1000, 1920): > check_scaling_error(1920, i, False) > print(f'Error: {error_count} | Valid: {valid_count}\n\n') > > error_count = 0 You need to reset valid_count too here. > print(f'==== Test with delta=0.5 ====') > for i in range(1000, 1920): > check_scaling_error(1920, i, True) > print(f'Error: {error_count} | Valid: {valid_count}\n\n') > ``` > > Excerpt of the output is retrieved: > ``` > ==== Test without delta=0.5 ==== > ... > ... > scale_error: src_w: 1920 | dst_w:1915 | scaled_w:1914 > scale_error: src_w: 1920 | dst_w:1916 | scaled_w:1915 > scale_error: src_w: 1920 | dst_w:1917 | scaled_w:1916 > scale_error: src_w: 1920 | dst_w:1918 | scaled_w:1917 > scale_error: src_w: 1920 | dst_w:1919 | scaled_w:1918 > Error: 859 | Valid: 61 > > ==== Test with delta=0.5 ==== > Error: 0 | Valid: 981 > ``` > Hence, fixing the scaling rounding error by adding 0.5 to the > frame dimensions before applying the scale. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > drivers/media/platform/nxp/dw100/dw100.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/nxp/dw100/dw100.c b/drivers/media/platform/nxp/dw100/dw100.c > index 42712ccff754..541706f0aec4 100644 > --- a/drivers/media/platform/nxp/dw100/dw100.c > +++ b/drivers/media/platform/nxp/dw100/dw100.c > @@ -984,6 +984,7 @@ static int dw100_s_selection(struct file *file, void *fh, > u32 qscalex, qscaley, qscale; > int x, y, w, h; > unsigned int wframe, hframe; > + uint32_t zero_point_five; > > if (sel->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) > return -EINVAL; > @@ -1032,8 +1033,9 @@ static int dw100_s_selection(struct file *file, void *fh, > } > } > > - w = (u32)((((u64)wframe << 16) * qscale) >> 32); > - h = (u32)((((u64)hframe << 16) * qscale) >> 32); > + zero_point_five = 1 << 15; > + w = (u32)(((((u64)wframe << 16)+zero_point_five) * qscale) >> 32); I'm having trouble understanding what you're really fixing here. It seems the patch ensures that the crop rectangle requested by the application doesn't get adjusted, but doesn't take into account what is actually programmed to the hardware. Looking at dw100_hw_set_src_crop(), I see that the hardware scaling factor is expressed as a UQ1.7 integer. Shouldn't the rounding applied by the hardware need to be reported in the crop rectangle returned by dw100_s_selection(), instead of pretending it can be pixel-perfect ? At the very least I would expect this function to use Q1.7 encoding for the scaling factor, and apply the same rounding as the hardware to calculate the width and height back. > + h = (u32)(((((u64)hframe << 16)+zero_point_five) * qscale) >> 32); Missing spaces around '+'/ > x = x + (sel->r.width - w) / 2; > y = y + (sel->r.height - h) / 2; > x = min(wframe - w, (unsigned int)max(0, x));
diff --git a/drivers/media/platform/nxp/dw100/dw100.c b/drivers/media/platform/nxp/dw100/dw100.c index 42712ccff754..541706f0aec4 100644 --- a/drivers/media/platform/nxp/dw100/dw100.c +++ b/drivers/media/platform/nxp/dw100/dw100.c @@ -984,6 +984,7 @@ static int dw100_s_selection(struct file *file, void *fh, u32 qscalex, qscaley, qscale; int x, y, w, h; unsigned int wframe, hframe; + uint32_t zero_point_five; if (sel->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) return -EINVAL; @@ -1032,8 +1033,9 @@ static int dw100_s_selection(struct file *file, void *fh, } } - w = (u32)((((u64)wframe << 16) * qscale) >> 32); - h = (u32)((((u64)hframe << 16) * qscale) >> 32); + zero_point_five = 1 << 15; + w = (u32)(((((u64)wframe << 16)+zero_point_five) * qscale) >> 32); + h = (u32)(((((u64)hframe << 16)+zero_point_five) * qscale) >> 32); x = x + (sel->r.width - w) / 2; y = y + (sel->r.height - h) / 2; x = min(wframe - w, (unsigned int)max(0, x));