| Message ID | 20241220150759.709756-10-pobrn@protonmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabas On Fri, Dec 20, 2024 at 03:08:41PM +0000, Barnabás Pőcze wrote: > Do a consistency check to ensure that the return value > matches the number of buffers actually created. Could you please make sure your commit messages in the series span to up to 72 cols ? > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > --- > src/apps/lc-compliance/helpers/capture.cpp | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp > index b473e0773..5cc7635f7 100644 > --- a/src/apps/lc-compliance/helpers/capture.cpp > +++ b/src/apps/lc-compliance/helpers/capture.cpp > @@ -49,6 +49,7 @@ void Capture::start() > > ASSERT_GE(count, 0) << "Failed to allocate buffers"; > EXPECT_EQ(count, config_->at(0).bufferCount) << "Allocated less buffers than expected"; > + ASSERT_EQ(count, allocator_.buffers(stream).size()) << "Unexpected number of buffers in allocator"; mmm, FrameBufferAllocator::allocate() returns the number of allocated buffers, if this doesn't match allocator_.buffers(stream).size()) it's likely a problem in the FrameBufferAllocator implementation rather than an issue here. Unless I missed something, I would drop this patch > > camera_->requestCompleted.connect(this, &Capture::requestComplete); > > -- > 2.47.1 > >
Hi 2025. január 7., kedd 18:08 keltezéssel, Jacopo Mondi <jacopo.mondi@ideasonboard.com> írta: > Hi Barnabas > > On Fri, Dec 20, 2024 at 03:08:41PM +0000, Barnabás Pőcze wrote: > > Do a consistency check to ensure that the return value > > matches the number of buffers actually created. > > Could you please make sure your commit messages in the series span to > up to 72 cols ? > > > > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > > --- > > src/apps/lc-compliance/helpers/capture.cpp | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp > > index b473e0773..5cc7635f7 100644 > > --- a/src/apps/lc-compliance/helpers/capture.cpp > > +++ b/src/apps/lc-compliance/helpers/capture.cpp > > @@ -49,6 +49,7 @@ void Capture::start() > > > > ASSERT_GE(count, 0) << "Failed to allocate buffers"; > > EXPECT_EQ(count, config_->at(0).bufferCount) << "Allocated less buffers than expected"; > > + ASSERT_EQ(count, allocator_.buffers(stream).size()) << "Unexpected number of buffers in allocator"; > > mmm, FrameBufferAllocator::allocate() returns the number of allocated > buffers, if this doesn't match > > allocator_.buffers(stream).size()) > > it's likely a problem in the FrameBufferAllocator implementation > rather than an issue here. I think there should be some kind of a consistency check either in `FrameBufferAllocator::allocate()` or at least here. Regards, Barnabás Pőcze > > Unless I missed something, I would drop this patch > > > > > camera_->requestCompleted.connect(this, &Capture::requestComplete); > > > > -- > > 2.47.1 > > > > >
On Fri, Jan 10, 2025 at 09:46:23AM +0000, Barnabás Pőcze wrote: > 2025. január 7., kedd 18:08 keltezéssel, Jacopo Mondi írta: > > On Fri, Dec 20, 2024 at 03:08:41PM +0000, Barnabás Pőcze wrote: > > > Do a consistency check to ensure that the return value > > > matches the number of buffers actually created. > > > > Could you please make sure your commit messages in the series span to > > up to 72 cols ? > > > > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > > > --- > > > src/apps/lc-compliance/helpers/capture.cpp | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp > > > index b473e0773..5cc7635f7 100644 > > > --- a/src/apps/lc-compliance/helpers/capture.cpp > > > +++ b/src/apps/lc-compliance/helpers/capture.cpp > > > @@ -49,6 +49,7 @@ void Capture::start() > > > > > > ASSERT_GE(count, 0) << "Failed to allocate buffers"; > > > EXPECT_EQ(count, config_->at(0).bufferCount) << "Allocated less buffers than expected"; > > > + ASSERT_EQ(count, allocator_.buffers(stream).size()) << "Unexpected number of buffers in allocator"; > > > > mmm, FrameBufferAllocator::allocate() returns the number of allocated > > buffers, if this doesn't match > > > > allocator_.buffers(stream).size()) > > > > it's likely a problem in the FrameBufferAllocator implementation > > rather than an issue here. > > I think there should be some kind of a consistency check either in > `FrameBufferAllocator::allocate()` or at least here. Maybe it's a candidae for a unit test instead ? > > Unless I missed something, I would drop this patch > > > > > camera_->requestCompleted.connect(this, &Capture::requestComplete);
2025. január 10., péntek 11:52 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta: > On Fri, Jan 10, 2025 at 09:46:23AM +0000, Barnabás Pőcze wrote: > > 2025. január 7., kedd 18:08 keltezéssel, Jacopo Mondi írta: > > > On Fri, Dec 20, 2024 at 03:08:41PM +0000, Barnabás Pőcze wrote: > > > > Do a consistency check to ensure that the return value > > > > matches the number of buffers actually created. > > > > > > Could you please make sure your commit messages in the series span to > > > up to 72 cols ? > > > > > > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > > > > --- > > > > src/apps/lc-compliance/helpers/capture.cpp | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp > > > > index b473e0773..5cc7635f7 100644 > > > > --- a/src/apps/lc-compliance/helpers/capture.cpp > > > > +++ b/src/apps/lc-compliance/helpers/capture.cpp > > > > @@ -49,6 +49,7 @@ void Capture::start() > > > > > > > > ASSERT_GE(count, 0) << "Failed to allocate buffers"; > > > > EXPECT_EQ(count, config_->at(0).bufferCount) << "Allocated less buffers than expected"; > > > > + ASSERT_EQ(count, allocator_.buffers(stream).size()) << "Unexpected number of buffers in allocator"; > > > > > > mmm, FrameBufferAllocator::allocate() returns the number of allocated > > > buffers, if this doesn't match > > > > > > allocator_.buffers(stream).size()) > > > > > > it's likely a problem in the FrameBufferAllocator implementation > > > rather than an issue here. > > > > I think there should be some kind of a consistency check either in > > `FrameBufferAllocator::allocate()` or at least here. > > Maybe it's a candidae for a unit test instead ? Sorry, but I don't quite see what you mean. I was thinking along the lines of diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp index 3d53bde21..7f26e36b9 100644 --- a/src/libcamera/framebuffer_allocator.cpp +++ b/src/libcamera/framebuffer_allocator.cpp @@ -101,6 +101,8 @@ int FrameBufferAllocator::allocate(Stream *stream) if (ret < 0) buffers_.erase(it); + ASSERT(std::size_t(ret) == it->second.size()); + return ret; } or diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp index 3d53bde21..830c6fa1b 100644 --- a/src/libcamera/framebuffer_allocator.cpp +++ b/src/libcamera/framebuffer_allocator.cpp @@ -101,6 +101,15 @@ int FrameBufferAllocator::allocate(Stream *stream) if (ret < 0) buffers_.erase(it); + if (std::size_t(ret) != it->second.size()) { + LOG(Allocator, Error) + << "Pipeline handler bug: " + << "return value does not reflect number of allocated buffers: " + << "adjusting " << ret << " to " << it->second.size(); + + ret = it->second.size(); + } + return ret; } or possibly diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp index 3d53bde21..b583dbbc3 100644 --- a/src/libcamera/framebuffer_allocator.cpp +++ b/src/libcamera/framebuffer_allocator.cpp @@ -98,10 +98,12 @@ int FrameBufferAllocator::allocate(Stream *stream) << "Stream is not part of " << camera_->id() << " active configuration"; - if (ret < 0) + if (ret < 0) { buffers_.erase(it); + return ret; + } - return ret; + return it->second.size(); } /** I am not sure how a unit test could provide the same guarantees. Regards, Barnabás Pőcze > > > > Unless I missed something, I would drop this patch > > > > > > > camera_->requestCompleted.connect(this, &Capture::requestComplete); > > -- > Regards, > > Laurent Pinchart >
On Fri, Jan 10, 2025 at 12:15:52PM +0000, Barnabás Pőcze wrote: > 2025. január 10., péntek 11:52 keltezéssel, Laurent Pinchart írta: > > On Fri, Jan 10, 2025 at 09:46:23AM +0000, Barnabás Pőcze wrote: > > > 2025. január 7., kedd 18:08 keltezéssel, Jacopo Mondi írta: > > > > On Fri, Dec 20, 2024 at 03:08:41PM +0000, Barnabás Pőcze wrote: > > > > > Do a consistency check to ensure that the return value > > > > > matches the number of buffers actually created. > > > > > > > > Could you please make sure your commit messages in the series span to > > > > up to 72 cols ? > > > > > > > > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > > > > > --- > > > > > src/apps/lc-compliance/helpers/capture.cpp | 1 + > > > > > 1 file changed, 1 insertion(+) > > > > > > > > > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp > > > > > index b473e0773..5cc7635f7 100644 > > > > > --- a/src/apps/lc-compliance/helpers/capture.cpp > > > > > +++ b/src/apps/lc-compliance/helpers/capture.cpp > > > > > @@ -49,6 +49,7 @@ void Capture::start() > > > > > > > > > > ASSERT_GE(count, 0) << "Failed to allocate buffers"; > > > > > EXPECT_EQ(count, config_->at(0).bufferCount) << "Allocated less buffers than expected"; > > > > > + ASSERT_EQ(count, allocator_.buffers(stream).size()) << "Unexpected number of buffers in allocator"; > > > > > > > > mmm, FrameBufferAllocator::allocate() returns the number of allocated > > > > buffers, if this doesn't match > > > > > > > > allocator_.buffers(stream).size()) > > > > > > > > it's likely a problem in the FrameBufferAllocator implementation > > > > rather than an issue here. > > > > > > I think there should be some kind of a consistency check either in > > > `FrameBufferAllocator::allocate()` or at least here. > > > > Maybe it's a candidae for a unit test instead ? > > Sorry, but I don't quite see what you mean. I was thinking along the lines of > > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp > index 3d53bde21..7f26e36b9 100644 > --- a/src/libcamera/framebuffer_allocator.cpp > +++ b/src/libcamera/framebuffer_allocator.cpp > @@ -101,6 +101,8 @@ int FrameBufferAllocator::allocate(Stream *stream) > if (ret < 0) > buffers_.erase(it); > > + ASSERT(std::size_t(ret) == it->second.size()); > + > return ret; > } > > > or > > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp > index 3d53bde21..830c6fa1b 100644 > --- a/src/libcamera/framebuffer_allocator.cpp > +++ b/src/libcamera/framebuffer_allocator.cpp > @@ -101,6 +101,15 @@ int FrameBufferAllocator::allocate(Stream *stream) > if (ret < 0) > buffers_.erase(it); > > + if (std::size_t(ret) != it->second.size()) { > + LOG(Allocator, Error) > + << "Pipeline handler bug: " > + << "return value does not reflect number of allocated buffers: " > + << "adjusting " << ret << " to " << it->second.size(); > + > + ret = it->second.size(); > + } > + > return ret; > } > > > or possibly > > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp > index 3d53bde21..b583dbbc3 100644 > --- a/src/libcamera/framebuffer_allocator.cpp > +++ b/src/libcamera/framebuffer_allocator.cpp > @@ -98,10 +98,12 @@ int FrameBufferAllocator::allocate(Stream *stream) > << "Stream is not part of " << camera_->id() > << " active configuration"; > > - if (ret < 0) > + if (ret < 0) { > buffers_.erase(it); > + return ret; > + } > > - return ret; > + return it->second.size(); > } > > /** > > > I am not sure how a unit test could provide the same guarantees. I see what you mean now. It seems to me we may be facing an API design issue. The function returns the number of allocated buffers in two different ways, through the return value, and through the buffers vector. If we changed the API to return 0 on success, we would get rid of the issue in the first place. If we don't want to change the API, and if we're concerned that some pipeline handlers would implement this incorrectly, we could make PipelineHandler::exportFrameBuffers() return 0 on success, and return buffers->size() from Camera::exportFrameBuffers(). That way the ret == buffers->size() invariant will be implemented in a single place. > > > > Unless I missed something, I would drop this patch > > > > > > > > > camera_->requestCompleted.connect(this, &Capture::requestComplete);
diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp index b473e0773..5cc7635f7 100644 --- a/src/apps/lc-compliance/helpers/capture.cpp +++ b/src/apps/lc-compliance/helpers/capture.cpp @@ -49,6 +49,7 @@ void Capture::start() ASSERT_GE(count, 0) << "Failed to allocate buffers"; EXPECT_EQ(count, config_->at(0).bufferCount) << "Allocated less buffers than expected"; + ASSERT_EQ(count, allocator_.buffers(stream).size()) << "Unexpected number of buffers in allocator"; camera_->requestCompleted.connect(this, &Capture::requestComplete);
Do a consistency check to ensure that the return value matches the number of buffers actually created. Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> --- src/apps/lc-compliance/helpers/capture.cpp | 1 + 1 file changed, 1 insertion(+)