[libcamera-devel,v3,02/11] android: camera_device: Generate JPEG sizes
diff mbox series

Message ID 20200908134142.27470-3-jacopo@jmondi.org
State Accepted
Headers show
Series
  • android: camera_device: Fix JPEG/RAW sizes
Related show

Commit Message

Jacopo Mondi Sept. 8, 2020, 1:41 p.m. UTC 
When producing the list of image resolutions to claim as supported by the
camera HAL, the JPEG stream was assumed to be 'always valid' as, at the
time, there was no JPEG support in place at all.

With the introduction of support for JPEG compression, reporting
non-valid sizes as supported obviously causes troubles.

In order to avoid reporting non-supported resolutions as supported,
produce the list of available JPEG sizes by using the ones supported
by the YCbCr_420_888 format, from which the JPEG stream is encoded.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/android/camera_device.cpp | 40 ++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 15 deletions(-)

Comments

Niklas Söderlund Sept. 10, 2020, 10:33 a.m. UTC | #1 
Hi Jacopo,

Thanks for your work.

On 2020-09-08 15:41:33 +0200, Jacopo Mondi wrote:
> When producing the list of image resolutions to claim as supported by the
> camera HAL, the JPEG stream was assumed to be 'always valid' as, at the
> time, there was no JPEG support in place at all.
> 
> With the introduction of support for JPEG compression, reporting
> non-valid sizes as supported obviously causes troubles.
> 
> In order to avoid reporting non-supported resolutions as supported,
> produce the list of available JPEG sizes by using the ones supported
> by the YCbCr_420_888 format, from which the JPEG stream is encoded.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/android/camera_device.cpp | 40 ++++++++++++++++++++++-------------
>  1 file changed, 25 insertions(+), 15 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 28fb3868c082..1b2e12d6d33c 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -363,17 +363,21 @@ int CameraDevice::initializeStreamConfigurations()
>  		const std::vector<PixelFormat> &libcameraFormats =
>  			camera3Format.libcameraFormats;
>  
> +		/*
> +		 * JPEG is always supported, either produced directly by the
> +		 * camera, or encoded in the HAL.
> +		 */
> +		if (androidFormat == HAL_PIXEL_FORMAT_BLOB) {
> +			formatsMap_[androidFormat] = formats::MJPEG;
> +			continue;
> +		}
> +
>  		/*
>  		 * Test the libcamera formats that can produce images
>  		 * compatible with the format defined by Android.
>  		 */
>  		PixelFormat mappedFormat;
>  		for (const PixelFormat &pixelFormat : libcameraFormats) {
> -			/* \todo Fixed mapping for JPEG. */
> -			if (androidFormat == HAL_PIXEL_FORMAT_BLOB) {
> -				mappedFormat = formats::MJPEG;
> -				break;
> -			}
>  
>  			/*
>  			 * The stream configuration size can be adjusted,
> @@ -416,19 +420,25 @@ int CameraDevice::initializeStreamConfigurations()
>  			cfg.size = res;
>  
>  			CameraConfiguration::Status status = cameraConfig->validate();
> -			/*
> -			 * Unconditionally report we can produce JPEG.
> -			 *
> -			 * \todo The JPEG stream will be implemented as an
> -			 * HAL-only stream, but some cameras can produce it
> -			 * directly. As of now, claim support for JPEG without
> -			 * inspecting where the JPEG stream is produced.
> -			 */
> -			if (androidFormat != HAL_PIXEL_FORMAT_BLOB &&
> -			    status != CameraConfiguration::Valid)
> +			if (status != CameraConfiguration::Valid)
>  				continue;
>  
>  			streamConfigurations_.push_back({ res, androidFormat });
> +
> +			/*
> +			 * If the format is HAL_PIXEL_FORMAT_YCbCr_420_888
> +			 * from which JPEG is produced, add an entry for
> +			 * the JPEG stream.
> +			 *
> +			 * \todo Wire the JPEG encoder to query the supported
> +			 * sizes provided a list of formats it can encode.

nit: Is this needed? Are we expecting the JPEG encoder to reject some 
resolutions?

In any case,

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> +			 *
> +			 * \todo Support JPEG streams produced by the Camera
> +			 * natively.
> +			 */
> +			if (androidFormat == HAL_PIXEL_FORMAT_YCbCr_420_888)
> +				streamConfigurations_.push_back(
> +					{ res, HAL_PIXEL_FORMAT_BLOB });
>  		}
>  	}
>  
> -- 
> 2.28.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Jacopo Mondi Sept. 10, 2020, 11:14 a.m. UTC | #2 
Hi Niklas,

