[libcamera-devel] [PATCH 05/10] libcamera: pipeline: Use existing variable definitions

[Kieran Bingham]

Hi Niklas,

On 15/10/2020 12:21, Niklas Söderlund wrote:
> Hi Kieran,
> 
> On 2020-10-15 11:52:20 +0100, Kieran Bingham wrote:
>> Hi Niklas,
>>
>> On 14/10/2020 13:27, Niklas Söderlund wrote:
>>> Hi Kieran,
>>>
>>> On 2020-10-13 16:12:36 +0100, Kieran Bingham wrote:
>>>> Prevent variable aliasing by removing the redeclaration of variables
>>>> with the same name (and type) where the existing variable can be reused.
>>>>
>>>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>>> ---
>>>>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 2 +-
>>>>  src/libcamera/pipeline/raspberrypi/rpi_stream.cpp  | 2 +-
>>>>  src/libcamera/pipeline/simple/simple.cpp           | 4 ++--
>>>>  3 files changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>>>> index 85e0a1f26ab6..2d70d984a276 100644
>>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>>>> @@ -1008,7 +1008,7 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
>>>>  			 */
>>>>  			unsigned int i;
>>>>  			for (i = 0; i < data->dropFrameCount_; i++) {
>>>> -				int ret = stream->queueBuffer(nullptr);
>>>> +				ret = stream->queueBuffer(nullptr);
>>>>  				if (ret)
>>>>  					return ret;
>>>>  			}
>>>> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
>>>> index 1a42cc17bcba..17e38924d653 100644
>>>> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
>>>> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
>>>> @@ -163,7 +163,7 @@ void Stream::returnBuffer(FrameBuffer *buffer)
>>>>  	 * If so, do it now as availableBuffers_ will not be empty.
>>>>  	 */
>>>>  	while (!requestBuffers_.empty()) {
>>>> -		FrameBuffer *buffer = requestBuffers_.front();
>>>> +		buffer = requestBuffers_.front();
>>>
>>> Same comment as for 2/10 about reusing a function argument.
>>
>> This one seems a bit less clear-cut.
>>
>> The incoming buffer is one that gets put on the availableBuffers_ queue.
>>
>> This new 'buffer' is used from two locations. Both the availableBuffers_
>> queue and the requestBuffers_ queue.
>>
>> So there's no clear 'This is a raw Buffer' case like before.
> 
> Is not this 'buffer' use-case to contain a pointer to a buffer to 
> satisfy a request we could not before as we might be in a buffer 
> underrun situiation?
> 
>>
>> We could say that it is distinct, that it is the 'next buffer if
>> identified' to queue to the device...
>>
>> So that could give a name of ...
>>
>>  nextBuffer, or deviceBuffer ... or just 'buffer'?
> 
> Looking at the context I would name it requestBuffer.

Sold to the gentleman at the back.
V2 shortly.

--
Kieran


> 
>>
>> I'm inclined to leave this as just re-using the buffer variable
>> currently....
>>
>> Any thoughts or better naming?
>>
>> --
>> Kieran
>>
>>
>>
>>
>>>
>>>>  
>>>>  		if (!buffer) {
>>>>  			/*
>>>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>>>> index 999c44515023..33daa2fb1b7b 100644
>>>> --- a/src/libcamera/pipeline/simple/simple.cpp
>>>> +++ b/src/libcamera/pipeline/simple/simple.cpp
>>>> @@ -612,8 +612,8 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
>>>>  	useConverter_ = config->needConversion();
>>>>  
>>>>  	if (useConverter_) {
>>>> -		int ret = converter_->configure(pipeConfig.pixelFormat,
>>>> -						pipeConfig.captureSize, &cfg);
>>>> +		ret = converter_->configure(pipeConfig.pixelFormat,
>>>> +					    pipeConfig.captureSize, &cfg);
>>>>  		if (ret < 0) {
>>>>  			LOG(SimplePipeline, Error)
>>>>  				<< "Unable to configure converter";
>>>> -- 
>>>> 2.25.1
>>>>
>>>> _______________________________________________
>>>> libcamera-devel mailing list
>>>> libcamera-devel at lists.libcamera.org
>>>> https://lists.libcamera.org/listinfo/libcamera-devel
>>>
>>
>> -- 
>> Regards
>> --
>> Kieran
> 

-- 
Regards
--
Kieran