[libcamera-devel,2/4] android: camera_device: Use HAL_PIXEL_FORMAT_* defines for formats
diff mbox series

Message ID 20200723173942.98182-3-niklas.soderlund@ragnatech.se
State Superseded
Headers show
Series
  • android: camera_device: Improve on format related tags
Related show

Commit Message

Niklas Söderlund July 23, 2020, 5:39 p.m. UTC 
The documentation says the HAL_PIXEL_FORMAT_* defines shall be used for
formats instead of ANDROID_SCALER_AVAILABLE_FORMATS_* for the
ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS and
ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS tags.

This have worked in the past as the numerical value of the two sets are
the same for the formats supported.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/android/camera_device.cpp | 8 ++++----
 src/android/camera_device.h   | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

Comments

Jacopo Mondi July 24, 2020, 8:34 a.m. UTC | #1 
Hi Niklas,

On Thu, Jul 23, 2020 at 07:39:40PM +0200, Niklas Söderlund wrote:
> The documentation says the HAL_PIXEL_FORMAT_* defines shall be used for
> formats instead of ANDROID_SCALER_AVAILABLE_FORMATS_* for the
> ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS and
> ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS tags.
>
> This have worked in the past as the numerical value of the two sets are
> the same for the formats supported.

I don't find any mention of this in the documentation, but as the
numerical values are the same (very nice Android, yes...) if this
allow removing the scaler formats, I think that's ok

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

Thanks
   j

