[libcamera-devel] [PATCH 08/14] py: move conversion funcs to libcamera.utils

[Laurent Pinchart]

Hi Tomi,

On Tue, May 17, 2022 at 12:22:08PM +0300, Tomi Valkeinen wrote:
> On 17/05/2022 11:55, Laurent Pinchart wrote:
> > On Tue, May 17, 2022 at 11:38:03AM +0300, Tomi Valkeinen wrote:
> >> On 17/05/2022 11:23, Laurent Pinchart wrote:
> >>> On Mon, May 16, 2022 at 05:10:16PM +0300, Tomi Valkeinen wrote:
> >>>> cam_qt.py has a bunch of utility functions for pixel format conversions.
> >>>> Create a new libcamera submodule for these.
> >>>
> >>> I'm not sure I like this much. Do we really want to create a format
> >>> conversion library that will we'll eventually have to maintain with a
> >>> stable ABI ?
> >>
> >> That's a good question. I've been thinking a lot about what exactly
> >> should be bindings module contain.
> >>
> >> Just the bare-bone bindings? Then it's probably not very pythonic and
> >> not easy to use.
> >>
> >> Or try to keep to the bare bones feature-wise (i.e. no conversions
> >> features like this), but add convenience features to make it look more
> >> pythonic? Then the API deviates from the C++ API.
> >>
> >> Or bare-bone bindings, but a separate, pure python library on top? What
> >> do we call the libraries, then =). And if everyone uses the pure python
> >> lib, what is the point of a separate bare-bone library?
> > 
> > I'd like the bare-bone bindings to be as close as possible to the C++
> > API, with differences being clearly documented and globally consistent
> > (e.g. all toString() functions could be replaced by __str__(), camelCase
> > translated to snake_case, ...). That's more or less what we have today,
> > and it doesn't seem problematic to me.
> > 
> > I expect we'll have convenience helpers that offer a higher-level API,
> > implemented in Python, and I expect both the low-level and the
> > high-level APIs to be used by different applications. Naming is a good
> > question though.
> 
> Just thinking out loud, I think we have some classes of "levels":
> 
> 1) Really bare bones, reflects C++ API if at all possible
> 
> 2) Minor pythonic helpers (like the geometry constructors taking tuples, 
> or unpacking). I think these are usually add-on functions/properties to 
> existing classes.
> 
> 3) Helper classes like MappedFrameBuffer
> 
> 4) Higher level module which possibly fully hides the libcamera module below
> 
> The last one is clearly outside the scope here. 2) is currently in the 
> native bindings. I think it could be moved to the __init__.py of the 
> libcamera module, although I'm not sure if that's always easily 
> possible. 3) Would be in a submodule.

Agreed. For 2) it could still be in C++ if that's the easiest option.

> This would give the user the following options:
> 
> import libcamera._libcamera (to get the bare-bones)
> import libcamera (to get the bare-bones + helper methods)
> 
> I don't think anyone would use libcamera._libcamera, and that's also 
> implied by the naming.
> 
> Then again, if no one uses libcamera._libcamera directly, having 2) in 
> the native bindings is not really an issue.

I'd make 2) part of the native bindings, yes, but would likely keep that
to "pythonic adaptations" of the API, without new features even if
small) that are not about making integration with Python easier.

> >> I'm currently thinking that this all should be under one libcamera
> >> module. "libcamera._libcamera" is the bare-bones native bindings (as it
> >> already is), and libcamera module exposes a more pythonic version with
> >> some helper classes (like the MappedFrameBuffer class we already have).
> >> We can then have other submodules, which are not needed for libcamera
> >> use, but provide extra features, like libcamera.utils. Perhaps
> >> MappedFrameBuffer should also be moved there.
> > 
> > I like the idea of moving extra features that are not provided by the
> > C++ API and that are not higher-level wrappers to a separate module.
> > MappedFrameBuffer should go there indeed (until libcamera gets a C++
> > Image class).
> > 
> >> I think the main libcamera and the libcamera.utils can be considered
> >> separate modules, and I don't see a problem with having stable libcamera
> >> API, but libcamera.utils is not stable. But to make it more obvious, we
> >> could as well have, say, libcamera.experimental module.
> > 
> > What's the point in an API that can't be used by anyone without a high
> > risk of breakage ? :-)
> 
> Well, things should be easy with Python. If the user has a raw bayer 
> camera, and the user asks how to see the image, and we reply that you 
> need to implement your own debayering function...
> 
> I think somehow we should offer code snippets or helpers that make using 
> libcamera easy, even if we don't really support those helpers. Of 
> course, preferably these kind of features should be found in other 
> libraries that specialize in that feature-set.

If we offer a CFA interpolation function, people will start using it,
complain when it breaks, and request improvements (in the quality and/or
speed for instance). I fear that project would very quickly get out of
hands. If we made it a separate Python library we could declare it as
unmaintained and welcome take-overs.

> >> That said, conversion functionality is something I'd expect to find from
> >> some other Python module. But if I recall right, I couldn't find a
> >> working bayer conversion.
> > 
> > What bothers me with a library that implement format conversion is that
> > it will very quickly become a large project of its own, so I don't think
> > it should be part of libcamera.
> 
> It shouldn't be part of the main libcamera module. But I don't see a 
> problem with offering experimental/example code as a submodule. It can 
> be a separate installable.

-- 
Regards,

Laurent Pinchart