[libcamera-devel] py: cam: Fix demosaic overflow issue

18382 diff mbox series

The demosaic code first expands the buffer datatype to uint16, and then
shifts the data left so that the 8, 10 and 12 bitspp formats all become
16 bitspp.

It then, eventually, uses np.einsum to calculate averages, but this
averaging sums multiple uint16 values together, and stores them in
uint16 storage. As in the first step we shifted the values left,
possibly getting values close to the maximum of uint16 range, we, of
course, overflow when summing them together. This leads to rather bad
looking images.

Fix this by dropping the original shift. It serves no purpose, and is
probably a remnant of some early testing code. This way the largest
numbers we are summing together are 12 bit values, and as we use a 3x3
window from which we fetch values, for a single rgb plane, the max
number of 12 bit values is 5 (for green). Sum of 5 12 bit values is well
below the 16 bit maximum.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 src/py/cam/helpers.py | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Hi Tomi,

Thank you for the patch.

On Mon, Mar 13, 2023 at 09:28:13AM +0200, Tomi Valkeinen via libcamera-devel wrote:
> The demosaic code first expands the buffer datatype to uint16, and then
> shifts the data left so that the 8, 10 and 12 bitspp formats all become
> 16 bitspp.
> 
> It then, eventually, uses np.einsum to calculate averages, but this
> averaging sums multiple uint16 values together, and stores them in
> uint16 storage. As in the first step we shifted the values left,
> possibly getting values close to the maximum of uint16 range, we, of
> course, overflow when summing them together. This leads to rather bad
> looking images.
> 
> Fix this by dropping the original shift. It serves no purpose, and is
> probably a remnant of some early testing code. This way the largest
> numbers we are summing together are 12 bit values, and as we use a 3x3
> window from which we fetch values, for a single rgb plane, the max
> number of 12 bit values is 5 (for green). Sum of 5 12 bit values is well
> below the 16 bit maximum.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  src/py/cam/helpers.py | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/src/py/cam/helpers.py b/src/py/cam/helpers.py
> index 6b32a134..2d906667 100644
> --- a/src/py/cam/helpers.py
> +++ b/src/py/cam/helpers.py
> @@ -117,14 +117,12 @@ def to_rgb(fmt, size, data):
>          bayer_pattern = fmt[1:5]
>          bitspp = int(fmt[5:])
>  
> -        # \todo shifting leaves the lowest bits 0
>          if bitspp == 8:
>              data = data.reshape((h, w))
> -            data = data.astype(np.uint16) << 8
> +            data = data.astype(np.uint16)
>          elif bitspp in [10, 12]:
>              data = data.view(np.uint16)
>              data = data.reshape((h, w))
> -            data = data << (16 - bitspp)
>          else:
>              raise Exception('Bad bitspp:' + str(bitspp))
>  
> @@ -145,7 +143,7 @@ def to_rgb(fmt, size, data):
>          b0 = (idx % 2, idx // 2)
>  
>          rgb = demosaic(data, r0, g0, g1, b0)
> -        rgb = (rgb >> 8).astype(np.uint8)
> +        rgb = (rgb >> (bitspp - 8)).astype(np.uint8)
>  
>      else:
>          rgb = None

Quoting Tomi Valkeinen via libcamera-devel (2023-03-13 07:28:13)
> The demosaic code first expands the buffer datatype to uint16, and then
> shifts the data left so that the 8, 10 and 12 bitspp formats all become
> 16 bitspp.
> 
> It then, eventually, uses np.einsum to calculate averages, but this
> averaging sums multiple uint16 values together, and stores them in
> uint16 storage. As in the first step we shifted the values left,
> possibly getting values close to the maximum of uint16 range, we, of
> course, overflow when summing them together. This leads to rather bad
> looking images.
> 
> Fix this by dropping the original shift. It serves no purpose, and is
> probably a remnant of some early testing code. This way the largest
> numbers we are summing together are 12 bit values, and as we use a 3x3
> window from which we fetch values, for a single rgb plane, the max
> number of 12 bit values is 5 (for green). Sum of 5 12 bit values is well
> below the 16 bit maximum.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

This looks reasonable to me.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> ---
>  src/py/cam/helpers.py | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/src/py/cam/helpers.py b/src/py/cam/helpers.py
> index 6b32a134..2d906667 100644
> --- a/src/py/cam/helpers.py
> +++ b/src/py/cam/helpers.py
> @@ -117,14 +117,12 @@ def to_rgb(fmt, size, data):
>          bayer_pattern = fmt[1:5]
>          bitspp = int(fmt[5:])
>  
> -        # \todo shifting leaves the lowest bits 0
>          if bitspp == 8:
>              data = data.reshape((h, w))
> -            data = data.astype(np.uint16) << 8
> +            data = data.astype(np.uint16)
>          elif bitspp in [10, 12]:
>              data = data.view(np.uint16)
>              data = data.reshape((h, w))
> -            data = data << (16 - bitspp)
>          else:
>              raise Exception('Bad bitspp:' + str(bitspp))
>  
> @@ -145,7 +143,7 @@ def to_rgb(fmt, size, data):
>          b0 = (idx % 2, idx // 2)
>  
>          rgb = demosaic(data, r0, g0, g1, b0)
> -        rgb = (rgb >> 8).astype(np.uint8)
> +        rgb = (rgb >> (bitspp - 8)).astype(np.uint8)
>  
>      else:
>          rgb = None
> -- 
> 2.34.1
>