Message ID | 20200629163916.1815321-4-kieran.bingham@ideasonboard.com |
---|---|
State | Accepted |
Headers | show |
Series |
|
Related | show |
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@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"; > + 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. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > } > > /* 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); >
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@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@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); >> >
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"; + 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() << ")"; } /* 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);
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@ideasonboard.com> --- src/android/camera_device.cpp | 29 +++++++++++++++++++++-------- src/android/camera_device.h | 1 + 2 files changed, 22 insertions(+), 8 deletions(-)