[libcamera-devel] [PATCH v3 4/7] libcamera: V4L2BufferCache: Use the entry reference

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2020-03-05 01:28:36 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Thu, Mar 05, 2020 at 12:22:43AM +0100, Niklas Söderlund wrote:
> > Instead of looking up the index in the storage vector use the reference
> > to it created at the beginning of the loop.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> 
> This could have been fixed in the next patches, when modifying the code.
> Are you trying to improve your patch count ? ;-)

If I wanted to inflate my patch count I know of more efficient ways to 
do so then this ;-P

I write patches as I would like them to read them if I where a reviewer.

Fixing things around the real change in the patch is a red flag for me 
when reviewing. I have to spend time reviewing such patches by 
questioning myself as I cant see why the change is needed only later to 
find out that that line is changed just because we are modifying things 
in the area anyhow. So whenever I find my self starting to describe more 
then one change in the commit message I try break it out to it's own 
patch, even if it's trivial.

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> > ---
> >  src/libcamera/v4l2_videodevice.cpp | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > index d88cb0bd0771e545..268de60bc7965f58 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -216,7 +216,7 @@ int V4L2BufferCache::get(const FrameBuffer &buffer)
> >  			use = index;
> >  
> >  		/* Try to find a cache hit by comparing the planes. */
> > -		if (cache_[index] == buffer) {
> > +		if (entry == buffer) {
> >  			hit = true;
> >  			use = index;
> >  			break;
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Niklas Söderlund