<div dir="auto"><span class="gmail_chip gmail_plusreply" dir="auto"><a href="mailto:jacopo@jmondi.org" style="color:#15c;text-decoration:underline" target="_blank" rel="noreferrer">+Jacopo Mondi</a></span><span> </span><div dir="auto"><span><span class="gmail_chip gmail_plusreply" dir="auto"><a href="mailto:kieran.bingham@ideasonboard.com" style="color:#15c;text-decoration:underline" target="_blank" rel="noreferrer">+Kieran Bingham</a></span><span> </span></span></div><div dir="auto"><span><span><span class="gmail_chip gmail_plusreply" dir="auto"><a href="mailto:paul.elder@ideasonboard.com" style="color:#15c;text-decoration:underline">+paul.elder@ideasonboard.com</a></span><span> </span></span></span></div><div dir="auto"><span><span><span><span class="gmail_chip gmail_plusreply" dir="auto"><a href="mailto:laurent.pinchart@ideasonboard.com" style="color:#15c;text-decoration:underline">+Laurent Pinchart</a></span><span> </span></span></span></span></div><div dir="auto"><span><span><span><span><br></span></span></span></span></div><div dir="auto"><span><span><span><span>My attempt at keeping git-send-email from breaking the thread has failed. Looks like I need to adjust my incantation. Continuing here:</span></span></span></span></div><div dir="auto"><span><span><span><span><br></span></span></span></span></div><div dir="auto"><br><span style="font-size:12.8px">Hi Nicholas,</span><br style="font-size:12.8px"><br style="font-size:12.8px"><span style="font-size:12.8px">On Tue, Oct 25, 2022 at 11:40:11PM -0500, Nicholas Roth wrote:</span><br style="font-size:12.8px"><span style="font-size:12.8px">> +Kieran Bingham <</span><a href="mailto:kieran.bingham@ideasonboard.com" style="text-decoration-line:none;color:rgb(66,133,244);font-size:12.8px">kieran.bingham@ideasonboard.com</a><span style="font-size:12.8px">> since Laurent is out.</span><div style="color:rgb(80,0,80);font-size:12.8px" dir="auto"><br>><br>> > > Could this be computed the same way we do it in the IPU3 IPA module ?<br>> ><br>> > It looks like each of the computed IPU3 FrameDurationLimit values<br>> > correspond to the minimum and maximum v4l2 blanking intervals. I suppose<br>>> that makes sense, though I suspect the minimum frame duration will be<br>> > constrained by i2c bandwidth and ISP limitations at the highest resolutions<br>> > rather than the sensor blanking intervals based on the datasheet.<br>><br><br></div><span style="font-size:12.8px">>Mostly for sake of discussion, as you correctly noticed we report as</span><br style="font-size:12.8px"><span style="font-size:12.8px">>the camera frame duration the sensor's frame duration. I think this is</span><br style="font-size:12.8px"><span style="font-size:12.8px">>correct, as there might indeed be other bottlenecks in the capture</span> <span style="font-size:12.8px">pipeline, but I guess it's hard to quantify them in terms of latency</span><br style="font-size:12.8px"><span style="font-size:12.8px">>and they depend on the current camera configuration, ie the number</span><br style="font-size:12.8px"><span style="font-size:12.8px">> and resolution of the streams that are processed in a given capture session.</span><br style="font-size:12.8px"><br style="font-size:12.8px"><span style="font-size:12.8px">>I wonder if all platforms behaves the same when overloaded, some</span><br style="font-size:12.8px"><span style="font-size:12.8px">>might introduce latencies, some might drop buffers ? I'm also afraid</span><br style="font-size:12.8px"><span style="font-size:12.8px">>not all the numbers we would need to quantify something are actually</span><br style="font-size:12.8px"><span style="font-size:12.8px">>available...</span><br style="font-size:12.8px"><br style="font-size:12.8px"><span style="font-size:12.8px">>The sensor frame duration is shared by all streams generated from the same</span><br style="font-size:12.8px"><span style="font-size:12.8px">>and can be reported as a camera global parameter.</span><br style="font-size:12.8px"><br style="font-size:12.8px"><span style="font-size:12.8px">>I2c bandwidth doesn't seem a concern to me, during streaming i2c</span><br style="font-size:12.8px"><span style="font-size:12.8px">>traffic should be negligible and limited to the control of a few</span><br style="font-size:12.8px"><span style="font-size:12.8px">>sensor's parameters which are controlled at run-time, such as the</span><br style="font-size:12.8px"><span style="font-size:12.8px">exposure time and gain.</span></div><div dir="auto"><br></div><div dir="auto">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.<br style="font-size:12.8px"><br style="font-size:12.8px"><span style="font-size:12.8px">>There might of course be multiple processing steps to take into</span><br style="font-size:12.8px"><span style="font-size:12.8px">>account when actually computing the full duration of the frame since</span><br style="font-size:12.8px"><span style="font-size:12.8px">>the time it has been produced by the sensor to the time it is</span><br style="font-size:12.8px"><span style="font-size:12.8px">>delivered to applications. Android in example has a dedicated</span><br style="font-size:12.8px"><span style="font-size:12.8px">>property for 'stalling' streams that goes through, in example, JPEG</span><br style="font-size:12.8px"><span style="font-size:12.8px">>encoding, something that is not exactly in scope for libcamera (and</span><br style="font-size:12.8px"><span style="font-size:12.8px">>which we don't fully handle in the HAL yet).</span></div><div dir="auto"><span style="font-size:12.8px"><br></span></div><div dir="auto"><span style="font-size:12.8px">I believe in the RK3399 there is a max FPS for MJPEG of ~30fps at 1080p. That's a great point.<br></span><div style="color:rgb(80,0,80);font-size:12.8px" dir="auto"><br><br>>> I'm not really sure what the best thing to do here is. I'm concerned that<br>>> if we follow the IPU3 computation, we could easily end up asking for the<br>>> ISP to push more pixels than it has throughput for. A very conservative<br><br></div><span style="font-size:12.8px">>I don't have numbers here and I presume they're very platform</span><br style="font-size:12.8px"><span style="font-size:12.8px">>dependent, but I don't think we actually hit that limit on IPU3 which</span><br style="font-size:12.8px"><span style="font-size:12.8px">>is where the HAL has been mostly tested on. Which platform are you</span><br style="font-size:12.8px"><span style="font-size:12.8px">>working with ?</span></div><div dir="auto"><span style="font-size:12.8px"><br></span></div><div dir="auto"><span style="font-size:12.8px">I'm working with a PinePhone Pro. FOSS-compatible phone designed to run Linux with an RK3399 (Android 7 era).<br></span><div style="color:rgb(80,0,80);font-size:12.8px" dir="auto"><br><br>> approach might be to cap this at a few FPS, which is the advertised rate at<br>> the highest resolution, but I would not enjoy using the camera that way.<br>> Curious to read others' thoughts on this.<br>><br>> As a side-note, I'm really not sure why the IPU3 IPA treats<br>> FrameDurationLimits as a 3-tuple, when control_ids.yaml explicitly states:<br>> description: |<br>> The minimum and maximum (in that order) frame duration,<br>> expressed in microseconds.<br>> ...<br>> size: [2]<br><br></div><span style="font-size:12.8px">>What you see initialized with 3 values is the ControlInfo, which</span><br style="font-size:12.8px"><span style="font-size:12.8px">>reports the max, min and default values for a control. The control</span><br style="font-size:12.8px"><span style="font-size:12.8px">>itself accepts two values as indicated by the documentation.</span></div><div dir="auto"><span style="font-size:12.8px"><br></span></div><div dir="auto"><span style="font-size:12.8px">Noted, and I could confirm in the source. It does seem strange to be passing arguments nothing asked for though.<br></span><div style="color:rgb(80,0,80);font-size:12.8px" dir="auto"><br>><br>> It looks like updateControls is only called once, during init(). We would<br>> need a hook that changes this static configuration for each video mode.<br><br></div><span style="font-size:12.8px">>I do see updateControls() be called by configure() as well, and I see</span><br style="font-size:12.8px"><span style="font-size:12.8px">>the pipeline handler correctly updating the ControlInfoMap of camera</span><br style="font-size:12.8px"><span style="font-size:12.8px">>controls. Am I missing something ?</span><span><span><br></span></span></div><div dir="auto"><span style="font-size:12.8px"><br></span></div><div dir="auto"><span style="font-size:12.8px">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.</span></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Oct 27, 2022, 12:55 AM <<a href="mailto:nicholas@rothemail.net" target="_blank" rel="noreferrer">nicholas@rothemail.net</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">From: Nicholas Roth <<a href="mailto:nicholas@rothemail.net" rel="noreferrer noreferrer" target="_blank">nicholas@rothemail.net</a>><br>
<br>
Currently, the Android HAL does not work on rkisp1-based devices because<br>
required FrameDurationLimits metadata is missing from the ISP<br>
implementation.<br>
<br>
This change introduces sensible defaults for FrameDurationLimits that<br>
should generally work most of the time.<br>
<br>
In theory, it should be possible to implement more sophisticated logic<br>
to detect frame durations. The appropriate API hooks for doing so exist<br>
and are used in IPAIPU3::updateControls(), although the implementation<br>
may be suspect. This is left as future work.<br>
<br>
Signed-off-by: Nicholas Roth <<a href="mailto:nicholas@rothemail.net" rel="noreferrer noreferrer" target="_blank">nicholas@rothemail.net</a>><br>
---<br>
src/ipa/rkisp1/rkisp1.cpp | 7 +++++++<br>
1 file changed, 7 insertions(+)<br>
<br>
diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp<br>
index ba3c547e..c536852c 100644<br>
--- a/src/ipa/rkisp1/rkisp1.cpp<br>
+++ b/src/ipa/rkisp1/rkisp1.cpp<br>
@@ -100,6 +100,13 @@ const ControlInfoMap::Map rkisp1Controls{<br>
{ &controls::Contrast, ControlInfo(0.0f, 1.993f) },<br>
{ &controls::Saturation, ControlInfo(0.0f, 1.993f) },<br>
{ &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },<br>
+ /* libcamera requires a fixed value for minimum frame duration,<br>
+ * but this depends on the frame size and the rkisp1 device datasheets<br>
+ * measure this in pixels per second. Neither the datasheets nor the driver<br>
+ * specify a maximum. The minimum below is for 1920x1920. The maximum<br>
+ * corresponds to two seconds. */<br>
+ { &controls::FrameDurationLimits, ControlInfo(48505, 2000000) },<br>
+ { &controls::draft::MaxLatency, ControlInfo(0) },<br>
{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },<br>
};<br>
<br>
-- <br>
2.34.1<br>
<br>
</blockquote></div>