[00/12] Preparation for per-frame-controls and initial tests

19688 mbox series

Hi everyone,

unknowingly I stumbled into a topic that is way larger than I thought in the 
beginning. Delayed-controls and a closely related topic per-frame-controls.

I (now) know that a lot of work already went into this, noteably by 
Jacopo Mondi, David Plowman and Naushir Patuck:
https://patchwork.libcamera.org/cover/16458/
https://github.com/raspberrypi/libcamera/tree/pfc
Thanks for that!

I started off without per-frame-controls in mind (only delayed-controls)  
and soon realized, that these two are closely related. (It's difficult to
test delayed-controls in a real pipeline without working per-frame-controls)

To be able to tackle the whole topic in the long run I'd like to split it into parts:

1. Get a initial set of tests on mainline
I'd like to have them as a discussion basis and a reference to test against. 
These shouldn't be treated as mandatory for now (ideas on how to express that in 
code are very welcome)
- Initial lc-compliance tests for delayed-controls (and because these are system tests
 also for per-frame-controls)
- A reworked delayed controls class that hopefully makes things easier
- Tested on rkisp1 (details see below)

2. Get an agreement on "RaspberryPi" mode
In the previous work by David and Naushir, the (in my understanding) most prominent
requiremnet was the ability to set a control ASAP even when multiple 
requests are queued already (which makes complete sense IMHO). This requirement 
shall not get lost and I think there is a way to make it work.

3. Get the tests to pass on rkisp1, rpi and ipu3
I guess this will take a while and involve different parties, so let's not wait
until everything works everywhere...

4. Make the tests mandatory (details tbd)

This series contains part 1.

One change of thought is, that we add the ability to push requests to delayed
controls for a given sequence number and to re-push for the 
same sequence number. This has the benefit, that we don't have to decide 
early on if we operate in strict per-frame-controls mode (sometimes called 
"android mode") or in "raspberry pi" mode.

Stefan Klug (12):
  libcamera: lc-compliance: Add controls param to start() function
  libcamera: lc-compliance: Add TimeSheet class
  libcamera: lc-compliance: Add initial set of per-frame-control tests
  libcamera: delayed_controls: Update unit tests to expected semantics
  libcamera: delayed_controls: Rename class members
  libcamera: delayed_controls: Rework delayed controls implementation
  libcamera: delayed_controls: Add some logging
  libcamera: delayed_controls: Add ctrls list parameter to reset()
    function
  pipeline: rkisp1: Move call to setSensorControls.emit()
  pipeline: rkisp1: Add more debug logging
  pipeline: rkisp1: Fix per-frame-controls in manual mode
  pipeline: rkisp1: Apply initial controls

 include/libcamera/internal/delayed_controls.h |  20 +-
 include/libcamera/ipa/rkisp1.mojom            |   7 +-
 src/apps/lc-compliance/capture_test.cpp       |  46 +++
 src/apps/lc-compliance/meson.build            |   2 +
 src/apps/lc-compliance/per_frame_controls.cpp | 316 ++++++++++++++++++
 src/apps/lc-compliance/per_frame_controls.h   |  43 +++
 src/apps/lc-compliance/simple_capture.cpp     |   4 +-
 src/apps/lc-compliance/simple_capture.h       |   2 +-
 src/apps/lc-compliance/time_sheet.cpp         | 145 ++++++++
 src/apps/lc-compliance/time_sheet.h           |  55 +++
 src/ipa/rkisp1/algorithms/agc.cpp             |   4 +-
 src/ipa/rkisp1/rkisp1.cpp                     |  49 ++-
 src/libcamera/delayed_controls.cpp            | 235 ++++++++++---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  11 +-
 test/delayed_controls.cpp                     | 203 ++++++++---
 15 files changed, 1021 insertions(+), 121 deletions(-)
 create mode 100644 src/apps/lc-compliance/per_frame_controls.cpp
 create mode 100644 src/apps/lc-compliance/per_frame_controls.h
 create mode 100644 src/apps/lc-compliance/time_sheet.cpp
 create mode 100644 src/apps/lc-compliance/time_sheet.h

Hi Stefan,

I tried to send this for testing @ gitlab, but the patches didn't apply
to master.

What is your current base?

./send-for-testing.sh 4220
From https://git.libcamera.org/libcamera/libcamera
   2e2ba223f3a2..d54abd32affd  master     -> libcamera.org/master
From gitlab.freedesktop.org:camera/libcamera
   2e2ba223f3a2..d54abd32affd  master     -> gl.fdo/master
