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

[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 at ideasonboard.com>
> > > Signed-off-by: Jacopo Mondi <jacopo at 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 });
> > >  		}
> > >  	}
> > >

-- 
Regards,

Laurent Pinchart