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

[Naushir Patuck]

Hi David and Laurent,

On Sun, 14 Mar 2021 at 22:30, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> Hi David,
>
> 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?


>
>   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.  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.


>
> - 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.

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
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210316/b4f12475/attachment.htm>