| Message ID | 20210727135440.53640-2-umang.jain@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, Thank you for the patch. On Tue, Jul 27, 2021 at 07:24:39PM +0530, Umang Jain wrote: > Currently, all UVC cameras are marked with CameraLocationExternal [1] > property, since there is no universal information or standard, to > know the location of these cameras. However, in the libcamera HAL layer, > we can make an informed decision whether its external or internal, > simply by checking the presence of it, in the HAL configuration file. > > If the camera is found to be present on the HAL configuration file, > treat it as internal and assign its numerical id accordingly. > > [1] 76809320bb1a ("libcamera: pipeline: uvcvideo: Treat all UVC cameras > as external") > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/android/camera_device.cpp | 10 +++++++++- > src/android/camera_hal_manager.cpp | 14 ++++++++++++++ > 2 files changed, 23 insertions(+), 1 deletion(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 678cde23..ba4e2d15 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -330,7 +330,15 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData) > facing_ = CAMERA_FACING_BACK; > break; > case properties::CameraLocationExternal: > - facing_ = CAMERA_FACING_EXTERNAL; > + /* > + * If the camera is 'internal' as found by > + * CameraHalManager, use its location from > + * HAL config file. > + */ > + if (id_ < 1000 && cameraConfigData) > + facing_ = cameraConfigData->facing; > + else > + facing_ = CAMERA_FACING_EXTERNAL; > break; > } > > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp > index 4cd67544..1a9b3413 100644 > --- a/src/android/camera_hal_manager.cpp > +++ b/src/android/camera_hal_manager.cpp > @@ -133,6 +133,20 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) > } > } > > + /* > + * In some cases, particularly in UVC cameras, the camera location is defaulted > + * to 'External'. However, if the HAL config file mentions the camera in question, > + * it shall mean that the camera is integrated to the system so, override to treat > + * it correctly as a 'internal' camera. Could you please wrap this at 80 columns ? I wouldn't say "defaulted", but instead talk about what the libcamera core does. Maybe something similar to the following. /* * Some cameras whose location is reported by libcamera as external may * actually be internal to the device. This is common with UVC cameras * that can be integrated in a laptop, but are all considered by * libcamera as external. The true location for those cameras is * specified in the HAL configuration file. If the camera location is * external and a configuration entry exists for it, override the * location. */ > + */ > + if (isCameraNew && isCameraExternal && halConfig_.exists()) { Is the isCameraNew check correct ? Even if the camera has been seen before, we should override isCameraExternal. The id, however, should only be overridden when the camera is new. > + const CameraConfigData *configData = halConfig_.cameraConfigData(cam->id()); It would be nice to avoid the double lookup of camera config data if possible. The CameraDevice::create() call could be moved further down, just before camera->initialize(), which may give us an opportunity to refactor the code. > + if (configData && configData->facing != CAMERA_FACING_EXTERNAL) { > + isCameraExternal = false; > + id = numInternalCameras_; > + } > + } > + > /* Create a CameraDevice instance to wrap the libcamera Camera. */ > std::unique_ptr<CameraDevice> camera = CameraDevice::create(id, cam); >
On 27/07/2021 16:27, Laurent Pinchart wrote: > Hi Umang, > > Thank you for the patch. > > On Tue, Jul 27, 2021 at 07:24:39PM +0530, Umang Jain wrote: >> Currently, all UVC cameras are marked with CameraLocationExternal [1] >> property, since there is no universal information or standard, to >> know the location of these cameras. However, in the libcamera HAL layer, >> we can make an informed decision whether its external or internal, >> simply by checking the presence of it, in the HAL configuration file. >> >> If the camera is found to be present on the HAL configuration file, >> treat it as internal and assign its numerical id accordingly. >> >> [1] 76809320bb1a ("libcamera: pipeline: uvcvideo: Treat all UVC cameras >> as external") >> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> --- >> src/android/camera_device.cpp | 10 +++++++++- >> src/android/camera_hal_manager.cpp | 14 ++++++++++++++ >> 2 files changed, 23 insertions(+), 1 deletion(-) >> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >> index 678cde23..ba4e2d15 100644 >> --- a/src/android/camera_device.cpp >> +++ b/src/android/camera_device.cpp >> @@ -330,7 +330,15 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData) >> facing_ = CAMERA_FACING_BACK; >> break; >> case properties::CameraLocationExternal: >> - facing_ = CAMERA_FACING_EXTERNAL; >> + /* >> + * If the camera is 'internal' as found by >> + * CameraHalManager, use its location from >> + * HAL config file. >> + */ >> + if (id_ < 1000 && cameraConfigData) Is this to 'prevent' setting it to cameraConfigData->facing in the event that id >= 1000? Will that work? won't it get set regardless in the lines after this case statement? : > if (cameraConfigData && cameraConfigData->facing != -1 && > facing_ != cameraConfigData->facing) { > LOG(HAL, Warning) > << "Camera location does not match" > << " configuration file. Using " << facing_; > } Also - I see that the cameraConfigData->facing is never used without first checking that it is not set to -1, which your lines do not check for ... I don't know if this is a critical requirement or not. >> + facing_ = cameraConfigData->facing; >> + else >> + facing_ = CAMERA_FACING_EXTERNAL; >> break; >> } >> >> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp >> index 4cd67544..1a9b3413 100644 >> --- a/src/android/camera_hal_manager.cpp >> +++ b/src/android/camera_hal_manager.cpp >> @@ -133,6 +133,20 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) >> } >> } >> >> + /* >> + * In some cases, particularly in UVC cameras, the camera location is defaulted >> + * to 'External'. However, if the HAL config file mentions the camera in question, >> + * it shall mean that the camera is integrated to the system so, override to treat >> + * it correctly as a 'internal' camera. > > Could you please wrap this at 80 columns ? > > I wouldn't say "defaulted", but instead talk about what the libcamera > core does. Maybe something similar to the following. > > /* > * Some cameras whose location is reported by libcamera as external may > * actually be internal to the device. This is common with UVC cameras > * that can be integrated in a laptop, but are all considered by > * libcamera as external. The true location for those cameras is > * specified in the HAL configuration file. If the camera location is > * external and a configuration entry exists for it, override the > * location. > */ > >> + */ >> + if (isCameraNew && isCameraExternal && halConfig_.exists()) { > > Is the isCameraNew check correct ? Even if the camera has been seen > before, we should override isCameraExternal. The id, however, should > only be overridden when the camera is new. > >> + const CameraConfigData *configData = halConfig_.cameraConfigData(cam->id()); > > It would be nice to avoid the double lookup of camera config data if > possible. The CameraDevice::create() call could be moved further down, > just before camera->initialize(), which may give us an opportunity to > refactor the code. > >> + if (configData && configData->facing != CAMERA_FACING_EXTERNAL) { >> + isCameraExternal = false; >> + id = numInternalCameras_; >> + } >> + } >> + >> /* Create a CameraDevice instance to wrap the libcamera Camera. */ >> std::unique_ptr<CameraDevice> camera = CameraDevice::create(id, cam); >> >
Hi Laurent, On 7/27/21 8:57 PM, Laurent Pinchart wrote: > Hi Umang, > > Thank you for the patch. > > On Tue, Jul 27, 2021 at 07:24:39PM +0530, Umang Jain wrote: >> Currently, all UVC cameras are marked with CameraLocationExternal [1] >> property, since there is no universal information or standard, to >> know the location of these cameras. However, in the libcamera HAL layer, >> we can make an informed decision whether its external or internal, >> simply by checking the presence of it, in the HAL configuration file. >> >> If the camera is found to be present on the HAL configuration file, >> treat it as internal and assign its numerical id accordingly. >> >> [1] 76809320bb1a ("libcamera: pipeline: uvcvideo: Treat all UVC cameras >> as external") >> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> --- >> src/android/camera_device.cpp | 10 +++++++++- >> src/android/camera_hal_manager.cpp | 14 ++++++++++++++ >> 2 files changed, 23 insertions(+), 1 deletion(-) >> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >> index 678cde23..ba4e2d15 100644 >> --- a/src/android/camera_device.cpp >> +++ b/src/android/camera_device.cpp >> @@ -330,7 +330,15 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData) >> facing_ = CAMERA_FACING_BACK; >> break; >> case properties::CameraLocationExternal: >> - facing_ = CAMERA_FACING_EXTERNAL; >> + /* >> + * If the camera is 'internal' as found by >> + * CameraHalManager, use its location from >> + * HAL config file. >> + */ >> + if (id_ < 1000 && cameraConfigData) >> + facing_ = cameraConfigData->facing; >> + else >> + facing_ = CAMERA_FACING_EXTERNAL; >> break; >> } >> >> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp >> index 4cd67544..1a9b3413 100644 >> --- a/src/android/camera_hal_manager.cpp >> +++ b/src/android/camera_hal_manager.cpp >> @@ -133,6 +133,20 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) >> } >> } >> >> + /* >> + * In some cases, particularly in UVC cameras, the camera location is defaulted >> + * to 'External'. However, if the HAL config file mentions the camera in question, >> + * it shall mean that the camera is integrated to the system so, override to treat >> + * it correctly as a 'internal' camera. > Could you please wrap this at 80 columns ? oops! > > I wouldn't say "defaulted", but instead talk about what the libcamera > core does. Maybe something similar to the following. > > /* > * Some cameras whose location is reported by libcamera as external may > * actually be internal to the device. This is common with UVC cameras > * that can be integrated in a laptop, but are all considered by > * libcamera as external. The true location for those cameras is > * specified in the HAL configuration file. If the camera location is > * external and a configuration entry exists for it, override the > * location. > */ > >> + */ >> + if (isCameraNew && isCameraExternal && halConfig_.exists()) { > Is the isCameraNew check correct ? Even if the camera has been seen > before, we should override isCameraExternal. The id, however, should > only be overridden when the camera is new. I didn't understand the part ... "if camera has been seen before, we should override isCameraExternal" part. I think this check holds. If the internal UVC camera is seen before, it will be re-assigned same numeric-id (which, as per this patch, will be < 1000). Hence, I do not think we need to over-ride anything here, because CameraHalManager already knows that it is an internal camera (based on previous-id re-assign at the start of the function). Am I missing something? > >> + const CameraConfigData *configData = halConfig_.cameraConfigData(cam->id()); > It would be nice to avoid the double lookup of camera config data if > possible. The CameraDevice::create() call could be moved further down, > just before camera->initialize(), which may give us an opportunity to > refactor the code. Yeah makes sense, I'll see how can I refactor a bit to avoid double-lookup. > >> + if (configData && configData->facing != CAMERA_FACING_EXTERNAL) { >> + isCameraExternal = false; >> + id = numInternalCameras_; >> + } >> + } >> + >> /* Create a CameraDevice instance to wrap the libcamera Camera. */ >> std::unique_ptr<CameraDevice> camera = CameraDevice::create(id, cam); >>
Hi Kieran? On 7/27/21 9:09 PM, Kieran Bingham wrote: > On 27/07/2021 16:27, Laurent Pinchart wrote: >> Hi Umang, >> >> Thank you for the patch. >> >> On Tue, Jul 27, 2021 at 07:24:39PM +0530, Umang Jain wrote: >>> Currently, all UVC cameras are marked with CameraLocationExternal [1] >>> property, since there is no universal information or standard, to >>> know the location of these cameras. However, in the libcamera HAL layer, >>> we can make an informed decision whether its external or internal, >>> simply by checking the presence of it, in the HAL configuration file. >>> >>> If the camera is found to be present on the HAL configuration file, >>> treat it as internal and assign its numerical id accordingly. >>> >>> [1] 76809320bb1a ("libcamera: pipeline: uvcvideo: Treat all UVC cameras >>> as external") >>> >>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >>> --- >>> src/android/camera_device.cpp | 10 +++++++++- >>> src/android/camera_hal_manager.cpp | 14 ++++++++++++++ >>> 2 files changed, 23 insertions(+), 1 deletion(-) >>> >>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >>> index 678cde23..ba4e2d15 100644 >>> --- a/src/android/camera_device.cpp >>> +++ b/src/android/camera_device.cpp >>> @@ -330,7 +330,15 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData) >>> facing_ = CAMERA_FACING_BACK; >>> break; >>> case properties::CameraLocationExternal: >>> - facing_ = CAMERA_FACING_EXTERNAL; >>> + /* >>> + * If the camera is 'internal' as found by >>> + * CameraHalManager, use its location from >>> + * HAL config file. >>> + */ >>> + if (id_ < 1000 && cameraConfigData) > Is this to 'prevent' setting it to cameraConfigData->facing in the event > that id >= 1000? Not sure, if I understand it correctly, but this is the check for the situation where libcamera-core reports camera's properties::Location as CameraLocationExternal. See 76809320bb1a. So, before setting the facing_, we just check whether the CameraHalManager has classified the camera as internal(id < 1000) or external(remove-able, id >= 1000) to the system. If it's internal, we grab/set the facing_ from the hal config file. Does it make sense? > > Will that work? won't it get set regardless in the lines after this case > statement? : which lines? The switch-case block is inside a if block, followed by else if / else. > >> if (cameraConfigData && cameraConfigData->facing != -1 && >> facing_ != cameraConfigData->facing) { >> LOG(HAL, Warning) >> << "Camera location does not match" >> << " configuration file. Using " << facing_; >> } > > Also - I see that the cameraConfigData->facing is never used without > first checking that it is not set to -1, which your lines do not check > for ... I don't know if this is a critical requirement or not. > > > > >>> + facing_ = cameraConfigData->facing; >>> + else >>> + facing_ = CAMERA_FACING_EXTERNAL; >>> break; >>> } >>> >>> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp >>> index 4cd67544..1a9b3413 100644 >>> --- a/src/android/camera_hal_manager.cpp >>> +++ b/src/android/camera_hal_manager.cpp >>> @@ -133,6 +133,20 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) >>> } >>> } >>> >>> + /* >>> + * In some cases, particularly in UVC cameras, the camera location is defaulted >>> + * to 'External'. However, if the HAL config file mentions the camera in question, >>> + * it shall mean that the camera is integrated to the system so, override to treat >>> + * it correctly as a 'internal' camera. >> Could you please wrap this at 80 columns ? >> >> I wouldn't say "defaulted", but instead talk about what the libcamera >> core does. Maybe something similar to the following. >> >> /* >> * Some cameras whose location is reported by libcamera as external may >> * actually be internal to the device. This is common with UVC cameras >> * that can be integrated in a laptop, but are all considered by >> * libcamera as external. The true location for those cameras is >> * specified in the HAL configuration file. If the camera location is >> * external and a configuration entry exists for it, override the >> * location. >> */ >> >>> + */ >>> + if (isCameraNew && isCameraExternal && halConfig_.exists()) { >> Is the isCameraNew check correct ? Even if the camera has been seen >> before, we should override isCameraExternal. The id, however, should >> only be overridden when the camera is new. >> >>> + const CameraConfigData *configData = halConfig_.cameraConfigData(cam->id()); >> It would be nice to avoid the double lookup of camera config data if >> possible. The CameraDevice::create() call could be moved further down, >> just before camera->initialize(), which may give us an opportunity to >> refactor the code. >> >>> + if (configData && configData->facing != CAMERA_FACING_EXTERNAL) { >>> + isCameraExternal = false; >>> + id = numInternalCameras_; >>> + } >>> + } >>> + >>> /* Create a CameraDevice instance to wrap the libcamera Camera. */ >>> std::unique_ptr<CameraDevice> camera = CameraDevice::create(id, cam); >>>
On Tue, Jul 27, 2021 at 11:18:58PM +0530, Umang Jain wrote: > On 7/27/21 8:57 PM, Laurent Pinchart wrote: > > On Tue, Jul 27, 2021 at 07:24:39PM +0530, Umang Jain wrote: > >> Currently, all UVC cameras are marked with CameraLocationExternal [1] > >> property, since there is no universal information or standard, to > >> know the location of these cameras. However, in the libcamera HAL layer, > >> we can make an informed decision whether its external or internal, > >> simply by checking the presence of it, in the HAL configuration file. > >> > >> If the camera is found to be present on the HAL configuration file, > >> treat it as internal and assign its numerical id accordingly. > >> > >> [1] 76809320bb1a ("libcamera: pipeline: uvcvideo: Treat all UVC cameras > >> as external") > >> > >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > >> --- > >> src/android/camera_device.cpp | 10 +++++++++- > >> src/android/camera_hal_manager.cpp | 14 ++++++++++++++ > >> 2 files changed, 23 insertions(+), 1 deletion(-) > >> > >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > >> index 678cde23..ba4e2d15 100644 > >> --- a/src/android/camera_device.cpp > >> +++ b/src/android/camera_device.cpp > >> @@ -330,7 +330,15 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData) > >> facing_ = CAMERA_FACING_BACK; > >> break; > >> case properties::CameraLocationExternal: > >> - facing_ = CAMERA_FACING_EXTERNAL; > >> + /* > >> + * If the camera is 'internal' as found by > >> + * CameraHalManager, use its location from > >> + * HAL config file. > >> + */ > >> + if (id_ < 1000 && cameraConfigData) > >> + facing_ = cameraConfigData->facing; > >> + else > >> + facing_ = CAMERA_FACING_EXTERNAL; > >> break; > >> } > >> > >> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp > >> index 4cd67544..1a9b3413 100644 > >> --- a/src/android/camera_hal_manager.cpp > >> +++ b/src/android/camera_hal_manager.cpp > >> @@ -133,6 +133,20 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) > >> } > >> } > >> > >> + /* > >> + * In some cases, particularly in UVC cameras, the camera location is defaulted > >> + * to 'External'. However, if the HAL config file mentions the camera in question, > >> + * it shall mean that the camera is integrated to the system so, override to treat > >> + * it correctly as a 'internal' camera. > > > > Could you please wrap this at 80 columns ? > > oops! > > > I wouldn't say "defaulted", but instead talk about what the libcamera > > core does. Maybe something similar to the following. > > > > /* > > * Some cameras whose location is reported by libcamera as external may > > * actually be internal to the device. This is common with UVC cameras > > * that can be integrated in a laptop, but are all considered by > > * libcamera as external. The true location for those cameras is > > * specified in the HAL configuration file. If the camera location is > > * external and a configuration entry exists for it, override the > > * location. > > */ > > > >> + */ > >> + if (isCameraNew && isCameraExternal && halConfig_.exists()) { > > > > Is the isCameraNew check correct ? Even if the camera has been seen > > before, we should override isCameraExternal. The id, however, should > > only be overridden when the camera is new. > > I didn't understand the part ... "if camera has been seen before, we > should override isCameraExternal" part. > > I think this check holds. If the internal UVC camera is seen before, it > will be re-assigned same numeric-id (which, as per this patch, will be < > 1000). Hence, I do not think we need to over-ride anything here, because > CameraHalManager already knows that it is an internal camera (based on > previous-id re-assign at the start of the function). Am I missing > something? You're right, isCameraExternal is only used when isCameraNew == true. > >> + const CameraConfigData *configData = halConfig_.cameraConfigData(cam->id()); > > > > It would be nice to avoid the double lookup of camera config data if > > possible. The CameraDevice::create() call could be moved further down, > > just before camera->initialize(), which may give us an opportunity to > > refactor the code. > > Yeah makes sense, I'll see how can I refactor a bit to avoid double-lookup. > > >> + if (configData && configData->facing != CAMERA_FACING_EXTERNAL) { > >> + isCameraExternal = false; > >> + id = numInternalCameras_; > >> + } > >> + } > >> + > >> /* Create a CameraDevice instance to wrap the libcamera Camera. */ > >> std::unique_ptr<CameraDevice> camera = CameraDevice::create(id, cam); > >>
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 678cde23..ba4e2d15 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -330,7 +330,15 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData) facing_ = CAMERA_FACING_BACK; break; case properties::CameraLocationExternal: - facing_ = CAMERA_FACING_EXTERNAL; + /* + * If the camera is 'internal' as found by + * CameraHalManager, use its location from + * HAL config file. + */ + if (id_ < 1000 && cameraConfigData) + facing_ = cameraConfigData->facing; + else + facing_ = CAMERA_FACING_EXTERNAL; break; } diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp index 4cd67544..1a9b3413 100644 --- a/src/android/camera_hal_manager.cpp +++ b/src/android/camera_hal_manager.cpp @@ -133,6 +133,20 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) } } + /* + * In some cases, particularly in UVC cameras, the camera location is defaulted + * to 'External'. However, if the HAL config file mentions the camera in question, + * it shall mean that the camera is integrated to the system so, override to treat + * it correctly as a 'internal' camera. + */ + if (isCameraNew && isCameraExternal && halConfig_.exists()) { + const CameraConfigData *configData = halConfig_.cameraConfigData(cam->id()); + if (configData && configData->facing != CAMERA_FACING_EXTERNAL) { + isCameraExternal = false; + id = numInternalCameras_; + } + } + /* Create a CameraDevice instance to wrap the libcamera Camera. */ std::unique_ptr<CameraDevice> camera = CameraDevice::create(id, cam);
Currently, all UVC cameras are marked with CameraLocationExternal [1] property, since there is no universal information or standard, to know the location of these cameras. However, in the libcamera HAL layer, we can make an informed decision whether its external or internal, simply by checking the presence of it, in the HAL configuration file. If the camera is found to be present on the HAL configuration file, treat it as internal and assign its numerical id accordingly. [1] 76809320bb1a ("libcamera: pipeline: uvcvideo: Treat all UVC cameras as external") Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- src/android/camera_device.cpp | 10 +++++++++- src/android/camera_hal_manager.cpp | 14 ++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-)