[libcamera-devel,RFC,00/17] libcamera: Replace CameraData with Camera::Private

13086 mbox series

Hello,

This patch series reworks the libcamera core and pipeline handlers to
drop the CameraData class, replacing it with Camera::Private.

The Camera::Private class implements the d-pointer design pattern for
the Camera class, and stores internal camera data that shouldn't be
visible through the public API. The CameraData class is a container used
by pipeline handlers to store per-camera private data. The two classes
thus overlap in their purpose.

This series cleans up this situation by merging CameraData into
Camera::Private. Pipeline handlers then subclass Camera::Private instead
of CameraData, and pass the private data directly to the Camera
constructor.

The series starts with two documentation enhancements for the Extensible
class (01/17 and 02/17). Patch 03/17 then allows decoupling the
construction of the public and private classes when using the Extensible
infrastructure. This is required to allow pipeline handlers to
instantiate their custom class derived from Camera::Extensible and pass
it to the Camera constructor manually.

Patches 04/17 and 05/17 are additional documentation improvements.
Patches 06/17 to 08/17 make the Camera::Private class visible and usable
to pipeline handlers, and patch 09/17 then moves the members of the
existing CameraData class to Camera::Private. Patches 10/17 to 15/17
update pipeline handlers individual to migrate to Camera::Private. Patch
16/17 then cleans up by dropping the CameraData class, and patch 17/17
updates the pipeline handler guide accordingly.

Patch 03/17 is probably the biggest open question in this RFC. The
alternative would be to force pipeline handlers to subclass the Camera
class, which I believe should be doable (I haven't tried it though) but
would be a more intrusive change in the pipeline handlers. Decoupling
the instantiation of the public and private classes doesn't seem to
result in awful code, but it requires the public class constructor to
manually initialize the private class instance (at least in some cases).
I'm not entirely sure if this is a slipery slope or not.

The rest of the infrastructure, however, holds up pretty well I believe,
especially access control to the private classes that can use the usual
C++ public/protected/private access specifiers to restrict some members
to the corresponding public class only.

All pipeline handlers have been compile-tested, and I've runtime-tested
uvcvideo and vimc. Once the approach gets approved (or rejected, and the
series rewritten :-)), I'll test the other pipeline handlers.

Laurent Pinchart (17):
  libcamera: base: class: Document Extensible::_d() functions
  libcamera: base: class: Link LIBCAMERA_O_PTR to Extensible
    documentation
  libcamera: base: class: Don't pass Extensible pointer to Private
    constructor
  libcamera: frame_buffer: Document the FrameBuffer::Private class
  Documentation: Doxygen: Don't exclude Private classes
  libcamera: camera: Move Camera::Private to header file
  libcamera: camera: Make Camera::Private members private
  libcamera: camera: Pass Private pointer to Camera constructor
  libcamera: pipeline_handler: Move CameraData members to
    Camera::Private
  libcamera: pipeline: simple: Migrate to Camera::Private
  libcamera: pipeline: uvcvideo: Migrate to Camera::Private
  libcamera: pipeline: vimc: Migrate to Camera::Private
  libcamera: pipeline: rkisp1: Migrate to Camera::Private
  libcamera: pipeline: raspberrypi: Migrate to Camera::Private
  libcamera: pipeline: ipu3: Migrate to Camera::Private
  libcamera: pipeline_handler: Drop CameraData class
  Documentation: guides: pipeline-handler: Migrate to Camera::Private

 Documentation/Doxyfile.in                     |   2 +-
 Documentation/guides/pipeline-handler.rst     |  67 +++++-----
 include/libcamera/base/class.h                |   5 +-
 include/libcamera/camera.h                    |   5 +-
 include/libcamera/internal/camera.h           |  70 ++++++++++
 include/libcamera/internal/framebuffer.h      |   2 +-
 include/libcamera/internal/meson.build        |   1 +
 include/libcamera/internal/pipeline_handler.h |  29 +----
 src/android/camera_hal_config.cpp             |   7 +-
 src/android/mm/cros_camera_buffer.cpp         |   4 +-
 src/android/mm/generic_camera_buffer.cpp      |   3 +-
 src/libcamera/base/class.cpp                  |  30 ++++-
 src/libcamera/camera.cpp                      | 120 ++++++++++-------
 src/libcamera/camera_manager.cpp              |   8 +-
 src/libcamera/framebuffer.cpp                 |  16 ++-
 src/libcamera/pipeline/ipu3/ipu3.cpp          |  38 +++---
 .../pipeline/raspberrypi/raspberrypi.cpp      |  24 ++--
 src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  26 ++--
 src/libcamera/pipeline/simple/simple.cpp      |  25 ++--
 src/libcamera/pipeline/uvcvideo/uvcvideo.cpp  |  19 +--
 src/libcamera/pipeline/vimc/vimc.cpp          |  19 +--
 src/libcamera/pipeline_handler.cpp            | 121 ++----------------
 22 files changed, 321 insertions(+), 320 deletions(-)
 create mode 100644 include/libcamera/internal/camera.h