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

[Laurent Pinchart]

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.

  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.

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

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

> 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