| Message ID | 20210211085527.44667-3-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul, Thanks for your work. 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@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. If you do opt to keep the addition of CameraLocationUnknown you should also update cam utility to handle the new location value. > } > properties_.set(properties::Location, propertyValue); > > -- > 2.27.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Paul, On 11/02/2021 08:55, Paul Elder wrote: > Instead of choosing some arbitrary location for the sensor when its > location is unknown, set it explicitly to unknown. This probably confirms that we should always expect the property to be initialised to something. I guess if this were initialised to zero by default, then this would become implicit, but being explicit is nice all the same. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Signed-off-by: Paul Elder <paul.elder@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; > } > properties_.set(properties::Location, propertyValue); > >
Hi Paul, Thank you for the patch. 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@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. > 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@ideasonboard.com> > > } > > properties_.set(properties::Location, propertyValue); > >
Hi Laurent, On 2021-02-11 23:44:44 +0200, Laurent Pinchart wrote: > Hi Paul, > > Thank you for the patch. > > 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@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? > > > 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@ideasonboard.com> > > > > } > > > properties_.set(properties::Location, propertyValue); > > > > > -- > Regards, > > Laurent Pinchart
Hi Niklas, 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@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 ? > > > 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@ideasonboard.com> > > > > > > } > > > > properties_.set(properties::Location, propertyValue); > > > >
Hi Laurent, On 2021-02-12 00:26:02 +0200, Laurent Pinchart wrote: > Hi Niklas, > > 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@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. 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. > > > > > 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@ideasonboard.com> > > > > > > > > } > > > > > properties_.set(properties::Location, propertyValue); > > > > > > > -- > Regards, > > Laurent Pinchart
Hi Niklas, On Thu, Feb 11, 2021 at 11:35:58PM +0100, Niklas Söderlund wrote: > Hi Laurent, > > On 2021-02-12 00:26:02 +0200, Laurent Pinchart wrote: > > Hi Niklas, > > > > 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@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. > > 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. Yes, I agree that there really should be a firmware or configuration file or /something/ that indicates the location. The issue that we're having is what if such configuration is absent. In such case I think it's better to have a distintion between some default value and the fact that the location is unknown, and then the application can decide what to do in the latter case. > > > > > > > If you do opt to keep the addition of CameraLocationUnknown you should > > > > > also update cam utility to handle the new location value. Ah, yes. Thanks, Paul > > > > Yes, that should be part of this series. > > > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > > > > } > > > > > > properties_.set(properties::Location, propertyValue); > > > > > >
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@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 ? > > > > > 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@ideasonboard.com> > > > > > > > > > > } > > > > > > properties_.set(properties::Location, propertyValue); > > > > > >
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@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@ideasonboard.com> > > > > > > > > > > > > } > > > > > > > properties_.set(properties::Location, propertyValue); > > > > > > > > > -- > Regards, > > 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@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@ideasonboard.com> > > > > > > > > > > > > > > } > > > > > > > > properties_.set(properties::Location, propertyValue); > > > > > > > >
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; } properties_.set(properties::Location, propertyValue);
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@ideasonboard.com> --- src/libcamera/camera_sensor.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)