[v6,0/2] Fix ControlSerializer deserializing array controls
mbox series

Message ID 20251117080818.3009835-1-paul.elder@ideasonboard.com
Headers show
Series
  • Fix ControlSerializer deserializing array controls
Related show

Message

Paul Elder Nov. 17, 2025, 8:08 a.m. UTC
This series fixes bug 285 [0] where running IPAs in isolation with array
controls would cause the control deserialization to fail, since the
deserializer didn't properly deserialize array controls.

This is fixed by making the deserializer properly deserialize array
controls, by adding all ControlValue metadata to the serialized form of
ControlInfoMap.

v6 notably moves ipa_control_value_entry into ipa_control_info_entry, as
opposed to serializing them in the same level like before.

v5 contains minor cleanups.

v4 notably cleans up unused fields in metatata structs, so it touches
ControlList as well.

In v3 we reuse struct ipa_control_value_entry to store the relevant
information instead, and add them to the serialized form of
ControlInfoMap.

In v2 the arrayness and size are stored in the serialized form of
ControlValue instead of using the information registered in the
ControlId. This allows us to support variable-length arrays (which v1
didn't), and allows us to support both non-array and array types min/max
ControlValues in the ControlInfo, depending on which type of min/max
value makes more sense for the specific control.

[0] https://bugs.libcamera.org/show_bug.cgi?id=285

Paul Elder (2):
  libcamera: control_serializer: Add array info to serialized
    ControlValue
  ipa: ipu3, mali-c55, rkisp1, rpi: Fix reporting non-scalar controls

 .../libcamera/internal/control_serializer.h   |   8 +-
 include/libcamera/ipa/ipa_controls.h          |  16 +-
 src/ipa/ipu3/ipu3.cpp                         |   4 +-
 src/ipa/mali-c55/mali-c55.cpp                 |   4 +-
 src/ipa/rkisp1/algorithms/awb.cpp             |   4 +-
 src/ipa/rkisp1/rkisp1.cpp                     |   3 +-
 src/ipa/rpi/common/ipa_base.cpp               |   7 +-
 src/libcamera/control_serializer.cpp          | 106 +++++++----
 src/libcamera/ipa_controls.cpp                | 178 ++++++++++--------
 9 files changed, 197 insertions(+), 133 deletions(-)

Comments

Jacopo Mondi Nov. 20, 2025, 3:45 p.m. UTC | #1
Hi Paul, Stefan

I'm running with this patches applied, but probably camshark need an
updated as well

  File "camshark/venv/lib64/python3.13/site-packages/pyqtgraph/parametertree/Parameter.py", line 785, in child
    param = self.names[names[0]]
            ~~~~~~~~~~^^^^^^^^^^
KeyError: 'ColourGains'

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "camshark/src/camshark/camshark.py", line 585, in on_camera_info
    param = controlsParam.child(name)
  File "camshark/venv/lib64/python3.13/site-packages/pyqtgraph/parametertree/Parameter.py", line 787, in child
    raise KeyError(f"Parameter {self.name()} has no child named {names[0]}") from e
KeyError: 'Parameter Controls has no child named ColourGains'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "camshark/src/camshark/camshark.py", line 587, in on_camera_info
    p = self.create_parameter_for_control(name, desc)
  File "camshark/src/camshark/camshark.py", line 537, in create_parameter_for_control
    if desc['default'] not in span:
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^
ValueError: operands could not be broadcast together with shapes (201,) (2,)

let me know if I should file a bug report

On Mon, Nov 17, 2025 at 05:08:13PM +0900, Paul Elder wrote:
> This series fixes bug 285 [0] where running IPAs in isolation with array
> controls would cause the control deserialization to fail, since the
> deserializer didn't properly deserialize array controls.
>
> This is fixed by making the deserializer properly deserialize array
> controls, by adding all ControlValue metadata to the serialized form of
> ControlInfoMap.
>
> v6 notably moves ipa_control_value_entry into ipa_control_info_entry, as
> opposed to serializing them in the same level like before.
>
> v5 contains minor cleanups.
>
> v4 notably cleans up unused fields in metatata structs, so it touches
> ControlList as well.
>
> In v3 we reuse struct ipa_control_value_entry to store the relevant
> information instead, and add them to the serialized form of
> ControlInfoMap.
>
> In v2 the arrayness and size are stored in the serialized form of
> ControlValue instead of using the information registered in the
> ControlId. This allows us to support variable-length arrays (which v1
> didn't), and allows us to support both non-array and array types min/max
> ControlValues in the ControlInfo, depending on which type of min/max
> value makes more sense for the specific control.
>
> [0] https://bugs.libcamera.org/show_bug.cgi?id=285
>
> Paul Elder (2):
>   libcamera: control_serializer: Add array info to serialized
>     ControlValue
>   ipa: ipu3, mali-c55, rkisp1, rpi: Fix reporting non-scalar controls
>
>  .../libcamera/internal/control_serializer.h   |   8 +-
>  include/libcamera/ipa/ipa_controls.h          |  16 +-
>  src/ipa/ipu3/ipu3.cpp                         |   4 +-
>  src/ipa/mali-c55/mali-c55.cpp                 |   4 +-
>  src/ipa/rkisp1/algorithms/awb.cpp             |   4 +-
>  src/ipa/rkisp1/rkisp1.cpp                     |   3 +-
>  src/ipa/rpi/common/ipa_base.cpp               |   7 +-
>  src/libcamera/control_serializer.cpp          | 106 +++++++----
>  src/libcamera/ipa_controls.cpp                | 178 ++++++++++--------
>  9 files changed, 197 insertions(+), 133 deletions(-)
>
> --
> 2.47.2
>
Jacopo Mondi Nov. 24, 2025, 11:22 a.m. UTC | #2
Hello again

with these patches applied I can succesfully run '--list-controls'
which was the original use case reported in bug
https://gitlab.freedesktop.org/camera/libcamera/-/issues/285

However, trying to -set- any control in isolation mode fails:

[2:22:39.059666250] [846] FATAL Serializer control_serializer.cpp:628 A list of V4L2 controls requires a ControlInfoMap
8559.087839 (30.08 fps) cam0-stream0 seq: 000048 bytesused: 2073600/1036800
Backtrace:
libcamera::ControlList libcamera::ControlSerializer::deserialize<libcamera::ControlList>(libcamera::ByteStreamBuffer&)+0x350 (../src/libcamera/control_serializer.cpp:631)
libcamera::IPADataSerializer<libcamera::ControlList, void>::deserialize(__gnu_cxx::__normal_iterator<unsigned char const*, std::vector<unsigned char, std::allocator<unsigned char> > >, __gnu_cxx::__normal_iterato
r<unsigned char const*, std::vector<unsigned char, std::allocator<unsigned char> > >, libcamera::ControlSerializer*)+0x2a8 (../src/libcamera/ipa_data_serializer.cpp:394)
IPAProxyRkISP1Worker::readyRead()+0x1320 (src/libcamera/proxy/worker/rkisp1_ipa_proxy_worker.cpp:301)
libcamera::BoundMethodMember<IPAProxyRkISP1Worker, void>::activate(bool)+0x80 (../include/libcamera/base/bound_method.h:169)
libcamera::Signal<>::emit()+0x70 (../include/libcamera/base/signal.h:146)
libcamera::IPCUnixSocket::dataNotifier()+0x15c (../src/libcamera/ipc_unixsocket.cpp:348)
libcamera::BoundMethodMember<libcamera::IPCUnixSocket, void>::activate(bool)+0x80 (../include/libcamera/base/bound_method.h:169)
libcamera::Signal<>::emit()+0x70 (../include/libcamera/base/signal.h:146)
libcamera::EventDispatcherPoll::processNotifiers(std::vector<pollfd, std::allocator<pollfd> > const&)+0x260 (../src/libcamera/base/event_dispatcher_poll.cpp:280)
libcamera::EventDispatcherPoll::processEvents()+0x1f4 (../src/libcamera/base/event_dispatcher_poll.cpp:171)
IPAProxyRkISP1Worker::run()+0x34 (src/libcamera/proxy/worker/rkisp1_ipa_proxy_worker.cpp:392)
main+0x3ec (src/libcamera/proxy/worker/rkisp1_ipa_proxy_worker.cpp:556)
??? [0x0000ffffab89229c] (/usr/lib/aarch64-linux-gnu/libc.so.6 [0x0000ffffab89229c])
__libc_start_main+0x9c (/usr/lib/aarch64-linux-gnu/libc.so.6 [0x0000ffffab89237c])
Exiting

I've filled in more details in the issue.

On Thu, Nov 20, 2025 at 04:45:02PM +0100, Jacopo Mondi wrote:
> Hi Paul, Stefan
>
> I'm running with this patches applied, but probably camshark need an
> updated as well
>
>   File "camshark/venv/lib64/python3.13/site-packages/pyqtgraph/parametertree/Parameter.py", line 785, in child
>     param = self.names[names[0]]
>             ~~~~~~~~~~^^^^^^^^^^
> KeyError: 'ColourGains'
>
> The above exception was the direct cause of the following exception:
>
> Traceback (most recent call last):
>   File "camshark/src/camshark/camshark.py", line 585, in on_camera_info
>     param = controlsParam.child(name)
>   File "camshark/venv/lib64/python3.13/site-packages/pyqtgraph/parametertree/Parameter.py", line 787, in child
>     raise KeyError(f"Parameter {self.name()} has no child named {names[0]}") from e
> KeyError: 'Parameter Controls has no child named ColourGains'
>
> During handling of the above exception, another exception occurred:
>
> Traceback (most recent call last):
>   File "camshark/src/camshark/camshark.py", line 587, in on_camera_info
>     p = self.create_parameter_for_control(name, desc)
>   File "camshark/src/camshark/camshark.py", line 537, in create_parameter_for_control
>     if desc['default'] not in span:
>        ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> ValueError: operands could not be broadcast together with shapes (201,) (2,)
>
> let me know if I should file a bug report
>
> On Mon, Nov 17, 2025 at 05:08:13PM +0900, Paul Elder wrote:
> > This series fixes bug 285 [0] where running IPAs in isolation with array
> > controls would cause the control deserialization to fail, since the
> > deserializer didn't properly deserialize array controls.
> >
> > This is fixed by making the deserializer properly deserialize array
> > controls, by adding all ControlValue metadata to the serialized form of
> > ControlInfoMap.
> >
> > v6 notably moves ipa_control_value_entry into ipa_control_info_entry, as
> > opposed to serializing them in the same level like before.
> >
> > v5 contains minor cleanups.
> >
> > v4 notably cleans up unused fields in metatata structs, so it touches
> > ControlList as well.
> >
> > In v3 we reuse struct ipa_control_value_entry to store the relevant
> > information instead, and add them to the serialized form of
> > ControlInfoMap.
> >
> > In v2 the arrayness and size are stored in the serialized form of
> > ControlValue instead of using the information registered in the
> > ControlId. This allows us to support variable-length arrays (which v1
> > didn't), and allows us to support both non-array and array types min/max
> > ControlValues in the ControlInfo, depending on which type of min/max
> > value makes more sense for the specific control.
> >
> > [0] https://bugs.libcamera.org/show_bug.cgi?id=285
> >
> > Paul Elder (2):
> >   libcamera: control_serializer: Add array info to serialized
> >     ControlValue
> >   ipa: ipu3, mali-c55, rkisp1, rpi: Fix reporting non-scalar controls
> >
> >  .../libcamera/internal/control_serializer.h   |   8 +-
> >  include/libcamera/ipa/ipa_controls.h          |  16 +-
> >  src/ipa/ipu3/ipu3.cpp                         |   4 +-
> >  src/ipa/mali-c55/mali-c55.cpp                 |   4 +-
> >  src/ipa/rkisp1/algorithms/awb.cpp             |   4 +-
> >  src/ipa/rkisp1/rkisp1.cpp                     |   3 +-
> >  src/ipa/rpi/common/ipa_base.cpp               |   7 +-
> >  src/libcamera/control_serializer.cpp          | 106 +++++++----
> >  src/libcamera/ipa_controls.cpp                | 178 ++++++++++--------
> >  9 files changed, 197 insertions(+), 133 deletions(-)
> >
> > --
> > 2.47.2
> >
Jacopo Mondi Nov. 24, 2025, 3:24 p.m. UTC | #3
I've updated the issue, and the error is not related to serialization
but it is a consequence of the cam test application assigning to a
Request's control list a newly created one which doesn't have a
valid ControlInfoMap.

In general we should disallow any pattern like

Request->controls() = new ControlList();

and only allow applications to set/merge new controls in the
ControlList created by the Request class constructor.

On Mon, Nov 24, 2025 at 12:22:49PM +0100, Jacopo Mondi wrote:
> Hello again
>
> with these patches applied I can succesfully run '--list-controls'
> which was the original use case reported in bug
> https://gitlab.freedesktop.org/camera/libcamera/-/issues/285
>
> However, trying to -set- any control in isolation mode fails:
>
> [2:22:39.059666250] [846] FATAL Serializer control_serializer.cpp:628 A list of V4L2 controls requires a ControlInfoMap
> 8559.087839 (30.08 fps) cam0-stream0 seq: 000048 bytesused: 2073600/1036800
> Backtrace:
> libcamera::ControlList libcamera::ControlSerializer::deserialize<libcamera::ControlList>(libcamera::ByteStreamBuffer&)+0x350 (../src/libcamera/control_serializer.cpp:631)
> libcamera::IPADataSerializer<libcamera::ControlList, void>::deserialize(__gnu_cxx::__normal_iterator<unsigned char const*, std::vector<unsigned char, std::allocator<unsigned char> > >, __gnu_cxx::__normal_iterato
> r<unsigned char const*, std::vector<unsigned char, std::allocator<unsigned char> > >, libcamera::ControlSerializer*)+0x2a8 (../src/libcamera/ipa_data_serializer.cpp:394)
> IPAProxyRkISP1Worker::readyRead()+0x1320 (src/libcamera/proxy/worker/rkisp1_ipa_proxy_worker.cpp:301)
> libcamera::BoundMethodMember<IPAProxyRkISP1Worker, void>::activate(bool)+0x80 (../include/libcamera/base/bound_method.h:169)
> libcamera::Signal<>::emit()+0x70 (../include/libcamera/base/signal.h:146)
> libcamera::IPCUnixSocket::dataNotifier()+0x15c (../src/libcamera/ipc_unixsocket.cpp:348)
> libcamera::BoundMethodMember<libcamera::IPCUnixSocket, void>::activate(bool)+0x80 (../include/libcamera/base/bound_method.h:169)
> libcamera::Signal<>::emit()+0x70 (../include/libcamera/base/signal.h:146)
> libcamera::EventDispatcherPoll::processNotifiers(std::vector<pollfd, std::allocator<pollfd> > const&)+0x260 (../src/libcamera/base/event_dispatcher_poll.cpp:280)
> libcamera::EventDispatcherPoll::processEvents()+0x1f4 (../src/libcamera/base/event_dispatcher_poll.cpp:171)
> IPAProxyRkISP1Worker::run()+0x34 (src/libcamera/proxy/worker/rkisp1_ipa_proxy_worker.cpp:392)
> main+0x3ec (src/libcamera/proxy/worker/rkisp1_ipa_proxy_worker.cpp:556)
> ??? [0x0000ffffab89229c] (/usr/lib/aarch64-linux-gnu/libc.so.6 [0x0000ffffab89229c])
> __libc_start_main+0x9c (/usr/lib/aarch64-linux-gnu/libc.so.6 [0x0000ffffab89237c])
> Exiting
>
> I've filled in more details in the issue.
>
> On Thu, Nov 20, 2025 at 04:45:02PM +0100, Jacopo Mondi wrote:
> > Hi Paul, Stefan
> >
> > I'm running with this patches applied, but probably camshark need an
> > updated as well
> >
> >   File "camshark/venv/lib64/python3.13/site-packages/pyqtgraph/parametertree/Parameter.py", line 785, in child
> >     param = self.names[names[0]]
> >             ~~~~~~~~~~^^^^^^^^^^
> > KeyError: 'ColourGains'
> >
> > The above exception was the direct cause of the following exception:
> >
> > Traceback (most recent call last):
> >   File "camshark/src/camshark/camshark.py", line 585, in on_camera_info
> >     param = controlsParam.child(name)
> >   File "camshark/venv/lib64/python3.13/site-packages/pyqtgraph/parametertree/Parameter.py", line 787, in child
> >     raise KeyError(f"Parameter {self.name()} has no child named {names[0]}") from e
> > KeyError: 'Parameter Controls has no child named ColourGains'
> >
> > During handling of the above exception, another exception occurred:
> >
> > Traceback (most recent call last):
> >   File "camshark/src/camshark/camshark.py", line 587, in on_camera_info
> >     p = self.create_parameter_for_control(name, desc)
> >   File "camshark/src/camshark/camshark.py", line 537, in create_parameter_for_control
> >     if desc['default'] not in span:
> >        ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > ValueError: operands could not be broadcast together with shapes (201,) (2,)
> >
> > let me know if I should file a bug report
> >
> > On Mon, Nov 17, 2025 at 05:08:13PM +0900, Paul Elder wrote:
> > > This series fixes bug 285 [0] where running IPAs in isolation with array
> > > controls would cause the control deserialization to fail, since the
> > > deserializer didn't properly deserialize array controls.
> > >
> > > This is fixed by making the deserializer properly deserialize array
> > > controls, by adding all ControlValue metadata to the serialized form of
> > > ControlInfoMap.
> > >
> > > v6 notably moves ipa_control_value_entry into ipa_control_info_entry, as
> > > opposed to serializing them in the same level like before.
> > >
> > > v5 contains minor cleanups.
> > >
> > > v4 notably cleans up unused fields in metatata structs, so it touches
> > > ControlList as well.
> > >
> > > In v3 we reuse struct ipa_control_value_entry to store the relevant
> > > information instead, and add them to the serialized form of
> > > ControlInfoMap.
> > >
> > > In v2 the arrayness and size are stored in the serialized form of
> > > ControlValue instead of using the information registered in the
> > > ControlId. This allows us to support variable-length arrays (which v1
> > > didn't), and allows us to support both non-array and array types min/max
> > > ControlValues in the ControlInfo, depending on which type of min/max
> > > value makes more sense for the specific control.
> > >
> > > [0] https://bugs.libcamera.org/show_bug.cgi?id=285
> > >
> > > Paul Elder (2):
> > >   libcamera: control_serializer: Add array info to serialized
> > >     ControlValue
> > >   ipa: ipu3, mali-c55, rkisp1, rpi: Fix reporting non-scalar controls
> > >
> > >  .../libcamera/internal/control_serializer.h   |   8 +-
> > >  include/libcamera/ipa/ipa_controls.h          |  16 +-
> > >  src/ipa/ipu3/ipu3.cpp                         |   4 +-
> > >  src/ipa/mali-c55/mali-c55.cpp                 |   4 +-
> > >  src/ipa/rkisp1/algorithms/awb.cpp             |   4 +-
> > >  src/ipa/rkisp1/rkisp1.cpp                     |   3 +-
> > >  src/ipa/rpi/common/ipa_base.cpp               |   7 +-
> > >  src/libcamera/control_serializer.cpp          | 106 +++++++----
> > >  src/libcamera/ipa_controls.cpp                | 178 ++++++++++--------
> > >  9 files changed, 197 insertions(+), 133 deletions(-)
> > >
> > > --
> > > 2.47.2
> > >