[libcamera-devel,v2,00/14] Use ControlList for both libcamera and V4L2 controls
mbox series

Message ID 20191012184407.31684-1-laurent.pinchart@ideasonboard.com
Headers show
Series
  • Use ControlList for both libcamera and V4L2 controls
Related show

Message

Laurent Pinchart Oct. 12, 2019, 6:43 p.m. UTC
Hello,

This patch series generalises usage of ControlList for all controls,
both libcamera and V4L2.

The rationale is that using a single container to store controls allows
better sharing of code, especially when we will implement serialisation
and deserialisation. Additionally, a single container type simplifies
the IPA API.

Compared to v1, the series has been rebased on top of the IPA API, which
unveiled issues I hadn't foreseen. Large parts of the code have thus
been rewritten.

Patches 01/14 and 02/14 are small cleanups and fixes, and patch 03/14
restores the global list of libcamera controls that used to be
auto-generated. While this wasn't strictly required in v1, this version
now depends on it.

The next three patches are also small enhancements to simplify usage of
ControlValue, ControlList and ControlId (04/14 to 06/14).

Patch 07/14 is the first large change, and extends the ControlLis API to
support accessing controls by numerical ID. This was previously
implemented in the V4L2ControlList class and got moved to ControlList as
this API will likely be used for deserialisation. As a result, the
ControlList now needs to be constructed with a reference to a
ControlIdMap to support mapping numerical IDs to ControlId instances.

Patch 08/14 adds a new test for V4L2 controls, based on the vivid
driver. The test uses the existing V4L2ControlList API, and gets
migrated to ControlList further down in the series, to ensure that the
API switch doesn't create regressions.

Patch 09/14 is another small enhancement, after which patch 10/14 starts
the migration of V4L2 controls by using ControlId in V4L2ControlInfo. To
do so, a new V4L2ControlId class is introduced that simply wraps
ControlId with a convenience constructor. Patch 11/14 then removes the
V4L2ControlInfo::type field that duplicates ControlId::type, and patch
12/14 turns V4L2ControlInfoMap into a real class in preparation for
extending it.

Patch 13/14 is the bulk of the work, and by far the largest in the
series (but with a nice net removal of 168 lines of code). It moves
V4L2ControlList to become an optional thin convenience wrapper around
ControlList, and ControlList can be used directly for V4L2 controls if
desired.

Finally patch 14/14 merges the control and v4l2controls fields in
IPAOperationData, simplifying the IPA API.

More code sharing between V4L2 and libcamera controls may still be
possible, especially around V4L2ControlInfo and V4L2ControlInfoMap. I
believe it can be built on top of this series.

The series is based on top of the master branch, patches have been
compiled and tested individually with gcc 5.4.0, and the whole series
has been compiled with gcc 5 to 9 and clang 6 to 8. Niklas, would you be
able to run your manual rkisp1 IPA tests ?

