Message ID | 20201021154043.511274-1-kieran.bingham@ideasonboard.com |
---|---|
State | Superseded |
Delegated to: | Kieran Bingham |
Headers | show |
Series |
|
Related | show |
Hi Kieran, On Wed, Oct 21, 2020 at 04:40:43PM +0100, Kieran Bingham wrote: > Take the example code for generating a camera name from 'cam' and use > it when reporting cameras within the simple-cam application. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > > I wonder if this cameraName() might be something that > should go into libcamera::utils as a public API helper? > > > I know we want to allow applications to decide their own naming too, but > I think we've discussed in the past about having a helper library on top > to make things easier for application developers ? > > > simple-cam.cpp | 30 +++++++++++++++++++++++++++--- > 1 file changed, 27 insertions(+), 3 deletions(-) > > diff --git a/simple-cam.cpp b/simple-cam.cpp > index 727bb6d86480..1b6fd3939bf6 100644 > --- a/simple-cam.cpp > +++ b/simple-cam.cpp > @@ -60,6 +60,30 @@ static void requestComplete(Request *request) > camera->queueRequest(request); > } > > +std::string cameraName(std::shared_ptr<libcamera::Camera> camera) nit: camera can be a reference, or just a raw pointer. No need to increase/decrease the usage count just for the scope of this function. > +{ > + const ControlList &props = camera->properties(); > + std::string name; > + > + switch (props.get(properties::Location)) { > + case properties::CameraLocationFront: > + name = "Internal front camera"; > + break; I wonder if defaulting to "Front" in CameraSensor is still a good idea or we should let applications deals with that case... > + case properties::CameraLocationBack: > + name = "Internal back camera"; > + break; > + case properties::CameraLocationExternal: > + name = "External camera"; > + if (props.contains(properties::Model)) > + name += " '" + props.get(properties::Model) + "'"; Why model is only considered for external cameras ? With the last question clarified Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > + break; > + } > + > + name += " (" + camera->id() + ")"; > + > + return name; > +} > + > int main() > { > /* > @@ -77,11 +101,11 @@ int main() > cm->start(); > > /* > - * Just as a test, list all id's of the Camera registered in the > - * system. They are indexed by name by the CameraManager. > + * Just as a test, generate names of the Cameras registered in the > + * system, and list them. > */ > for (auto const &camera : cm->cameras()) > - std::cout << camera->id() << std::endl; > + std::cout << " - " << cameraName(camera) << std::endl; > > /* > * -------------------------------------------------------------------- > -- > 2.25.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
On 21/10/2020 17:26, Jacopo Mondi wrote: > Hi Kieran, > > On Wed, Oct 21, 2020 at 04:40:43PM +0100, Kieran Bingham wrote: >> Take the example code for generating a camera name from 'cam' and use >> it when reporting cameras within the simple-cam application. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> >> I wonder if this cameraName() might be something that >> should go into libcamera::utils as a public API helper? >> >> >> I know we want to allow applications to decide their own naming too, but >> I think we've discussed in the past about having a helper library on top >> to make things easier for application developers ? >> >> >> simple-cam.cpp | 30 +++++++++++++++++++++++++++--- >> 1 file changed, 27 insertions(+), 3 deletions(-) >> >> diff --git a/simple-cam.cpp b/simple-cam.cpp >> index 727bb6d86480..1b6fd3939bf6 100644 >> --- a/simple-cam.cpp >> +++ b/simple-cam.cpp >> @@ -60,6 +60,30 @@ static void requestComplete(Request *request) >> camera->queueRequest(request); >> } >> >> +std::string cameraName(std::shared_ptr<libcamera::Camera> camera) > > nit: camera can be a reference, or just a raw pointer. No need to I started with a raw pointer, but got a compile error, indicating that the type available in : for (auto const &camera : cm->cameras()) was a shared pointer. So that's what I put in and it compiled ;-) I presume I could also do a camera.get() ? > increase/decrease the usage count just for the scope of this function. > >> +{ >> + const ControlList &props = camera->properties(); >> + std::string name; >> + >> + switch (props.get(properties::Location)) { >> + case properties::CameraLocationFront: >> + name = "Internal front camera"; >> + break; > > I wonder if defaulting to "Front" in CameraSensor is still a good idea > or we should let applications deals with that case... > >> + case properties::CameraLocationBack: >> + name = "Internal back camera"; >> + break; >> + case properties::CameraLocationExternal: >> + name = "External camera"; >> + if (props.contains(properties::Model)) >> + name += " '" + props.get(properties::Model) + "'"; > > Why model is only considered for external cameras ? > The code there is taken verbatim from src/cam/ IMO, any change here should have a corresponding change there - or ... all of this should go in to some helper library (libcamera::utils?) so it can be handled as a common use case. > With the last question clarified > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks. k > > Thanks > j > > >> + break; >> + } >> + >> + name += " (" + camera->id() + ")"; >> + >> + return name; >> +} >> + >> int main() >> { >> /* >> @@ -77,11 +101,11 @@ int main() >> cm->start(); >> >> /* >> - * Just as a test, list all id's of the Camera registered in the >> - * system. They are indexed by name by the CameraManager. >> + * Just as a test, generate names of the Cameras registered in the >> + * system, and list them. >> */ >> for (auto const &camera : cm->cameras()) >> - std::cout << camera->id() << std::endl; >> + std::cout << " - " << cameraName(camera) << std::endl; >> >> /* >> * -------------------------------------------------------------------- >> -- >> 2.25.1 >> >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel
Hi Kieran, Thank you for the patch. On Wed, Oct 21, 2020 at 05:31:18PM +0100, Kieran Bingham wrote: > On 21/10/2020 17:26, Jacopo Mondi wrote: > > On Wed, Oct 21, 2020 at 04:40:43PM +0100, Kieran Bingham wrote: > >> Take the example code for generating a camera name from 'cam' and use > >> it when reporting cameras within the simple-cam application. > >> > >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> --- > >> > >> I wonder if this cameraName() might be something that > >> should go into libcamera::utils as a public API helper? > >> > >> I know we want to allow applications to decide their own naming too, but > >> I think we've discussed in the past about having a helper library on top > >> to make things easier for application developers ? > >> > >> simple-cam.cpp | 30 +++++++++++++++++++++++++++--- > >> 1 file changed, 27 insertions(+), 3 deletions(-) > >> > >> diff --git a/simple-cam.cpp b/simple-cam.cpp > >> index 727bb6d86480..1b6fd3939bf6 100644 > >> --- a/simple-cam.cpp > >> +++ b/simple-cam.cpp > >> @@ -60,6 +60,30 @@ static void requestComplete(Request *request) > >> camera->queueRequest(request); > >> } > >> > >> +std::string cameraName(std::shared_ptr<libcamera::Camera> camera) > > > > nit: camera can be a reference, or just a raw pointer. No need to > > I started with a raw pointer, but got a compile error, indicating that > the type available in : > > for (auto const &camera : cm->cameras()) > > was a shared pointer. So that's what I put in and it compiled ;-) > > I presume I could also do a camera.get() ? Yes, I think that's better to avoid constructing/destroying a shared pointer for little gain. > > increase/decrease the usage count just for the scope of this function. > > > >> +{ > >> + const ControlList &props = camera->properties(); > >> + std::string name; > >> + > >> + switch (props.get(properties::Location)) { > >> + case properties::CameraLocationFront: > >> + name = "Internal front camera"; > >> + break; > > > > I wonder if defaulting to "Front" in CameraSensor is still a good idea > > or we should let applications deals with that case... > > > >> + case properties::CameraLocationBack: > >> + name = "Internal back camera"; > >> + break; > >> + case properties::CameraLocationExternal: > >> + name = "External camera"; > >> + if (props.contains(properties::Model)) > >> + name += " '" + props.get(properties::Model) + "'"; > > > > Why model is only considered for external cameras ? > > The code there is taken verbatim from src/cam/ > > IMO, any change here should have a corresponding change there - or ... > all of this should go in to some helper library (libcamera::utils?) so > it can be handled as a common use case. A libcamera utility library is needed (and we'll have to discuss whether it should be a separate library, or part of the same binary). I'm however not convinced this particular feature belongs there, as the idea behind our camera naming scheme is to let applications construct the names in the way that best fits their use cases, including localizing them if needed. We have code duplication between cam and simplecam because they're both demo applications, but it doesn't meant it would be a candidate for libcamera itself. Additionally, I think the application guide should be updated to explain how applications are supposed to construct a camera name. > > With the last question clarified > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > >> + break; > >> + } > >> + > >> + name += " (" + camera->id() + ")"; > >> + > >> + return name; > >> +} > >> + > >> int main() > >> { > >> /* > >> @@ -77,11 +101,11 @@ int main() > >> cm->start(); > >> > >> /* > >> - * Just as a test, list all id's of the Camera registered in the > >> - * system. They are indexed by name by the CameraManager. > >> + * Just as a test, generate names of the Cameras registered in the > >> + * system, and list them. > >> */ > >> for (auto const &camera : cm->cameras()) > >> - std::cout << camera->id() << std::endl; > >> + std::cout << " - " << cameraName(camera) << std::endl; > >> > >> /* > >> * --------------------------------------------------------------------
diff --git a/simple-cam.cpp b/simple-cam.cpp index 727bb6d86480..1b6fd3939bf6 100644 --- a/simple-cam.cpp +++ b/simple-cam.cpp @@ -60,6 +60,30 @@ static void requestComplete(Request *request) camera->queueRequest(request); } +std::string cameraName(std::shared_ptr<libcamera::Camera> camera) +{ + const ControlList &props = camera->properties(); + std::string name; + + switch (props.get(properties::Location)) { + case properties::CameraLocationFront: + name = "Internal front camera"; + break; + case properties::CameraLocationBack: + name = "Internal back camera"; + break; + case properties::CameraLocationExternal: + name = "External camera"; + if (props.contains(properties::Model)) + name += " '" + props.get(properties::Model) + "'"; + break; + } + + name += " (" + camera->id() + ")"; + + return name; +} + int main() { /* @@ -77,11 +101,11 @@ int main() cm->start(); /* - * Just as a test, list all id's of the Camera registered in the - * system. They are indexed by name by the CameraManager. + * Just as a test, generate names of the Cameras registered in the + * system, and list them. */ for (auto const &camera : cm->cameras()) - std::cout << camera->id() << std::endl; + std::cout << " - " << cameraName(camera) << std::endl; /* * --------------------------------------------------------------------
Take the example code for generating a camera name from 'cam' and use it when reporting cameras within the simple-cam application. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- I wonder if this cameraName() might be something that should go into libcamera::utils as a public API helper? I know we want to allow applications to decide their own naming too, but I think we've discussed in the past about having a helper library on top to make things easier for application developers ? simple-cam.cpp | 30 +++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-)