[2/2] libcamera: Add Unsigned 16-bits Control Type
diff mbox series

Message ID 20241011175653.3530945-3-chenghaoyang@chromium.org
State Superseded
Headers show
Series
  • Add U16 & U32 support in controls
Related show

Commit Message

Cheng-Hao Yang Oct. 11, 2024, 5:55 p.m. UTC 
From: Yudhistira Erlandinata <yerlandinata@google.com>

Some camera metadata is of length 16-bits,
for example JPEG metadata headers.

Signed-off-by: Yudhistira Erlandinata <yerlandinata@chromium.org>
Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
---
 include/libcamera/controls.h  | 6 ++++++
 src/libcamera/controls.cpp    | 8 ++++++++
 src/libcamera/v4l2_device.cpp | 9 +++++++++
 3 files changed, 23 insertions(+)

Comments

Kieran Bingham Oct. 21, 2024, 4:27 p.m. UTC | #1 
Quoting Harvey Yang (2024-10-11 18:55:12)
> From: Yudhistira Erlandinata <yerlandinata@google.com>
> 
> Some camera metadata is of length 16-bits,
> for example JPEG metadata headers.
> 
> Signed-off-by: Yudhistira Erlandinata <yerlandinata@chromium.org>
> Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> ---
>  include/libcamera/controls.h  | 6 ++++++
>  src/libcamera/controls.cpp    | 8 ++++++++
>  src/libcamera/v4l2_device.cpp | 9 +++++++++
>  3 files changed, 23 insertions(+)
> 
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index 25f68040d..1987ca5f5 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -29,6 +29,7 @@ enum ControlType {
>         ControlTypeNone,
>         ControlTypeBool,
>         ControlTypeByte,
> +       ControlTypeUnsigned16,
>         ControlTypeUnsigned32,
>         ControlTypeInteger32,
>         ControlTypeInteger64,
> @@ -63,6 +64,11 @@ struct control_type<uint8_t> {
>         static constexpr std::size_t size = 0;
>  };
>  
> +template<>
> +struct control_type<uint16_t> {
> +       static constexpr ControlType value = ControlTypeUnsigned16;

Does this also need to set the size? What is size - why is it always
zero? 

> +};
> +
>  template<>
>  struct control_type<uint32_t> {
>         static constexpr ControlType value = ControlTypeUnsigned32;
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> index 8ae295191..3a840bb22 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -54,6 +54,7 @@ static constexpr size_t ControlValueSize[] = {
>         [ControlTypeNone]               = 0,
>         [ControlTypeBool]               = sizeof(bool),
>         [ControlTypeByte]               = sizeof(uint8_t),
> +       [ControlTypeUnsigned16]         = sizeof(uint16_t),
>         [ControlTypeUnsigned32]         = sizeof(uint32_t),
>         [ControlTypeInteger32]          = sizeof(int32_t),
>         [ControlTypeInteger64]          = sizeof(int64_t),
> @@ -75,6 +76,8 @@ static constexpr size_t ControlValueSize[] = {
>   * The control stores a boolean value
>   * \var ControlTypeByte
>   * The control stores a byte value as an unsigned 8-bit integer
> + * \var ControlTypeUnsigned16
> + * The control stores an unsigned 16-bit integer value
>   * \var ControlTypeUnsigned32
>   * The control stores an unsigned 32-bit integer value
>   * \var ControlTypeInteger32
> @@ -233,6 +236,11 @@ std::string ControlValue::toString() const
>                         str += std::to_string(*value);
>                         break;
>                 }
> +               case ControlTypeUnsigned16: {
> +                       const uint16_t *value = reinterpret_cast<const uint16_t *>(data);
> +                       str += std::to_string(*value);
> +                       break;
> +               }
>                 case ControlTypeUnsigned32: {
>                         const uint32_t *value = reinterpret_cast<const uint32_t *>(data);
>                         str += std::to_string(*value);
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index f3454ba24..cb7327171 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -492,6 +492,9 @@ ControlType V4L2Device::v4l2CtrlType(uint32_t ctrlType)
>         case V4L2_CTRL_TYPE_BOOLEAN:
>                 return ControlTypeBool;
>  
> +       case V4L2_CTRL_TYPE_U16:
> +               return ControlTypeUnsigned16;
> +
>         case V4L2_CTRL_TYPE_U32:
>                 return ControlTypeUnsigned32;
>  
> @@ -543,6 +546,11 @@ std::optional<ControlInfo> V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl
>                                    static_cast<uint8_t>(ctrl.maximum),
>                                    static_cast<uint8_t>(ctrl.default_value));
>  
> +       case V4L2_CTRL_TYPE_U16:
> +               return ControlInfo(static_cast<uint16_t>(ctrl.minimum),
> +                                  static_cast<uint16_t>(ctrl.maximum),
> +                                  static_cast<uint16_t>(ctrl.default_value));
> +
>         case V4L2_CTRL_TYPE_U32:
>                 return ControlInfo(static_cast<uint32_t>(ctrl.minimum),
>                                    static_cast<uint32_t>(ctrl.maximum),
> @@ -634,6 +642,7 @@ void V4L2Device::listControls()
>                 case V4L2_CTRL_TYPE_BITMASK:
>                 case V4L2_CTRL_TYPE_INTEGER_MENU:
>                 case V4L2_CTRL_TYPE_U8:
> +               case V4L2_CTRL_TYPE_U16:
>                 case V4L2_CTRL_TYPE_U32:
>                         break;
>                 /* \todo Support other control types. */
> -- 
> 2.47.0.rc1.288.g06298d1525-goog
>
Cheng-Hao Yang Oct. 22, 2024, 9:51 a.m. UTC | #2 
Hi Kieran,

On Tue, Oct 22, 2024 at 12:27 AM Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Quoting Harvey Yang (2024-10-11 18:55:12)
> > From: Yudhistira Erlandinata <yerlandinata@google.com>
> >
> > Some camera metadata is of length 16-bits,
> > for example JPEG metadata headers.
> >
> > Signed-off-by: Yudhistira Erlandinata <yerlandinata@chromium.org>
> > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> > ---
> >  include/libcamera/controls.h  | 6 ++++++
> >  src/libcamera/controls.cpp    | 8 ++++++++
> >  src/libcamera/v4l2_device.cpp | 9 +++++++++
> >  3 files changed, 23 insertions(+)
> >
> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > index 25f68040d..1987ca5f5 100644
> > --- a/include/libcamera/controls.h
> > +++ b/include/libcamera/controls.h
> > @@ -29,6 +29,7 @@ enum ControlType {
> >         ControlTypeNone,
> >         ControlTypeBool,
> >         ControlTypeByte,
> > +       ControlTypeUnsigned16,
> >         ControlTypeUnsigned32,
> >         ControlTypeInteger32,
> >         ControlTypeInteger64,
> > @@ -63,6 +64,11 @@ struct control_type<uint8_t> {
> >         static constexpr std::size_t size = 0;
> >  };
> >
> > +template<>
> > +struct control_type<uint16_t> {
> > +       static constexpr ControlType value = ControlTypeUnsigned16;
>
> Does this also need to set the size? What is size - why is it always
> zero?

Yes, you're right.

I think it's the default value for the potential array size:
https://git.libcamera.org/libcamera/libcamera.git/commit/?id=ab67fdd210e30ad3806f10cb5f6d4e325059f8de

BR,
Harvey

>
> > +};
> > +
> >  template<>
> >  struct control_type<uint32_t> {
> >         static constexpr ControlType value = ControlTypeUnsigned32;
> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > index 8ae295191..3a840bb22 100644
> > --- a/src/libcamera/controls.cpp
> > +++ b/src/libcamera/controls.cpp
> > @@ -54,6 +54,7 @@ static constexpr size_t ControlValueSize[] = {
> >         [ControlTypeNone]               = 0,
> >         [ControlTypeBool]               = sizeof(bool),
> >         [ControlTypeByte]               = sizeof(uint8_t),
> > +       [ControlTypeUnsigned16]         = sizeof(uint16_t),
> >         [ControlTypeUnsigned32]         = sizeof(uint32_t),
> >         [ControlTypeInteger32]          = sizeof(int32_t),
> >         [ControlTypeInteger64]          = sizeof(int64_t),
> > @@ -75,6 +76,8 @@ static constexpr size_t ControlValueSize[] = {
> >   * The control stores a boolean value
> >   * \var ControlTypeByte
> >   * The control stores a byte value as an unsigned 8-bit integer
> > + * \var ControlTypeUnsigned16
> > + * The control stores an unsigned 16-bit integer value
> >   * \var ControlTypeUnsigned32
> >   * The control stores an unsigned 32-bit integer value
> >   * \var ControlTypeInteger32
> > @@ -233,6 +236,11 @@ std::string ControlValue::toString() const
> >                         str += std::to_string(*value);
> >                         break;
> >                 }
> > +               case ControlTypeUnsigned16: {
> > +                       const uint16_t *value = reinterpret_cast<const uint16_t *>(data);
> > +                       str += std::to_string(*value);
> > +                       break;
> > +               }
> >                 case ControlTypeUnsigned32: {
> >                         const uint32_t *value = reinterpret_cast<const uint32_t *>(data);
> >                         str += std::to_string(*value);
> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > index f3454ba24..cb7327171 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -492,6 +492,9 @@ ControlType V4L2Device::v4l2CtrlType(uint32_t ctrlType)
> >         case V4L2_CTRL_TYPE_BOOLEAN:
> >                 return ControlTypeBool;
> >
> > +       case V4L2_CTRL_TYPE_U16:
> > +               return ControlTypeUnsigned16;
> > +
> >         case V4L2_CTRL_TYPE_U32:
> >                 return ControlTypeUnsigned32;
> >
> > @@ -543,6 +546,11 @@ std::optional<ControlInfo> V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl
> >                                    static_cast<uint8_t>(ctrl.maximum),
> >                                    static_cast<uint8_t>(ctrl.default_value));
> >
> > +       case V4L2_CTRL_TYPE_U16:
> > +               return ControlInfo(static_cast<uint16_t>(ctrl.minimum),
> > +                                  static_cast<uint16_t>(ctrl.maximum),
> > +                                  static_cast<uint16_t>(ctrl.default_value));
> > +
> >         case V4L2_CTRL_TYPE_U32:
> >                 return ControlInfo(static_cast<uint32_t>(ctrl.minimum),
> >                                    static_cast<uint32_t>(ctrl.maximum),
> > @@ -634,6 +642,7 @@ void V4L2Device::listControls()
> >                 case V4L2_CTRL_TYPE_BITMASK:
> >                 case V4L2_CTRL_TYPE_INTEGER_MENU:
> >                 case V4L2_CTRL_TYPE_U8:
> > +               case V4L2_CTRL_TYPE_U16:
> >                 case V4L2_CTRL_TYPE_U32:
> >                         break;
> >                 /* \todo Support other control types. */
> > --
> > 2.47.0.rc1.288.g06298d1525-goog
> >
Kieran Bingham Oct. 22, 2024, 10:20 a.m. UTC | #3 
Quoting Cheng-Hao Yang (2024-10-22 10:51:09)
> Hi Kieran,
> 
> On Tue, Oct 22, 2024 at 12:27 AM Kieran Bingham
> <kieran.bingham@ideasonboard.com> wrote:
> >
> > Quoting Harvey Yang (2024-10-11 18:55:12)
> > > From: Yudhistira Erlandinata <yerlandinata@google.com>
> > >
> > > Some camera metadata is of length 16-bits,
> > > for example JPEG metadata headers.
> > >
> > > Signed-off-by: Yudhistira Erlandinata <yerlandinata@chromium.org>
> > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> > > ---
> > >  include/libcamera/controls.h  | 6 ++++++
> > >  src/libcamera/controls.cpp    | 8 ++++++++
> > >  src/libcamera/v4l2_device.cpp | 9 +++++++++
> > >  3 files changed, 23 insertions(+)
> > >
> > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > > index 25f68040d..1987ca5f5 100644
> > > --- a/include/libcamera/controls.h
> > > +++ b/include/libcamera/controls.h
> > > @@ -29,6 +29,7 @@ enum ControlType {
> > >         ControlTypeNone,
> > >         ControlTypeBool,
> > >         ControlTypeByte,
> > > +       ControlTypeUnsigned16,
> > >         ControlTypeUnsigned32,
> > >         ControlTypeInteger32,
> > >         ControlTypeInteger64,
> > > @@ -63,6 +64,11 @@ struct control_type<uint8_t> {
> > >         static constexpr std::size_t size = 0;
> > >  };
> > >
> > > +template<>
> > > +struct control_type<uint16_t> {
> > > +       static constexpr ControlType value = ControlTypeUnsigned16;
> >
> > Does this also need to set the size? What is size - why is it always
> > zero?
> 
> Yes, you're right.
> 
> I think it's the default value for the potential array size:
> https://git.libcamera.org/libcamera/libcamera.git/commit/?id=ab67fdd210e30ad3806f10cb5f6d4e325059f8de
> 

Aha, thanks - that's where it's from...

--
Kieran

Patch
diff mbox series 

diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
index 25f68040d..1987ca5f5 100644
--- a/include/libcamera/controls.h
+++ b/include/libcamera/controls.h
@@ -29,6 +29,7 @@  enum ControlType {
 	ControlTypeNone,
 	ControlTypeBool,
 	ControlTypeByte,
+	ControlTypeUnsigned16,
 	ControlTypeUnsigned32,
 	ControlTypeInteger32,
 	ControlTypeInteger64,
@@ -63,6 +64,11 @@  struct control_type<uint8_t> {
 	static constexpr std::size_t size = 0;
 };
 
+template<>
+struct control_type<uint16_t> {
+	static constexpr ControlType value = ControlTypeUnsigned16;
+};
+
 template<>
 struct control_type<uint32_t> {
 	static constexpr ControlType value = ControlTypeUnsigned32;
diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
index 8ae295191..3a840bb22 100644
--- a/src/libcamera/controls.cpp
+++ b/src/libcamera/controls.cpp
@@ -54,6 +54,7 @@  static constexpr size_t ControlValueSize[] = {
 	[ControlTypeNone]		= 0,
 	[ControlTypeBool]		= sizeof(bool),
 	[ControlTypeByte]		= sizeof(uint8_t),
+	[ControlTypeUnsigned16]		= sizeof(uint16_t),
 	[ControlTypeUnsigned32]		= sizeof(uint32_t),
 	[ControlTypeInteger32]		= sizeof(int32_t),
 	[ControlTypeInteger64]		= sizeof(int64_t),
@@ -75,6 +76,8 @@  static constexpr size_t ControlValueSize[] = {
  * The control stores a boolean value
  * \var ControlTypeByte
  * The control stores a byte value as an unsigned 8-bit integer
+ * \var ControlTypeUnsigned16
+ * The control stores an unsigned 16-bit integer value
  * \var ControlTypeUnsigned32
  * The control stores an unsigned 32-bit integer value
  * \var ControlTypeInteger32
@@ -233,6 +236,11 @@  std::string ControlValue::toString() const
 			str += std::to_string(*value);
 			break;
 		}
+		case ControlTypeUnsigned16: {
+			const uint16_t *value = reinterpret_cast<const uint16_t *>(data);
+			str += std::to_string(*value);
+			break;
+		}
 		case ControlTypeUnsigned32: {
 			const uint32_t *value = reinterpret_cast<const uint32_t *>(data);
 			str += std::to_string(*value);
diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
index f3454ba24..cb7327171 100644
--- a/src/libcamera/v4l2_device.cpp
+++ b/src/libcamera/v4l2_device.cpp
@@ -492,6 +492,9 @@  ControlType V4L2Device::v4l2CtrlType(uint32_t ctrlType)
 	case V4L2_CTRL_TYPE_BOOLEAN:
 		return ControlTypeBool;
 
+	case V4L2_CTRL_TYPE_U16:
+		return ControlTypeUnsigned16;
+
 	case V4L2_CTRL_TYPE_U32:
 		return ControlTypeUnsigned32;
 
@@ -543,6 +546,11 @@  std::optional<ControlInfo> V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl
 				   static_cast<uint8_t>(ctrl.maximum),
 				   static_cast<uint8_t>(ctrl.default_value));
 
+	case V4L2_CTRL_TYPE_U16:
+		return ControlInfo(static_cast<uint16_t>(ctrl.minimum),
+				   static_cast<uint16_t>(ctrl.maximum),
+				   static_cast<uint16_t>(ctrl.default_value));
+
 	case V4L2_CTRL_TYPE_U32:
 		return ControlInfo(static_cast<uint32_t>(ctrl.minimum),
 				   static_cast<uint32_t>(ctrl.maximum),
@@ -634,6 +642,7 @@  void V4L2Device::listControls()
 		case V4L2_CTRL_TYPE_BITMASK:
 		case V4L2_CTRL_TYPE_INTEGER_MENU:
 		case V4L2_CTRL_TYPE_U8:
+		case V4L2_CTRL_TYPE_U16:
 		case V4L2_CTRL_TYPE_U32:
 			break;
 		/* \todo Support other control types. */