[libcamera-devel,v2,2/4] libcamera: camera_sensor: Set default sensor location to Unknown
diff mbox series

Message ID 20210212054816.53561-3-paul.elder@ideasonboard.com
State Changes Requested
Headers show
Series
  • Add Unknown camera location
Related show

Commit Message

Paul Elder Feb. 12, 2021, 5:48 a.m. UTC 
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>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

---
No change in v2
---
 src/libcamera/camera_sensor.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Niklas Söderlund Feb. 12, 2021, 8:10 a.m. UTC | #1 
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@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@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?

>  	}
>  	properties_.set(properties::Location, propertyValue);
>  
> -- 
> 2.27.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Jacopo Mondi Feb. 12, 2021, 8:54 a.m. UTC | #2 
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@ideasonboard.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@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 this it's fair for libcamera to not assume any arbitrary defaults
and have firmware fixed :)

> >  	}
> >  	properties_.set(properties::Location, propertyValue);
> >
> > --
> > 2.27.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Niklas Söderlund Feb. 12, 2021, 9:07 a.m. UTC | #3 
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@ideasonboard.com>
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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 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 ;-)

> 
> > >  	}
> > >  	properties_.set(properties::Location, propertyValue);
> > >
> > > --
> > > 2.27.0
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel@lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
> >
> > --
> > Regards,
> > Niklas Söderlund
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
Jacopo Mondi Feb. 12, 2021, 9:23 a.m. UTC | #4 
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@ideasonboard.com>
> > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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@lists.libcamera.org
> > > > https://lists.libcamera.org/listinfo/libcamera-devel
> > >
> > > --
> > > Regards,
> > > Niklas Söderlund
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel@lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund
Niklas Söderlund Feb. 12, 2021, 9:36 a.m. UTC | #5 
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@ideasonboard.com>
> > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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@lists.libcamera.org
> > > > > https://lists.libcamera.org/listinfo/libcamera-devel
> > > >
> > > > --
> > > > Regards,
> > > > Niklas Söderlund
> > > > _______________________________________________
> > > > libcamera-devel mailing list
> > > > libcamera-devel@lists.libcamera.org
> > > > https://lists.libcamera.org/listinfo/libcamera-devel
> >
> > --
> > Regards,
> > Niklas Söderlund

Patch
diff mbox series 

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