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

[Niklas Söderlund]

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.

> 
> 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,
Niklas Söderlund