| Message ID | 20200323103938.11026-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Delegated to: | Laurent Pinchart |
| Headers | show |
| Series |
|
| Related | show |
This should obviously have s/qcam/cam/ in the subject line. On Mon, Mar 23, 2020 at 12:39:38PM +0200, Laurent Pinchart wrote: > The code handling the stream role option retrieves the role property and > converts it to a string in every branch. Cache it and use the cached > value. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/cam/main.cpp | 16 ++++++++++------ > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > index 2a0a830ff371..56059ab6c047 100644 > --- a/src/cam/main.cpp > +++ b/src/cam/main.cpp > @@ -209,17 +209,21 @@ int CamApp::prepareConfig() > for (auto const &value : streamOptions) { > KeyValueParser::Options opt = value.toKeyValues(); > > - if (!opt.isSet("role")) { > - roles.push_back(StreamRole::VideoRecording); > - } else if (opt["role"].toString() == "viewfinder") { > + std::string role; > + if (opt.isSet("role")) > + role = opt["role"].toString(); > + else > + role = "viewfinder"; > + > + if (role == "viewfinder") { > roles.push_back(StreamRole::Viewfinder); > - } else if (opt["role"].toString() == "video") { > + } else if (role == "video") { > roles.push_back(StreamRole::VideoRecording); > - } else if (opt["role"].toString() == "still") { > + } else if (role == "still") { > roles.push_back(StreamRole::StillCapture); > } else { > std::cerr << "Unknown stream role " > - << opt["role"].toString() << std::endl; > + << role << std::endl; > return -EINVAL; > } > }
Hi Laurent, On 23/03/2020 10:40, Laurent Pinchart wrote: > This should obviously have s/qcam/cam/ in the subject line. > > On Mon, Mar 23, 2020 at 12:39:38PM +0200, Laurent Pinchart wrote: >> The code handling the stream role option retrieves the role property and >> converts it to a string in every branch. Cache it and use the cached >> value. >> Trivial suggestion, but otherwise LGTM. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> --- >> src/cam/main.cpp | 16 ++++++++++------ >> 1 file changed, 10 insertions(+), 6 deletions(-) >> >> diff --git a/src/cam/main.cpp b/src/cam/main.cpp >> index 2a0a830ff371..56059ab6c047 100644 >> --- a/src/cam/main.cpp >> +++ b/src/cam/main.cpp >> @@ -209,17 +209,21 @@ int CamApp::prepareConfig() >> for (auto const &value : streamOptions) { >> KeyValueParser::Options opt = value.toKeyValues(); >> >> - if (!opt.isSet("role")) { >> - roles.push_back(StreamRole::VideoRecording); >> - } else if (opt["role"].toString() == "viewfinder") { >> + std::string role; >> + if (opt.isSet("role")) >> + role = opt["role"].toString(); >> + else >> + role = "viewfinder"; That almost looks worth a tertiary to make it a single line? role = opt.isSet("role") ? opt["role"].toString() : "viewfinder"; ? >> + >> + if (role == "viewfinder") { >> roles.push_back(StreamRole::Viewfinder); >> - } else if (opt["role"].toString() == "video") { >> + } else if (role == "video") { >> roles.push_back(StreamRole::VideoRecording); >> - } else if (opt["role"].toString() == "still") { >> + } else if (role == "still") { >> roles.push_back(StreamRole::StillCapture); >> } else { >> std::cerr << "Unknown stream role " >> - << opt["role"].toString() << std::endl; >> + << role << std::endl; >> return -EINVAL; >> } >> } >
Hi Kieran, On Mon, Mar 23, 2020 at 11:00:47AM +0000, Kieran Bingham wrote: > On 23/03/2020 10:40, Laurent Pinchart wrote: > > This should obviously have s/qcam/cam/ in the subject line. > > > > On Mon, Mar 23, 2020 at 12:39:38PM +0200, Laurent Pinchart wrote: > >> The code handling the stream role option retrieves the role property and > >> converts it to a string in every branch. Cache it and use the cached > >> value. > > Trivial suggestion, but otherwise LGTM. > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> --- > >> src/cam/main.cpp | 16 ++++++++++------ > >> 1 file changed, 10 insertions(+), 6 deletions(-) > >> > >> diff --git a/src/cam/main.cpp b/src/cam/main.cpp > >> index 2a0a830ff371..56059ab6c047 100644 > >> --- a/src/cam/main.cpp > >> +++ b/src/cam/main.cpp > >> @@ -209,17 +209,21 @@ int CamApp::prepareConfig() > >> for (auto const &value : streamOptions) { > >> KeyValueParser::Options opt = value.toKeyValues(); > >> > >> - if (!opt.isSet("role")) { > >> - roles.push_back(StreamRole::VideoRecording); > >> - } else if (opt["role"].toString() == "viewfinder") { > >> + std::string role; > >> + if (opt.isSet("role")) > >> + role = opt["role"].toString(); > >> + else > >> + role = "viewfinder"; > > That almost looks worth a tertiary to make it a single line? > role = opt.isSet("role") ? opt["role"].toString() : "viewfinder"; > > ? Sure. With identation and line break, it becomes std::string role = opt.isSet("role") ? opt["role"].toString() : "viewfinder"; > >> + > >> + if (role == "viewfinder") { > >> roles.push_back(StreamRole::Viewfinder); > >> - } else if (opt["role"].toString() == "video") { > >> + } else if (role == "video") { > >> roles.push_back(StreamRole::VideoRecording); > >> - } else if (opt["role"].toString() == "still") { > >> + } else if (role == "still") { > >> roles.push_back(StreamRole::StillCapture); > >> } else { > >> std::cerr << "Unknown stream role " > >> - << opt["role"].toString() << std::endl; > >> + << role << std::endl; > >> return -EINVAL; > >> } > >> }
Hi Laurent, On 23/03/2020 11:11, Laurent Pinchart wrote: > Hi Kieran, > > On Mon, Mar 23, 2020 at 11:00:47AM +0000, Kieran Bingham wrote: >> On 23/03/2020 10:40, Laurent Pinchart wrote: >>> This should obviously have s/qcam/cam/ in the subject line. >>> >>> On Mon, Mar 23, 2020 at 12:39:38PM +0200, Laurent Pinchart wrote: >>>> The code handling the stream role option retrieves the role property and >>>> converts it to a string in every branch. Cache it and use the cached >>>> value. >> >> Trivial suggestion, but otherwise LGTM. >> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>>> --- >>>> src/cam/main.cpp | 16 ++++++++++------ >>>> 1 file changed, 10 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/src/cam/main.cpp b/src/cam/main.cpp >>>> index 2a0a830ff371..56059ab6c047 100644 >>>> --- a/src/cam/main.cpp >>>> +++ b/src/cam/main.cpp >>>> @@ -209,17 +209,21 @@ int CamApp::prepareConfig() >>>> for (auto const &value : streamOptions) { >>>> KeyValueParser::Options opt = value.toKeyValues(); >>>> >>>> - if (!opt.isSet("role")) { >>>> - roles.push_back(StreamRole::VideoRecording); >>>> - } else if (opt["role"].toString() == "viewfinder") { >>>> + std::string role; >>>> + if (opt.isSet("role")) >>>> + role = opt["role"].toString(); >>>> + else >>>> + role = "viewfinder"; >> >> That almost looks worth a tertiary to make it a single line? >> role = opt.isSet("role") ? opt["role"].toString() : "viewfinder"; >> >> ? > > Sure. With identation and line break, it becomes > > std::string role = opt.isSet("role") > ? opt["role"].toString() > : "viewfinder"; Ah of course, you can also inline the declaration too then, but that increases line length a little. Personally I like tertiaries for small 'default' statements like this, so I prefer that - but it's up to you :) it's only minor. > >>>> + >>>> + if (role == "viewfinder") { >>>> roles.push_back(StreamRole::Viewfinder); >>>> - } else if (opt["role"].toString() == "video") { >>>> + } else if (role == "video") { >>>> roles.push_back(StreamRole::VideoRecording); >>>> - } else if (opt["role"].toString() == "still") { >>>> + } else if (role == "still") { >>>> roles.push_back(StreamRole::StillCapture); >>>> } else { >>>> std::cerr << "Unknown stream role " >>>> - << opt["role"].toString() << std::endl; >>>> + << role << std::endl; >>>> return -EINVAL; >>>> } >>>> } >
Hi Laurent, On 2020-03-23 12:40:25 +0200, Laurent Pinchart wrote: > This should obviously have s/qcam/cam/ in the subject line. With this fixed and regardless on how you choose to act on Kieran's suggestion, Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > On Mon, Mar 23, 2020 at 12:39:38PM +0200, Laurent Pinchart wrote: > > The code handling the stream role option retrieves the role property and > > converts it to a string in every branch. Cache it and use the cached > > value. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/cam/main.cpp | 16 ++++++++++------ > > 1 file changed, 10 insertions(+), 6 deletions(-) > > > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > > index 2a0a830ff371..56059ab6c047 100644 > > --- a/src/cam/main.cpp > > +++ b/src/cam/main.cpp > > @@ -209,17 +209,21 @@ int CamApp::prepareConfig() > > for (auto const &value : streamOptions) { > > KeyValueParser::Options opt = value.toKeyValues(); > > > > - if (!opt.isSet("role")) { > > - roles.push_back(StreamRole::VideoRecording); > > - } else if (opt["role"].toString() == "viewfinder") { > > + std::string role; > > + if (opt.isSet("role")) > > + role = opt["role"].toString(); > > + else > > + role = "viewfinder"; > > + > > + if (role == "viewfinder") { > > roles.push_back(StreamRole::Viewfinder); > > - } else if (opt["role"].toString() == "video") { > > + } else if (role == "video") { > > roles.push_back(StreamRole::VideoRecording); > > - } else if (opt["role"].toString() == "still") { > > + } else if (role == "still") { > > roles.push_back(StreamRole::StillCapture); > > } else { > > std::cerr << "Unknown stream role " > > - << opt["role"].toString() << std::endl; > > + << role << std::endl; > > return -EINVAL; > > } > > } > > -- > Regards, > > Laurent Pinchart > _______________________________________________ > 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 2a0a830ff371..56059ab6c047 100644 --- a/src/cam/main.cpp +++ b/src/cam/main.cpp @@ -209,17 +209,21 @@ int CamApp::prepareConfig() for (auto const &value : streamOptions) { KeyValueParser::Options opt = value.toKeyValues(); - if (!opt.isSet("role")) { - roles.push_back(StreamRole::VideoRecording); - } else if (opt["role"].toString() == "viewfinder") { + std::string role; + if (opt.isSet("role")) + role = opt["role"].toString(); + else + role = "viewfinder"; + + if (role == "viewfinder") { roles.push_back(StreamRole::Viewfinder); - } else if (opt["role"].toString() == "video") { + } else if (role == "video") { roles.push_back(StreamRole::VideoRecording); - } else if (opt["role"].toString() == "still") { + } else if (role == "still") { roles.push_back(StreamRole::StillCapture); } else { std::cerr << "Unknown stream role " - << opt["role"].toString() << std::endl; + << role << std::endl; return -EINVAL; } }
The code handling the stream role option retrieves the role property and converts it to a string in every branch. Cache it and use the cached value. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/cam/main.cpp | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)