| Message ID | 20201013151241.3557005-6-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@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. > > 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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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. 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'? 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@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel >
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@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@lists.libcamera.org > >> https://lists.libcamera.org/listinfo/libcamera-devel > > > > -- > Regards > -- > Kieran
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@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@lists.libcamera.org >>>> https://lists.libcamera.org/listinfo/libcamera-devel >>> >> >> -- >> Regards >> -- >> Kieran >
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(); 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";
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@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(-)