<div dir="ltr"><div dir="ltr">Hi David and Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, 14 Mar 2021 at 22:30, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi David,<br>
<br>
On Wed, Mar 10, 2021 at 05:23:47PM +0000, David Plowman wrote:<br>
> Hi<br>
> <br>
> I'm just submitting this patch for comments in the first instance<br>
> (mostly from Naush, I guess, but everyone is welcome!). It's part of<br>
> our plan for more flexible handling of metadata from the sensor.<br>
> <br>
> (The background is that we have some interesting sensors coming up<br>
> that give us other forms of embedded data, not just register<br>
> dumps, and we need to be able to deal with those!)<br>
> <br>
> The plan is to give our CamHelpers a Prepare() and a Process() method,<br>
> just like all our algorithms. As usual, Prepare() runs just before the<br>
> ISP starts, Process() just after. A version of Prepare() is provided<br>
> that has basically just sucked that little bit of<br>
> register-dump-parsing functionality out of the IPA file<br>
> (raspberrypi.cpp). Process() does nothing by default.<br>
> <br>
> There aren't actually many changes, but some observations on what I've<br>
> done:<br>
> <br>
> * I've not updated various CamHelper comments yet, that can wait!<br>
> <br>
> * I've made the Prepare() method responsible for reading the delayed<br>
> control values if we can't use the metadata to get the<br>
> exposure/gain. I wonder if perhaps that is better left in the<br>
> IPA. Prepare() might indicate via a return value whether it found<br>
> them in the embedded data or not.<br>
> <br>
> * The parser object is completely hidden behind the helper now, so the<br>
> distinction between them is rather blurring. Maybe they could be<br>
> combined, but that can happen in a later patch.<br>
<br>
I'm trying to figure out how this will play along with the refactoring<br>
of camera sensor helpers. There are a few points to consider:<br>
<br>
- The helpers are not specific to the Raspberry Pi IPA, and should thus<br>
be moved to a common location. This is more or less a mechanical<br>
change and shouldn't cause any issue by itself. The code could be<br>
moved to libipa for instance.<br>
<br>
- We have sensor-specific data on the pipeline handler side, provided by<br>
the CameraSensor class and passed to the IPA through CameraSensorInfo.<br>
All the information is currently retrieved from the kernel driver.<br>
Work is planned to create a sensor database that will allow hardcoding<br>
sensor-specific information in libcamera.<br></blockquote><div><br></div><div>Would the sensor database be a drop in replacement for the CamHelper,</div><div>or do you think we will still have a CamHelper doing certain operations and</div><div>the sensor database is for static sensor properties?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
I'd like to see if we could centralize all sensor-specific information<br>
on the pipeline handler side, or least for static data. This would<br>
cover information related to delayed controls, which is currently<br>
provided by the IPA but not used by it, but also information about<br>
mistrusted frames, that would then be passed to the IPA by the<br>
pipeline handler.<br></blockquote><div><br></div><div>This does seem like the right approach to me.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- There's a need for sensor-specific code (as opposed to data),<br>
currently used on the IPA side. There's room for refactoring this<br>
though, including replacing the virtual Gain and GainCode functions<br>
with parametric formulas. For instance, the CCS specification computes<br>
the sensor gain as either<br>
<br>
- gain = (m0 * x + c0) / (m1 * x + c1), with x being the register<br>
value, m0, m1, c0 and c1 being static parameters, and either m0 or<br>
m1 being equal to 0 ; or<br>
<br>
- gain = a * 2^x, with a and x being register values.<br>
<br>
It seems this wouldn't match the IMX290, but we could add additional<br>
formulas.<br></blockquote><div><br></div><div>I think allowing this to remain virtual might still be needed.</div><div><br></div><div>Some sensors have entirely tables based gain -> code translations, so</div><div>we should not restrict to only parametric formulas. Similar comment for</div><div>exposure lines -> time calculations. Some sensors I've encountered</div><div>(global shutter and ultra long exposure modes) do not follow the typical</div><div>line length * frame_length approach.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- For embedded data parsing, I wonder if it would be best handled in the<br>
IPA or in the pipeline handler. The latter would have the advantage of<br>
gathering more sensor-specific code on the pipeline handler side, as<br>
well as not having to involve the IPA in the parsing for other types<br>
of ancillary data produced by the sensors.<br></blockquote><div><br></div><div>Having the embedded data parsing in the pipeline handler does seem</div><div>like a reasonable thing to do. However, we must keep in mind that</div><div>there may be vendor specific data in there (e.g. focus pixel stats) that</div><div>will need to be passed into the IPA. This may happen through named</div><div>controls, or some other opaque way.</div><div><br></div><div>Regards,</div><div>Naush</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> David Plowman (1):<br>
> ipa: raspberrypi: Use CamHelpers to generalise embedded data parsing<br>
> <br>
> src/ipa/raspberrypi/cam_helper.cpp | 49 ++++++++++++++++<br>
> src/ipa/raspberrypi/cam_helper.hpp | 14 ++++-<br>
> src/ipa/raspberrypi/raspberrypi.cpp | 88 ++++++++---------------------<br>
> 3 files changed, 84 insertions(+), 67 deletions(-)<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
_______________________________________________<br>
libcamera-devel mailing list<br>
<a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
<a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
</blockquote></div></div>