| Message ID | 20200304232246.325384-5-niklas.soderlund@ragnatech.se |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@ragnatech.se> This could have been fixed in the next patches, when modifying the code. Are you trying to improve your patch count ? ;-) Reviewed-by: Laurent Pinchart <laurent.pinchart@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;
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@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@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
Hi Niklas, On Thu, Mar 05, 2020 at 09:08:20PM +0100, Niklas Söderlund wrote: > On 2020-03-05 01:28:36 +0200, Laurent Pinchart wrote: > > 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@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. Hence the "While at it, ..." mentions in the commit message :-) Without them I agree it can be confusing. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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;
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;
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@ragnatech.se> --- src/libcamera/v4l2_videodevice.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)