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

[Kieran Bingham]

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

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

-- 
Regards
--
Kieran