HEAD is now at 2e2ba223f3a2 libcamera: framebuffer_allocator: Remove entry if allocation fails
Previous HEAD position was 2e2ba223f3a2 libcamera: framebuffer_allocator: Remove entry if allocation fails
Branch 'testing/4220' set up to track remote branch 'master' from 'libcamera.org'.
Switched to a new branch 'testing/4220'
Applying: libcamera: delayed_controls: add ctrls list to reset function
error: sha1 information is lacking or useless (include/libcamera/internal/delayed_controls.h).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 libcamera: delayed_controls: add ctrls list to reset function
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

--
Kieran



Quoting Stefan Klug (2024-03-13 10:56:32)
> Hi everyone,
> 
> unknowingly I stumbled into a topic that is way larger than I thought in the 
> beginning. Delayed-controls and a closely related topic per-frame-controls.
> 
> I (now) know that a lot of work already went into this, noteably by 
> Jacopo Mondi, David Plowman and Naushir Patuck:
> https://patchwork.libcamera.org/cover/16458/
> https://github.com/raspberrypi/libcamera/tree/pfc
> Thanks for that!
> 
> I started off without per-frame-controls in mind (only delayed-controls)  
> and soon realized, that these two are closely related. (It's difficult to
> test delayed-controls in a real pipeline without working per-frame-controls)
> 
> To be able to tackle the whole topic in the long run I'd like to split it into parts:
> 
> 1. Get a initial set of tests on mainline
> I'd like to have them as a discussion basis and a reference to test against. 
> These shouldn't be treated as mandatory for now (ideas on how to express that in 
> code are very welcome)
> - Initial lc-compliance tests for delayed-controls (and because these are system tests
>  also for per-frame-controls)
> - A reworked delayed controls class that hopefully makes things easier
> - Tested on rkisp1 (details see below)
> 
> 2. Get an agreement on "RaspberryPi" mode
> In the previous work by David and Naushir, the (in my understanding) most prominent
> requiremnet was the ability to set a control ASAP even when multiple 
> requests are queued already (which makes complete sense IMHO). This requirement 
> shall not get lost and I think there is a way to make it work.
> 
> 3. Get the tests to pass on rkisp1, rpi and ipu3
> I guess this will take a while and involve different parties, so let's not wait
> until everything works everywhere...
> 
> 4. Make the tests mandatory (details tbd)
> 
> This series contains part 1.
> 
> One change of thought is, that we add the ability to push requests to delayed
> controls for a given sequence number and to re-push for the 
> same sequence number. This has the benefit, that we don't have to decide 
> early on if we operate in strict per-frame-controls mode (sometimes called 
> "android mode") or in "raspberry pi" mode.
> 
> Stefan Klug (12):
>   libcamera: lc-compliance: Add controls param to start() function
>   libcamera: lc-compliance: Add TimeSheet class
>   libcamera: lc-compliance: Add initial set of per-frame-control tests
>   libcamera: delayed_controls: Update unit tests to expected semantics
>   libcamera: delayed_controls: Rename class members
>   libcamera: delayed_controls: Rework delayed controls implementation
>   libcamera: delayed_controls: Add some logging
>   libcamera: delayed_controls: Add ctrls list parameter to reset()
>     function
>   pipeline: rkisp1: Move call to setSensorControls.emit()
>   pipeline: rkisp1: Add more debug logging
>   pipeline: rkisp1: Fix per-frame-controls in manual mode
>   pipeline: rkisp1: Apply initial controls
> 
>  include/libcamera/internal/delayed_controls.h |  20 +-
>  include/libcamera/ipa/rkisp1.mojom            |   7 +-
>  src/apps/lc-compliance/capture_test.cpp       |  46 +++
>  src/apps/lc-compliance/meson.build            |   2 +
>  src/apps/lc-compliance/per_frame_controls.cpp | 316 ++++++++++++++++++
>  src/apps/lc-compliance/per_frame_controls.h   |  43 +++
>  src/apps/lc-compliance/simple_capture.cpp     |   4 +-
>  src/apps/lc-compliance/simple_capture.h       |   2 +-
>  src/apps/lc-compliance/time_sheet.cpp         | 145 ++++++++
>  src/apps/lc-compliance/time_sheet.h           |  55 +++
>  src/ipa/rkisp1/algorithms/agc.cpp             |   4 +-
>  src/ipa/rkisp1/rkisp1.cpp                     |  49 ++-
>  src/libcamera/delayed_controls.cpp            | 235 ++++++++++---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  11 +-
>  test/delayed_controls.cpp                     | 203 ++++++++---
>  15 files changed, 1021 insertions(+), 121 deletions(-)
>  create mode 100644 src/apps/lc-compliance/per_frame_controls.cpp
>  create mode 100644 src/apps/lc-compliance/per_frame_controls.h
>  create mode 100644 src/apps/lc-compliance/time_sheet.cpp
>  create mode 100644 src/apps/lc-compliance/time_sheet.h
> 
> -- 
> 2.40.1
>