[libcamera-devel,v2,02/12] android: camera_device: Generate JPEG sizes

9463 diff mbox series

When producing the list of image resolution 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 | 43 +++++++++++++++++++++++------------
 1 file changed, 28 insertions(+), 15 deletions(-)

Hi Jacopo,

Thank you for the patch.

On Wed, Sep 02, 2020 at 05:22:26PM +0200, Jacopo Mondi wrote:
> When producing the list of image resolution to claim as supported by the

s/resolution/resolutions/

> 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.

Down the line we'll likely need to scale down images for JPEG
compression, but for now this looks fine to me.

> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/android/camera_device.cpp | 43 +++++++++++++++++++++++------------
>  1 file changed, 28 insertions(+), 15 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index ad0d7fd15d90..8a8072123961 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -363,17 +363,27 @@ int CameraDevice::initializeStreamConfigurations()
>  		const std::vector<PixelFormat> &libcameraFormats =
>  			camera3Format.libcameraFormats;
>  
> +		/*
> +		 * Fixed format mapping for JPEG.
> +		 *
> +		 * The list of supported JPEG resolutions is generated
> +		 * from the list of resolutions supported by
> +		 * HAL_PIXEL_FORMAT_YCbCr_420_888 from which JPEG is produced.

This doesn seem to correspond to the code below. Should it read

		/*
		 * JPEG is always supported, either produced directly by the
		 * camera, or encoded in the HAL.
		 */

?

> +		 *
> +		 * \todo Wire the JPEG encoder interface to query the list
> +		 * of supported resolutions.

Is this still valid ?

> +		 */
> +		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 +426,22 @@ 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.

We shouldn't do this if the device can provide JPEG natively. Is this
something you'd like to address already ? Otherwise we should add a
\todo item for this too.

> +			 */
> +			if (androidFormat == HAL_PIXEL_FORMAT_YCbCr_420_888)
> +				streamConfigurations_.push_back(
> +					{ res, HAL_PIXEL_FORMAT_BLOB });
>  		}
>  	}
>

Hi,

On Sun, Sep 6, 2020 at 2:49 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Sep 02, 2020 at 05:22:26PM +0200, Jacopo Mondi wrote:
> > When producing the list of image resolution to claim as supported by the
>
> s/resolution/resolutions/
>
> > 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.
>
> Down the line we'll likely need to scale down images for JPEG
> compression, but for now this looks fine to me.
>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/android/camera_device.cpp | 43 +++++++++++++++++++++++------------
> >  1 file changed, 28 insertions(+), 15 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index ad0d7fd15d90..8a8072123961 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -363,17 +363,27 @@ int CameraDevice::initializeStreamConfigurations()
> >               const std::vector<PixelFormat> &libcameraFormats =
> >                       camera3Format.libcameraFormats;
> >
> > +             /*
> > +              * Fixed format mapping for JPEG.
> > +              *
> > +              * The list of supported JPEG resolutions is generated
> > +              * from the list of resolutions supported by
> > +              * HAL_PIXEL_FORMAT_YCbCr_420_888 from which JPEG is produced.
>
> This doesn seem to correspond to the code below. Should it read
>
>                 /*
>                  * JPEG is always supported, either produced directly by the
>                  * camera, or encoded in the HAL.
>                  */
>
> ?
>
> > +              *
> > +              * \todo Wire the JPEG encoder interface to query the list
> > +              * of supported resolutions.
>
> Is this still valid ?
>
> > +              */
> > +             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 +426,22 @@ 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 });

This comment is probably the same as Laurent's one.
The change looks to allow using native jpeg output streams.

> > +
> > +                     /*
> > +                      * 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.
>
> We shouldn't do this if the device can provide JPEG natively. Is this
> something you'd like to address already ? Otherwise we should add a
> \todo item for this too.
>
> > +                      */
> > +                     if (androidFormat == HAL_PIXEL_FORMAT_YCbCr_420_888)
> > +                             streamConfigurations_.push_back(
> > +                                     { res, HAL_PIXEL_FORMAT_BLOB });
> >               }
> >       }
> >
>
> --
> Regards,
>
> Laurent Pinchart

Best Regards,
-Hiro
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Laurent,

