[libcamera-devel] [PATCH] qcam: main: Cache lookup of role property

[Laurent Pinchart]

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 at ideasonboard.com>
> 
> >> Signed-off-by: Laurent Pinchart <laurent.pinchart at 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;
> >>  			}
> >>  		}

-- 
Regards,

Laurent Pinchart