| Message ID | 20221027055515.321791-4-nicholas@rothemail.net |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
+Jacopo Mondi <jacopo@jmondi.org> +Kieran Bingham <kieran.bingham@ideasonboard.com> +paul.elder@ideasonboard.com <paul.elder@ideasonboard.com> +Laurent Pinchart <laurent.pinchart@ideasonboard.com> My attempt at keeping git-send-email from breaking the thread has failed. Looks like I need to adjust my incantation. Continuing here: Hi Nicholas, On Tue, Oct 25, 2022 at 11:40:11PM -0500, Nicholas Roth wrote: > +Kieran Bingham <kieran.bingham@ideasonboard.com> since Laurent is out. > > > > Could this be computed the same way we do it in the IPU3 IPA module ? > > > > It looks like each of the computed IPU3 FrameDurationLimit values > > correspond to the minimum and maximum v4l2 blanking intervals. I suppose >> that makes sense, though I suspect the minimum frame duration will be > > constrained by i2c bandwidth and ISP limitations at the highest resolutions > > rather than the sensor blanking intervals based on the datasheet. > >Mostly for sake of discussion, as you correctly noticed we report as >the camera frame duration the sensor's frame duration. I think this is >correct, as there might indeed be other bottlenecks in the capture pipeline, but I guess it's hard to quantify them in terms of latency >and they depend on the current camera configuration, ie the number > and resolution of the streams that are processed in a given capture session. >I wonder if all platforms behaves the same when overloaded, some >might introduce latencies, some might drop buffers ? I'm also afraid >not all the numbers we would need to quantify something are actually >available... >The sensor frame duration is shared by all streams generated from the same >and can be reported as a camera global parameter. >I2c bandwidth doesn't seem a concern to me, during streaming i2c >traffic should be negligible and limited to the control of a few >sensor's parameters which are controlled at run-time, such as the exposure time and gain. I was under the initial impression from the datasheet that the sensor streamed it's output over i2c but we probably agree that wouldn't make sense. If that's the case that, this sounds right. >There might of course be multiple processing steps to take into >account when actually computing the full duration of the frame since >the time it has been produced by the sensor to the time it is >delivered to applications. Android in example has a dedicated >property for 'stalling' streams that goes through, in example, JPEG >encoding, something that is not exactly in scope for libcamera (and >which we don't fully handle in the HAL yet). I believe in the RK3399 there is a max FPS for MJPEG of ~30fps at 1080p. That's a great point. >> I'm not really sure what the best thing to do here is. I'm concerned that >> if we follow the IPU3 computation, we could easily end up asking for the >> ISP to push more pixels than it has throughput for. A very conservative >I don't have numbers here and I presume they're very platform >dependent, but I don't think we actually hit that limit on IPU3 which >is where the HAL has been mostly tested on. Which platform are you >working with ? I'm working with a PinePhone Pro. FOSS-compatible phone designed to run Linux with an RK3399 (Android 7 era). > approach might be to cap this at a few FPS, which is the advertised rate at > the highest resolution, but I would not enjoy using the camera that way. > Curious to read others' thoughts on this. > > As a side-note, I'm really not sure why the IPU3 IPA treats > FrameDurationLimits as a 3-tuple, when control_ids.yaml explicitly states: > description: | > The minimum and maximum (in that order) frame duration, > expressed in microseconds. > ... > size: [2] >What you see initialized with 3 values is the ControlInfo, which >reports the max, min and default values for a control. The control >itself accepts two values as indicated by the documentation. Noted, and I could confirm in the source. It does seem strange to be passing arguments nothing asked for though. > > It looks like updateControls is only called once, during init(). We would > need a hook that changes this static configuration for each video mode. >I do see updateControls() be called by configure() as well, and I see >the pipeline handler correctly updating the ControlInfoMap of camera >controls. Am I missing something ? Nope! As I said in a previous email, this could be a viable option. I'm not sure I trust the IPU3 computation though so I'd like to go for a lighter-weight default until I can validate that this works, even if we do decide to go with the sensor blanking interval here. On Thu, Oct 27, 2022, 12:55 AM <nicholas@rothemail.net> 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 > 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 | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > index ba3c547e..c536852c 100644 > --- a/src/ipa/rkisp1/rkisp1.cpp > +++ b/src/ipa/rkisp1/rkisp1.cpp > @@ -100,6 +100,13 @@ 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::MaxLatency, ControlInfo(0) }, > { &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..c536852c 100644 --- a/src/ipa/rkisp1/rkisp1.cpp +++ b/src/ipa/rkisp1/rkisp1.cpp @@ -100,6 +100,13 @@ 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::MaxLatency, ControlInfo(0) }, { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }, };