| Message ID | 20240703104004.184783-7-stefan.klug@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Stefan Klug (2024-07-03 11:39:53) > Move black levels for tuning files that contained a BLC block into > the camera sensor helpers. > > ov4689.yaml had 66@12bit while the datasheet states 64@12bit. Use the > value from the datasheet (scaled to 16bit). I wonder who chose 66 here. Was it measured by someone perhaps? Git blame states "Mikhail Rudenko" (Added to Cc) Mikhail, any opinion on this change? Anyway, I don't object to this right now - I think once we release our updates to the IPA and tuning tools, most sensors 'supported' in libcamera will likely benefit from being '(re)tuned' correctly and 'fully' (and consistently?). Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > ov5640.yaml had 256@12bit while the datasheet states 16@10bit. Looking > at the commit message the 256 most likely stems from the imx219 tuning > file and 16@10bit is the same as the 64@12bit from the ov4689. This > seems more likely and is therefore used. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > src/ipa/libipa/camera_sensor_helper.cpp | 2 ++ > src/ipa/rkisp1/data/ov4689.yaml | 4 ---- > src/ipa/rkisp1/data/ov5640.yaml | 4 ---- > 3 files changed, 2 insertions(+), 8 deletions(-) > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp > index 3d0e756927f0..982e35beaa90 100644 > --- a/src/ipa/libipa/camera_sensor_helper.cpp > +++ b/src/ipa/libipa/camera_sensor_helper.cpp > @@ -542,6 +542,7 @@ class CameraSensorHelperOv4689 : public CameraSensorHelper > public: > CameraSensorHelperOv4689() > { > + blackLevel_ = 1024; > gainType_ = AnalogueGainLinear; > gainConstants_.linear = { 1, 0, 0, 128 }; > } > @@ -553,6 +554,7 @@ class CameraSensorHelperOv5640 : public CameraSensorHelper > public: > CameraSensorHelperOv5640() > { > + blackLevel_ = 1024; > gainType_ = AnalogueGainLinear; > gainConstants_.linear = { 1, 0, 0, 16 }; > } > diff --git a/src/ipa/rkisp1/data/ov4689.yaml b/src/ipa/rkisp1/data/ov4689.yaml > index 2068684cafcd..609012967e02 100644 > --- a/src/ipa/rkisp1/data/ov4689.yaml > +++ b/src/ipa/rkisp1/data/ov4689.yaml > @@ -6,8 +6,4 @@ algorithms: > - Agc: > - Awb: > - BlackLevelCorrection: > - R: 66 > - Gr: 66 > - Gb: 66 > - B: 66 > ... > diff --git a/src/ipa/rkisp1/data/ov5640.yaml b/src/ipa/rkisp1/data/ov5640.yaml > index 897b83cb435b..4b21d412e44e 100644 > --- a/src/ipa/rkisp1/data/ov5640.yaml > +++ b/src/ipa/rkisp1/data/ov5640.yaml > @@ -6,10 +6,6 @@ algorithms: > - Agc: > - Awb: > - BlackLevelCorrection: > - R: 256 > - Gr: 256 > - Gb: 256 > - B: 256 > - ColorProcessing: > - GammaSensorLinearization: > x-intervals: [ 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2 ] > -- > 2.43.0 >
On Wed, Jul 03, 2024 at 01:20:56PM +0100, Kieran Bingham wrote: > Quoting Stefan Klug (2024-07-03 11:39:53) > > Move black levels for tuning files that contained a BLC block into > > the camera sensor helpers. > > > > ov4689.yaml had 66@12bit while the datasheet states 64@12bit. Use the > > value from the datasheet (scaled to 16bit). > > I wonder who chose 66 here. Was it measured by someone perhaps? > > Git blame states "Mikhail Rudenko" (Added to Cc) Mikhail, any opinion on > this change? > > Anyway, I don't object to this right now - I think once we release our > updates to the IPA and tuning tools, most sensors 'supported' in > libcamera will likely benefit from being '(re)tuned' correctly and > 'fully' (and consistently?). > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > ov5640.yaml had 256@12bit while the datasheet states 16@10bit. Looking > > at the commit message the 256 most likely stems from the imx219 tuning > > file and 16@10bit is the same as the 64@12bit from the ov4689. This > > seems more likely and is therefore used. > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > --- > > src/ipa/libipa/camera_sensor_helper.cpp | 2 ++ > > src/ipa/rkisp1/data/ov4689.yaml | 4 ---- > > src/ipa/rkisp1/data/ov5640.yaml | 4 ---- > > 3 files changed, 2 insertions(+), 8 deletions(-) > > > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp > > index 3d0e756927f0..982e35beaa90 100644 > > --- a/src/ipa/libipa/camera_sensor_helper.cpp > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp > > @@ -542,6 +542,7 @@ class CameraSensorHelperOv4689 : public CameraSensorHelper > > public: > > CameraSensorHelperOv4689() > > { > > + blackLevel_ = 1024; I wonder if blackLevel_ = 16 << 6; would be more readable. I suppose what you want to see at a glance, the 16-bit value, or the value for the native sensor bit depth. Up to you. If you decide to change it, please update all helpers. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > gainType_ = AnalogueGainLinear; > > gainConstants_.linear = { 1, 0, 0, 128 }; > > } > > @@ -553,6 +554,7 @@ class CameraSensorHelperOv5640 : public CameraSensorHelper > > public: > > CameraSensorHelperOv5640() > > { > > + blackLevel_ = 1024; > > gainType_ = AnalogueGainLinear; > > gainConstants_.linear = { 1, 0, 0, 16 }; > > } > > diff --git a/src/ipa/rkisp1/data/ov4689.yaml b/src/ipa/rkisp1/data/ov4689.yaml > > index 2068684cafcd..609012967e02 100644 > > --- a/src/ipa/rkisp1/data/ov4689.yaml > > +++ b/src/ipa/rkisp1/data/ov4689.yaml > > @@ -6,8 +6,4 @@ algorithms: > > - Agc: > > - Awb: > > - BlackLevelCorrection: > > - R: 66 > > - Gr: 66 > > - Gb: 66 > > - B: 66 > > ... > > diff --git a/src/ipa/rkisp1/data/ov5640.yaml b/src/ipa/rkisp1/data/ov5640.yaml > > index 897b83cb435b..4b21d412e44e 100644 > > --- a/src/ipa/rkisp1/data/ov5640.yaml > > +++ b/src/ipa/rkisp1/data/ov5640.yaml > > @@ -6,10 +6,6 @@ algorithms: > > - Agc: > > - Awb: > > - BlackLevelCorrection: > > - R: 256 > > - Gr: 256 > > - Gb: 256 > > - B: 256 > > - ColorProcessing: > > - GammaSensorLinearization: > > x-intervals: [ 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2 ]
On Wed, Jul 03, 2024 at 03:48:19PM +0300, Laurent Pinchart wrote: > On Wed, Jul 03, 2024 at 01:20:56PM +0100, Kieran Bingham wrote: > > Quoting Stefan Klug (2024-07-03 11:39:53) > > > Move black levels for tuning files that contained a BLC block into > > > the camera sensor helpers. > > > > > > ov4689.yaml had 66@12bit while the datasheet states 64@12bit. Use the > > > value from the datasheet (scaled to 16bit). > > > > I wonder who chose 66 here. Was it measured by someone perhaps? > > > > Git blame states "Mikhail Rudenko" (Added to Cc) Mikhail, any opinion on > > this change? > > > > Anyway, I don't object to this right now - I think once we release our > > updates to the IPA and tuning tools, most sensors 'supported' in > > libcamera will likely benefit from being '(re)tuned' correctly and > > 'fully' (and consistently?). > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > ov5640.yaml had 256@12bit while the datasheet states 16@10bit. Looking > > > at the commit message the 256 most likely stems from the imx219 tuning > > > file and 16@10bit is the same as the 64@12bit from the ov4689. This > > > seems more likely and is therefore used. > > > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > --- > > > src/ipa/libipa/camera_sensor_helper.cpp | 2 ++ > > > src/ipa/rkisp1/data/ov4689.yaml | 4 ---- > > > src/ipa/rkisp1/data/ov5640.yaml | 4 ---- > > > 3 files changed, 2 insertions(+), 8 deletions(-) > > > > > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp > > > index 3d0e756927f0..982e35beaa90 100644 > > > --- a/src/ipa/libipa/camera_sensor_helper.cpp > > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp > > > @@ -542,6 +542,7 @@ class CameraSensorHelperOv4689 : public CameraSensorHelper > > > public: > > > CameraSensorHelperOv4689() > > > { > > > + blackLevel_ = 1024; > > I wonder if > > blackLevel_ = 16 << 6; > > would be more readable. I suppose what you want to see at a glance, the > 16-bit value, or the value for the native sensor bit depth. Up to you. > If you decide to change it, please update all helpers. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> That is a difficult one. I'd like to stick to the current version. Without it, I think I would have missed that all Omnivision black levels are the same, even though the documentation mentions 64@12 and 16@10. Would it help to include comments above with the origin of the information? Regards, Stefan > > > > gainType_ = AnalogueGainLinear; > > > gainConstants_.linear = { 1, 0, 0, 128 }; > > > } > > > @@ -553,6 +554,7 @@ class CameraSensorHelperOv5640 : public CameraSensorHelper > > > public: > > > CameraSensorHelperOv5640() > > > { > > > + blackLevel_ = 1024; > > > gainType_ = AnalogueGainLinear; > > > gainConstants_.linear = { 1, 0, 0, 16 }; > > > } > > > diff --git a/src/ipa/rkisp1/data/ov4689.yaml b/src/ipa/rkisp1/data/ov4689.yaml > > > index 2068684cafcd..609012967e02 100644 > > > --- a/src/ipa/rkisp1/data/ov4689.yaml > > > +++ b/src/ipa/rkisp1/data/ov4689.yaml > > > @@ -6,8 +6,4 @@ algorithms: > > > - Agc: > > > - Awb: > > > - BlackLevelCorrection: > > > - R: 66 > > > - Gr: 66 > > > - Gb: 66 > > > - B: 66 > > > ... > > > diff --git a/src/ipa/rkisp1/data/ov5640.yaml b/src/ipa/rkisp1/data/ov5640.yaml > > > index 897b83cb435b..4b21d412e44e 100644 > > > --- a/src/ipa/rkisp1/data/ov5640.yaml > > > +++ b/src/ipa/rkisp1/data/ov5640.yaml > > > @@ -6,10 +6,6 @@ algorithms: > > > - Agc: > > > - Awb: > > > - BlackLevelCorrection: > > > - R: 256 > > > - Gr: 256 > > > - Gb: 256 > > > - B: 256 > > > - ColorProcessing: > > > - GammaSensorLinearization: > > > x-intervals: [ 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2 ] > > -- > Regards, > > Laurent Pinchart
Quoting Laurent Pinchart (2024-07-03 13:48:19) > On Wed, Jul 03, 2024 at 01:20:56PM +0100, Kieran Bingham wrote: > > Quoting Stefan Klug (2024-07-03 11:39:53) > > > Move black levels for tuning files that contained a BLC block into > > > the camera sensor helpers. > > > > > > ov4689.yaml had 66@12bit while the datasheet states 64@12bit. Use the > > > value from the datasheet (scaled to 16bit). > > > > I wonder who chose 66 here. Was it measured by someone perhaps? > > > > Git blame states "Mikhail Rudenko" (Added to Cc) Mikhail, any opinion on > > this change? > > > > Anyway, I don't object to this right now - I think once we release our > > updates to the IPA and tuning tools, most sensors 'supported' in > > libcamera will likely benefit from being '(re)tuned' correctly and > > 'fully' (and consistently?). > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > ov5640.yaml had 256@12bit while the datasheet states 16@10bit. Looking > > > at the commit message the 256 most likely stems from the imx219 tuning > > > file and 16@10bit is the same as the 64@12bit from the ov4689. This > > > seems more likely and is therefore used. > > > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > --- > > > src/ipa/libipa/camera_sensor_helper.cpp | 2 ++ > > > src/ipa/rkisp1/data/ov4689.yaml | 4 ---- > > > src/ipa/rkisp1/data/ov5640.yaml | 4 ---- > > > 3 files changed, 2 insertions(+), 8 deletions(-) > > > > > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp > > > index 3d0e756927f0..982e35beaa90 100644 > > > --- a/src/ipa/libipa/camera_sensor_helper.cpp > > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp > > > @@ -542,6 +542,7 @@ class CameraSensorHelperOv4689 : public CameraSensorHelper > > > public: > > > CameraSensorHelperOv4689() > > > { > > > + blackLevel_ = 1024; > > I wonder if > > blackLevel_ = 16 << 6; I like this idea ... except the '<< 6' isn't particularly clear at conveying the 16 is in 10 bit. I guess we could add a helper blackLevel_ = to16Bit(10 /*bit*/, 16) #define to16Bit(depth, val) ((val) << (16 - (depth))) Or some other magic ... but I definitely recall my confusion when I first looked at the black levels in a datasheet and saw different values at different bit depths, and had to convert again for RPi. The operation is simple of course - but it's the reason/intent that may not be clear to a new comer - so doing something to make that easier to bring up a new sensor is a good thing IMO. Could be something that gets tackled later though. -- Kieran > > would be more readable. I suppose what you want to see at a glance, the > 16-bit value, or the value for the native sensor bit depth. Up to you. > If you decide to change it, please update all helpers. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > gainType_ = AnalogueGainLinear; > > > gainConstants_.linear = { 1, 0, 0, 128 }; > > > } > > > @@ -553,6 +554,7 @@ class CameraSensorHelperOv5640 : public CameraSensorHelper > > > public: > > > CameraSensorHelperOv5640() > > > { > > > + blackLevel_ = 1024; > > > gainType_ = AnalogueGainLinear; > > > gainConstants_.linear = { 1, 0, 0, 16 }; > > > } > > > diff --git a/src/ipa/rkisp1/data/ov4689.yaml b/src/ipa/rkisp1/data/ov4689.yaml > > > index 2068684cafcd..609012967e02 100644 > > > --- a/src/ipa/rkisp1/data/ov4689.yaml > > > +++ b/src/ipa/rkisp1/data/ov4689.yaml > > > @@ -6,8 +6,4 @@ algorithms: > > > - Agc: > > > - Awb: > > > - BlackLevelCorrection: > > > - R: 66 > > > - Gr: 66 > > > - Gb: 66 > > > - B: 66 > > > ... > > > diff --git a/src/ipa/rkisp1/data/ov5640.yaml b/src/ipa/rkisp1/data/ov5640.yaml > > > index 897b83cb435b..4b21d412e44e 100644 > > > --- a/src/ipa/rkisp1/data/ov5640.yaml > > > +++ b/src/ipa/rkisp1/data/ov5640.yaml > > > @@ -6,10 +6,6 @@ algorithms: > > > - Agc: > > > - Awb: > > > - BlackLevelCorrection: > > > - R: 256 > > > - Gr: 256 > > > - Gb: 256 > > > - B: 256 > > > - ColorProcessing: > > > - GammaSensorLinearization: > > > x-intervals: [ 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2 ] > > -- > Regards, > > Laurent Pinchart
On Wed, Jul 03, 2024 at 03:11:04PM +0200, Stefan Klug wrote: > On Wed, Jul 03, 2024 at 03:48:19PM +0300, Laurent Pinchart wrote: > > On Wed, Jul 03, 2024 at 01:20:56PM +0100, Kieran Bingham wrote: > > > Quoting Stefan Klug (2024-07-03 11:39:53) > > > > Move black levels for tuning files that contained a BLC block into > > > > the camera sensor helpers. > > > > > > > > ov4689.yaml had 66@12bit while the datasheet states 64@12bit. Use the > > > > value from the datasheet (scaled to 16bit). > > > > > > I wonder who chose 66 here. Was it measured by someone perhaps? > > > > > > Git blame states "Mikhail Rudenko" (Added to Cc) Mikhail, any opinion on > > > this change? > > > > > > Anyway, I don't object to this right now - I think once we release our > > > updates to the IPA and tuning tools, most sensors 'supported' in > > > libcamera will likely benefit from being '(re)tuned' correctly and > > > 'fully' (and consistently?). > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > ov5640.yaml had 256@12bit while the datasheet states 16@10bit. Looking > > > > at the commit message the 256 most likely stems from the imx219 tuning > > > > file and 16@10bit is the same as the 64@12bit from the ov4689. This > > > > seems more likely and is therefore used. > > > > > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > > --- > > > > src/ipa/libipa/camera_sensor_helper.cpp | 2 ++ > > > > src/ipa/rkisp1/data/ov4689.yaml | 4 ---- > > > > src/ipa/rkisp1/data/ov5640.yaml | 4 ---- > > > > 3 files changed, 2 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp > > > > index 3d0e756927f0..982e35beaa90 100644 > > > > --- a/src/ipa/libipa/camera_sensor_helper.cpp > > > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp > > > > @@ -542,6 +542,7 @@ class CameraSensorHelperOv4689 : public CameraSensorHelper > > > > public: > > > > CameraSensorHelperOv4689() > > > > { > > > > + blackLevel_ = 1024; > > > > I wonder if > > > > blackLevel_ = 16 << 6; > > > > would be more readable. I suppose what you want to see at a glance, the > > 16-bit value, or the value for the native sensor bit depth. Up to you. > > If you decide to change it, please update all helpers. > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > That is a difficult one. I'd like to stick to the current version. > Without it, I think I would have missed that all Omnivision black levels > are the same, even though the documentation mentions 64@12 and 16@10. > Would it help to include comments above with the origin of the > information? I wouldn't mind comments. Up to you. > > > > gainType_ = AnalogueGainLinear; > > > > gainConstants_.linear = { 1, 0, 0, 128 }; > > > > } > > > > @@ -553,6 +554,7 @@ class CameraSensorHelperOv5640 : public CameraSensorHelper > > > > public: > > > > CameraSensorHelperOv5640() > > > > { > > > > + blackLevel_ = 1024; > > > > gainType_ = AnalogueGainLinear; > > > > gainConstants_.linear = { 1, 0, 0, 16 }; > > > > } > > > > diff --git a/src/ipa/rkisp1/data/ov4689.yaml b/src/ipa/rkisp1/data/ov4689.yaml > > > > index 2068684cafcd..609012967e02 100644 > > > > --- a/src/ipa/rkisp1/data/ov4689.yaml > > > > +++ b/src/ipa/rkisp1/data/ov4689.yaml > > > > @@ -6,8 +6,4 @@ algorithms: > > > > - Agc: > > > > - Awb: > > > > - BlackLevelCorrection: > > > > - R: 66 > > > > - Gr: 66 > > > > - Gb: 66 > > > > - B: 66 > > > > ... > > > > diff --git a/src/ipa/rkisp1/data/ov5640.yaml b/src/ipa/rkisp1/data/ov5640.yaml > > > > index 897b83cb435b..4b21d412e44e 100644 > > > > --- a/src/ipa/rkisp1/data/ov5640.yaml > > > > +++ b/src/ipa/rkisp1/data/ov5640.yaml > > > > @@ -6,10 +6,6 @@ algorithms: > > > > - Agc: > > > > - Awb: > > > > - BlackLevelCorrection: > > > > - R: 256 > > > > - Gr: 256 > > > > - Gb: 256 > > > > - B: 256 > > > > - ColorProcessing: > > > > - GammaSensorLinearization: > > > > x-intervals: [ 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2 ]
On Wed, Jul 03, 2024 at 02:19:00PM +0100, Kieran Bingham wrote: > Quoting Laurent Pinchart (2024-07-03 13:48:19) > > On Wed, Jul 03, 2024 at 01:20:56PM +0100, Kieran Bingham wrote: > > > Quoting Stefan Klug (2024-07-03 11:39:53) > > > > Move black levels for tuning files that contained a BLC block into > > > > the camera sensor helpers. > > > > > > > > ov4689.yaml had 66@12bit while the datasheet states 64@12bit. Use the > > > > value from the datasheet (scaled to 16bit). > > > > > > I wonder who chose 66 here. Was it measured by someone perhaps? > > > > > > Git blame states "Mikhail Rudenko" (Added to Cc) Mikhail, any opinion on > > > this change? > > > > > > Anyway, I don't object to this right now - I think once we release our > > > updates to the IPA and tuning tools, most sensors 'supported' in > > > libcamera will likely benefit from being '(re)tuned' correctly and > > > 'fully' (and consistently?). > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > ov5640.yaml had 256@12bit while the datasheet states 16@10bit. Looking > > > > at the commit message the 256 most likely stems from the imx219 tuning > > > > file and 16@10bit is the same as the 64@12bit from the ov4689. This > > > > seems more likely and is therefore used. > > > > > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > > --- > > > > src/ipa/libipa/camera_sensor_helper.cpp | 2 ++ > > > > src/ipa/rkisp1/data/ov4689.yaml | 4 ---- > > > > src/ipa/rkisp1/data/ov5640.yaml | 4 ---- > > > > 3 files changed, 2 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp > > > > index 3d0e756927f0..982e35beaa90 100644 > > > > --- a/src/ipa/libipa/camera_sensor_helper.cpp > > > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp > > > > @@ -542,6 +542,7 @@ class CameraSensorHelperOv4689 : public CameraSensorHelper > > > > public: > > > > CameraSensorHelperOv4689() > > > > { > > > > + blackLevel_ = 1024; > > > > I wonder if > > > > blackLevel_ = 16 << 6; > > I like this idea ... except the '<< 6' isn't particularly clear at > conveying the 16 is in 10 bit. Possibly blackLevel_ = 16 << (16 - 10); but then you start wondering if the left 16 is related to the right 16 :-) I think we can stop bikeshedding on this and let Stefan decide. > > I guess we could add a helper > blackLevel_ = to16Bit(10 /*bit*/, 16) > > #define to16Bit(depth, val) ((val) << (16 - (depth))) It's functions or function templates in C++ ;-) > Or some other magic ... but I definitely recall my confusion when I > first looked at the black levels in a datasheet and saw different values > at different bit depths, and had to convert again for RPi. The operation > is simple of course - but it's the reason/intent that may not be clear > to a new comer - so doing something to make that easier to bring up a > new sensor is a good thing IMO. > > Could be something that gets tackled later though. > > > > > would be more readable. I suppose what you want to see at a glance, the > > 16-bit value, or the value for the native sensor bit depth. Up to you. > > If you decide to change it, please update all helpers. > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > gainType_ = AnalogueGainLinear; > > > > gainConstants_.linear = { 1, 0, 0, 128 }; > > > > } > > > > @@ -553,6 +554,7 @@ class CameraSensorHelperOv5640 : public CameraSensorHelper > > > > public: > > > > CameraSensorHelperOv5640() > > > > { > > > > + blackLevel_ = 1024; > > > > gainType_ = AnalogueGainLinear; > > > > gainConstants_.linear = { 1, 0, 0, 16 }; > > > > } > > > > diff --git a/src/ipa/rkisp1/data/ov4689.yaml b/src/ipa/rkisp1/data/ov4689.yaml > > > > index 2068684cafcd..609012967e02 100644 > > > > --- a/src/ipa/rkisp1/data/ov4689.yaml > > > > +++ b/src/ipa/rkisp1/data/ov4689.yaml > > > > @@ -6,8 +6,4 @@ algorithms: > > > > - Agc: > > > > - Awb: > > > > - BlackLevelCorrection: > > > > - R: 66 > > > > - Gr: 66 > > > > - Gb: 66 > > > > - B: 66 > > > > ... > > > > diff --git a/src/ipa/rkisp1/data/ov5640.yaml b/src/ipa/rkisp1/data/ov5640.yaml > > > > index 897b83cb435b..4b21d412e44e 100644 > > > > --- a/src/ipa/rkisp1/data/ov5640.yaml > > > > +++ b/src/ipa/rkisp1/data/ov5640.yaml > > > > @@ -6,10 +6,6 @@ algorithms: > > > > - Agc: > > > > - Awb: > > > > - BlackLevelCorrection: > > > > - R: 256 > > > > - Gr: 256 > > > > - Gb: 256 > > > > - B: 256 > > > > - ColorProcessing: > > > > - GammaSensorLinearization: > > > > x-intervals: [ 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2 ]
Hi, Kieran! On 2024-07-03 at 13:20 +01, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > Quoting Stefan Klug (2024-07-03 11:39:53) >> Move black levels for tuning files that contained a BLC block into >> the camera sensor helpers. >> >> ov4689.yaml had 66@12bit while the datasheet states 64@12bit. Use the >> value from the datasheet (scaled to 16bit). > > I wonder who chose 66 here. Was it measured by someone perhaps? > > Git blame states "Mikhail Rudenko" (Added to Cc) Mikhail, any opinion on > this change? Back then I measured that number, not knowing that it's specified in the datasheet. I think we should stick to the datasheet, so Acked-by: Mikhail Rudenko <mike.rudenko@gmail.com> > Anyway, I don't object to this right now - I think once we release our > updates to the IPA and tuning tools, most sensors 'supported' in > libcamera will likely benefit from being '(re)tuned' correctly and > 'fully' (and consistently?). > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> ov5640.yaml had 256@12bit while the datasheet states 16@10bit. Looking >> at the commit message the 256 most likely stems from the imx219 tuning >> file and 16@10bit is the same as the 64@12bit from the ov4689. This >> seems more likely and is therefore used. >> >> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> >> --- >> src/ipa/libipa/camera_sensor_helper.cpp | 2 ++ >> src/ipa/rkisp1/data/ov4689.yaml | 4 ---- >> src/ipa/rkisp1/data/ov5640.yaml | 4 ---- >> 3 files changed, 2 insertions(+), 8 deletions(-) >> >> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp >> index 3d0e756927f0..982e35beaa90 100644 >> --- a/src/ipa/libipa/camera_sensor_helper.cpp >> +++ b/src/ipa/libipa/camera_sensor_helper.cpp >> @@ -542,6 +542,7 @@ class CameraSensorHelperOv4689 : public CameraSensorHelper >> public: >> CameraSensorHelperOv4689() >> { >> + blackLevel_ = 1024; >> gainType_ = AnalogueGainLinear; >> gainConstants_.linear = { 1, 0, 0, 128 }; >> } >> @@ -553,6 +554,7 @@ class CameraSensorHelperOv5640 : public CameraSensorHelper >> public: >> CameraSensorHelperOv5640() >> { >> + blackLevel_ = 1024; >> gainType_ = AnalogueGainLinear; >> gainConstants_.linear = { 1, 0, 0, 16 }; >> } >> diff --git a/src/ipa/rkisp1/data/ov4689.yaml b/src/ipa/rkisp1/data/ov4689.yaml >> index 2068684cafcd..609012967e02 100644 >> --- a/src/ipa/rkisp1/data/ov4689.yaml >> +++ b/src/ipa/rkisp1/data/ov4689.yaml >> @@ -6,8 +6,4 @@ algorithms: >> - Agc: >> - Awb: >> - BlackLevelCorrection: >> - R: 66 >> - Gr: 66 >> - Gb: 66 >> - B: 66 >> ... >> diff --git a/src/ipa/rkisp1/data/ov5640.yaml b/src/ipa/rkisp1/data/ov5640.yaml >> index 897b83cb435b..4b21d412e44e 100644 >> --- a/src/ipa/rkisp1/data/ov5640.yaml >> +++ b/src/ipa/rkisp1/data/ov5640.yaml >> @@ -6,10 +6,6 @@ algorithms: >> - Agc: >> - Awb: >> - BlackLevelCorrection: >> - R: 256 >> - Gr: 256 >> - Gb: 256 >> - B: 256 >> - ColorProcessing: >> - GammaSensorLinearization: >> x-intervals: [ 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2 ] >> -- >> 2.43.0 >> -- Best regards, Mikhail Rudenko
diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp index 3d0e756927f0..982e35beaa90 100644 --- a/src/ipa/libipa/camera_sensor_helper.cpp +++ b/src/ipa/libipa/camera_sensor_helper.cpp @@ -542,6 +542,7 @@ class CameraSensorHelperOv4689 : public CameraSensorHelper public: CameraSensorHelperOv4689() { + blackLevel_ = 1024; gainType_ = AnalogueGainLinear; gainConstants_.linear = { 1, 0, 0, 128 }; } @@ -553,6 +554,7 @@ class CameraSensorHelperOv5640 : public CameraSensorHelper public: CameraSensorHelperOv5640() { + blackLevel_ = 1024; gainType_ = AnalogueGainLinear; gainConstants_.linear = { 1, 0, 0, 16 }; } diff --git a/src/ipa/rkisp1/data/ov4689.yaml b/src/ipa/rkisp1/data/ov4689.yaml index 2068684cafcd..609012967e02 100644 --- a/src/ipa/rkisp1/data/ov4689.yaml +++ b/src/ipa/rkisp1/data/ov4689.yaml @@ -6,8 +6,4 @@ algorithms: - Agc: - Awb: - BlackLevelCorrection: - R: 66 - Gr: 66 - Gb: 66 - B: 66 ... diff --git a/src/ipa/rkisp1/data/ov5640.yaml b/src/ipa/rkisp1/data/ov5640.yaml index 897b83cb435b..4b21d412e44e 100644 --- a/src/ipa/rkisp1/data/ov5640.yaml +++ b/src/ipa/rkisp1/data/ov5640.yaml @@ -6,10 +6,6 @@ algorithms: - Agc: - Awb: - BlackLevelCorrection: - R: 256 - Gr: 256 - Gb: 256 - B: 256 - ColorProcessing: - GammaSensorLinearization: x-intervals: [ 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2 ]
Move black levels for tuning files that contained a BLC block into the camera sensor helpers. ov4689.yaml had 66@12bit while the datasheet states 64@12bit. Use the value from the datasheet (scaled to 16bit). ov5640.yaml had 256@12bit while the datasheet states 16@10bit. Looking at the commit message the 256 most likely stems from the imx219 tuning file and 16@10bit is the same as the 64@12bit from the ov4689. This seems more likely and is therefore used. Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- src/ipa/libipa/camera_sensor_helper.cpp | 2 ++ src/ipa/rkisp1/data/ov4689.yaml | 4 ---- src/ipa/rkisp1/data/ov5640.yaml | 4 ---- 3 files changed, 2 insertions(+), 8 deletions(-)