>
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/android/camera_device.cpp | 8 ++++----
>  src/android/camera_device.h   | 2 +-
>  2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 6e7673583f6a8f85..96dd8d5a99082966 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -317,14 +317,14 @@ int CameraDevice::initializeStreamConfigurations()
>  			    status != CameraConfiguration::Valid)
>  				continue;
>
> -			streamConfigurations_.push_back({ res, camera3Format.scalerFormat });
> +			streamConfigurations_.push_back({ res, androidFormat });
>  		}
>  	}
>
>  	LOG(HAL, Debug) << "Collected stream configuration map: ";
>  	for (const auto &entry : streamConfigurations_)
>  		LOG(HAL, Debug) << "{ " << entry.resolution.toString() << " - "
> -				<< utils::hex(entry.androidScalerCode) << " }";
> +				<< utils::hex(entry.androidFormat) << " }";
>
>  	return 0;
>  }
> @@ -658,7 +658,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
>  	std::vector<uint32_t> availableStreamConfigurations;
>  	availableStreamConfigurations.reserve(streamConfigurations_.size() * 4);
>  	for (const auto &entry : streamConfigurations_) {
> -		availableStreamConfigurations.push_back(entry.androidScalerCode);
> +		availableStreamConfigurations.push_back(entry.androidFormat);
>  		availableStreamConfigurations.push_back(entry.resolution.width);
>  		availableStreamConfigurations.push_back(entry.resolution.height);
>  		availableStreamConfigurations.push_back(
> @@ -679,7 +679,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
>  	std::vector<int64_t> minFrameDurations;
>  	minFrameDurations.reserve(streamConfigurations_.size() * 4);
>  	for (const auto &entry : streamConfigurations_) {
> -		minFrameDurations.push_back(entry.androidScalerCode);
> +		minFrameDurations.push_back(entry.androidFormat);
>  		minFrameDurations.push_back(entry.resolution.width);
>  		minFrameDurations.push_back(entry.resolution.height);
>  		minFrameDurations.push_back(33333333);
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 5b8b9c3e26e2871e..00472c21938871a1 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -75,7 +75,7 @@ private:
>
>  	struct Camera3StreamConfiguration {
>  		libcamera::Size resolution;
> -		int androidScalerCode;
> +		int androidFormat;
>  	};
>
>  	int initializeStreamConfigurations();
> --
> 2.27.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Jacopo Mondi July 24, 2020, 8:43 a.m. UTC | #2 
Hi Niklas,

On Thu, Jul 23, 2020 at 07:39:40PM +0200, Niklas Söderlund wrote:
> The documentation says the HAL_PIXEL_FORMAT_* defines shall be used for
> formats instead of ANDROID_SCALER_AVAILABLE_FORMATS_* for the
> ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS and
> ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS tags.
>
> This have worked in the past as the numerical value of the two sets are
> the same for the formats supported.

I wonder if in the future we would need scaler formats for other
properties, but as the numerical values are the same, we could use the
HAL_PIXEL_FORMAT_* definitions.

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

Thanks
  j

>
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/android/camera_device.cpp | 8 ++++----
>  src/android/camera_device.h   | 2 +-
>  2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 6e7673583f6a8f85..96dd8d5a99082966 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -317,14 +317,14 @@ int CameraDevice::initializeStreamConfigurations()
>  			    status != CameraConfiguration::Valid)
>  				continue;
>
> -			streamConfigurations_.push_back({ res, camera3Format.scalerFormat });
> +			streamConfigurations_.push_back({ res, androidFormat });
>  		}
>  	}
>
>  	LOG(HAL, Debug) << "Collected stream configuration map: ";
>  	for (const auto &entry : streamConfigurations_)
>  		LOG(HAL, Debug) << "{ " << entry.resolution.toString() << " - "
> -				<< utils::hex(entry.androidScalerCode) << " }";
> +				<< utils::hex(entry.androidFormat) << " }";
>
>  	return 0;
>  }
> @@ -658,7 +658,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
>  	std::vector<uint32_t> availableStreamConfigurations;
>  	availableStreamConfigurations.reserve(streamConfigurations_.size() * 4);
>  	for (const auto &entry : streamConfigurations_) {
> -		availableStreamConfigurations.push_back(entry.androidScalerCode);
> +		availableStreamConfigurations.push_back(entry.androidFormat);
>  		availableStreamConfigurations.push_back(entry.resolution.width);
>  		availableStreamConfigurations.push_back(entry.resolution.height);
>  		availableStreamConfigurations.push_back(
> @@ -679,7 +679,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
>  	std::vector<int64_t> minFrameDurations;
>  	minFrameDurations.reserve(streamConfigurations_.size() * 4);
>  	for (const auto &entry : streamConfigurations_) {
> -		minFrameDurations.push_back(entry.androidScalerCode);
> +		minFrameDurations.push_back(entry.androidFormat);
>  		minFrameDurations.push_back(entry.resolution.width);
>  		minFrameDurations.push_back(entry.resolution.height);
>  		minFrameDurations.push_back(33333333);
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 5b8b9c3e26e2871e..00472c21938871a1 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -75,7 +75,7 @@ private:
>
>  	struct Camera3StreamConfiguration {
>  		libcamera::Size resolution;
> -		int androidScalerCode;
> +		int androidFormat;
>  	};
>
>  	int initializeStreamConfigurations();
> --
> 2.27.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Niklas Söderlund July 24, 2020, 8:44 a.m. UTC | #3 
Hi Jacopo,

Thanks for your feedback.

On 2020-07-24 10:43:23 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Thu, Jul 23, 2020 at 07:39:40PM +0200, Niklas Söderlund wrote:
> > The documentation says the HAL_PIXEL_FORMAT_* defines shall be used for
> > formats instead of ANDROID_SCALER_AVAILABLE_FORMATS_* for the
> > ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS and
> > ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS tags.
> >
> > This have worked in the past as the numerical value of the two sets are
> > the same for the formats supported.
> 
> I wonder if in the future we would need scaler formats for other
> properties, but as the numerical values are the same, we could use the
> HAL_PIXEL_FORMAT_* definitions.
> 
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Was this comment and tag indented for 3/4 ?

> 
> Thanks
>   j
> 
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> >  src/android/camera_device.cpp | 8 ++++----
> >  src/android/camera_device.h   | 2 +-
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 6e7673583f6a8f85..96dd8d5a99082966 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -317,14 +317,14 @@ int CameraDevice::initializeStreamConfigurations()
> >  			    status != CameraConfiguration::Valid)
> >  				continue;
> >
> > -			streamConfigurations_.push_back({ res, camera3Format.scalerFormat });
> > +			streamConfigurations_.push_back({ res, androidFormat });
> >  		}
> >  	}
> >
> >  	LOG(HAL, Debug) << "Collected stream configuration map: ";
> >  	for (const auto &entry : streamConfigurations_)
> >  		LOG(HAL, Debug) << "{ " << entry.resolution.toString() << " - "
> > -				<< utils::hex(entry.androidScalerCode) << " }";
> > +				<< utils::hex(entry.androidFormat) << " }";
> >
> >  	return 0;
> >  }
> > @@ -658,7 +658,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
> >  	std::vector<uint32_t> availableStreamConfigurations;
> >  	availableStreamConfigurations.reserve(streamConfigurations_.size() * 4);
> >  	for (const auto &entry : streamConfigurations_) {
> > -		availableStreamConfigurations.push_back(entry.androidScalerCode);
> > +		availableStreamConfigurations.push_back(entry.androidFormat);
> >  		availableStreamConfigurations.push_back(entry.resolution.width);
> >  		availableStreamConfigurations.push_back(entry.resolution.height);
> >  		availableStreamConfigurations.push_back(
> > @@ -679,7 +679,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
> >  	std::vector<int64_t> minFrameDurations;
> >  	minFrameDurations.reserve(streamConfigurations_.size() * 4);
> >  	for (const auto &entry : streamConfigurations_) {
> > -		minFrameDurations.push_back(entry.androidScalerCode);
> > +		minFrameDurations.push_back(entry.androidFormat);
> >  		minFrameDurations.push_back(entry.resolution.width);
> >  		minFrameDurations.push_back(entry.resolution.height);
> >  		minFrameDurations.push_back(33333333);
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index 5b8b9c3e26e2871e..00472c21938871a1 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -75,7 +75,7 @@ private:
> >
> >  	struct Camera3StreamConfiguration {
> >  		libcamera::Size resolution;
> > -		int androidScalerCode;
> > +		int androidFormat;
> >  	};
> >
> >  	int initializeStreamConfigurations();
> > --
> > 2.27.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
Jacopo Mondi July 24, 2020, 9 a.m. UTC | #4 
Hi Niklas,

On Fri, Jul 24, 2020 at 10:44:23AM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your feedback.
>
> On 2020-07-24 10:43:23 +0200, Jacopo Mondi wrote:
> > Hi Niklas,
> >
> > On Thu, Jul 23, 2020 at 07:39:40PM +0200, Niklas Söderlund wrote:
> > > The documentation says the HAL_PIXEL_FORMAT_* defines shall be used for
> > > formats instead of ANDROID_SCALER_AVAILABLE_FORMATS_* for the
> > > ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS and
> > > ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS tags.
> > >
> > > This have worked in the past as the numerical value of the two sets are
> > > the same for the formats supported.
> >
> > I wonder if in the future we would need scaler formats for other
> > properties, but as the numerical values are the same, we could use the
> > HAL_PIXEL_FORMAT_* definitions.
> >
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>
> Was this comment and tag indented for 3/4 ?
>

Yes :)

Sorry for confusion

Patch
diff mbox series 

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index 6e7673583f6a8f85..96dd8d5a99082966 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -317,14 +317,14 @@  int CameraDevice::initializeStreamConfigurations()
 			    status != CameraConfiguration::Valid)
 				continue;
 
-			streamConfigurations_.push_back({ res, camera3Format.scalerFormat });
+			streamConfigurations_.push_back({ res, androidFormat });
 		}
 	}
 
 	LOG(HAL, Debug) << "Collected stream configuration map: ";
 	for (const auto &entry : streamConfigurations_)
 		LOG(HAL, Debug) << "{ " << entry.resolution.toString() << " - "
