| Message ID | 20210709114736.875760-1-umang.jain@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, On 09/07/2021 12:47, Umang Jain wrote: > Populating of the cache in V4L2BufferCache's constructor is incorrect. > The cache_ in V4L2BufferCache is a vector of struct Entry, whose > constructor takes the form: > > Entry(bool free, uint64_t lastUsed, const FrameBuffer &buffer) > > Passing a FrameBuffer::Plane vector via buffer->planes() is > incorrect. Rectify by passing in a Framebuffer reference. > > Signed-off-by: Umang Jain <umang.jain@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 3d2d99b4..da2af6a1 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -183,7 +183,7 @@ V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> > for (const std::unique_ptr<FrameBuffer> &buffer : buffers) > cache_.emplace_back(true, > lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel), > - buffer->planes()); > + *buffer.get()); This is curious indeed. After all, it's compiling so it is working - so I assume here we are constructing a new FrameBuffer from the buffer->planes() as there is a constructor from FrameBuffer() to do just that. I think that means that we will be making a copy of the Planes and possibly duplicating everything just so that we can make a temporary copy/reference to pass to the entry constructor. At which point, the entry makes another copy. So I think this is a correct change, and removes some redundant constructions, but I think the copying of the fd's themselves is handled with a shared pointer - so I don't think there's any particular performance penalty otherwise, but this still seems to be a good change. I think that's a Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> from me, but I feel a little uncertain - so I'll be interested to see what another pair of eyes makes of this. -- Kieran > } > > V4L2BufferCache::~V4L2BufferCache() >
Hello, On Fri, Jul 09, 2021 at 03:13:18PM +0100, Kieran Bingham wrote: > Hi Umang, > > On 09/07/2021 12:47, Umang Jain wrote: > > Populating of the cache in V4L2BufferCache's constructor is incorrect. > > The cache_ in V4L2BufferCache is a vector of struct Entry, whose > > constructor takes the form: > > > > Entry(bool free, uint64_t lastUsed, const FrameBuffer &buffer) > > > > Passing a FrameBuffer::Plane vector via buffer->planes() is > > incorrect. Rectify by passing in a Framebuffer reference. Why is it incorrect ? We have a constructor for that :) > > > > Signed-off-by: Umang Jain <umang.jain@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 3d2d99b4..da2af6a1 100644 > > --- a/src/libcamera/v4l2_videodevice.cpp > > +++ b/src/libcamera/v4l2_videodevice.cpp > > @@ -183,7 +183,7 @@ V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> > > for (const std::unique_ptr<FrameBuffer> &buffer : buffers) > > cache_.emplace_back(true, > > lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel), > > - buffer->planes()); > > + *buffer.get()); > > This is curious indeed. > > After all, it's compiling so it is working - so I assume here we are > constructing a new FrameBuffer from the buffer->planes() as there is a > constructor from FrameBuffer() to do just that. > > I think that means that we will be making a copy of the Planes and > possibly duplicating everything just so that we can make a temporary > copy/reference to pass to the entry constructor. doesn't buffer->planes() returns a reference ? > > At which point, the entry makes another copy. The Framebuffer::Framebuffer(const std::vector<Plane> &planes, unsigned int cookie = 0) constructor indeed copies the planes vector in it's planes_ but I think calling the copy constructor as this patch is doing shouldn't make any difference, right ? Umang, did you see any issue related to this code path ? Thanks j > > So I think this is a correct change, and removes some redundant > constructions, but I think the copying of the fd's themselves is handled > with a shared pointer - so I don't think there's any particular > performance penalty otherwise, but this still seems to be a good change. > > I think that's a > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > from me, but I feel a little uncertain - so I'll be interested to see > what another pair of eyes makes of this. > > > > -- > Kieran > > > > > } > > > > V4L2BufferCache::~V4L2BufferCache() > >
Hello, On Fri, Jul 09, 2021 at 06:02:55PM +0200, Jacopo Mondi wrote: > On Fri, Jul 09, 2021 at 03:13:18PM +0100, Kieran Bingham wrote: > > On 09/07/2021 12:47, Umang Jain wrote: > > > Populating of the cache in V4L2BufferCache's constructor is incorrect. > > > The cache_ in V4L2BufferCache is a vector of struct Entry, whose > > > constructor takes the form: > > > > > > Entry(bool free, uint64_t lastUsed, const FrameBuffer &buffer) > > > > > > Passing a FrameBuffer::Plane vector via buffer->planes() is > > > incorrect. Rectify by passing in a Framebuffer reference. > > Why is it incorrect ? We have a constructor for that :) That's what I was going to say too. It's not wrong per se, but may not be the optimal implementation. > > > Signed-off-by: Umang Jain <umang.jain@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 3d2d99b4..da2af6a1 100644 > > > --- a/src/libcamera/v4l2_videodevice.cpp > > > +++ b/src/libcamera/v4l2_videodevice.cpp > > > @@ -183,7 +183,7 @@ V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> > > > for (const std::unique_ptr<FrameBuffer> &buffer : buffers) > > > cache_.emplace_back(true, > > > lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel), > > > - buffer->planes()); > > > + *buffer.get()); > > > > This is curious indeed. > > > > After all, it's compiling so it is working - so I assume here we are > > constructing a new FrameBuffer from the buffer->planes() as there is a > > constructor from FrameBuffer() to do just that. > > > > I think that means that we will be making a copy of the Planes and > > possibly duplicating everything just so that we can make a temporary > > copy/reference to pass to the entry constructor. > > doesn't buffer->planes() returns a reference ? > > > At which point, the entry makes another copy. > > The > Framebuffer::Framebuffer(const std::vector<Plane> &planes, unsigned int cookie = 0) > > constructor indeed copies the planes vector in it's planes_ but I > think calling the copy constructor as this patch is doing shouldn't > make any difference, right ? > > Umang, did you see any issue related to this code path ? It shoudn't make any functional difference indeed. If it does, there's something that I don't get, and I'd like to know about it. If our understanding that this makes no functional difference, the commit message should be updated, and then Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > So I think this is a correct change, and removes some redundant > > constructions, but I think the copying of the fd's themselves is handled > > with a shared pointer - so I don't think there's any particular > > performance penalty otherwise, but this still seems to be a good change. > > > > I think that's a > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > from me, but I feel a little uncertain - so I'll be interested to see > > what another pair of eyes makes of this. > > > > > } > > > > > > V4L2BufferCache::~V4L2BufferCache()
Hi all, On 7/12/21 1:54 AM, Laurent Pinchart wrote: > Hello, > > On Fri, Jul 09, 2021 at 06:02:55PM +0200, Jacopo Mondi wrote: >> On Fri, Jul 09, 2021 at 03:13:18PM +0100, Kieran Bingham wrote: >>> On 09/07/2021 12:47, Umang Jain wrote: >>>> Populating of the cache in V4L2BufferCache's constructor is incorrect. >>>> The cache_ in V4L2BufferCache is a vector of struct Entry, whose >>>> constructor takes the form: >>>> >>>> Entry(bool free, uint64_t lastUsed, const FrameBuffer &buffer) >>>> >>>> Passing a FrameBuffer::Plane vector via buffer->planes() is >>>> incorrect. Rectify by passing in a Framebuffer reference. >> Why is it incorrect ? We have a constructor for that :) > That's what I was going to say too. It's not wrong per se, but may not > be the optimal implementation. > >>>> Signed-off-by: Umang Jain <umang.jain@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 3d2d99b4..da2af6a1 100644 >>>> --- a/src/libcamera/v4l2_videodevice.cpp >>>> +++ b/src/libcamera/v4l2_videodevice.cpp >>>> @@ -183,7 +183,7 @@ V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> >>>> for (const std::unique_ptr<FrameBuffer> &buffer : buffers) >>>> cache_.emplace_back(true, >>>> lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel), >>>> - buffer->planes()); >>>> + *buffer.get()); >>> This is curious indeed. >>> >>> After all, it's compiling so it is working - so I assume here we are >>> constructing a new FrameBuffer from the buffer->planes() as there is a >>> constructor from FrameBuffer() to do just that. >>> >>> I think that means that we will be making a copy of the Planes and >>> possibly duplicating everything just so that we can make a temporary >>> copy/reference to pass to the entry constructor. >> doesn't buffer->planes() returns a reference ? >> >>> At which point, the entry makes another copy. >> The >> Framebuffer::Framebuffer(const std::vector<Plane> &planes, unsigned int cookie = 0) >> >> constructor indeed copies the planes vector in it's planes_ but I >> think calling the copy constructor as this patch is doing shouldn't >> make any difference, right ? Right, my understanding was mistaken due ignorance of this constructor. Thanks for pointing this out. >> >> Umang, did you see any issue related to this code path ? no, I didn't see any issue. > It shoudn't make any functional difference indeed. If it does, there's > something that I don't get, and I'd like to know about it. > > If our understanding that this makes no functional difference, the > commit message should be updated, and then Yes, no functional difference. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>> So I think this is a correct change, and removes some redundant >>> constructions, but I think the copying of the fd's themselves is handled >>> with a shared pointer - so I don't think there's any particular >>> performance penalty otherwise, but this still seems to be a good change. Ok, I am revamped the commit message and posted a new v2 under the subject "libcamera: v4l2_videodevice: Avoid extra construction of Framebuffer" >>> >>> I think that's a >>> >>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>> >>> from me, but I feel a little uncertain - so I'll be interested to see >>> what another pair of eyes makes of this. >>> >>>> } >>>> >>>> V4L2BufferCache::~V4L2BufferCache()
diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp index 3d2d99b4..da2af6a1 100644 --- a/src/libcamera/v4l2_videodevice.cpp +++ b/src/libcamera/v4l2_videodevice.cpp @@ -183,7 +183,7 @@ V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> for (const std::unique_ptr<FrameBuffer> &buffer : buffers) cache_.emplace_back(true, lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel), - buffer->planes()); + *buffer.get()); } V4L2BufferCache::~V4L2BufferCache()
Populating of the cache in V4L2BufferCache's constructor is incorrect. The cache_ in V4L2BufferCache is a vector of struct Entry, whose constructor takes the form: Entry(bool free, uint64_t lastUsed, const FrameBuffer &buffer) Passing a FrameBuffer::Plane vector via buffer->planes() is incorrect. Rectify by passing in a Framebuffer reference. Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- src/libcamera/v4l2_videodevice.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)