[libcamera-devel] [PATCH v2 2/4] libcamera: camera_sensor: Set default sensor location to Unknown

[Jacopo Mondi]

Hi Niklas,

On Fri, Feb 12, 2021 at 10:07:58AM +0100, Niklas Söderlund wrote:
> Hi Jacopo,
>
> On 2021-02-12 09:54:45 +0100, Jacopo Mondi wrote:
> > Hi Niklas,
> >
> > On Fri, Feb 12, 2021 at 09:10:34AM +0100, Niklas Söderlund wrote:
> > > Hello Paul,
> > >
> > > Thanks for your work.
> > >
> > > On 2021-02-12 14:48:14 +0900, Paul Elder wrote:
> > > > Instead of choosing some arbitrary location for the sensor when its
> > > > location is unknown, set it explicitly to unknown.
> > > >
> > > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > >
> > > > ---
> > > > No change in v2
> > > > ---
> > > >  src/libcamera/camera_sensor.cpp | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > > > index c9e8d49b..474055ba 100644
> > > > --- a/src/libcamera/camera_sensor.cpp
> > > > +++ b/src/libcamera/camera_sensor.cpp
> > > > @@ -446,7 +446,7 @@ int CameraSensor::initProperties()
> > > >  			break;
> > > >  		}
> > > >  	} else {
> > > > -		propertyValue = properties::CameraLocationExternal;
> > > > +		propertyValue = properties::CameraLocationUnknown;
> > >
> > > I'm still unconvinced about the use-case for unknown :-)
> > >
> > > To me it looks like we are pushing the problem of an incomplete firmware
> > > description on to applications. How should an application handle a
> > > external camera vs an unknown one?
> > >
> > > I would like to understand the advantage of adding a new location here
> > > other then just to pass a CTS test that as far as I understands it fails
> > > as the device it's running on have incomplete firmware. Is that not a
> > > valid fail of CTS?
> > >
> >
> > Your concerns are valid, but is it any better to default to 'Front" if
> > the information is not available in the firmware. Bear in mind that
> > location is also used to construct the unique camera name, and
> > instead of having two cameras arbitrary assigned to the front location
> > isn't it better to just report that the location isn't known ?
>
> I think it's better to default to something useful and LOG(Error) that
> we default. I would default as we do to external. That CTS fails due to
> this is just a consequence of incomplete camera integration.
>

I'm sorry I don't agree here. Defaulting to something reasonable is an
operation that happens in many places in libcamera, but assuming
FRONT/EXTERNAL is a totally arbitrary choice that might provide
application a false sense of security.

What if an application wants to open a camera that is reported as
front and is actually installed on the back ? As we have seen a
camera being 'external' as implications for Android as I assume it
could have for other applications that might decide to rule out
external pluggable cameras.

> >
> > I this it's fair for libcamera to not assume any arbitrary defaults
> > and have firmware fixed :)
>
> So we should refuse to instantiate cameras who don't report locations, I
> like it ;-)
>

I wish :)

No, but reported "I don't know and cannot tell" will require
application and system integrator to find that information out
somehow. If integrators can and want change the acpi tables or DTS
they should do so, otherwise I expect downstreams will hardcode those
information somehow.

But again, as we don't have any criteria to chose what to default to,
I guess it's better to just report we don't know and cannot retrieve
that information.

> >
> > > >  	}
> > > >  	properties_.set(properties::Location, propertyValue);
> > > >
> > > > --
> > > > 2.27.0
> > > >
> > > > _______________________________________________
> > > > libcamera-devel mailing list
> > > > libcamera-devel at lists.libcamera.org
> > > > https://lists.libcamera.org/listinfo/libcamera-devel
> > >
> > > --
> > > Regards,
> > > Niklas Söderlund
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund