<div dir="ltr"><div dir="ltr">Hi Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 4 May 2021 at 13:45, 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 Naush,<br>
<br>
On Tue, Mar 16, 2021 at 10:33:40AM +0000, Naushir Patuck wrote:<br>
> On Sun, 14 Mar 2021 at 22:30, Laurent Pinchart wrote:<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>
> <br>
> Would the sensor database be a drop in replacement for the CamHelper,<br>
> or do you think we will still have a CamHelper doing certain operations and<br>
> the sensor database is for static sensor properties?<br>
<br>
The sensor database will certainly contain static information. Generally<br>
speaking, the more we can use static data, the better. We can then have<br>
a cam helper class that would use the static data to perform common<br>
tasks, such as converting exposure time and gain between sensor-specific<br>
units and standard units. Those functions wouldn't need to be virtual<br>
anymore.<br>
<br>
I believe we'll still need virtual functions though, for things like<br>
embedded data parsing at least, and possibly other operations, as not<br>
everything can be expressed as data (well, the logic of a function can<br>
be expressed as data, in the worst case as assembly instructions, but<br>
that's going too far :-) - jokes aside, where to draw the line is an<br>
interesting question).<br></blockquote><div><br></div><div>Yes, this is a tricky one. I suspect we will just need to add support for</div><div>a whole bunch of sensors slowly, and a logical partition may emerge.</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>
> <br>
> This does seem like the right approach to me.<br>
> <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>
> <br>
> I think allowing this to remain virtual might still be needed.<br>
> <br>
> Some sensors have entirely tables based gain -> code translations, so<br>
> we should not restrict to only parametric formulas.<br>
<br>
:-( Is this common ? The tables could be stored in the sensor database<br>
though.<br></blockquote><div><br></div><div>I would not say it is common, but I have encountered a few over the years.</div><div>In these cases, storing a table in the database should not be too bothersome</div><div>I would hope.</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>
> Similar comment for<br>
> exposure lines -> time calculations. Some sensors I've encountered<br>
> (global shutter and ultra long exposure modes) do not follow the typical<br>
> line length * frame_length approach.<br>
<br>
Do they need very device-specific conversion, or are there a set of<br>
common conversions that would cover all our needs ?<br></blockquote><div><br></div><div>Again, this is not common, but I have seen device specific conversions</div><div>that may be required. Unfortunately, this may not even be vendor specific.</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>
> <br>
> Having the embedded data parsing in the pipeline handler does seem<br>
> like a reasonable thing to do. However, we must keep in mind that<br>
> there may be vendor specific data in there (e.g. focus pixel stats) that<br>
> will need to be passed into the IPA. This may happen through named<br>
> controls, or some other opaque way.<br>
<br>
Good point. When sensors produce raw PDAF statistics, are they<br>
transmitted as part of the "regular" embedded data, or separately ?<br></blockquote><div><br></div><div></div><div>From my very little experience with such sensors, it comes as embedded</div><div>data - the register set itself includes pdaf statistics. I do know one other</div><div>vendor that can provide an entirely separate stream for such statistics,</div><div>but our Unciam block does not support more than 2 streams.</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></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>
</blockquote></div></div>