Message ID | 20210319130120.141563-2-jacopo@jmondi.org |
---|---|
State | Accepted |
Headers | show |
Series |
|
Related | show |
Hi Jacopo, Thank you for the patch. On Fri, Mar 19, 2021 at 02:01:18PM +0100, Jacopo Mondi wrote: > In preparation to register the Location property only if the firware > interface provides it, do not assume it is available and build the > camera name using the camera sensor model as a fallback. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/cam/main.cpp | 32 ++++++++++++++++++++------------ > 1 file changed, 20 insertions(+), 12 deletions(-) > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > index e01be63a7058..c087cdd97332 100644 > --- a/src/cam/main.cpp > +++ b/src/cam/main.cpp > @@ -379,18 +379,26 @@ std::string const CamApp::cameraName(const 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; > + if (props.contains(properties::Location)) { > + 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; > + } > + } else if (props.contains(properties::Model)) { > + /* > + * If the camera location is not availble use the camera model > + * to build the camera name. > + */ > + name = "'" + props.get(properties::Model) + "'"; > } > > name += " (" + camera->id() + ")"; If none of the conditions above is true, there will be an extra space. I initially thought utils::join() could be useful here, but that's not a public API :-S This could be an option, I'm sure there are other equally valid or better solutions. diff --git a/src/cam/main.cpp b/src/cam/main.cpp index e01be63a7058..3adfe2c34969 100644 --- a/src/cam/main.cpp +++ b/src/cam/main.cpp @@ -377,23 +377,34 @@ int CamApp::run() std::string const CamApp::cameraName(const Camera *camera) { const ControlList &props = camera->properties(); + bool addModel = true; 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; + /* + * Construct the name from the camera location, model and ID. The model + * is only used if the location isn't present or is set to External. + */ + + if (props.contains(properties::Location)) { + switch (props.get(properties::Location)) { + case properties::CameraLocationFront: + name = "Internal front camera "; + addModel = false; + break; + case properties::CameraLocationBack: + name = "Internal back camera "; + addModel = false; + break; + case properties::CameraLocationExternal: + name = "External camera "; + break; + } } - name += " (" + camera->id() + ")"; + if (addModel && props.contains(properties::Model)) + name += "'" + props.get(properties::Model) + "' "; + + name += "(" + camera->id() + ")"; return name; } It's a detail, and regardless of what option you pick, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Hi Laurent, On Sat, Mar 20, 2021 at 10:55:07PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Fri, Mar 19, 2021 at 02:01:18PM +0100, Jacopo Mondi wrote: > > In preparation to register the Location property only if the firware > > interface provides it, do not assume it is available and build the > > camera name using the camera sensor model as a fallback. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/cam/main.cpp | 32 ++++++++++++++++++++------------ > > 1 file changed, 20 insertions(+), 12 deletions(-) > > > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > > index e01be63a7058..c087cdd97332 100644 > > --- a/src/cam/main.cpp > > +++ b/src/cam/main.cpp > > @@ -379,18 +379,26 @@ std::string const CamApp::cameraName(const 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; > > + if (props.contains(properties::Location)) { > > + 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; > > + } > > + } else if (props.contains(properties::Model)) { > > + /* > > + * If the camera location is not availble use the camera model > > + * to build the camera name. > > + */ > > + name = "'" + props.get(properties::Model) + "'"; > > } > > > > name += " (" + camera->id() + ")"; > > If none of the conditions above is true, there will be an extra space. Isn't the Model property always registered by the CameraSensor class ? > I initially thought utils::join() could be useful here, but that's not a > public API :-S This could be an option, I'm sure there are other equally > valid or better solutions. > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > index e01be63a7058..3adfe2c34969 100644 > --- a/src/cam/main.cpp > +++ b/src/cam/main.cpp > @@ -377,23 +377,34 @@ int CamApp::run() > std::string const CamApp::cameraName(const Camera *camera) > { > const ControlList &props = camera->properties(); > + bool addModel = true; > 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; > + /* > + * Construct the name from the camera location, model and ID. The model > + * is only used if the location isn't present or is set to External. > + */ > + > + if (props.contains(properties::Location)) { > + switch (props.get(properties::Location)) { > + case properties::CameraLocationFront: > + name = "Internal front camera "; > + addModel = false; > + break; > + case properties::CameraLocationBack:2 > + name = "Internal back camera "; > + addModel = false; > + break; > + case properties::CameraLocationExternal: > + name = "External camera "; > + break; > + } > } > > - name += " (" + camera->id() + ")"; > + if (addModel && props.contains(properties::Model)) > + name += "'" + props.get(properties::Model) + "' "; > + > + name += "(" + camera->id() + ")"; > > return name; > } > > It's a detail, and regardless of what option you pick, Anyway, using an addModel flag would make the code more readable so I'll refactor. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Thanks j > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Sun, Mar 21, 2021 at 02:36:22PM +0100, Jacopo Mondi wrote: > On Sat, Mar 20, 2021 at 10:55:07PM +0200, Laurent Pinchart wrote: > > On Fri, Mar 19, 2021 at 02:01:18PM +0100, Jacopo Mondi wrote: > > > In preparation to register the Location property only if the firware > > > interface provides it, do not assume it is available and build the > > > camera name using the camera sensor model as a fallback. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > src/cam/main.cpp | 32 ++++++++++++++++++++------------ > > > 1 file changed, 20 insertions(+), 12 deletions(-) > > > > > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > > > index e01be63a7058..c087cdd97332 100644 > > > --- a/src/cam/main.cpp > > > +++ b/src/cam/main.cpp > > > @@ -379,18 +379,26 @@ std::string const CamApp::cameraName(const 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; > > > + if (props.contains(properties::Location)) { > > > + 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; > > > + } > > > + } else if (props.contains(properties::Model)) { > > > + /* > > > + * If the camera location is not availble use the camera model > > > + * to build the camera name. > > > + */ > > > + name = "'" + props.get(properties::Model) + "'"; > > > } > > > > > > name += " (" + camera->id() + ")"; > > > > If none of the conditions above is true, there will be an extra space. > > Isn't the Model property always registered by the CameraSensor class ? Good question. I have assumed it isn't, as you call props.contains(Properties::Model) :-) I may have been wrong. Note that it's not just about the CameraSensor class, as not all pipeline handlers use it (most notably the UVC pipeline handler). What matters is if the property is documented as required or not. > > I initially thought utils::join() could be useful here, but that's not a > > public API :-S This could be an option, I'm sure there are other equally > > valid or better solutions. > > > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > > index e01be63a7058..3adfe2c34969 100644 > > --- a/src/cam/main.cpp > > +++ b/src/cam/main.cpp > > @@ -377,23 +377,34 @@ int CamApp::run() > > std::string const CamApp::cameraName(const Camera *camera) > > { > > const ControlList &props = camera->properties(); > > + bool addModel = true; > > 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; > > + /* > > + * Construct the name from the camera location, model and ID. The model > > + * is only used if the location isn't present or is set to External. > > + */ > > + > > + if (props.contains(properties::Location)) { > > + switch (props.get(properties::Location)) { > > + case properties::CameraLocationFront: > > + name = "Internal front camera "; > > + addModel = false; > > + break; > > + case properties::CameraLocationBack:2 > > + name = "Internal back camera "; > > + addModel = false; > > + break; > > + case properties::CameraLocationExternal: > > + name = "External camera "; > > + break; > > + } > > } > > > > - name += " (" + camera->id() + ")"; > > + if (addModel && props.contains(properties::Model)) > > + name += "'" + props.get(properties::Model) + "' "; > > + > > + name += "(" + camera->id() + ")"; > > > > return name; > > } > > > > It's a detail, and regardless of what option you pick, > > Anyway, using an addModel flag would make the code more readable so > I'll refactor. > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Hi Laurent, On Sun, Mar 21, 2021 at 03:47:26PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > On Sun, Mar 21, 2021 at 02:36:22PM +0100, Jacopo Mondi wrote: > > On Sat, Mar 20, 2021 at 10:55:07PM +0200, Laurent Pinchart wrote: > > > On Fri, Mar 19, 2021 at 02:01:18PM +0100, Jacopo Mondi wrote: > > > > In preparation to register the Location property only if the firware > > > > interface provides it, do not assume it is available and build the > > > > camera name using the camera sensor model as a fallback. > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > --- > > > > src/cam/main.cpp | 32 ++++++++++++++++++++------------ > > > > 1 file changed, 20 insertions(+), 12 deletions(-) > > > > > > > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > > > > index e01be63a7058..c087cdd97332 100644 > > > > --- a/src/cam/main.cpp > > > > +++ b/src/cam/main.cpp > > > > @@ -379,18 +379,26 @@ std::string const CamApp::cameraName(const 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; > > > > + if (props.contains(properties::Location)) { > > > > + 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; > > > > + } > > > > + } else if (props.contains(properties::Model)) { > > > > + /* > > > > + * If the camera location is not availble use the camera model > > > > + * to build the camera name. > > > > + */ > > > > + name = "'" + props.get(properties::Model) + "'"; > > > > } > > > > > > > > name += " (" + camera->id() + ")"; > > > > > > If none of the conditions above is true, there will be an extra space. > > > > Isn't the Model property always registered by the CameraSensor class ? > > Good question. I have assumed it isn't, as you call > props.contains(Properties::Model) :-) I may have been wrong. Note that And I call props.contains() as it was there already :) > it's not just about the CameraSensor class, as not all pipeline handlers > use it (most notably the UVC pipeline handler). What matters is if the > property is documented as required or not. > The current implementation of the UVC pipeline handler registers Model, but as it's not documented as mandatory I woudl stay safe and continue checking for its presence. I'll take your suggestion in and push! Thanks j > > > I initially thought utils::join() could be useful here, but that's not a > > > public API :-S This could be an option, I'm sure there are other equally > > > valid or better solutions. > > > > > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > > > index e01be63a7058..3adfe2c34969 100644 > > > --- a/src/cam/main.cpp > > > +++ b/src/cam/main.cpp > > > @@ -377,23 +377,34 @@ int CamApp::run() > > > std::string const CamApp::cameraName(const Camera *camera) > > > { > > > const ControlList &props = camera->properties(); > > > + bool addModel = true; > > > 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; > > > + /* > > > + * Construct the name from the camera location, model and ID. The model > > > + * is only used if the location isn't present or is set to External. > > > + */ > > > + > > > + if (props.contains(properties::Location)) { > > > + switch (props.get(properties::Location)) { > > > + case properties::CameraLocationFront: > > > + name = "Internal front camera "; > > > + addModel = false; > > > + break; > > > + case properties::CameraLocationBack:2 > > > + name = "Internal back camera "; > > > + addModel = false; > > > + break; > > > + case properties::CameraLocationExternal: > > > + name = "External camera "; > > > + break; > > > + } > > > } > > > > > > - name += " (" + camera->id() + ")"; > > > + if (addModel && props.contains(properties::Model)) > > > + name += "'" + props.get(properties::Model) + "' "; > > > + > > > + name += "(" + camera->id() + ")"; > > > > > > return name; > > > } > > > > > > It's a detail, and regardless of what option you pick, > > > > Anyway, using an addModel flag would make the code more readable so > > I'll refactor. > > > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Fri, Mar 19, 2021 at 02:01:18PM +0100, Jacopo Mondi wrote: > In preparation to register the Location property only if the firware > interface provides it, do not assume it is available and build the > camera name using the camera sensor model as a fallback. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > src/cam/main.cpp | 32 ++++++++++++++++++++------------ > 1 file changed, 20 insertions(+), 12 deletions(-) > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > index e01be63a7058..c087cdd97332 100644 > --- a/src/cam/main.cpp > +++ b/src/cam/main.cpp > @@ -379,18 +379,26 @@ std::string const CamApp::cameraName(const 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; > + if (props.contains(properties::Location)) { > + 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; > + } > + } else if (props.contains(properties::Model)) { > + /* > + * If the camera location is not availble use the camera model > + * to build the camera name. > + */ > + name = "'" + props.get(properties::Model) + "'"; > } > > name += " (" + camera->id() + ")"; > -- > 2.30.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
diff --git a/src/cam/main.cpp b/src/cam/main.cpp index e01be63a7058..c087cdd97332 100644 --- a/src/cam/main.cpp +++ b/src/cam/main.cpp @@ -379,18 +379,26 @@ std::string const CamApp::cameraName(const 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; + if (props.contains(properties::Location)) { + 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; + } + } else if (props.contains(properties::Model)) { + /* + * If the camera location is not availble use the camera model + * to build the camera name. + */ + name = "'" + props.get(properties::Model) + "'"; } name += " (" + camera->id() + ")";
In preparation to register the Location property only if the firware interface provides it, do not assume it is available and build the camera name using the camera sensor model as a fallback. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/cam/main.cpp | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-)