[libcamera-devel] [RFC PATCH 0/1] Raspberry Pi generalised embedded data parsing

[Naushir Patuck]

Hi Laurent,

On Tue, 4 May 2021 at 13:45, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> Hi Naush,
>
> On Tue, Mar 16, 2021 at 10:33:40AM +0000, Naushir Patuck wrote:
> > On Sun, 14 Mar 2021 at 22:30, Laurent Pinchart wrote:
> > > On Wed, Mar 10, 2021 at 05:23:47PM +0000, David Plowman wrote:
> > > > Hi
> > > >
> > > > I'm just submitting this patch for comments in the first instance
> > > > (mostly from Naush, I guess, but everyone is welcome!). It's part of
> > > > our plan for more flexible handling of metadata from the sensor.
> > > >
> > > > (The background is that we have some interesting sensors coming up
> > > > that give us other forms of embedded data, not just register
> > > > dumps, and we need to be able to deal with those!)
> > > >
> > > > The plan is to give our CamHelpers a Prepare() and a Process()
> method,
> > > > just like all our algorithms. As usual, Prepare() runs just before
> the
> > > > ISP starts, Process() just after. A version of Prepare() is provided
> > > > that has basically just sucked that little bit of
> > > > register-dump-parsing functionality out of the IPA file
> > > > (raspberrypi.cpp). Process() does nothing by default.
> > > >
> > > > There aren't actually many changes, but some observations on what
> I've
> > > > done:
> > > >
> > > > * I've not updated various CamHelper comments yet, that can wait!
> > > >
> > > > * I've made the Prepare() method responsible for reading the delayed
> > > >   control values if we can't use the metadata to get the
> > > >   exposure/gain. I wonder if perhaps that is better left in the
> > > >   IPA. Prepare() might indicate via a return value whether it found
> > > >   them in the embedded data or not.
> > > >
> > > > * The parser object is completely hidden behind the helper now, so
> the
> > > >   distinction between them is rather blurring. Maybe they could be
> > > >   combined, but that can happen in a later patch.
> > >
> > > I'm trying to figure out how this will play along with the refactoring
> > > of camera sensor helpers. There are a few points to consider:
> > >
> > > - The helpers are not specific to the Raspberry Pi IPA, and should thus
> > >   be moved to a common location. This is more or less a mechanical
> > >   change and shouldn't cause any issue by itself. The code could be
> > >   moved to libipa for instance.
> > >
> > > - We have sensor-specific data on the pipeline handler side, provided
> by
> > >   the CameraSensor class and passed to the IPA through
> CameraSensorInfo.
> > >   All the information is currently retrieved from the kernel driver.
> > >   Work is planned to create a sensor database that will allow
> hardcoding
> > >   sensor-specific information in libcamera.
> >
> > Would the sensor database be a drop in replacement for the CamHelper,
> > or do you think we will still have a CamHelper doing certain operations
> and
> > the sensor database is for static sensor properties?
>
> The sensor database will certainly contain static information. Generally
> speaking, the more we can use static data, the better. We can then have
> a cam helper class that would use the static data to perform common
> tasks, such as converting exposure time and gain between sensor-specific
> units and standard units. Those functions wouldn't need to be virtual
> anymore.
>
> I believe we'll still need virtual functions though, for things like
> embedded data parsing at least, and possibly other operations, as not
> everything can be expressed as data (well, the logic of a function can
> be expressed as data, in the worst case as assembly instructions, but
> that's going too far :-) - jokes aside, where to draw the line is an
> interesting question).
>

Yes, this is a tricky one.  I suspect we will just need to add support for
a whole bunch of sensors slowly, and a logical partition may emerge.


>
> > >   I'd like to see if we could centralize all sensor-specific
> information
> > >   on the pipeline handler side, or least for static data. This would
> > >   cover information related to delayed controls, which is currently
> > >   provided by the IPA but not used by it, but also information about
> > >   mistrusted frames, that would then be passed to the IPA by the
> > >   pipeline handler.
> >
> > This does seem like the right approach to me.
> >
> > > - There's a need for sensor-specific code (as opposed to data),
> > >   currently used on the IPA side. There's room for refactoring this
> > >   though, including replacing the virtual Gain and GainCode functions
> > >   with parametric formulas. For instance, the CCS specification
> computes
> > >   the sensor gain as either
> > >
> > >   - gain = (m0 * x + c0) / (m1 * x + c1), with x being the register
> > >     value, m0, m1, c0 and c1 being static parameters, and either m0 or
> > >     m1 being equal to 0 ; or
> > >
> > >   - gain = a * 2^x, with a and x being register values.
> > >
> > >   It seems this wouldn't match the IMX290, but we could add additional
> > >   formulas.
> >
> > I think allowing this to remain virtual might still be needed.
> >
> > Some sensors have entirely tables based gain -> code translations, so
> > we should not restrict to only parametric formulas.
>
> :-( Is this common ? The tables could be stored in the sensor database
> though.
>

I would not say it is common, but I have encountered a few over the years.
In these cases, storing a table in the database should not be too bothersome
I would hope.


>
> > Similar comment for
> > exposure lines -> time calculations.  Some sensors I've encountered
> > (global shutter and ultra long exposure modes) do not follow the typical
> > line length * frame_length approach.
>
> Do they need very device-specific conversion, or are there a set of
> common conversions that would cover all our needs ?
>

Again, this is not common, but I have seen device specific conversions
that may be required.  Unfortunately, this may not even be vendor specific.


>
> > > - For embedded data parsing, I wonder if it would be best handled in
> the
> > >   IPA or in the pipeline handler. The latter would have the advantage
> of
> > >   gathering more sensor-specific code on the pipeline handler side, as
> > >   well as not having to involve the IPA in the parsing for other types
> > >   of ancillary data produced by the sensors.
> >
> > Having the embedded data parsing in the pipeline handler does seem
> > like a reasonable thing to do.  However, we must keep in mind that
> > there may be vendor specific data in there (e.g. focus pixel stats) that
> > will need to be passed into the IPA.  This may happen through named
> > controls, or some other opaque way.
>
> Good point. When sensors produce raw PDAF statistics, are they
> transmitted as part of the "regular" embedded data, or separately ?
>

>From my very little experience with such sensors, it comes as embedded
data - the register set itself includes pdaf statistics.  I do know one
other
vendor that can provide an entirely separate stream for such statistics,
but our Unciam block does not support more than 2 streams.

Regards,
Naush


> > > > David Plowman (1):
> > > >   ipa: raspberrypi: Use CamHelpers to generalise embedded data
> parsing
> > > >
> > > >  src/ipa/raspberrypi/cam_helper.cpp  | 49 ++++++++++++++++
> > > >  src/ipa/raspberrypi/cam_helper.hpp  | 14 ++++-
> > > >  src/ipa/raspberrypi/raspberrypi.cpp | 88
> ++++++++---------------------
> > > >  3 files changed, 84 insertions(+), 67 deletions(-)
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210505/e2bacdb1/attachment-0001.htm>