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

[Niklas Söderlund]

Hi Laurent,

On 2021-02-12 17:59:18 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> On Thu, Feb 11, 2021 at 11:35:58PM +0100, Niklas Söderlund wrote:
> > On 2021-02-12 00:26:02 +0200, Laurent Pinchart wrote:
> > > On Thu, Feb 11, 2021 at 10:58:24PM +0100, Niklas Söderlund wrote:
> > > > On 2021-02-11 23:44:44 +0200, Laurent Pinchart wrote:
> > > > > On Thu, Feb 11, 2021 at 02:24:19PM +0100, Niklas Söderlund wrote:
> > > > > > On 2021-02-11 17:55:26 +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>
> > > > > > > ---
> > > > > > >  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 wonder if it would not make more sens to just set the location to 
> > > > > > front here? What additional use-case do we cover by adding the unkown 
> > > > > > location?
> > > > > >
> > > > > > If we want to highlight we don't know where a camera is would it not be 
> > > > > > better to LOG() that we don't know but assume front. I'm thinking from 
> > > > > > an application point of view is it not kind of messy to have to deal 
> > > > > > with a firmware description that is incomplete? I guess all users will 
> > > > > > do what you do in this series for the HAL and default it to something 
> > > > > > else.
> > > > > 
> > > > > Isn't it better to let the application decide though, instead of
> > > > > pretending we know ? The application could then decide how to deal with
> > > > > the situation depending on its use cases, which are not known to
> > > > > libcamera.
> > > > 
> > > > I'd say it depends :-)
> > > > 
> > > > Down the road I envision the camera location to always be mandatory.  
> > > > Either read from firmware or a platform configuration file. If this 
> > > > holds true I think adding a unknown location now is just pushing the 
> > > > problem down the road. On the other hand if we think we will have 
> > > > cameras with an unknown location and see it as a valid use-case I think 
> > > > this patch is correct.
> > > > 
> > > > I'm however not convinced we have a good use-case for a camera with an 
> > > > unknown location. Do you have an example of such use-case?
> > > 
> > > How would you describe, for instance, a Raspberry Pi camera sitting on a
> > > desk ?
> > 
> > I would describe it as external, just like a UVC camera at the end of a 
> > cable. If I then make a product of the RPi/UVC camera and mount it in a 
> > frame I would expect a firmware or other integration work (configuation 
> > file?) addition to describe how I mounted it.
> 
> Yes, if it was integrated in a product, then it should definitely have a
> location set in the firmware.
> 
> > IMHO that way libcamera applications can make a better deductions about 
> > use-cases then if it has to handle the subtle differences between 
> > external and unknown.
> 
> We're back at square one then, how do we tell the difference between a
> camera that has no fixed/defined location, and a real external camera
> from an Android point of view ?

I think that if we keep the Location property mandatory (as it is today) 
we should in the end not default it to anything in core but simply 
refuse to present any camera who does not provide its location. Either 
retrieved either from firmware, configuration file/script or maybe 
default in the pipeline handler. Maybe it make sens for UVC to default 
to external if no location is specified for example.

If we think adding an Unknown location is a good idea I would argue its 
nicer to make the Location property optional and simply not set if we 
don't know the location.

My fear is that however we implement a solution to "we don't know where 
this camera is located" that pushes the problem to applications the 
result will be that most Linux applications simply won't bother and 
Location will become a "HAL only property". Or worse application A will 
default Unknown to Front while application B will default it to 
External, creating confusion for users.

If I where a Linux camera application hacker and saw the Location 
property will either not be set or set to Unknown for most of the 
platforms I target (Linux desktops/laptops) I would simply use the 
camera model name in my UI.

Looking at all the R-b tags this series have I understand I'm in 
minority :-) I will not block this series to be merged, but I think this 
will be a missed opportunity to make Linux cameras a bit more 
user-friendly for moms and pops as end users.

> 
> > > > > > If you do opt to keep the addition of CameraLocationUnknown you should 
> > > > > > also update cam utility to handle the new location value.
> > > > > 
> > > > > Yes, that should be part of this series.
> > > > > 
> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > > > 
> > > > > > >  	}
> > > > > > >  	properties_.set(properties::Location, propertyValue);
> > > > > > >  
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Niklas Söderlund