-				<< utils::hex(entry.androidScalerCode) << " }";
+				<< utils::hex(entry.androidFormat) << " }";
 
 	return 0;
 }
@@ -658,7 +658,7 @@  const camera_metadata_t *CameraDevice::getStaticMetadata()
 	std::vector<uint32_t> availableStreamConfigurations;
 	availableStreamConfigurations.reserve(streamConfigurations_.size() * 4);
 	for (const auto &entry : streamConfigurations_) {
-		availableStreamConfigurations.push_back(entry.androidScalerCode);
+		availableStreamConfigurations.push_back(entry.androidFormat);
 		availableStreamConfigurations.push_back(entry.resolution.width);
 		availableStreamConfigurations.push_back(entry.resolution.height);
 		availableStreamConfigurations.push_back(
@@ -679,7 +679,7 @@  const camera_metadata_t *CameraDevice::getStaticMetadata()
 	std::vector<int64_t> minFrameDurations;
 	minFrameDurations.reserve(streamConfigurations_.size() * 4);
 	for (const auto &entry : streamConfigurations_) {
-		minFrameDurations.push_back(entry.androidScalerCode);
+		minFrameDurations.push_back(entry.androidFormat);
 		minFrameDurations.push_back(entry.resolution.width);
 		minFrameDurations.push_back(entry.resolution.height);
 		minFrameDurations.push_back(33333333);
diff --git a/src/android/camera_device.h b/src/android/camera_device.h
index 5b8b9c3e26e2871e..00472c21938871a1 100644
--- a/src/android/camera_device.h
+++ b/src/android/camera_device.h
@@ -75,7 +75,7 @@  private:
 
 	struct Camera3StreamConfiguration {
 		libcamera::Size resolution;
-		int androidScalerCode;
+		int androidFormat;
 	};
 
 	int initializeStreamConfigurations();