[libcamera-devel] [PATCH 3/4] android: camera_device: Provide a toPixelFormat helper
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Jun 30 16:06:52 CEST 2020
Hi Laurent,
On 29/06/2020 21:20, Laurent Pinchart wrote:
> Hi Kieran,
>
> Thank you for the patch.
>
> On Mon, Jun 29, 2020 at 05:39:15PM +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>
>> ---
>> src/android/camera_device.cpp | 29 +++++++++++++++++++++--------
>> src/android/camera_device.h | 1 +
>> 2 files changed, 22 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>> index f788c11e7254..a6410f42fee8 100644
>> --- a/src/android/camera_device.cpp
>> +++ b/src/android/camera_device.cpp
>> @@ -923,6 +923,20 @@ 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";
>
> You could reduce the number of lines with
>
> LOG(HAL, Error)
> << "Requested format " << utils::hex(format)
> << " not supported";
>
Indeed, that's what I get for copy-paste moving that code.
I'll reformat.
>
>> + return PixelFormat();
>> + }
>> +
>> + return it->second;
>> +}
>> +
>> /*
>> * Inspect the stream_list to produce a list of StreamConfiguration to
>> * be use to configure the Camera.
>> @@ -932,11 +946,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() << ")";
>
> I wonder if it's really needed here, but I suppose it doesn't hurt
> either.
This was what caused me to make this patch in the first place ;-)
Personally I think this is indeed far more helpful, as it goes from:
> [0:01:14.456109430] [3466] INFO HAL camera_device.cpp:956 'ov13858 8-0010': Stream #0, direction: 0, width: 320, height: 240, format: 0x00000022
to
> [0:01:14.456109430] [3466] INFO HAL camera_device.cpp:956 'ov13858 8-0010': Stream #0, direction: 0, width: 320, height: 240, format: 0x00000022 (NV12)
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
Thanks...
>
>> }
>>
>> /* Only one stream is supported. */
>> @@ -947,13 +964,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 +982,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);
>>
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list