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

[Laurent Pinchart]

Hi Niklas,

On Sat, Feb 13, 2021 at 11:37:56AM +0100, Niklas Söderlund wrote:
> On 2021-02-12 17:59:18 +0200, Laurent Pinchart wrote:
> > 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.

For external cameras, the External location makes sense :-)

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

That's a good point, there's little point in adding an Unknown location
when we can convey the same meaning by not setting the property.

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

That's the whole point of leaving the decision to applications though,
they have different use cases, and should thus be able to react
differently to cameras having an unknown location.

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

We certainly want pipeline handlers to report camera locations. The two
questions that need to be answered today is what to do until we get
there (from that point of view I'd favour the least intrusive approach,
which will be the easiest to revert or update once a known location
becomes mandatory), and if we should accept use cases that have no
meaningful way to select a location (a Raspberry Pi with a camera
hanging off a flat cable for instance). The two questions may call for
different options (if we're unlucky).

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

No no, I think we should continue the discussion before merging this.

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