On Thu, Sep 10, 2020 at 12:33:06PM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your work.
>
> On 2020-09-08 15:41:33 +0200, Jacopo Mondi wrote:
> > When producing the list of image resolutions to claim as supported by the
> > camera HAL, the JPEG stream was assumed to be 'always valid' as, at the
> > time, there was no JPEG support in place at all.
> >
> > With the introduction of support for JPEG compression, reporting
> > non-valid sizes as supported obviously causes troubles.
> >
> > In order to avoid reporting non-supported resolutions as supported,
> > produce the list of available JPEG sizes by using the ones supported
> > by the YCbCr_420_888 format, from which the JPEG stream is encoded.
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/android/camera_device.cpp | 40 ++++++++++++++++++++++-------------
> >  1 file changed, 25 insertions(+), 15 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 28fb3868c082..1b2e12d6d33c 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -363,17 +363,21 @@ int CameraDevice::initializeStreamConfigurations()
> >  		const std::vector<PixelFormat> &libcameraFormats =
> >  			camera3Format.libcameraFormats;
> >
> > +		/*
> > +		 * JPEG is always supported, either produced directly by the
> > +		 * camera, or encoded in the HAL.
> > +		 */
> > +		if (androidFormat == HAL_PIXEL_FORMAT_BLOB) {
> > +			formatsMap_[androidFormat] = formats::MJPEG;
> > +			continue;
> > +		}
> > +
> >  		/*
> >  		 * Test the libcamera formats that can produce images
> >  		 * compatible with the format defined by Android.
> >  		 */
> >  		PixelFormat mappedFormat;
> >  		for (const PixelFormat &pixelFormat : libcameraFormats) {
> > -			/* \todo Fixed mapping for JPEG. */
> > -			if (androidFormat == HAL_PIXEL_FORMAT_BLOB) {
> > -				mappedFormat = formats::MJPEG;
> > -				break;
> > -			}
> >
> >  			/*
> >  			 * The stream configuration size can be adjusted,
> > @@ -416,19 +420,25 @@ int CameraDevice::initializeStreamConfigurations()
> >  			cfg.size = res;
> >
> >  			CameraConfiguration::Status status = cameraConfig->validate();
> > -			/*
> > -			 * Unconditionally report we can produce JPEG.
> > -			 *
> > -			 * \todo The JPEG stream will be implemented as an
> > -			 * HAL-only stream, but some cameras can produce it
> > -			 * directly. As of now, claim support for JPEG without
> > -			 * inspecting where the JPEG stream is produced.
> > -			 */
> > -			if (androidFormat != HAL_PIXEL_FORMAT_BLOB &&
> > -			    status != CameraConfiguration::Valid)
> > +			if (status != CameraConfiguration::Valid)
> >  				continue;
> >
> >  			streamConfigurations_.push_back({ res, androidFormat });
> > +
> > +			/*
> > +			 * If the format is HAL_PIXEL_FORMAT_YCbCr_420_888
> > +			 * from which JPEG is produced, add an entry for
> > +			 * the JPEG stream.
> > +			 *
> > +			 * \todo Wire the JPEG encoder to query the supported
> > +			 * sizes provided a list of formats it can encode.
>
> nit: Is this needed? Are we expecting the JPEG encoder to reject some
> resolutions?

It's more about alignments and limitations in the supported
resolutions.

>
> In any case,
>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
>
> > +			 *
> > +			 * \todo Support JPEG streams produced by the Camera
> > +			 * natively.
> > +			 */
> > +			if (androidFormat == HAL_PIXEL_FORMAT_YCbCr_420_888)
> > +				streamConfigurations_.push_back(
> > +					{ res, HAL_PIXEL_FORMAT_BLOB });
> >  		}
> >  	}
> >
> > --
> > 2.28.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund
Hirokazu Honda Sept. 11, 2020, 2:02 a.m. UTC | #3 
Thanks for the patch. LGTM.

