[libcamera-devel] [PATCH v2 2/9] android: camera_device: Provide a toPixelFormat helper

[Jacopo Mondi]

Hi Kieran

On Fri, Jul 03, 2020 at 10:19:58AM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
>
> On 03/07/2020 10:04, Jacopo Mondi wrote:
> > Sorry, one nit
> >
> > On Thu, Jul 02, 2020 at 10:36:47PM +0100, Kieran Bingham wrote:
> >> Rather than converting pixelformats through the map, and then
> >> dereferencing the iterator later, create a helper to explicitly return a
> >> PixelFormat type.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >> ---
> >>  src/android/camera_device.cpp | 28 ++++++++++++++++++++--------
> >>  src/android/camera_device.h   |  1 +
> >>  2 files changed, 21 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> >> index f788c11e7254..b9031ff0c2a4 100644
> >> --- a/src/android/camera_device.cpp
> >> +++ b/src/android/camera_device.cpp
> >> @@ -923,6 +923,19 @@ const camera_metadata_t *CameraDevice::constructDefaultRequestSettings(int type)
> >>  	return requestTemplate->get();
> >>  }
> >>
> >> +PixelFormat CameraDevice::toPixelFormat(int format)
> >> +{
> >> +	/* Translate Android format code to libcamera pixel format. */
> >> +	auto it = formatsMap_.find(format);
> >> +	if (it == formatsMap_.end()) {
> >> +		LOG(HAL, Error) << "Requested format " << utils::hex(format)
> >> +				<< " not supported";
> >> +		return PixelFormat();
> >> +	}
> >> +
> >> +	return it->second;
> >> +}
> >> +
> >>  /*
> >>   * Inspect the stream_list to produce a list of StreamConfiguration to
> >>   * be use to configure the Camera.
> >> @@ -932,11 +945,14 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >>  	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
> >>  		camera3_stream_t *stream = stream_list->streams[i];
> >>
> >> +		PixelFormat format = toPixelFormat(stream->format);
> >> +
> >>  		LOG(HAL, Info) << "Stream #" << i
> >>  			       << ", direction: " << stream->stream_type
> >>  			       << ", width: " << stream->width
> >>  			       << ", height: " << stream->height
> >> -			       << ", format: " << utils::hex(stream->format);
> >> +			       << ", format: " << utils::hex(stream->format)
> >> +			       << " (" << format.toString() << ")";
> >
> > Is it a good idea to report the mapped format when priting information
> > about the android requested streams ? Just wondering...
> >
>
> It certainly helps to know what the stream configurations are:
>
> Without:
>
> > <line-prefix-snip> Stream #0, direction: 0, width: 2560, height: 1920, format: 0x00000023
> > <line-prefix-snip> Stream #1, direction: 0, width: 2560, height: 1920, format: 0x00000021
>
> With:
>
> > <line-prefix-snip> Stream #0, direction: 0, width: 2560, height: 1920, format: 0x00000023 (NV12)
> > <line-prefix-snip> Stream #1, direction: 0, width: 2560, height: 1920, format: 0x00000021 (MJPEG)
>
>
> Otherwise, we can add a toString() on the android formats of course,
>
> But it also really helps in the cros-camera-tests too to see when a
> format is requested that we don't yet handle:
>
> > <line-prefix-snip> Stream #0, direction: 0, width: 320, height: 240, format: 0x20203859 (<INVALID>)
>

All good, it was an honest question as the mapping is something we
implement, not something provided by the framework, and initially the
printout was meant to indentify what we receive

> --
> Kieran
>
>
>
> >>  	}
> >>
> >>  	/* Only one stream is supported. */
> >> @@ -947,13 +963,9 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >>  	camera3_stream_t *camera3Stream = stream_list->streams[0];
> >>
> >>  	/* Translate Android format code to libcamera pixel format. */
> >> -	auto it = formatsMap_.find(camera3Stream->format);
> >> -	if (it == formatsMap_.end()) {
> >> -		LOG(HAL, Error) << "Requested format "
> >> -				<< utils::hex(camera3Stream->format)
> >> -				<< " not supported";
> >> +	PixelFormat format = toPixelFormat(camera3Stream->format);
> >> +	if (!format.isValid())
> >>  		return -EINVAL;
> >> -	}
> >>
> >>  	/*
> >>  	 * Hardcode viewfinder role, replacing the generated configuration
> >> @@ -969,7 +981,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >>  	StreamConfiguration *streamConfiguration = &config_->at(0);
> >>  	streamConfiguration->size.width = camera3Stream->width;
> >>  	streamConfiguration->size.height = camera3Stream->height;
> >> -	streamConfiguration->pixelFormat = it->second;
> >> +	streamConfiguration->pixelFormat = format;
> >>
> >>  	switch (config_->validate()) {
> >>  	case CameraConfiguration::Valid:
> >> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> >> index ed11410a5577..5bd6cf416156 100644
> >> --- a/src/android/camera_device.h
> >> +++ b/src/android/camera_device.h
> >> @@ -72,6 +72,7 @@ private:
> >>  	std::tuple<uint32_t, uint32_t> calculateStaticMetadataSize();
> >>  	void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
> >>  	void notifyError(uint32_t frameNumber, camera3_stream_t *stream);
> >> +	libcamera::PixelFormat toPixelFormat(int format);
> >>  	std::unique_ptr<CameraMetadata> getResultMetadata(int frame_number,
> >>  							  int64_t timestamp);
> >>
> >> --
> >> 2.25.1
> >>
> >> _______________________________________________
> >> libcamera-devel mailing list
> >> libcamera-devel at lists.libcamera.org
> >> https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards
> --
> Kieran