| Message ID | 20210219104544.53217-3-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
On 19/02/2021 10:45, Paul Elder wrote: > Our android HAL implementation currently does not support external > cameras, so if the camera location property is external, set it to > front. > > This allows the following CTS test to pass: > - android.hardware.camera2.cts.CameraManagerTest#testCameraManagerGetDeviceIdList > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > --- > src/android/camera_device.cpp | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 1e2a5b5f..16cb8c6d 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -405,7 +405,11 @@ int CameraDevice::initialize() > facing_ = CAMERA_FACING_BACK; > break; > case properties::CameraLocationExternal: > - facing_ = CAMERA_FACING_EXTERNAL; > + /* > + * \todo Set this to EXTERNAL once we support > + * HARDWARE_LEVEL_EXTERNAL > + */ > + facing_ = CAMERA_FACING_FRONT; I would be tempted to add a warning log here too, as it's distinctly separate from the setting of the properties::CameraLocationExternal, which can certainly be correctly set to External (on UVC) for example, but which would come through here. I hadn't realised/been aware of this HARDWARE_LEVEL_EXTERNAL issues, I wonder if that was some how related to the difficulties we still had in even getting a Brio YUV UVC camera to work. I suspect it is, as perhaps with HARDWARE_LEVEL_EXTERNAL we can better define the actual capabilities /streams of the external device. Anyway, with or without the warning added, as this is a known workaround: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > break; > } > } >
Hi Paul, Thanks for your work. On 2021-02-19 11:05:14 +0000, Kieran Bingham wrote: > On 19/02/2021 10:45, Paul Elder wrote: > > Our android HAL implementation currently does not support external > > cameras, so if the camera location property is external, set it to > > front. > > > > This allows the following CTS test to pass: > > - android.hardware.camera2.cts.CameraManagerTest#testCameraManagerGetDeviceIdList > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > > src/android/camera_device.cpp | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index 1e2a5b5f..16cb8c6d 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -405,7 +405,11 @@ int CameraDevice::initialize() > > facing_ = CAMERA_FACING_BACK; > > break; > > case properties::CameraLocationExternal: > > - facing_ = CAMERA_FACING_EXTERNAL; > > + /* > > + * \todo Set this to EXTERNAL once we support > > + * HARDWARE_LEVEL_EXTERNAL > > + */ > > + facing_ = CAMERA_FACING_FRONT; > > I would be tempted to add a warning log here too, as it's distinctly > separate from the setting of the properties::CameraLocationExternal, > which can certainly be correctly set to External (on UVC) for example, > but which would come through here. > > I hadn't realised/been aware of this HARDWARE_LEVEL_EXTERNAL issues, I > wonder if that was some how related to the difficulties we still had in > even getting a Brio YUV UVC camera to work. > > I suspect it is, as perhaps with HARDWARE_LEVEL_EXTERNAL we can better > define the actual capabilities /streams of the external device. > > > Anyway, with or without the warning added, as this is a known workaround: With a warning printout added, Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > break; > > } > > } > > > > -- > Regards > -- > Kieran > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hello, On Fri, Feb 19, 2021 at 11:05:14AM +0000, Kieran Bingham wrote: > On 19/02/2021 10:45, Paul Elder wrote: > > Our android HAL implementation currently does not support external > > cameras, so if the camera location property is external, set it to > > front. > > > > This allows the following CTS test to pass: > > - android.hardware.camera2.cts.CameraManagerTest#testCameraManagerGetDeviceIdList > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > > src/android/camera_device.cpp | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index 1e2a5b5f..16cb8c6d 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -405,7 +405,11 @@ int CameraDevice::initialize() > > facing_ = CAMERA_FACING_BACK; > > break; > > case properties::CameraLocationExternal: > > - facing_ = CAMERA_FACING_EXTERNAL; > > + /* > > + * \todo Set this to EXTERNAL once we support > > + * HARDWARE_LEVEL_EXTERNAL > > + */ > > + facing_ = CAMERA_FACING_FRONT; > > I would be tempted to add a warning log here too, as it's distinctly > separate from the setting of the properties::CameraLocationExternal, > which can certainly be correctly set to External (on UVC) for example, > but which would come through here. It's a todo item, and we have lots of them for which we don't print any warning. I'm not sure to see why this one would be special. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > I hadn't realised/been aware of this HARDWARE_LEVEL_EXTERNAL issues, I > wonder if that was some how related to the difficulties we still had in > even getting a Brio YUV UVC camera to work. > > I suspect it is, as perhaps with HARDWARE_LEVEL_EXTERNAL we can better > define the actual capabilities /streams of the external device. > > > Anyway, with or without the warning added, as this is a known workaround: > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > break; > > } > > }
On 21/02/2021 19:21, Laurent Pinchart wrote: > Hello, > > On Fri, Feb 19, 2021 at 11:05:14AM +0000, Kieran Bingham wrote: >> On 19/02/2021 10:45, Paul Elder wrote: >>> Our android HAL implementation currently does not support external >>> cameras, so if the camera location property is external, set it to >>> front. >>> >>> This allows the following CTS test to pass: >>> - android.hardware.camera2.cts.CameraManagerTest#testCameraManagerGetDeviceIdList >>> >>> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> >>> --- >>> src/android/camera_device.cpp | 6 +++++- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >>> index 1e2a5b5f..16cb8c6d 100644 >>> --- a/src/android/camera_device.cpp >>> +++ b/src/android/camera_device.cpp >>> @@ -405,7 +405,11 @@ int CameraDevice::initialize() >>> facing_ = CAMERA_FACING_BACK; >>> break; >>> case properties::CameraLocationExternal: >>> - facing_ = CAMERA_FACING_EXTERNAL; >>> + /* >>> + * \todo Set this to EXTERNAL once we support >>> + * HARDWARE_LEVEL_EXTERNAL >>> + */ >>> + facing_ = CAMERA_FACING_FRONT; >> >> I would be tempted to add a warning log here too, as it's distinctly >> separate from the setting of the properties::CameraLocationExternal, >> which can certainly be correctly set to External (on UVC) for example, >> but which would come through here. > > It's a todo item, and we have lots of them for which we don't print any > warning. I'm not sure to see why this one would be special. Because this one impacts the actual expected output. An 'External' camera is being renamed / replaced to being 'Front'. If this was explicitly set (for example, an RPi, or UVC camera) we're changing the reported location. Consider that the user *expected* this camera to be external, and yet when run through the HAL was always reported 'Front'. - It's that *change* which makes me think a warning is worth printing. It's not about having a todo action, it's reporting that something unexpectedly changed internally. > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> I hadn't realised/been aware of this HARDWARE_LEVEL_EXTERNAL issues, I >> wonder if that was some how related to the difficulties we still had in >> even getting a Brio YUV UVC camera to work. >> >> I suspect it is, as perhaps with HARDWARE_LEVEL_EXTERNAL we can better >> define the actual capabilities /streams of the external device. But as I said: >> Anyway, with or without the warning added, as this is a known workaround: >> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >>> break; >>> } >>> } >
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 1e2a5b5f..16cb8c6d 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -405,7 +405,11 @@ int CameraDevice::initialize() facing_ = CAMERA_FACING_BACK; break; case properties::CameraLocationExternal: - facing_ = CAMERA_FACING_EXTERNAL; + /* + * \todo Set this to EXTERNAL once we support + * HARDWARE_LEVEL_EXTERNAL + */ + facing_ = CAMERA_FACING_FRONT; break; } }
Our android HAL implementation currently does not support external cameras, so if the camera location property is external, set it to front. This allows the following CTS test to pass: - android.hardware.camera2.cts.CameraManagerTest#testCameraManagerGetDeviceIdList Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- src/android/camera_device.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)