[libcamera-devel,v2,4/6] android: camera_capabilities: Fix the type of the capability vector
diff mbox series

Message ID 20211220232629.1485890-5-paul.elder@ideasonboard.com
State Accepted
Headers show
Series
  • android: Miscellaneous fixes
Related show

Commit Message

Paul Elder Dec. 20, 2021, 11:26 p.m. UTC 
The type of elements of the capability vector that is set in the static
metadata must be uint8_t. The enum will not suffice, as it is int32_t.
Fix this.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/android/camera_capabilities.cpp | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Jacopo Mondi Dec. 21, 2021, 10:42 a.m. UTC | #1 
Hi Paul,

On Mon, Dec 20, 2021 at 05:26:27PM -0600, Paul Elder wrote:
> The type of elements of the capability vector that is set in the static
> metadata must be uint8_t. The enum will not suffice, as it is int32_t.
> Fix this.
>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/android/camera_capabilities.cpp | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> index 6d383486..ea2aaf58 100644
> --- a/src/android/camera_capabilities.cpp
> +++ b/src/android/camera_capabilities.cpp
> @@ -1389,7 +1389,8 @@ int CameraCapabilities::initializeStaticMetadata()
>
>  	/* Check capabilities */
>  	capabilities_ = computeCapabilities();
> -	std::vector<camera_metadata_enum_android_request_available_capabilities>
> +	/* This *must* uint8_t. */
> +	std::vector<uint8_t>
>  		capsVec(capabilities_.begin(), capabilities_.end());

android.request.availableCapabilities is indeed documented as [byte x n]

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

>  	staticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CAPABILITIES, capsVec);
>
> --
> 2.27.0
>
Kieran Bingham Dec. 21, 2021, 11:55 a.m. UTC | #2 
Quoting Jacopo Mondi (2021-12-21 10:42:43)
> Hi Paul,
> 
> On Mon, Dec 20, 2021 at 05:26:27PM -0600, Paul Elder wrote:
> > The type of elements of the capability vector that is set in the static
> > metadata must be uint8_t. The enum will not suffice, as it is int32_t.
> > Fix this.
> >
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  src/android/camera_capabilities.cpp | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> > index 6d383486..ea2aaf58 100644
> > --- a/src/android/camera_capabilities.cpp
> > +++ b/src/android/camera_capabilities.cpp
> > @@ -1389,7 +1389,8 @@ int CameraCapabilities::initializeStaticMetadata()
> >
> >       /* Check capabilities */
> >       capabilities_ = computeCapabilities();
> > -     std::vector<camera_metadata_enum_android_request_available_capabilities>
> > +     /* This *must* uint8_t. */

Still missing the 'be' that I highlighted in a previous version though
;-)

--
Kieran


> > +     std::vector<uint8_t>
> >               capsVec(capabilities_.begin(), capabilities_.end());
> 
> android.request.availableCapabilities is indeed documented as [byte x n]
> 
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> Thanks
>   j
> 
> >       staticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CAPABILITIES, capsVec);
> >
> > --
> > 2.27.0
> >
Jacopo Mondi Dec. 21, 2021, 12:46 p.m. UTC | #3 
Hi again

On Tue, Dec 21, 2021 at 11:55:18AM +0000, Kieran Bingham wrote:
> Quoting Jacopo Mondi (2021-12-21 10:42:43)
> > Hi Paul,
> >
> > On Mon, Dec 20, 2021 at 05:26:27PM -0600, Paul Elder wrote:
> > > The type of elements of the capability vector that is set in the static
> > > metadata must be uint8_t. The enum will not suffice, as it is int32_t.
> > > Fix this.
> > >
> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > ---
> > >  src/android/camera_capabilities.cpp | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> > > index 6d383486..ea2aaf58 100644
> > > --- a/src/android/camera_capabilities.cpp
> > > +++ b/src/android/camera_capabilities.cpp
> > > @@ -1389,7 +1389,8 @@ int CameraCapabilities::initializeStaticMetadata()
> > >
> > >       /* Check capabilities */
> > >       capabilities_ = computeCapabilities();
> > > -     std::vector<camera_metadata_enum_android_request_available_capabilities>
> > > +     /* This *must* uint8_t. */
>
> Still missing the 'be' that I highlighted in a previous version though
> ;-)

Do we need the comment at all ?

>
> --
> Kieran
>
>
> > > +     std::vector<uint8_t>
> > >               capsVec(capabilities_.begin(), capabilities_.end());
> >
> > android.request.availableCapabilities is indeed documented as [byte x n]
> >
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> >
> > Thanks
> >   j
> >
> > >       staticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CAPABILITIES, capsVec);
> > >
> > > --
> > > 2.27.0
> > >
Paul Elder Dec. 21, 2021, 4:17 p.m. UTC | #4 
Hello,

On Tue, Dec 21, 2021 at 01:46:09PM +0100, Jacopo Mondi wrote:
> Hi again
> 
> On Tue, Dec 21, 2021 at 11:55:18AM +0000, Kieran Bingham wrote:
> > Quoting Jacopo Mondi (2021-12-21 10:42:43)
> > > Hi Paul,
> > >
> > > On Mon, Dec 20, 2021 at 05:26:27PM -0600, Paul Elder wrote:
> > > > The type of elements of the capability vector that is set in the static
> > > > metadata must be uint8_t. The enum will not suffice, as it is int32_t.
> > > > Fix this.
> > > >
> > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > ---
> > > >  src/android/camera_capabilities.cpp | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> > > > index 6d383486..ea2aaf58 100644
> > > > --- a/src/android/camera_capabilities.cpp
> > > > +++ b/src/android/camera_capabilities.cpp
> > > > @@ -1389,7 +1389,8 @@ int CameraCapabilities::initializeStaticMetadata()
> > > >
> > > >       /* Check capabilities */
> > > >       capabilities_ = computeCapabilities();
> > > > -     std::vector<camera_metadata_enum_android_request_available_capabilities>
> > > > +     /* This *must* uint8_t. */
> >
> > Still missing the 'be' that I highlighted in a previous version though
> > ;-)

Oh, that's what you were highlighting; I missed it :/

> 
> Do we need the comment at all ?

imo yes, otherwise I'd rather use the enum type because it's more
specific.


Paul

> 
> >
> > --
> > Kieran
> >
> >
> > > > +     std::vector<uint8_t>
> > > >               capsVec(capabilities_.begin(), capabilities_.end());
> > >
> > > android.request.availableCapabilities is indeed documented as [byte x n]
> > >
> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > >
> > > Thanks
> > >   j
> > >
> > > >       staticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CAPABILITIES, capsVec);
> > > >
> > > > --
> > > > 2.27.0
> > > >
Kieran Bingham Dec. 22, 2021, 10:27 a.m. UTC | #5 
Quoting paul.elder@ideasonboard.com (2021-12-21 16:17:19)
> Hello,
> 
> On Tue, Dec 21, 2021 at 01:46:09PM +0100, Jacopo Mondi wrote:
> > Hi again
> > 
> > On Tue, Dec 21, 2021 at 11:55:18AM +0000, Kieran Bingham wrote:
> > > Quoting Jacopo Mondi (2021-12-21 10:42:43)
> > > > Hi Paul,
> > > >
> > > > On Mon, Dec 20, 2021 at 05:26:27PM -0600, Paul Elder wrote:
> > > > > The type of elements of the capability vector that is set in the static
> > > > > metadata must be uint8_t. The enum will not suffice, as it is int32_t.
> > > > > Fix this.
> > > > >
> > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > > ---
> > > > >  src/android/camera_capabilities.cpp | 3 ++-
> > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> > > > > index 6d383486..ea2aaf58 100644
> > > > > --- a/src/android/camera_capabilities.cpp
> > > > > +++ b/src/android/camera_capabilities.cpp
> > > > > @@ -1389,7 +1389,8 @@ int CameraCapabilities::initializeStaticMetadata()
> > > > >
> > > > >       /* Check capabilities */
> > > > >       capabilities_ = computeCapabilities();
> > > > > -     std::vector<camera_metadata_enum_android_request_available_capabilities>
> > > > > +     /* This *must* uint8_t. */
> > >
> > > Still missing the 'be' that I highlighted in a previous version though
> > > ;-)
> 
> Oh, that's what you were highlighting; I missed it :/
> 
> > 
> > Do we need the comment at all ?
> 
> imo yes, otherwise I'd rather use the enum type because it's more
> specific.
> 
> 
> Paul
> 
> > 
> > >
> > > --
> > > Kieran
> > >
> > >
> > > > > +     std::vector<uint8_t>
> > > > >               capsVec(capabilities_.begin(), capabilities_.end());

It's minor, and quite late but:

Would that be suitable on one line now ?
--
Kieran


> > > >
> > > > android.request.availableCapabilities is indeed documented as [byte x n]
> > > >
> > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > > >
> > > > Thanks
> > > >   j
> > > >
> > > > >       staticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CAPABILITIES, capsVec);
> > > > >
> > > > > --
> > > > > 2.27.0
> > > > >

Patch
diff mbox series 

diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
index 6d383486..ea2aaf58 100644
--- a/src/android/camera_capabilities.cpp
+++ b/src/android/camera_capabilities.cpp
@@ -1389,7 +1389,8 @@  int CameraCapabilities::initializeStaticMetadata()
 
 	/* Check capabilities */
 	capabilities_ = computeCapabilities();
-	std::vector<camera_metadata_enum_android_request_available_capabilities>
+	/* This *must* uint8_t. */
+	std::vector<uint8_t>
 		capsVec(capabilities_.begin(), capabilities_.end());
 	staticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CAPABILITIES, capsVec);