On Thu, Sep 10, 2020 at 8:10 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Niklas,
>
> On Thu, Sep 10, 2020 at 12:33:06PM +0200, Niklas Söderlund wrote:
> > Hi Jacopo,
> >
> > Thanks for your work.
> >
> > On 2020-09-08 15:41:33 +0200, Jacopo Mondi wrote:
> > > When producing the list of image resolutions to claim as supported by the
> > > camera HAL, the JPEG stream was assumed to be 'always valid' as, at the
> > > time, there was no JPEG support in place at all.
> > >
> > > With the introduction of support for JPEG compression, reporting
> > > non-valid sizes as supported obviously causes troubles.
> > >
> > > In order to avoid reporting non-supported resolutions as supported,
> > > produce the list of available JPEG sizes by using the ones supported
> > > by the YCbCr_420_888 format, from which the JPEG stream is encoded.
> > >
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> > > ---
> > >  src/android/camera_device.cpp | 40 ++++++++++++++++++++++-------------
> > >  1 file changed, 25 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index 28fb3868c082..1b2e12d6d33c 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -363,17 +363,21 @@ int CameraDevice::initializeStreamConfigurations()
> > >             const std::vector<PixelFormat> &libcameraFormats =
> > >                     camera3Format.libcameraFormats;
> > >
> > > +           /*
> > > +            * JPEG is always supported, either produced directly by the
> > > +            * camera, or encoded in the HAL.
> > > +            */
> > > +           if (androidFormat == HAL_PIXEL_FORMAT_BLOB) {
> > > +                   formatsMap_[androidFormat] = formats::MJPEG;
> > > +                   continue;
> > > +           }
> > > +
> > >             /*
> > >              * Test the libcamera formats that can produce images
> > >              * compatible with the format defined by Android.
> > >              */
> > >             PixelFormat mappedFormat;
> > >             for (const PixelFormat &pixelFormat : libcameraFormats) {
> > > -                   /* \todo Fixed mapping for JPEG. */
> > > -                   if (androidFormat == HAL_PIXEL_FORMAT_BLOB) {
> > > -                           mappedFormat = formats::MJPEG;
> > > -                           break;
> > > -                   }
> > >
> > >                     /*
> > >                      * The stream configuration size can be adjusted,
> > > @@ -416,19 +420,25 @@ int CameraDevice::initializeStreamConfigurations()
> > >                     cfg.size = res;
> > >
> > >                     CameraConfiguration::Status status = cameraConfig->validate();
> > > -                   /*
> > > -                    * Unconditionally report we can produce JPEG.
> > > -                    *
> > > -                    * \todo The JPEG stream will be implemented as an
> > > -                    * HAL-only stream, but some cameras can produce it
> > > -                    * directly. As of now, claim support for JPEG without
> > > -                    * inspecting where the JPEG stream is produced.
> > > -                    */
> > > -                   if (androidFormat != HAL_PIXEL_FORMAT_BLOB &&
> > > -                       status != CameraConfiguration::Valid)
> > > +                   if (status != CameraConfiguration::Valid)
> > >                             continue;
> > >
> > >                     streamConfigurations_.push_back({ res, androidFormat });
> > > +
> > > +                   /*
> > > +                    * If the format is HAL_PIXEL_FORMAT_YCbCr_420_888
> > > +                    * from which JPEG is produced, add an entry for
> > > +                    * the JPEG stream.
> > > +                    *
> > > +                    * \todo Wire the JPEG encoder to query the supported
> > > +                    * sizes provided a list of formats it can encode.
> >
> > nit: Is this needed? Are we expecting the JPEG encoder to reject some
> > resolutions?
>
> It's more about alignments and limitations in the supported
> resolutions.
>
> >
> > In any case,
> >
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> >
> > > +                    *
> > > +                    * \todo Support JPEG streams produced by the Camera
> > > +                    * natively.
> > > +                    */
> > > +                   if (androidFormat == HAL_PIXEL_FORMAT_YCbCr_420_888)
> > > +                           streamConfigurations_.push_back(
> > > +                                   { res, HAL_PIXEL_FORMAT_BLOB });
> > >             }
> > >     }
> > >
> > > --
> > > 2.28.0
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel@lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
> >
> > --
> > Regards,
> > Niklas Söderlund
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series 

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index 28fb3868c082..1b2e12d6d33c 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -363,17 +363,21 @@  int CameraDevice::initializeStreamConfigurations()
 		const std::vector<PixelFormat> &libcameraFormats =
 			camera3Format.libcameraFormats;
 
+		/*
+		 * JPEG is always supported, either produced directly by the
+		 * camera, or encoded in the HAL.
+		 */
+		if (androidFormat == HAL_PIXEL_FORMAT_BLOB) {
+			formatsMap_[androidFormat] = formats::MJPEG;
+			continue;
+		}
+
 		/*
 		 * Test the libcamera formats that can produce images
 		 * compatible with the format defined by Android.
 		 */
 		PixelFormat mappedFormat;
 		for (const PixelFormat &pixelFormat : libcameraFormats) {
-			/* \todo Fixed mapping for JPEG. */
-			if (androidFormat == HAL_PIXEL_FORMAT_BLOB) {
-				mappedFormat = formats::MJPEG;
-				break;
-			}
 
 			/*
 			 * The stream configuration size can be adjusted,
@@ -416,19 +420,25 @@  int CameraDevice::initializeStreamConfigurations()
 			cfg.size = res;
 
 			CameraConfiguration::Status status = cameraConfig->validate();
-			/*
-			 * Unconditionally report we can produce JPEG.
-			 *
-			 * \todo The JPEG stream will be implemented as an
-			 * HAL-only stream, but some cameras can produce it
-			 * directly. As of now, claim support for JPEG without
-			 * inspecting where the JPEG stream is produced.
-			 */
-			if (androidFormat != HAL_PIXEL_FORMAT_BLOB &&
-			    status != CameraConfiguration::Valid)
+			if (status != CameraConfiguration::Valid)
 				continue;
 
 			streamConfigurations_.push_back({ res, androidFormat });
+
+			/*
+			 * If the format is HAL_PIXEL_FORMAT_YCbCr_420_888
+			 * from which JPEG is produced, add an entry for
+			 * the JPEG stream.
+			 *
+			 * \todo Wire the JPEG encoder to query the supported
+			 * sizes provided a list of formats it can encode.
+			 *
+			 * \todo Support JPEG streams produced by the Camera
+			 * natively.
+			 */
+			if (androidFormat == HAL_PIXEL_FORMAT_YCbCr_420_888)
+				streamConfigurations_.push_back(
+					{ res, HAL_PIXEL_FORMAT_BLOB });
 		}
 	}