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

[Kieran Bingham]

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

?
>> +
>> +			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