| Message ID | 20221028031726.4849-4-nicholas@rothemail.net |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Nicholas subject as ipa: rkisp1: Add FrameDurationLimits control This is not just for Android On Thu, Oct 27, 2022 at 10:17:19PM -0500, Nicholas Roth via libcamera-devel wrote: > From: Nicholas Roth <nicholas@rothemail.net> > > Currently, the Android HAL does not work on rkisp1-based devices because > required FrameDurationLimits metadata is missing from the ISP IPA = Image Processing Algorithm it's the software component part of libcamera that drives the image enhancement algorithms. IPA is libcamera lingo, in other context it might be simply called "3A library" even if it does way more than just Auto[exposure, white-balance, focus]. ISP = Image Signal Processor it's the HW component that performs image enhancement computations and produces statistics on the processed image The two terms are not interchangeable > implementation. > > This change introduces sensible defaults for FrameDurationLimits that > should generally work most of the time. > > In theory, it should be possible to implement more sophisticated logic > to detect frame durations. The appropriate API hooks for doing so exist > and are used in IPAIPU3::updateControls(), although the implementation > may be suspect. This is left as future work. > > Signed-off-by: Nicholas Roth <nicholas@rothemail.net> > --- > src/ipa/rkisp1/rkisp1.cpp | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > index ba3c547e..dd12c341 100644 > --- a/src/ipa/rkisp1/rkisp1.cpp > +++ b/src/ipa/rkisp1/rkisp1.cpp > @@ -100,6 +100,12 @@ const ControlInfoMap::Map rkisp1Controls{ > { &controls::Contrast, ControlInfo(0.0f, 1.993f) }, > { &controls::Saturation, ControlInfo(0.0f, 1.993f) }, > { &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) }, > + /* libcamera requires a fixed value for minimum frame duration, > + * but this depends on the frame size and the rkisp1 device datasheets > + * measure this in pixels per second. Neither the datasheets nor the driver What are you referring to as "the rkisp1 device datasheet measures this in pixels per second" ? Is it the ISP bandwidth ? Where is this information available from ? I'm still of opinion that we should report the sensor's duration limits, but maybe I'm missing a point here. > + * specify a maximum. The minimum below is for 1920x1920. The maximum Is 1920x1920 is the RkISP1 self-path maximum output resolution that you have used ? If the self-path has a scaler (something I'm not 100% sure) images in such resolution might be obtained by scaling the frame as produced by the sensor, or by simply cropping them. If you connect to the ISP a sensor whose minimum resolution and max framerate is 2592x1944@15FPS you would get a min frame duration of: (2592 + hblank) * (1944 + vblank) / pixel_rate = 66666 usec which is larger than what you report here, as the sensor would be clocking out frames at such rate, regardless of the final output resolution. Moreover I'm not sure how you got to 48505 from 1920x1920, have you taken into account the pixel rate of the sensor you're using ? This can't be hardcoded but should come from the CameraSensor, different platforms use different sensors and we can't assume anything about it in the IPA module. I would say that the ISP output size limits should not be taken into account. It would be different if we had a clear documentation about how bandwidth limitations constraints the durations, but I'm afraid the relationship between the two parameters is hard to measure, if ever documented. > + * corresponds to two seconds. */ Multi-line comments should be /* * comment on multiple * lines */ > + { &controls::FrameDurationLimits, ControlInfo(48505, 2000000) }, > { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }, > }; > > -- > 2.34.1 >
Hi all, On Fri, 28 Oct 2022 at 10:09, Jacopo Mondi via libcamera-devel < libcamera-devel@lists.libcamera.org> wrote: > Hi Nicholas > > subject as > > ipa: rkisp1: Add FrameDurationLimits control > > This is not just for Android > > On Thu, Oct 27, 2022 at 10:17:19PM -0500, Nicholas Roth via > libcamera-devel wrote: > > From: Nicholas Roth <nicholas@rothemail.net> > > > > Currently, the Android HAL does not work on rkisp1-based devices because > > required FrameDurationLimits metadata is missing from the ISP > > IPA = Image Processing Algorithm > it's the software component part of libcamera that drives the image > enhancement algorithms. IPA is libcamera lingo, in other context it > might be simply called "3A library" even if it does way more than just > Auto[exposure, white-balance, focus]. > > ISP = Image Signal Processor > it's the HW component that performs image enhancement computations and > produces statistics on the processed image > > The two terms are not interchangeable > > > implementation. > > > > This change introduces sensible defaults for FrameDurationLimits that > > should generally work most of the time. > > > > In theory, it should be possible to implement more sophisticated logic > > to detect frame durations. The appropriate API hooks for doing so exist > > and are used in IPAIPU3::updateControls(), although the implementation > > may be suspect. This is left as future work. > > > > Signed-off-by: Nicholas Roth <nicholas@rothemail.net> > > --- > > src/ipa/rkisp1/rkisp1.cpp | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > index ba3c547e..dd12c341 100644 > > --- a/src/ipa/rkisp1/rkisp1.cpp > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > @@ -100,6 +100,12 @@ const ControlInfoMap::Map rkisp1Controls{ > > { &controls::Contrast, ControlInfo(0.0f, 1.993f) }, > > { &controls::Saturation, ControlInfo(0.0f, 1.993f) }, > > { &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) }, > > + /* libcamera requires a fixed value for minimum frame duration, > > + * but this depends on the frame size and the rkisp1 device > datasheets > > + * measure this in pixels per second. Neither the datasheets nor > the driver > > What are you referring to as "the rkisp1 device datasheet measures > this in pixels per second" ? Is it the ISP bandwidth ? Where is this > information available from ? > > I'm still of opinion that we should report the sensor's duration > limits, but maybe I'm missing a point here. > > > > + * specify a maximum. The minimum below is for 1920x1920. The > maximum > > Is 1920x1920 is the RkISP1 self-path maximum output resolution that > you have used ? If the self-path has a scaler (something I'm not 100% > sure) images in such resolution might be obtained by scaling the frame > as produced by the sensor, or by simply cropping them. > > If you connect to the ISP a sensor whose minimum resolution and > max framerate is 2592x1944@15FPS you would get a min frame duration of: > > (2592 + hblank) * (1944 + vblank) / pixel_rate = 66666 > usec > > which is larger than what you report here, as the sensor would be > clocking out frames at such rate, regardless of the final output > resolution. Moreover I'm not sure how you got to 48505 from 1920x1920, > have you taken into account the pixel rate of the sensor you're using ? > This can't be hardcoded but should come from the CameraSensor, > different platforms use different sensors and we can't assume anything > about it in the IPA module. > > I would say that the ISP output size limits should not be taken into > account. It would be different if we had a clear documentation about > how bandwidth limitations constraints the durations, but I'm afraid > the relationship between the two parameters is hard to measure, if > ever documented. > I agree with Jacopo on this - FrameDurationLimits ought to specify the sensor limits and not the ISP. I can't speak for other platforms, but on a Raspberry Pi the ISP throughput depends on many many things, such as pipeline configuration, hardware clock rate, memory bandwidth availability to name a few. It would be wholly impractical to provide such a number in the FrameDurationLimits control. Naush > > > + * corresponds to two seconds. */ > > Multi-line comments should be > > /* > * comment on multiple > * lines > */ > > > + { &controls::FrameDurationLimits, ControlInfo(48505, 2000000) }, > > { &controls::draft::NoiseReductionMode, > ControlInfo(controls::draft::NoiseReductionModeValues) }, > > }; > > > > -- > > 2.34.1 > > >
diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp index ba3c547e..dd12c341 100644 --- a/src/ipa/rkisp1/rkisp1.cpp +++ b/src/ipa/rkisp1/rkisp1.cpp @@ -100,6 +100,12 @@ const ControlInfoMap::Map rkisp1Controls{ { &controls::Contrast, ControlInfo(0.0f, 1.993f) }, { &controls::Saturation, ControlInfo(0.0f, 1.993f) }, { &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) }, + /* libcamera requires a fixed value for minimum frame duration, + * but this depends on the frame size and the rkisp1 device datasheets + * measure this in pixels per second. Neither the datasheets nor the driver + * specify a maximum. The minimum below is for 1920x1920. The maximum + * corresponds to two seconds. */ + { &controls::FrameDurationLimits, ControlInfo(48505, 2000000) }, { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }, };