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

[Niklas Söderlund]

Hello Jacopo,

On 2021-02-12 10:23:19 +0100, Jacopo Mondi wrote:
> 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.

I'm sorry, I still don't see a use-case here other then to push 
incomplete firmware or integration configuration onto applications and 
get CTS to pass using IMHO a hack. But I seems to be in minority so I 
will let it go and move onto implementation :-)

If we don't know the location and are fine to pushing the problem to 
applications, is it not then nicer to make Location an optional property 
and simply not set it if we don't know the location?

> 
> > >
> > > > >  	}
> > > > >  	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

-- 
Regards,
Niklas Söderlund