Laurent Pinchart (14):
  libcamera: pipeline: rkisp1: Avoid copy assignment of V4L2 control map
  libcamera: control_ids: Fix documentation for controls namespace
  libcamera: control_ids: Generate map of all supported controls
  libcamera: controls: Add comparison operators for ControlValue
  libcamera: controls: Default ControlList validator argument to nullptr
  libcamera: controls: Store control name in ControlId
  libcamera: controls: Support accessing controls by numerical ID
  test: v4l2_videodevice: Add V4L2 control test
  libcamera: v4l2_device: Avoid copy of V4L2ControlInfo
  libcamera: v4l2_controls: Add V4L2ControlId
  libcamera: v4l2_controls: Remove V4L2ControlInfo type field
  libcamera: v4l2_controls: Turn V4L2ControlInfoMap into a class
  libcamera: v4l2_device: Replace V4L2ControlList with ControlList
  libcamera: ipa: Merge controls and v4l2controls in IPAOperationData

 include/ipa/ipa_interface.h              |   1 -
 include/libcamera/control_ids.h.in       |   2 +
 include/libcamera/controls.h             |  30 ++-
 src/ipa/rkisp1/rkisp1.cpp                |  22 +-
 src/libcamera/camera_sensor.cpp          |   4 +-
 src/libcamera/control_ids.cpp.in         |  20 +-
 src/libcamera/controls.cpp               | 181 ++++++++++++---
 src/libcamera/gen-controls.py            |  21 +-
 src/libcamera/include/camera_sensor.h    |   7 +-
 src/libcamera/include/v4l2_controls.h    |  74 +++---
 src/libcamera/include/v4l2_device.h      |   6 +-
 src/libcamera/ipa_interface.cpp          |   8 -
 src/libcamera/pipeline/ipu3/ipu3.cpp     |   9 +-
 src/libcamera/pipeline/rkisp1/rkisp1.cpp |   8 +-
 src/libcamera/pipeline/uvcvideo.cpp      |  24 +-
 src/libcamera/pipeline/vimc.cpp          |  25 +--
 src/libcamera/request.cpp                |   5 +-
 src/libcamera/v4l2_controls.cpp          | 273 ++++++++---------------
 src/libcamera/v4l2_device.cpp            |  71 +++---
 test/controls/control_list.cpp           |   2 +-
 test/v4l2_videodevice/controls.cpp       |  98 ++++++++
 test/v4l2_videodevice/meson.build        |   1 +
 22 files changed, 526 insertions(+), 366 deletions(-)
 create mode 100644 test/v4l2_videodevice/controls.cpp

Comments

Jacopo Mondi Oct. 13, 2019, 1:32 p.m. UTC | #1
Hi Laurent,
    thank you for this work

On Sat, Oct 12, 2019 at 09:43:53PM +0300, Laurent Pinchart wrote:
> Hello,
>
> This patch series generalises usage of ControlList for all controls,
> both libcamera and V4L2.

Thank you for having clarified my design related concerns.
You can hvae my
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
for the first 6 patches, while I'll review the other ones separately
but I think I will mostly find minor issues now.

Thanks
   j

>
> The rationale is that using a single container to store controls allows
> better sharing of code, especially when we will implement serialisation
> and deserialisation. Additionally, a single container type simplifies
> the IPA API.
>
> Compared to v1, the series has been rebased on top of the IPA API, which
> unveiled issues I hadn't foreseen. Large parts of the code have thus
> been rewritten.
>
> Patches 01/14 and 02/14 are small cleanups and fixes, and patch 03/14
> restores the global list of libcamera controls that used to be
> auto-generated. While this wasn't strictly required in v1, this version
> now depends on it.
>
> The next three patches are also small enhancements to simplify usage of
> ControlValue, ControlList and ControlId (04/14 to 06/14).
>
> Patch 07/14 is the first large change, and extends the ControlLis API to
> support accessing controls by numerical ID. This was previously
> implemented in the V4L2ControlList class and got moved to ControlList as
> this API will likely be used for deserialisation. As a result, the
> ControlList now needs to be constructed with a reference to a
> ControlIdMap to support mapping numerical IDs to ControlId instances.
>
> Patch 08/14 adds a new test for V4L2 controls, based on the vivid
> driver. The test uses the existing V4L2ControlList API, and gets
> migrated to ControlList further down in the series, to ensure that the
> API switch doesn't create regressions.
>
> Patch 09/14 is another small enhancement, after which patch 10/14 starts
> the migration of V4L2 controls by using ControlId in V4L2ControlInfo. To
> do so, a new V4L2ControlId class is introduced that simply wraps
> ControlId with a convenience constructor. Patch 11/14 then removes the
> V4L2ControlInfo::type field that duplicates ControlId::type, and patch
> 12/14 turns V4L2ControlInfoMap into a real class in preparation for
> extending it.
>
> Patch 13/14 is the bulk of the work, and by far the largest in the
> series (but with a nice net removal of 168 lines of code). It moves
> V4L2ControlList to become an optional thin convenience wrapper around
> ControlList, and ControlList can be used directly for V4L2 controls if
> desired.
>
> Finally patch 14/14 merges the control and v4l2controls fields in
> IPAOperationData, simplifying the IPA API.
>
> More code sharing between V4L2 and libcamera controls may still be
> possible, especially around V4L2ControlInfo and V4L2ControlInfoMap. I
> believe it can be built on top of this series.
>
> The series is based on top of the master branch, patches have been
> compiled and tested individually with gcc 5.4.0, and the whole series
> has been compiled with gcc 5 to 9 and clang 6 to 8. Niklas, would you be
> able to run your manual rkisp1 IPA tests ?
>
> Laurent Pinchart (14):
>   libcamera: pipeline: rkisp1: Avoid copy assignment of V4L2 control map
>   libcamera: control_ids: Fix documentation for controls namespace
>   libcamera: control_ids: Generate map of all supported controls
>   libcamera: controls: Add comparison operators for ControlValue
>   libcamera: controls: Default ControlList validator argument to nullptr
>   libcamera: controls: Store control name in ControlId
>   libcamera: controls: Support accessing controls by numerical ID
>   test: v4l2_videodevice: Add V4L2 control test
>   libcamera: v4l2_device: Avoid copy of V4L2ControlInfo
>   libcamera: v4l2_controls: Add V4L2ControlId
>   libcamera: v4l2_controls: Remove V4L2ControlInfo type field
>   libcamera: v4l2_controls: Turn V4L2ControlInfoMap into a class
>   libcamera: v4l2_device: Replace V4L2ControlList with ControlList
>   libcamera: ipa: Merge controls and v4l2controls in IPAOperationData
>
>  include/ipa/ipa_interface.h              |   1 -
>  include/libcamera/control_ids.h.in       |   2 +
>  include/libcamera/controls.h             |  30 ++-
>  src/ipa/rkisp1/rkisp1.cpp                |  22 +-
>  src/libcamera/camera_sensor.cpp          |   4 +-
>  src/libcamera/control_ids.cpp.in         |  20 +-
>  src/libcamera/controls.cpp               | 181 ++++++++++++---
>  src/libcamera/gen-controls.py            |  21 +-
>  src/libcamera/include/camera_sensor.h    |   7 +-
>  src/libcamera/include/v4l2_controls.h    |  74 +++---
>  src/libcamera/include/v4l2_device.h      |   6 +-
>  src/libcamera/ipa_interface.cpp          |   8 -
>  src/libcamera/pipeline/ipu3/ipu3.cpp     |   9 +-
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp |   8 +-
>  src/libcamera/pipeline/uvcvideo.cpp      |  24 +-
>  src/libcamera/pipeline/vimc.cpp          |  25 +--
>  src/libcamera/request.cpp                |   5 +-
>  src/libcamera/v4l2_controls.cpp          | 273 ++++++++---------------
>  src/libcamera/v4l2_device.cpp            |  71 +++---
>  test/controls/control_list.cpp           |   2 +-
>  test/v4l2_videodevice/controls.cpp       |  98 ++++++++
>  test/v4l2_videodevice/meson.build        |   1 +
>  22 files changed, 526 insertions(+), 366 deletions(-)
>  create mode 100644 test/v4l2_videodevice/controls.cpp
>
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Niklas Söderlund Oct. 13, 2019, 3:16 p.m. UTC | #2
Hi Laurent,

Thanks for your work.

On 2019-10-12 21:43:53 +0300, Laurent Pinchart wrote:
> The series is based on top of the master branch, patches have been
> compiled and tested individually with gcc 5.4.0, and the whole series
> has been compiled with gcc 5 to 9 and clang 6 to 8. Niklas, would you be
> able to run your manual rkisp1 IPA tests ?

I have now run my tests and all looks OK,

Tested-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>