| Message ID | 20260925-v4l2-events-v1-1-b2ec212eb5d0@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
2026. 09. 25. 17:16 keltezéssel, Daniel Scally írta: > Add a base and derived classes to represent V4L2 Events to prepare > for more generic event handling in V4L2Device. > > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > --- > include/libcamera/internal/v4l2_event.h | 158 ++++++++++++++++++ > src/libcamera/meson.build | 1 + > src/libcamera/v4l2_event.cpp | 288 ++++++++++++++++++++++++++++++++ > 3 files changed, 447 insertions(+) > > diff --git a/include/libcamera/internal/v4l2_event.h b/include/libcamera/internal/v4l2_event.h > new file mode 100644 > index 0000000000000000000000000000000000000000..8e8d539b253b9235ece2e732e12a7609a0a5e89b > --- /dev/null > +++ b/include/libcamera/internal/v4l2_event.h > @@ -0,0 +1,158 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2026, Ideas on Board Oy > + * > + * V4L2 Event representations > + */ > + > +#pragma once > + > +#include <array> > +#include <memory> > +#include <optional> > +#include <stdint.h> > +#include <utility> > + > +#include <linux/videodev2.h> > + > +namespace libcamera { > + > +class V4L2Event > +{ > +public: > + V4L2Event(const v4l2_event *event); > + > + enum Type { > + VerticalSync, > + EndOfStream, > + Control, > + FrameSync, > + SourceChange, > + MotionDetected, > + NumberOfEventTypes, > + }; > + > + static constexpr std::array<std::pair<uint32_t, Type>, Type::NumberOfEventTypes> typeMap = { { I think `NumberOfEventTypes` could be easily dropped: static constexpr std::pair<uint32_t, Type> typeMap[] = { ... }; you might also consider moving this entire array into `v4l2_event.cpp` since it seems it is only used there. > + { V4L2_EVENT_VSYNC, Type::VerticalSync }, > + { V4L2_EVENT_EOS, Type::EndOfStream }, > + { V4L2_EVENT_CTRL, Type::Control }, > + { V4L2_EVENT_FRAME_SYNC, Type::FrameSync }, > + { V4L2_EVENT_SOURCE_CHANGE, Type::SourceChange }, > + { V4L2_EVENT_MOTION_DET, Type::MotionDetected }, > + } }; > + > + static std::shared_ptr<V4L2Event> createEvent(const struct v4l2_event *event); > + Type type() { return type_; } Should be marked `const`. > + static std::optional<Type> typeFromV4L2(uint32_t type); > + static std::optional<uint32_t> typeToV4L2(Type type); > + > +private: > + Type type_; This loses the "id" of the event, no? And also other common fields like `timestamp`, `sequence`, etc? ... After reading the patch series, I'm wondering how much wrapping is considered desirable? Because my first impression based on the title of the patch series was that there will simply be a Signal<const v4l2_event &> eventReady; that seems a lot simpler to me than mirroring the entire hierarchy. It also makes it essentially trivial to support custom even types without core changes. Of course it can be argued that it will force the users to detail with v4l2 uapi details. But given that this is an internal type, I'm not sure if that is a big concern. Especially that e.g. `V4L2MotionDetectedEvent::flags` is not mapped to a mirrored flags type. Thoughts? Did you consider doing something very bare-bones like the one above? > +}; > + > +class V4L2EventSubscription > +{ > +public: > + V4L2EventSubscription(V4L2Event::Type type, uint32_t id = 0) > + : type_(type), id_(id) > + { > + } > + > + V4L2Event::Type type() { return type_; } > + uint32_t id() { return id_; } Both should be marked `const`. > + > + bool operator<(const V4L2EventSubscription &other) const > + { > + return std::tie(type_, id_) < std::tie(other.type_, other.id_); > + } > +private: > + V4L2Event::Type type_; > + uint32_t id_; > +}; > + > +class V4L2VerticalSyncEvent : public V4L2Event > +{ > +public: > + V4L2VerticalSyncEvent(const v4l2_event *event) > + : V4L2Event(event), field_(event->u.vsync.field) > + { > + } > + > + uint8_t field() { return field_; } > +private: > + uint8_t field_; > +}; > + > +class V4L2ControlEvent : public V4L2Event > +{ > +public: > + V4L2ControlEvent(const v4l2_event *event); > + > + uint32_t controlId() { return controlId_; } All of these getters should be marked `const`. > + uint32_t changes() { return changes_; } > + uint32_t type() { return type_; } > + uint32_t flags() { return flags_; } > + int64_t value() { return value_; } > + uint32_t min() { return min_; } > + uint32_t max() { return max_; } > + uint32_t step() { return step_; } > + uint32_t def() { return default_; } > +private: > + uint32_t controlId_; > + uint32_t changes_; > + uint32_t type_; > + uint32_t flags_; > + int64_t value_; > + uint32_t min_; > + uint32_t max_; > + uint32_t step_; > + uint32_t default_; > +}; > + > +class V4L2FrameSyncEvent : public V4L2Event > +{ > +public: > + V4L2FrameSyncEvent(const v4l2_event *event) > + : V4L2Event(event), sequence_(event->u.frame_sync.frame_sequence) > + { > + } > + > + uint32_t sequence() { return sequence_; } > +private: > + uint32_t sequence_; > +}; > + > +class V4L2SourceChangeEvent : public V4L2Event > +{ > +public: > + V4L2SourceChangeEvent(const v4l2_event *event) > + : V4L2Event(event), changes_(event->u.src_change.changes) > + { > + } > + > + uint32_t changes() { return changes_; } > + > +private: > + uint32_t changes_; > +}; > + > +class V4L2MotionDetectedEvent : public V4L2Event > +{ > +public: > + V4L2MotionDetectedEvent(const v4l2_event *event) > + : V4L2Event(event), flags_(event->u.motion_det.flags), > + frame_sequence_(event->u.motion_det.frame_sequence), > + region_mask_(event->u.motion_det.region_mask) > + { > + } > + > + uint32_t flags() { return flags_; } > + uint32_t frameSequence() { return frame_sequence_; } > + uint32_t regionMask() { return region_mask_; } > +private: > + uint32_t flags_; > + uint32_t frame_sequence_; > + uint32_t region_mask_; > +}; > + > +} /* namespace libcamera */ > [...]
Morning Barnabas, thanks for the comments On 02/10/2026 16:06, Barnabás Pőcze wrote: > 2026. 09. 25. 17:16 keltezéssel, Daniel Scally írta: >> Add a base and derived classes to represent V4L2 Events to prepare >> for more generic event handling in V4L2Device. >> >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> >> --- >> include/libcamera/internal/v4l2_event.h | 158 ++++++++++++++++++ >> src/libcamera/meson.build | 1 + >> src/libcamera/v4l2_event.cpp | 288 ++++++++++++++++++++++++++++++++ >> 3 files changed, 447 insertions(+) >> >> diff --git a/include/libcamera/internal/v4l2_event.h b/include/libcamera/internal/v4l2_event.h >> new file mode 100644 >> index 0000000000000000000000000000000000000000..8e8d539b253b9235ece2e732e12a7609a0a5e89b >> --- /dev/null >> +++ b/include/libcamera/internal/v4l2_event.h >> @@ -0,0 +1,158 @@ >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */ >> +/* >> + * Copyright (C) 2026, Ideas on Board Oy >> + * >> + * V4L2 Event representations >> + */ >> + >> +#pragma once >> + >> +#include <array> >> +#include <memory> >> +#include <optional> >> +#include <stdint.h> >> +#include <utility> >> + >> +#include <linux/videodev2.h> >> + >> +namespace libcamera { >> + >> +class V4L2Event >> +{ >> +public: >> + V4L2Event(const v4l2_event *event); >> + >> + enum Type { >> + VerticalSync, >> + EndOfStream, >> + Control, >> + FrameSync, >> + SourceChange, >> + MotionDetected, >> + NumberOfEventTypes, >> + }; >> + >> + static constexpr std::array<std::pair<uint32_t, Type>, Type::NumberOfEventTypes> typeMap = { { > > I think `NumberOfEventTypes` could be easily dropped: > > static constexpr std::pair<uint32_t, Type> typeMap[] = { > ... > }; > > you might also consider moving this entire array into `v4l2_event.cpp` since > it seems it is only used there. > ack> >> + { V4L2_EVENT_VSYNC, Type::VerticalSync }, >> + { V4L2_EVENT_EOS, Type::EndOfStream }, >> + { V4L2_EVENT_CTRL, Type::Control }, >> + { V4L2_EVENT_FRAME_SYNC, Type::FrameSync }, >> + { V4L2_EVENT_SOURCE_CHANGE, Type::SourceChange }, >> + { V4L2_EVENT_MOTION_DET, Type::MotionDetected }, >> + } }; >> + >> + static std::shared_ptr<V4L2Event> createEvent(const struct v4l2_event *event); >> + Type type() { return type_; } > > Should be marked `const`. ack> > >> + static std::optional<Type> typeFromV4L2(uint32_t type); >> + static std::optional<uint32_t> typeToV4L2(Type type); >> + >> +private: >> + Type type_; > > This loses the "id" of the event, no? And also other common fields > like `timestamp`, `sequence`, etc? > > ... > > After reading the patch series, I'm wondering how much wrapping is considered desirable? > Because my first impression based on the title of the patch series was that there will > simply be a > > Signal<const v4l2_event &> eventReady; > > that seems a lot simpler to me than mirroring the entire hierarchy. It also makes > it essentially trivial to support custom even types without core changes. > > Of course it can be argued that it will force the users to detail with v4l2 uapi details. > But given that this is an internal type, I'm not sure if that is a big concern. > Especially that e.g. `V4L2MotionDetectedEvent::flags` is not mapped to a mirrored > flags type. > > Thoughts? Did you consider doing something very bare-bones like the one above? Yes, I vacillated and perhaps overthought it...handling v4l2_event directly in the pipeline handlers seems more messy than is ideal to me...but perhaps the simplicity of it is better. I'll rework it to just send the struct v4l2_event as a signal and send that. Thanks Dan > > >> +}; >> + >> +class V4L2EventSubscription >> +{ >> +public: >> + V4L2EventSubscription(V4L2Event::Type type, uint32_t id = 0) >> + : type_(type), id_(id) >> + { >> + } >> + >> + V4L2Event::Type type() { return type_; } >> + uint32_t id() { return id_; } > > Both should be marked `const`. > > >> + >> + bool operator<(const V4L2EventSubscription &other) const >> + { >> + return std::tie(type_, id_) < std::tie(other.type_, other.id_); >> + } >> +private: >> + V4L2Event::Type type_; >> + uint32_t id_; >> +}; >> + >> +class V4L2VerticalSyncEvent : public V4L2Event >> +{ >> +public: >> + V4L2VerticalSyncEvent(const v4l2_event *event) >> + : V4L2Event(event), field_(event->u.vsync.field) >> + { >> + } >> + >> + uint8_t field() { return field_; } >> +private: >> + uint8_t field_; >> +}; >> + >> +class V4L2ControlEvent : public V4L2Event >> +{ >> +public: >> + V4L2ControlEvent(const v4l2_event *event); >> + >> + uint32_t controlId() { return controlId_; } > > All of these getters should be marked `const`. > > >> + uint32_t changes() { return changes_; } >> + uint32_t type() { return type_; } >> + uint32_t flags() { return flags_; } >> + int64_t value() { return value_; } >> + uint32_t min() { return min_; } >> + uint32_t max() { return max_; } >> + uint32_t step() { return step_; } >> + uint32_t def() { return default_; } >> +private: >> + uint32_t controlId_; >> + uint32_t changes_; >> + uint32_t type_; >> + uint32_t flags_; >> + int64_t value_; >> + uint32_t min_; >> + uint32_t max_; >> + uint32_t step_; >> + uint32_t default_; >> +}; >> + >> +class V4L2FrameSyncEvent : public V4L2Event >> +{ >> +public: >> + V4L2FrameSyncEvent(const v4l2_event *event) >> + : V4L2Event(event), sequence_(event->u.frame_sync.frame_sequence) >> + { >> + } >> + >> + uint32_t sequence() { return sequence_; } >> +private: >> + uint32_t sequence_; >> +}; >> + >> +class V4L2SourceChangeEvent : public V4L2Event >> +{ >> +public: >> + V4L2SourceChangeEvent(const v4l2_event *event) >> + : V4L2Event(event), changes_(event->u.src_change.changes) >> + { >> + } >> + >> + uint32_t changes() { return changes_; } >> + >> +private: >> + uint32_t changes_; >> +}; >> + >> +class V4L2MotionDetectedEvent : public V4L2Event >> +{ >> +public: >> + V4L2MotionDetectedEvent(const v4l2_event *event) >> + : V4L2Event(event), flags_(event->u.motion_det.flags), >> + frame_sequence_(event->u.motion_det.frame_sequence), >> + region_mask_(event->u.motion_det.region_mask) >> + { >> + } >> + >> + uint32_t flags() { return flags_; } >> + uint32_t frameSequence() { return frame_sequence_; } >> + uint32_t regionMask() { return region_mask_; } >> +private: >> + uint32_t flags_; >> + uint32_t frame_sequence_; >> + uint32_t region_mask_; >> +}; >> + >> +} /* namespace libcamera */ >> [...]
2026. 10. 05. 10:53 keltezéssel, Dan Scally írta: > Morning Barnabas, thanks for the comments > > On 02/10/2026 16:06, Barnabás Pőcze wrote: >> 2026. 09. 25. 17:16 keltezéssel, Daniel Scally írta: >>> Add a base and derived classes to represent V4L2 Events to prepare >>> for more generic event handling in V4L2Device. >>> >>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> >>> --- >>> include/libcamera/internal/v4l2_event.h | 158 ++++++++++++++++++ >>> src/libcamera/meson.build | 1 + >>> src/libcamera/v4l2_event.cpp | 288 ++++++++++++++++++++++++++++++++ >>> 3 files changed, 447 insertions(+) >>> >>> diff --git a/include/libcamera/internal/v4l2_event.h b/include/libcamera/internal/v4l2_event.h >>> new file mode 100644 >>> index 0000000000000000000000000000000000000000..8e8d539b253b9235ece2e732e12a7609a0a5e89b >>> --- /dev/null >>> +++ b/include/libcamera/internal/v4l2_event.h >>> @@ -0,0 +1,158 @@ >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */ >>> +/* >>> + * Copyright (C) 2026, Ideas on Board Oy >>> + * >>> + * V4L2 Event representations >>> + */ >>> + >>> +#pragma once >>> + >>> +#include <array> >>> +#include <memory> >>> +#include <optional> >>> +#include <stdint.h> >>> +#include <utility> >>> + >>> +#include <linux/videodev2.h> >>> + >>> +namespace libcamera { >>> + >>> +class V4L2Event >>> +{ >>> +public: >>> + V4L2Event(const v4l2_event *event); >>> + >>> + enum Type { >>> + VerticalSync, >>> + EndOfStream, >>> + Control, >>> + FrameSync, >>> + SourceChange, >>> + MotionDetected, >>> + NumberOfEventTypes, >>> + }; >>> + >>> + static constexpr std::array<std::pair<uint32_t, Type>, Type::NumberOfEventTypes> typeMap = { { >> >> I think `NumberOfEventTypes` could be easily dropped: >> >> static constexpr std::pair<uint32_t, Type> typeMap[] = { >> ... >> }; >> >> you might also consider moving this entire array into `v4l2_event.cpp` since >> it seems it is only used there. >> > > ack> >>> + { V4L2_EVENT_VSYNC, Type::VerticalSync }, >>> + { V4L2_EVENT_EOS, Type::EndOfStream }, >>> + { V4L2_EVENT_CTRL, Type::Control }, >>> + { V4L2_EVENT_FRAME_SYNC, Type::FrameSync }, >>> + { V4L2_EVENT_SOURCE_CHANGE, Type::SourceChange }, >>> + { V4L2_EVENT_MOTION_DET, Type::MotionDetected }, >>> + } }; >>> + >>> + static std::shared_ptr<V4L2Event> createEvent(const struct v4l2_event *event); >>> + Type type() { return type_; } >> >> Should be marked `const`. > > ack> >> >>> + static std::optional<Type> typeFromV4L2(uint32_t type); >>> + static std::optional<uint32_t> typeToV4L2(Type type); >>> + >>> +private: >>> + Type type_; >> >> This loses the "id" of the event, no? And also other common fields >> like `timestamp`, `sequence`, etc? >> >> ... >> >> After reading the patch series, I'm wondering how much wrapping is considered desirable? >> Because my first impression based on the title of the patch series was that there will >> simply be a >> >> Signal<const v4l2_event &> eventReady; >> >> that seems a lot simpler to me than mirroring the entire hierarchy. It also makes >> it essentially trivial to support custom even types without core changes. >> >> Of course it can be argued that it will force the users to detail with v4l2 uapi details. >> But given that this is an internal type, I'm not sure if that is a big concern. >> Especially that e.g. `V4L2MotionDetectedEvent::flags` is not mapped to a mirrored >> flags type. >> >> Thoughts? Did you consider doing something very bare-bones like the one above? > > Yes, I vacillated and perhaps overthought it...handling v4l2_event directly in the pipeline handlers seems more messy than is ideal to me...but perhaps the simplicity of it is better. > > I'll rework it to just send the struct v4l2_event as a signal and send that. I'm not at all sure it is better, but the simplicity looks appealing to me. Also not sure how much of the v4l2 uapi should be "mirrored" in libcamera. > > Thanks > Dan > >> >> >>> +}; >>> + >>> +class V4L2EventSubscription >>> +{ >>> +public: >>> + V4L2EventSubscription(V4L2Event::Type type, uint32_t id = 0) >>> + : type_(type), id_(id) >>> + { >>> + } >>> + >>> + V4L2Event::Type type() { return type_; } >>> + uint32_t id() { return id_; } >> >> Both should be marked `const`. >> >> >>> + >>> + bool operator<(const V4L2EventSubscription &other) const >>> + { >>> + return std::tie(type_, id_) < std::tie(other.type_, other.id_); >>> + } >>> +private: >>> + V4L2Event::Type type_; >>> + uint32_t id_; >>> +}; >>> + >>> +class V4L2VerticalSyncEvent : public V4L2Event >>> +{ >>> +public: >>> + V4L2VerticalSyncEvent(const v4l2_event *event) >>> + : V4L2Event(event), field_(event->u.vsync.field) >>> + { >>> + } >>> + >>> + uint8_t field() { return field_; } >>> +private: >>> + uint8_t field_; >>> +}; >>> + >>> +class V4L2ControlEvent : public V4L2Event >>> +{ >>> +public: >>> + V4L2ControlEvent(const v4l2_event *event); >>> + >>> + uint32_t controlId() { return controlId_; } >> >> All of these getters should be marked `const`. >> >> >>> + uint32_t changes() { return changes_; } >>> + uint32_t type() { return type_; } >>> + uint32_t flags() { return flags_; } >>> + int64_t value() { return value_; } >>> + uint32_t min() { return min_; } >>> + uint32_t max() { return max_; } >>> + uint32_t step() { return step_; } >>> + uint32_t def() { return default_; } >>> +private: >>> + uint32_t controlId_; >>> + uint32_t changes_; >>> + uint32_t type_; >>> + uint32_t flags_; >>> + int64_t value_; >>> + uint32_t min_; >>> + uint32_t max_; >>> + uint32_t step_; >>> + uint32_t default_; >>> +}; >>> + >>> +class V4L2FrameSyncEvent : public V4L2Event >>> +{ >>> +public: >>> + V4L2FrameSyncEvent(const v4l2_event *event) >>> + : V4L2Event(event), sequence_(event->u.frame_sync.frame_sequence) >>> + { >>> + } >>> + >>> + uint32_t sequence() { return sequence_; } >>> +private: >>> + uint32_t sequence_; >>> +}; >>> + >>> +class V4L2SourceChangeEvent : public V4L2Event >>> +{ >>> +public: >>> + V4L2SourceChangeEvent(const v4l2_event *event) >>> + : V4L2Event(event), changes_(event->u.src_change.changes) >>> + { >>> + } >>> + >>> + uint32_t changes() { return changes_; } >>> + >>> +private: >>> + uint32_t changes_; >>> +}; >>> + >>> +class V4L2MotionDetectedEvent : public V4L2Event >>> +{ >>> +public: >>> + V4L2MotionDetectedEvent(const v4l2_event *event) >>> + : V4L2Event(event), flags_(event->u.motion_det.flags), >>> + frame_sequence_(event->u.motion_det.frame_sequence), >>> + region_mask_(event->u.motion_det.region_mask) >>> + { >>> + } >>> + >>> + uint32_t flags() { return flags_; } >>> + uint32_t frameSequence() { return frame_sequence_; } >>> + uint32_t regionMask() { return region_mask_; } >>> +private: >>> + uint32_t flags_; >>> + uint32_t frame_sequence_; >>> + uint32_t region_mask_; >>> +}; >>> + >>> +} /* namespace libcamera */ >>> [...] >
On 05/10/2026 11:07, Barnabás Pőcze wrote: > 2026. 10. 05. 10:53 keltezéssel, Dan Scally írta: >> Morning Barnabas, thanks for the comments >> >> On 02/10/2026 16:06, Barnabás Pőcze wrote: >>> 2026. 09. 25. 17:16 keltezéssel, Daniel Scally írta: >>>> Add a base and derived classes to represent V4L2 Events to prepare >>>> for more generic event handling in V4L2Device. >>>> >>>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> >>>> --- >>>> include/libcamera/internal/v4l2_event.h | 158 ++++++++++++++++++ >>>> src/libcamera/meson.build | 1 + >>>> src/libcamera/v4l2_event.cpp | 288 ++++++++++++++++++++++++++++++++ >>>> 3 files changed, 447 insertions(+) >>>> >>>> diff --git a/include/libcamera/internal/v4l2_event.h b/include/libcamera/internal/v4l2_event.h >>>> new file mode 100644 >>>> index 0000000000000000000000000000000000000000..8e8d539b253b9235ece2e732e12a7609a0a5e89b >>>> --- /dev/null >>>> +++ b/include/libcamera/internal/v4l2_event.h >>>> @@ -0,0 +1,158 @@ >>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */ >>>> +/* >>>> + * Copyright (C) 2026, Ideas on Board Oy >>>> + * >>>> + * V4L2 Event representations >>>> + */ >>>> + >>>> +#pragma once >>>> + >>>> +#include <array> >>>> +#include <memory> >>>> +#include <optional> >>>> +#include <stdint.h> >>>> +#include <utility> >>>> + >>>> +#include <linux/videodev2.h> >>>> + >>>> +namespace libcamera { >>>> + >>>> +class V4L2Event >>>> +{ >>>> +public: >>>> + V4L2Event(const v4l2_event *event); >>>> + >>>> + enum Type { >>>> + VerticalSync, >>>> + EndOfStream, >>>> + Control, >>>> + FrameSync, >>>> + SourceChange, >>>> + MotionDetected, >>>> + NumberOfEventTypes, >>>> + }; >>>> + >>>> + static constexpr std::array<std::pair<uint32_t, Type>, Type::NumberOfEventTypes> typeMap = { { >>> >>> I think `NumberOfEventTypes` could be easily dropped: >>> >>> static constexpr std::pair<uint32_t, Type> typeMap[] = { >>> ... >>> }; >>> >>> you might also consider moving this entire array into `v4l2_event.cpp` since >>> it seems it is only used there. >>> >> >> ack> >>>> + { V4L2_EVENT_VSYNC, Type::VerticalSync }, >>>> + { V4L2_EVENT_EOS, Type::EndOfStream }, >>>> + { V4L2_EVENT_CTRL, Type::Control }, >>>> + { V4L2_EVENT_FRAME_SYNC, Type::FrameSync }, >>>> + { V4L2_EVENT_SOURCE_CHANGE, Type::SourceChange }, >>>> + { V4L2_EVENT_MOTION_DET, Type::MotionDetected }, >>>> + } }; >>>> + >>>> + static std::shared_ptr<V4L2Event> createEvent(const struct v4l2_event *event); >>>> + Type type() { return type_; } >>> >>> Should be marked `const`. >> >> ack> >>> >>>> + static std::optional<Type> typeFromV4L2(uint32_t type); >>>> + static std::optional<uint32_t> typeToV4L2(Type type); >>>> + >>>> +private: >>>> + Type type_; >>> >>> This loses the "id" of the event, no? And also other common fields >>> like `timestamp`, `sequence`, etc? >>> >>> ... >>> >>> After reading the patch series, I'm wondering how much wrapping is considered desirable? >>> Because my first impression based on the title of the patch series was that there will >>> simply be a >>> >>> Signal<const v4l2_event &> eventReady; >>> >>> that seems a lot simpler to me than mirroring the entire hierarchy. It also makes >>> it essentially trivial to support custom even types without core changes. >>> >>> Of course it can be argued that it will force the users to detail with v4l2 uapi details. >>> But given that this is an internal type, I'm not sure if that is a big concern. >>> Especially that e.g. `V4L2MotionDetectedEvent::flags` is not mapped to a mirrored >>> flags type. >>> >>> Thoughts? Did you consider doing something very bare-bones like the one above? >> >> Yes, I vacillated and perhaps overthought it...handling v4l2_event directly in the pipeline >> handlers seems more messy than is ideal to me...but perhaps the simplicity of it is better. >> >> I'll rework it to just send the struct v4l2_event as a signal and send that. > > I'm not at all sure it is better, but the simplicity looks appealing to me. > Also not sure how much of the v4l2 uapi should be "mirrored" in libcamera. Nor am I, but perhaps seeing both routes will make it easier to decide Dan > > >> >> Thanks >> Dan >> >>> >>> >>>> +}; >>>> + >>>> +class V4L2EventSubscription >>>> +{ >>>> +public: >>>> + V4L2EventSubscription(V4L2Event::Type type, uint32_t id = 0) >>>> + : type_(type), id_(id) >>>> + { >>>> + } >>>> + >>>> + V4L2Event::Type type() { return type_; } >>>> + uint32_t id() { return id_; } >>> >>> Both should be marked `const`. >>> >>> >>>> + >>>> + bool operator<(const V4L2EventSubscription &other) const >>>> + { >>>> + return std::tie(type_, id_) < std::tie(other.type_, other.id_); >>>> + } >>>> +private: >>>> + V4L2Event::Type type_; >>>> + uint32_t id_; >>>> +}; >>>> + >>>> +class V4L2VerticalSyncEvent : public V4L2Event >>>> +{ >>>> +public: >>>> + V4L2VerticalSyncEvent(const v4l2_event *event) >>>> + : V4L2Event(event), field_(event->u.vsync.field) >>>> + { >>>> + } >>>> + >>>> + uint8_t field() { return field_; } >>>> +private: >>>> + uint8_t field_; >>>> +}; >>>> + >>>> +class V4L2ControlEvent : public V4L2Event >>>> +{ >>>> +public: >>>> + V4L2ControlEvent(const v4l2_event *event); >>>> + >>>> + uint32_t controlId() { return controlId_; } >>> >>> All of these getters should be marked `const`. >>> >>> >>>> + uint32_t changes() { return changes_; } >>>> + uint32_t type() { return type_; } >>>> + uint32_t flags() { return flags_; } >>>> + int64_t value() { return value_; } >>>> + uint32_t min() { return min_; } >>>> + uint32_t max() { return max_; } >>>> + uint32_t step() { return step_; } >>>> + uint32_t def() { return default_; } >>>> +private: >>>> + uint32_t controlId_; >>>> + uint32_t changes_; >>>> + uint32_t type_; >>>> + uint32_t flags_; >>>> + int64_t value_; >>>> + uint32_t min_; >>>> + uint32_t max_; >>>> + uint32_t step_; >>>> + uint32_t default_; >>>> +}; >>>> + >>>> +class V4L2FrameSyncEvent : public V4L2Event >>>> +{ >>>> +public: >>>> + V4L2FrameSyncEvent(const v4l2_event *event) >>>> + : V4L2Event(event), sequence_(event->u.frame_sync.frame_sequence) >>>> + { >>>> + } >>>> + >>>> + uint32_t sequence() { return sequence_; } >>>> +private: >>>> + uint32_t sequence_; >>>> +}; >>>> + >>>> +class V4L2SourceChangeEvent : public V4L2Event >>>> +{ >>>> +public: >>>> + V4L2SourceChangeEvent(const v4l2_event *event) >>>> + : V4L2Event(event), changes_(event->u.src_change.changes) >>>> + { >>>> + } >>>> + >>>> + uint32_t changes() { return changes_; } >>>> + >>>> +private: >>>> + uint32_t changes_; >>>> +}; >>>> + >>>> +class V4L2MotionDetectedEvent : public V4L2Event >>>> +{ >>>> +public: >>>> + V4L2MotionDetectedEvent(const v4l2_event *event) >>>> + : V4L2Event(event), flags_(event->u.motion_det.flags), >>>> + frame_sequence_(event->u.motion_det.frame_sequence), >>>> + region_mask_(event->u.motion_det.region_mask) >>>> + { >>>> + } >>>> + >>>> + uint32_t flags() { return flags_; } >>>> + uint32_t frameSequence() { return frame_sequence_; } >>>> + uint32_t regionMask() { return region_mask_; } >>>> +private: >>>> + uint32_t flags_; >>>> + uint32_t frame_sequence_; >>>> + uint32_t region_mask_; >>>> +}; >>>> + >>>> +} /* namespace libcamera */ >>>> [...] >> >
diff --git a/include/libcamera/internal/v4l2_event.h b/include/libcamera/internal/v4l2_event.h new file mode 100644 index 0000000000000000000000000000000000000000..8e8d539b253b9235ece2e732e12a7609a0a5e89b --- /dev/null +++ b/include/libcamera/internal/v4l2_event.h @@ -0,0 +1,158 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2026, Ideas on Board Oy + * + * V4L2 Event representations + */ + +#pragma once + +#include <array> +#include <memory> +#include <optional> +#include <stdint.h> +#include <utility> + +#include <linux/videodev2.h> + +namespace libcamera { + +class V4L2Event +{ +public: + V4L2Event(const v4l2_event *event); + + enum Type { + VerticalSync, + EndOfStream, + Control, + FrameSync, + SourceChange, + MotionDetected, + NumberOfEventTypes, + }; + + static constexpr std::array<std::pair<uint32_t, Type>, Type::NumberOfEventTypes> typeMap = { { + { V4L2_EVENT_VSYNC, Type::VerticalSync }, + { V4L2_EVENT_EOS, Type::EndOfStream }, + { V4L2_EVENT_CTRL, Type::Control }, + { V4L2_EVENT_FRAME_SYNC, Type::FrameSync }, + { V4L2_EVENT_SOURCE_CHANGE, Type::SourceChange }, + { V4L2_EVENT_MOTION_DET, Type::MotionDetected }, + } }; + + static std::shared_ptr<V4L2Event> createEvent(const struct v4l2_event *event); + Type type() { return type_; } + static std::optional<Type> typeFromV4L2(uint32_t type); + static std::optional<uint32_t> typeToV4L2(Type type); + +private: + Type type_; +}; + +class V4L2EventSubscription +{ +public: + V4L2EventSubscription(V4L2Event::Type type, uint32_t id = 0) + : type_(type), id_(id) + { + } + + V4L2Event::Type type() { return type_; } + uint32_t id() { return id_; } + + bool operator<(const V4L2EventSubscription &other) const + { + return std::tie(type_, id_) < std::tie(other.type_, other.id_); + } +private: + V4L2Event::Type type_; + uint32_t id_; +}; + +class V4L2VerticalSyncEvent : public V4L2Event +{ +public: + V4L2VerticalSyncEvent(const v4l2_event *event) + : V4L2Event(event), field_(event->u.vsync.field) + { + } + + uint8_t field() { return field_; } +private: + uint8_t field_; +}; + +class V4L2ControlEvent : public V4L2Event +{ +public: + V4L2ControlEvent(const v4l2_event *event); + + uint32_t controlId() { return controlId_; } + uint32_t changes() { return changes_; } + uint32_t type() { return type_; } + uint32_t flags() { return flags_; } + int64_t value() { return value_; } + uint32_t min() { return min_; } + uint32_t max() { return max_; } + uint32_t step() { return step_; } + uint32_t def() { return default_; } +private: + uint32_t controlId_; + uint32_t changes_; + uint32_t type_; + uint32_t flags_; + int64_t value_; + uint32_t min_; + uint32_t max_; + uint32_t step_; + uint32_t default_; +}; + +class V4L2FrameSyncEvent : public V4L2Event +{ +public: + V4L2FrameSyncEvent(const v4l2_event *event) + : V4L2Event(event), sequence_(event->u.frame_sync.frame_sequence) + { + } + + uint32_t sequence() { return sequence_; } +private: + uint32_t sequence_; +}; + +class V4L2SourceChangeEvent : public V4L2Event +{ +public: + V4L2SourceChangeEvent(const v4l2_event *event) + : V4L2Event(event), changes_(event->u.src_change.changes) + { + } + + uint32_t changes() { return changes_; } + +private: + uint32_t changes_; +}; + +class V4L2MotionDetectedEvent : public V4L2Event +{ +public: + V4L2MotionDetectedEvent(const v4l2_event *event) + : V4L2Event(event), flags_(event->u.motion_det.flags), + frame_sequence_(event->u.motion_det.frame_sequence), + region_mask_(event->u.motion_det.region_mask) + { + } + + uint32_t flags() { return flags_; } + uint32_t frameSequence() { return frame_sequence_; } + uint32_t regionMask() { return region_mask_; } +private: + uint32_t flags_; + uint32_t frame_sequence_; + uint32_t region_mask_; +}; + +} /* namespace libcamera */ diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build index 17c1b2cb347902d509da35a114845aa4ca554623..04e4bbb1451f4666e6c4d8a79c14473838180241 100644 --- a/src/libcamera/meson.build +++ b/src/libcamera/meson.build @@ -53,6 +53,7 @@ libcamera_internal_sources = files([ 'source_paths.cpp', 'sysfs.cpp', 'v4l2_device.cpp', + 'v4l2_event.cpp', 'v4l2_pixelformat.cpp', 'v4l2_request.cpp', 'v4l2_subdevice.cpp', diff --git a/src/libcamera/v4l2_event.cpp b/src/libcamera/v4l2_event.cpp new file mode 100644 index 0000000000000000000000000000000000000000..7279eba6aadd04ed329ae36e0c2b9258cdb98d2b --- /dev/null +++ b/src/libcamera/v4l2_event.cpp @@ -0,0 +1,288 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2026, Ideas on Board Oy + * + * V4L2 Event representations + */ + +#include "libcamera/internal/v4l2_event.h" + +#include <assert.h> +#include <stdint.h> + +#include <linux/videodev2.h> + +#include <libcamera/base/log.h> + +/** + * \file v4l2_event.h + * \brief Handling of V4L2 Events + */ + +namespace libcamera { + +LOG_DEFINE_CATEGORY(V4L2Event) + +/** + * \class V4L2Event + * \brief Base class to represent V4L2 Events + * + * The V4L2Event class holds common utilities and helper functions, and is + * mostly intended to facilitate the creation of more specific derived classes. + */ + +/** + * \brief Construct a new V4L2Event::V4L2Event object + * + * \param[in] event The v4l2_event definition + */ +V4L2Event::V4L2Event(const v4l2_event *event) +{ + auto type = typeFromV4L2(event->type); + assert(type); + + type_ = *type; +} + +/** + * \enum V4L2Event::Type + * \brief Enumeration of possible V4L2 Event Types + * + * \var V4L2Event::VerticalSync + * \brief An event triggered on the vertical sync + * + * \var V4L2Event::EndOfStream + * \brief An event triggered when the end of a stream is reached + * + * \var V4L2Event::Control + * \brief An event triggered when the value of the specified control changes, + * if a button control is pressed or if the control's flags change. + * + * \var V4L2Event::FrameSync + * \brief An event triggered when reception of a frame has begun + * + * \var V4L2Event::SourceChange + * \brief Triggered when a source parameter is changed + * + * \var V4L2Event::MotionDetected + * \brief Triggered when motion is detected + * + */ + +/** + * \var V4L2Event::typeMap + * \brief Maps V4L2 types from the kernel header to V4l2Event::Type values + */ + +/** + * \brief Create a derivation of a \ref V4L2Event from the kernel's + * representation of one. + * + * This funtion allocates one of the derived classes of \ref V4L2Event, based on + * the input event's type member. + * + * \param[in] event The kernel uAPI representation of a V4L2 event + */ +std::shared_ptr<V4L2Event> V4L2Event::createEvent(const struct v4l2_event *event) +{ + switch (event->type) { + case V4L2_EVENT_VSYNC: + return std::make_shared<V4L2VerticalSyncEvent>(event); + case V4L2_EVENT_EOS: + return std::make_shared<V4L2Event>(event); + case V4L2_EVENT_CTRL: + return std::make_shared<V4L2ControlEvent>(event); + case V4L2_EVENT_FRAME_SYNC: + return std::make_shared<V4L2FrameSyncEvent>(event); + case V4L2_EVENT_SOURCE_CHANGE: + return std::make_shared<V4L2SourceChangeEvent>(event); + case V4L2_EVENT_MOTION_DET: + return std::make_shared<V4L2MotionDetectedEvent>(event); + default: + return nullptr; + } +} + +/** + * \fn V4L2Event::type() + * \brief Return the \ref V4L2Event::Type associated with this event + */ + +/** + * \brief Map the kernel V4L2 event type to a \ref V4L2Event::Type + * + * \param[in] type The kernel's event type + * \return std::optional<V4L2Event::Type> + */ +std::optional<V4L2Event::Type> V4L2Event::typeFromV4L2(uint32_t type) +{ + for (const auto &[v4l2Type, eventType] : typeMap) { + if (v4l2Type == type) + return eventType; + } + + return std::nullopt; +} + +/** + * \brief Map a \ref V4L2Event::Type to the kernel's equivalent + * + * \param[in] type The V4L2Event::Type to translate + * \return std::optional<uint32_t> + */ +std::optional<uint32_t> V4L2Event::typeToV4L2(V4L2Event::Type type) +{ + for (const auto &[v4l2Type, eventType] : typeMap) { + if (eventType == type) + return v4l2Type; + } + + return std::nullopt; +} + +/** + * \class V4L2EventSubscription + * \brief Class to represent the kernel uAPI's struct v4l2_event_subscription + */ + +/** + * \fn V4L2EventSubscription::V4L2EventSubscription + * \brief Constructs a new V4L2EventSubscription object + */ + +/** + * \fn V4L2EventSubscription::type + * \brief Returns the \ref V4L2Event::Type associated with this subscription + */ + +/** + * \fn V4L2EventSubscription::id + * \brief Returns the ID field for this subscription + */ + +/** + * \fn V4L2EventSubscription::operator<(const V4L2EventSubscription &other) const + * \brief "Less-than" comparator for event subscriptions + * + * This function allows us to check whether an equivalent subscription has + * already been made by storing an instance of the class in a std::set. With + * this operator the normal find() / insert() and erase() operations will + * work on the set. + */ + +/** + * \class V4L2VerticalSyncEvent + * \brief Representation of a V4L2Event of type VerticalSync + * + * \fn V4L2VerticalSyncEvent::V4L2VerticalSyncEvent + * \brief Construct a new V4L2VerticalSyncEvent object + * + * \param[in] event The v4l2_event definition + * + * \fn V4L2VerticalSyncEvent::field + * \brief The upcoming V4L2_FIELD_* + */ + +/** + * \class V4L2ControlEvent + * \brief Representation of a V4L2Event of type Control + * + * \fn V4L2ControlEvent::controlId + * \brief Return the V4L2_CID_* control ID associated with this event + * + * \fn V4L2ControlEvent::changes + * \brief Return the bit mask of V4L2_EVENT_CTRL_CH_* flags + * + * \fn V4L2ControlEvent::type + * \brief Return the V4L2_CTRL_TYPE_* for this control + * + * \fn V4L2ControlEvent::flags + * \brief Return the V4L2_CTRL_FL_* bitmask for this control + * + * \fn V4L2ControlEvent::value + * \brief Return the value of this control + * + * For the value, we take the value64 field from V4L2 and use it unconditionally + * + * \fn V4L2ControlEvent::min + * \brief Return the minimum value of this control + * + * \fn V4L2ControlEvent::max + * \brief Return the maximum value of this control + * + * \fn V4L2ControlEvent::step + * \brief Return the control value step + * + * \fn V4L2ControlEvent::def + * \brief Return the default value of this control + */ + +/** + * \brief Construct a new V4L2ControlEvent object + * + * \param[in] event The v4l2_event definition + */ +V4L2ControlEvent::V4L2ControlEvent(const struct v4l2_event *event) + : V4L2Event(event), controlId_(event->id) +{ + const v4l2_event_ctrl *ctrl = &event->u.ctrl; + + changes_ = ctrl->changes; + type_ = ctrl->type; + flags_ = ctrl->flags; + value_ = ctrl->value64; + min_ = ctrl->minimum; + max_ = ctrl->maximum; + step_ = ctrl->step; + default_ = ctrl->default_value; +} + +/** + * \class V4L2FrameSyncEvent + * \brief Representation of a V4L2Event of type FrameSync + * + * \fn V4L2FrameSyncEvent::V4L2FrameSyncEvent() + * \brief Construct a new V4L2FrameSyncEvent object + * + * \param[in] event The v4l2_event definition + * + * \fn V4L2FrameSyncEvent::sequence() + * \brief Return the frame sequence number for this Frame Sync event + */ + +/** + * \class V4L2SourceChangeEvent + * \brief Representation of a V4L2Event of type SourceChange + * + * \fn V4L2SourceChangeEvent::V4L2SourceChangeEvent() + * \brief Construct a new V4L2SourceChangeEvent object + * + * \param[in] event The v4l2_event definition + * + * \fn V4L2SourceChangeEvent::changes() + * \brief Return the change flags for this Source Change event + */ + +/** + * \class V4L2MotionDetectedEvent + * \brief Representation of a V4L2Event of type MotionDetected + * + * \fn V4L2MotionDetectedEvent::V4L2MotionDetectedEvent() + * \brief Construct a new V4L2MotionDetectedEvent object + * + * \param[in] event The v4l2_event definition + * + * \fn V4L2MotionDetectedEvent::flags + * \brief Flags for this motion detection event, from V4L2_EVENT_MD_FL_* + * + * \fn V4L2MotionDetectedEvent::frameSequence + * \brief The frame sequence number for this event + * + * This field is invalid unless \ref V4L2MotionDetectedEvent::flags includes + * the V4L2_EVENT_MD_FL_HAVE_FRAME_SEQ flag. + * + * \fn V4L2MotionDetectedEvent::regionMask + * \brief The bitmask of the regions that reported motion + */ + +} /* namespace libcamera */
Add a base and derived classes to represent V4L2 Events to prepare for more generic event handling in V4L2Device. Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> --- include/libcamera/internal/v4l2_event.h | 158 ++++++++++++++++++ src/libcamera/meson.build | 1 + src/libcamera/v4l2_event.cpp | 288 ++++++++++++++++++++++++++++++++ 3 files changed, 447 insertions(+)