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

[Laurent Pinchart]

Hi David,

On Tue, Mar 16, 2021 at 04:40:41PM +0000, David Plowman wrote:
> Hi Naush, Laurent
> 
> Thanks for the replies on this subject. In fact I'm still fiddling around,
> and discovering some new problems, so there's certainly still some way to
> go!
> 
> One new thing I'm wrestling with at the moment is a sensor where the
> "usual" relationship between binned and full res modes, in respect of noise
> levels, doesn't hold. I suppose this is all information you could encode in
> a database, though I wonder whether it might become quite burdensome.

It has to be encoded somewhere, right ? I don't know the details for
this specific sensor, but from your description I don't think we
could/should convey that information through V4L2, so it would need to
be stored somewhere in userspace.

> As things stand currently, I'm thinking about making
> CamHelper::SetCameraMode virtual, so that we can amend the mode for certain
> sensors. The CameraMode that we pass to our SwitchMode methods would have
> to come from the helper.

Following our recent discussions on the topic of virtual functions, I'm
not completely opposed to them as they have valid use cases, but I also
believe that most sensor features could be described with static data in
most cases. Static data has the advantage of providing more information
than "I've mangled the parameters you gave me, here's the result" to the
IPA, possibly enabling it to make more clever choices. Maybe a good
middle ground would be to make these camera helper functions virtual for
corner cases, with a base implementation that works for most sensors,
based on static data.

> On some of the other questions:
> 
> On Tue, 16 Mar 2021 at 10:33, 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.
>
> I don't like what I did here. I'd prefer to move the code that fetches
> values from the controls out of the helper into the IPA, that is,
> src/ipa/raspberrypi/raspberrypi.cpp.

Sounds good to me.

> >> > * 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 suspect a database is not enough. More below...
> 
> >>   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.
> 
> Agree. I have a sensor where some modes can't have a gain less than (in
> this case) 1.12x. I'm "hiding" that in the CamHelper methods. Also the
> binned mode produces 2x the signal level of the full res mode. This too I'm
> "hiding" in the CamHelper methods (though it doesn't feel like the best
> solution.) There seem to be quite a few "awkward" devices out there!

That seems a reasonable place to handle that, regarless of whether we
express the information as code, or as data used by helper functions. I
believe we'll end up merging the camera helpers and the camera sensor
database into a single entity at some point.

> >> - 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.
> 
> I'd be OK to see embedded data parsing move out of the helper, though I
> think that being able to cope with other types of metadata is a powerful
> feature. It might be focus pixels, or other kinds of image analysis. In
> general I would expect the CamHelper::Prepare() method to put this
> information into the Raspberry Pi metadata object. From there, an algorithm
> would be loaded by being listed in the json file, which would retrieve the
> item from the Raspberry Pi metadata and handle it.
> 
> I'm thinking I should perhaps submit another version that encapsulates my
> latest thoughts on what I think I need - then we can consider what might
> benefit from being done differently.

Sounds good to me too. We probably want to experiment anyway, as there
are quite a few new features and use cases here. We don't need to aim
for perfection from day one :-) Please consider my comments as part of
the brainstorming effort, I'm particularly interested in knowing what
you think makes sense, either right away or as a future enhancement, and
what wouldn't be a good idea. 

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