On Sat, Sep 05, 2020 at 08:49:01PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Sep 02, 2020 at 05:22:26PM +0200, Jacopo Mondi wrote:
> > When producing the list of image resolution to claim as supported by the
>
> s/resolution/resolutions/
>
> > 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.
>
> Down the line we'll likely need to scale down images for JPEG
> compression, but for now this looks fine to me.
>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/android/camera_device.cpp | 43 +++++++++++++++++++++++------------
> >  1 file changed, 28 insertions(+), 15 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index ad0d7fd15d90..8a8072123961 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -363,17 +363,27 @@ int CameraDevice::initializeStreamConfigurations()
> >  		const std::vector<PixelFormat> &libcameraFormats =
> >  			camera3Format.libcameraFormats;
> >
> > +		/*
> > +		 * Fixed format mapping for JPEG.
> > +		 *
> > +		 * The list of supported JPEG resolutions is generated
> > +		 * from the list of resolutions supported by
> > +		 * HAL_PIXEL_FORMAT_YCbCr_420_888 from which JPEG is produced.
>
> This doesn seem to correspond to the code below. Should it read

doesn't it ? JPEG is generated from YCbCr... but this comment is
actually a repetition of another one here below, so I guess I can
replace it.

>
> 		/*
> 		 * JPEG is always supported, either produced directly by the
> 		 * camera, or encoded in the HAL.
> 		 */
>
> ?
>
> > +		 *
> > +		 * \todo Wire the JPEG encoder interface to query the list
> > +		 * of supported resolutions.
>
> Is this still valid ?
>

I think so. As explained to Kieran in v1 I think all the
implementation of the Encoder interface should be able to return the
list of supported resolution to take into account things like, say,
alignment.

> > +		 */
> > +		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 +426,22 @@ 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.
>
> We shouldn't do this if the device can provide JPEG natively. Is this
> something you'd like to address already ? Otherwise we should add a
> \todo item for this too.

This isn't different from what we had before. I we want to support
JPEG natively produced from the camera, there's some work to do to
identify that at the beginning of this function.

A todo item is indeed appropriate.

Thanks
  j

>
> > +			 */
> > +			if (androidFormat == HAL_PIXEL_FORMAT_YCbCr_420_888)
> > +				streamConfigurations_.push_back(
> > +					{ res, HAL_PIXEL_FORMAT_BLOB });
> >  		}
> >  	}
> >
>
> --
> Regards,
>
> Laurent Pinchart

Hi Jacopo,

On Mon, Sep 07, 2020 at 10:29:52AM +0200, Jacopo Mondi wrote:
> On Sat, Sep 05, 2020 at 08:49:01PM +0300, Laurent Pinchart wrote:
> > On Wed, Sep 02, 2020 at 05:22:26PM +0200, Jacopo Mondi wrote:
> > > When producing the list of image resolution to claim as supported by the
> >
> > s/resolution/resolutions/
> >
> > > 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.
> >
> > Down the line we'll likely need to scale down images for JPEG
> > compression, but for now this looks fine to me.
> >
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  src/android/camera_device.cpp | 43 +++++++++++++++++++++++------------
> > >  1 file changed, 28 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index ad0d7fd15d90..8a8072123961 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -363,17 +363,27 @@ int CameraDevice::initializeStreamConfigurations()
> > >  		const std::vector<PixelFormat> &libcameraFormats =
> > >  			camera3Format.libcameraFormats;
> > >
> > > +		/*
> > > +		 * Fixed format mapping for JPEG.
> > > +		 *
> > > +		 * The list of supported JPEG resolutions is generated
> > > +		 * from the list of resolutions supported by
> > > +		 * HAL_PIXEL_FORMAT_YCbCr_420_888 from which JPEG is produced.
> >
> > This doesn seem to correspond to the code below. Should it read
> 
> doesn't it ? JPEG is generated from YCbCr... but this comment is
> actually a repetition of another one here below, so I guess I can
> replace it.

I'm not saying the comment is a lie :-) Just that it doesn't correspond
to the 4 lines directly after it.

> >
> > 		/*
> > 		 * JPEG is always supported, either produced directly by the
> > 		 * camera, or encoded in the HAL.
> > 		 */
> >
> > ?
> >
> > > +		 *
> > > +		 * \todo Wire the JPEG encoder interface to query the list
> > > +		 * of supported resolutions.
> >
> > Is this still valid ?
> 
> I think so. As explained to Kieran in v1 I think all the
> implementation of the Encoder interface should be able to return the
> list of supported resolution to take into account things like, say,
> alignment.
> 
> > > +		 */
> > > +		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 +426,22 @@ 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.
> >
> > We shouldn't do this if the device can provide JPEG natively. Is this
> > something you'd like to address already ? Otherwise we should add a
> > \todo item for this too.
> 
> This isn't different from what we had before. I we want to support
> JPEG natively produced from the camera, there's some work to do to
> identify that at the beginning of this function.

I'm not saying it's a regression :-) A todo is all that we need here I
think.

> A todo item is indeed appropriate.
> 
> > > +			 */
> > > +			if (androidFormat == HAL_PIXEL_FORMAT_YCbCr_420_888)
> > > +				streamConfigurations_.push_back(
> > > +					{ res, HAL_PIXEL_FORMAT_BLOB });
> > >  		}
> > >  	}
> > >