| Message ID | 20210730110154.181370-4-umang.jain@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Umang Jain |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, On Fri, Jul 30, 2021 at 04:31:53PM +0530, Umang Jain wrote: > Currently, all UVC cameras are reported with CameraLocationExternal [1] > by libcamera-core, since there is no universal information or standard, s/,$// > to know the location of these cameras. However, in the libcamera HAL > layer, we can make an informed decision whether its external or s/its/it's/ > internal, simply by checking the presence of it in the HAL s/the presence of it/its presence/ > configuration file. > > If the camera is found to be present in the HAL configuration file, s/to be present// > treat it as internal. CameraHalManager will now assign the numerical id > of the camera accordingly, based on which the facing of the camera is set I didn't think the facing of the camera affects the id? > as well (as per the HAL config file) while initializing the CameraDevice > wrapper. I don't quite understand this last part. Thanks, Paul > > [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 | 13 ++++++++++++- > src/android/camera_hal_manager.cpp | 17 +++++++++++++++++ > 2 files changed, 29 insertions(+), 1 deletion(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 678cde23..394ebc84 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -330,7 +330,18 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData) > facing_ = CAMERA_FACING_BACK; > break; > case properties::CameraLocationExternal: > - facing_ = CAMERA_FACING_EXTERNAL; > + /* > + * If the camera is reported as external, but > + * the CameraHalManager has overriden it, use > + * what is reported in the configuration file. > + * This typically happens for UVC cameras > + * reported as 'External' by libcamera but > + * installed in fixed position on the device. > + */ > + if (id_ < 1000 && cameraConfigData->facing != -1) > + 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 b364f62a..cce98fad 100644 > --- a/src/android/camera_hal_manager.cpp > +++ b/src/android/camera_hal_manager.cpp > @@ -145,6 +145,23 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) > } > > const CameraConfigData *cameraConfigData = halConfig_.cameraConfigData(cam->id()); > + > + /* > + * Some cameras whose location is reported by libcamera as external may > + * actually be internal to the device. This is common with UVC cameras > + * that are integrated in a laptop. In that case the real location > + * should be specified in the configuration file. > + * > + * If the camera location is external and a configuration > + * entry exists for it, override its location. > + */ > + if (isCameraNew && isCameraExternal) { > + if (cameraConfigData && cameraConfigData->facing != -1) { > + isCameraExternal = false; > + id = numInternalCameras_; > + } > + } > + > if (!isCameraExternal && !cameraConfigData) { > LOG(HAL, Error) > << "HAL configuration entry for internal camera " > -- > 2.31.0 >
Hi Paul, On 8/2/21 2:16 PM, paul.elder@ideasonboard.com wrote: > Hi Umang, > > On Fri, Jul 30, 2021 at 04:31:53PM +0530, Umang Jain wrote: >> Currently, all UVC cameras are reported with CameraLocationExternal [1] >> by libcamera-core, since there is no universal information or standard, > s/,$// > >> to know the location of these cameras. However, in the libcamera HAL >> layer, we can make an informed decision whether its external or > s/its/it's/ > >> internal, simply by checking the presence of it in the HAL > s/the presence of it/its presence/ > >> configuration file. >> >> If the camera is found to be present in the HAL configuration file, > s/to be present// > >> treat it as internal. CameraHalManager will now assign the numerical id >> of the camera accordingly, based on which the facing of the camera is set > I didn't think the facing of the camera affects the id? Well, it should be kept in sync. If the camera is internal (id < 1000), but it's facing is set to be external (as per checks in CameraDevice), then these are not in sync. We need to take care of that. I don't think anything bad will happen if these are out of sync. But why should we allow it to be out-of-sync if we handle it gracefully? > >> as well (as per the HAL config file) while initializing the CameraDevice >> wrapper. > I don't quite understand this last part. So in the CameraDevice::initialize(), the facing of the camera is set. If properties::Location doesn't match the location in the HAL config, CameraDevice complains a warning. But, we know that for UVC properties:::Location won't be matching the HAL config. In that case we rely on id assigned by CameraManager to see if its a internal camera or not. If yes, we just assign facing whatever is mentioned in the HAL config file :) > > > Thanks, > > Paul > >> [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 | 13 ++++++++++++- >> src/android/camera_hal_manager.cpp | 17 +++++++++++++++++ >> 2 files changed, 29 insertions(+), 1 deletion(-) >> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >> index 678cde23..394ebc84 100644 >> --- a/src/android/camera_device.cpp >> +++ b/src/android/camera_device.cpp >> @@ -330,7 +330,18 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData) >> facing_ = CAMERA_FACING_BACK; >> break; >> case properties::CameraLocationExternal: >> - facing_ = CAMERA_FACING_EXTERNAL; >> + /* >> + * If the camera is reported as external, but >> + * the CameraHalManager has overriden it, use >> + * what is reported in the configuration file. >> + * This typically happens for UVC cameras >> + * reported as 'External' by libcamera but >> + * installed in fixed position on the device. >> + */ >> + if (id_ < 1000 && cameraConfigData->facing != -1) >> + 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 b364f62a..cce98fad 100644 >> --- a/src/android/camera_hal_manager.cpp >> +++ b/src/android/camera_hal_manager.cpp >> @@ -145,6 +145,23 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) >> } >> >> const CameraConfigData *cameraConfigData = halConfig_.cameraConfigData(cam->id()); >> + >> + /* >> + * Some cameras whose location is reported by libcamera as external may >> + * actually be internal to the device. This is common with UVC cameras >> + * that are integrated in a laptop. In that case the real location >> + * should be specified in the configuration file. >> + * >> + * If the camera location is external and a configuration >> + * entry exists for it, override its location. >> + */ >> + if (isCameraNew && isCameraExternal) { >> + if (cameraConfigData && cameraConfigData->facing != -1) { >> + isCameraExternal = false; >> + id = numInternalCameras_; >> + } >> + } >> + >> if (!isCameraExternal && !cameraConfigData) { >> LOG(HAL, Error) >> << "HAL configuration entry for internal camera " >> -- >> 2.31.0 >>
Hi Umang, On Fri, Jul 30, 2021 at 04:31:53PM +0530, Umang Jain wrote: > Currently, all UVC cameras are reported with CameraLocationExternal [1] > by libcamera-core, 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 in the HAL configuration file, > treat it as internal. CameraHalManager will now assign the numerical id > of the camera accordingly, based on which the facing of the camera is set > as well (as per the HAL config file) while initializing the CameraDevice > wrapper. > > [1] 76809320bb1a ("libcamera: pipeline: uvcvideo: Treat all UVC cameras > as external") > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > --- > src/android/camera_device.cpp | 13 ++++++++++++- > src/android/camera_hal_manager.cpp | 17 +++++++++++++++++ > 2 files changed, 29 insertions(+), 1 deletion(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 678cde23..394ebc84 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -330,7 +330,18 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData) > facing_ = CAMERA_FACING_BACK; > break; > case properties::CameraLocationExternal: > - facing_ = CAMERA_FACING_EXTERNAL; > + /* > + * If the camera is reported as external, but > + * the CameraHalManager has overriden it, use > + * what is reported in the configuration file. > + * This typically happens for UVC cameras > + * reported as 'External' by libcamera but > + * installed in fixed position on the device. > + */ > + if (id_ < 1000 && cameraConfigData->facing != -1) > + 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 b364f62a..cce98fad 100644 > --- a/src/android/camera_hal_manager.cpp > +++ b/src/android/camera_hal_manager.cpp > @@ -145,6 +145,23 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) > } > > const CameraConfigData *cameraConfigData = halConfig_.cameraConfigData(cam->id()); > + > + /* > + * Some cameras whose location is reported by libcamera as external may > + * actually be internal to the device. This is common with UVC cameras > + * that are integrated in a laptop. In that case the real location > + * should be specified in the configuration file. > + * > + * If the camera location is external and a configuration > + * entry exists for it, override its location. > + */ > + if (isCameraNew && isCameraExternal) { > + if (cameraConfigData && cameraConfigData->facing != -1) { > + isCameraExternal = false; > + id = numInternalCameras_; > + } > + } > + > if (!isCameraExternal && !cameraConfigData) { > LOG(HAL, Error) > << "HAL configuration entry for internal camera " > -- > 2.31.0 >
Hi Umang, Thank you for the patch. On Mon, Aug 02, 2021 at 03:21:14PM +0530, Umang Jain wrote: > On 8/2/21 2:16 PM, paul.elder@ideasonboard.com wrote: > > On Fri, Jul 30, 2021 at 04:31:53PM +0530, Umang Jain wrote: > >> Currently, all UVC cameras are reported with CameraLocationExternal [1] > >> by libcamera-core, since there is no universal information or standard, > > > > s/,$// > > > >> to know the location of these cameras. However, in the libcamera HAL > >> layer, we can make an informed decision whether its external or > > > > s/its/it's/ > > > >> internal, simply by checking the presence of it in the HAL > > > > s/the presence of it/its presence/ > > > >> configuration file. > >> > >> If the camera is found to be present in the HAL configuration file, > > > > s/to be present// > > > >> treat it as internal. CameraHalManager will now assign the numerical id > >> of the camera accordingly, based on which the facing of the camera is set > > > > I didn't think the facing of the camera affects the id? > > Well, it should be kept in sync. If the camera is internal (id < 1000), > but it's facing is set to be external (as per checks in CameraDevice), > then these are not in sync. We need to take care of that. > > I don't think anything bad will happen if these are out of sync. But why > should we allow it to be out-of-sync if we handle it gracefully? I agree, it's best to keep them in sync. > >> as well (as per the HAL config file) while initializing the CameraDevice > >> wrapper. > > > > I don't quite understand this last part. > > So in the CameraDevice::initialize(), the facing of the camera is set. > If properties::Location doesn't match the location in the HAL config, > CameraDevice complains a warning. > > But, we know that for UVC properties:::Location won't be matching the > HAL config. In that case we rely on id assigned by CameraManager to see > if its a internal camera or not. If yes, we just assign facing whatever > is mentioned in the HAL config file :) The last sentence of the commit message can probably be clarified a bit. How about this ? "The CameraHalManager will now assign the numerical id of the camera accordingly when initializing the CameraDevice, based on the camera facing value set in the HAL config file." > >> [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 | 13 ++++++++++++- > >> src/android/camera_hal_manager.cpp | 17 +++++++++++++++++ > >> 2 files changed, 29 insertions(+), 1 deletion(-) > >> > >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > >> index 678cde23..394ebc84 100644 > >> --- a/src/android/camera_device.cpp > >> +++ b/src/android/camera_device.cpp > >> @@ -330,7 +330,18 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData) > >> facing_ = CAMERA_FACING_BACK; > >> break; > >> case properties::CameraLocationExternal: > >> - facing_ = CAMERA_FACING_EXTERNAL; > >> + /* > >> + * If the camera is reported as external, but > >> + * the CameraHalManager has overriden it, use > >> + * what is reported in the configuration file. > >> + * This typically happens for UVC cameras > >> + * reported as 'External' by libcamera but > >> + * installed in fixed position on the device. > >> + */ This could be reflowed to 80 columns. > >> + if (id_ < 1000 && cameraConfigData->facing != -1) Is the id_ check needed ? If libcamera reports the camera as external and the camera data overrides the facing value, the id will always be < 1000, won't it ? > >> + 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 b364f62a..cce98fad 100644 > >> --- a/src/android/camera_hal_manager.cpp > >> +++ b/src/android/camera_hal_manager.cpp > >> @@ -145,6 +145,23 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) > >> } > >> > >> const CameraConfigData *cameraConfigData = halConfig_.cameraConfigData(cam->id()); > >> + > >> + /* > >> + * Some cameras whose location is reported by libcamera as external may > >> + * actually be internal to the device. This is common with UVC cameras > >> + * that are integrated in a laptop. In that case the real location > >> + * should be specified in the configuration file. > >> + * > >> + * If the camera location is external and a configuration > >> + * entry exists for it, override its location. You could also reflow this to 80 columns. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> + */ > >> + if (isCameraNew && isCameraExternal) { > >> + if (cameraConfigData && cameraConfigData->facing != -1) { > >> + isCameraExternal = false; > >> + id = numInternalCameras_; > >> + } > >> + } > >> + > >> if (!isCameraExternal && !cameraConfigData) { > >> LOG(HAL, Error) > >> << "HAL configuration entry for internal camera "
Hi Laurent, On 8/2/21 10:54 PM, Laurent Pinchart wrote: > Hi Umang, > > Thank you for the patch. > > On Mon, Aug 02, 2021 at 03:21:14PM +0530, Umang Jain wrote: >> On 8/2/21 2:16 PM, paul.elder@ideasonboard.com wrote: >>> On Fri, Jul 30, 2021 at 04:31:53PM +0530, Umang Jain wrote: >>>> Currently, all UVC cameras are reported with CameraLocationExternal [1] >>>> by libcamera-core, since there is no universal information or standard, >>> s/,$// >>> >>>> to know the location of these cameras. However, in the libcamera HAL >>>> layer, we can make an informed decision whether its external or >>> s/its/it's/ >>> >>>> internal, simply by checking the presence of it in the HAL >>> s/the presence of it/its presence/ >>> >>>> configuration file. >>>> >>>> If the camera is found to be present in the HAL configuration file, >>> s/to be present// >>> >>>> treat it as internal. CameraHalManager will now assign the numerical id >>>> of the camera accordingly, based on which the facing of the camera is set >>> I didn't think the facing of the camera affects the id? >> Well, it should be kept in sync. If the camera is internal (id < 1000), >> but it's facing is set to be external (as per checks in CameraDevice), >> then these are not in sync. We need to take care of that. >> >> I don't think anything bad will happen if these are out of sync. But why >> should we allow it to be out-of-sync if we handle it gracefully? > I agree, it's best to keep them in sync. > >>>> as well (as per the HAL config file) while initializing the CameraDevice >>>> wrapper. >>> I don't quite understand this last part. >> So in the CameraDevice::initialize(), the facing of the camera is set. >> If properties::Location doesn't match the location in the HAL config, >> CameraDevice complains a warning. >> >> But, we know that for UVC properties:::Location won't be matching the >> HAL config. In that case we rely on id assigned by CameraManager to see >> if its a internal camera or not. If yes, we just assign facing whatever >> is mentioned in the HAL config file :) > The last sentence of the commit message can probably be clarified a bit. > How about this ? > > "The CameraHalManager will now assign the numerical id of the camera > accordingly when initializing the CameraDevice, based on the camera > facing value set in the HAL config file." > >>>> [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 | 13 ++++++++++++- >>>> src/android/camera_hal_manager.cpp | 17 +++++++++++++++++ >>>> 2 files changed, 29 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >>>> index 678cde23..394ebc84 100644 >>>> --- a/src/android/camera_device.cpp >>>> +++ b/src/android/camera_device.cpp >>>> @@ -330,7 +330,18 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData) >>>> facing_ = CAMERA_FACING_BACK; >>>> break; >>>> case properties::CameraLocationExternal: >>>> - facing_ = CAMERA_FACING_EXTERNAL; >>>> + /* >>>> + * If the camera is reported as external, but >>>> + * the CameraHalManager has overriden it, use >>>> + * what is reported in the configuration file. >>>> + * This typically happens for UVC cameras >>>> + * reported as 'External' by libcamera but >>>> + * installed in fixed position on the device. >>>> + */ > This could be reflowed to 80 columns. > >>>> + if (id_ < 1000 && cameraConfigData->facing != -1) > Is the id_ check needed ? If libcamera reports the camera as external > and the camera data overrides the facing value, the id will always be < > 1000, won't it ? Ah yes, we can drop the id_ check (although it helps to read the code tbh) and instead have + if (cameraConfigData && cameraConfigData->facing != -1) Should be fine as well. > >>>> + 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 b364f62a..cce98fad 100644 >>>> --- a/src/android/camera_hal_manager.cpp >>>> +++ b/src/android/camera_hal_manager.cpp >>>> @@ -145,6 +145,23 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) >>>> } >>>> >>>> const CameraConfigData *cameraConfigData = halConfig_.cameraConfigData(cam->id()); >>>> + >>>> + /* >>>> + * Some cameras whose location is reported by libcamera as external may >>>> + * actually be internal to the device. This is common with UVC cameras >>>> + * that are integrated in a laptop. In that case the real location >>>> + * should be specified in the configuration file. >>>> + * >>>> + * If the camera location is external and a configuration >>>> + * entry exists for it, override its location. > You could also reflow this to 80 columns. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>>> + */ >>>> + if (isCameraNew && isCameraExternal) { >>>> + if (cameraConfigData && cameraConfigData->facing != -1) { >>>> + isCameraExternal = false; >>>> + id = numInternalCameras_; >>>> + } >>>> + } >>>> + >>>> if (!isCameraExternal && !cameraConfigData) { >>>> LOG(HAL, Error) >>>> << "HAL configuration entry for internal camera "
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 678cde23..394ebc84 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -330,7 +330,18 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData) facing_ = CAMERA_FACING_BACK; break; case properties::CameraLocationExternal: - facing_ = CAMERA_FACING_EXTERNAL; + /* + * If the camera is reported as external, but + * the CameraHalManager has overriden it, use + * what is reported in the configuration file. + * This typically happens for UVC cameras + * reported as 'External' by libcamera but + * installed in fixed position on the device. + */ + if (id_ < 1000 && cameraConfigData->facing != -1) + 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 b364f62a..cce98fad 100644 --- a/src/android/camera_hal_manager.cpp +++ b/src/android/camera_hal_manager.cpp @@ -145,6 +145,23 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) } const CameraConfigData *cameraConfigData = halConfig_.cameraConfigData(cam->id()); + + /* + * Some cameras whose location is reported by libcamera as external may + * actually be internal to the device. This is common with UVC cameras + * that are integrated in a laptop. In that case the real location + * should be specified in the configuration file. + * + * If the camera location is external and a configuration + * entry exists for it, override its location. + */ + if (isCameraNew && isCameraExternal) { + if (cameraConfigData && cameraConfigData->facing != -1) { + isCameraExternal = false; + id = numInternalCameras_; + } + } + if (!isCameraExternal && !cameraConfigData) { LOG(HAL, Error) << "HAL configuration entry for internal camera "
Currently, all UVC cameras are reported with CameraLocationExternal [1] by libcamera-core, 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 in the HAL configuration file, treat it as internal. CameraHalManager will now assign the numerical id of the camera accordingly, based on which the facing of the camera is set as well (as per the HAL config file) while initializing the CameraDevice wrapper. [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 | 13 ++++++++++++- src/android/camera_hal_manager.cpp | 17 +++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-)