[RFC PATCH v1 09/12] apps: lc-compliance: Check number of buffers in allocator

[Barnabás Pőcze]

2025. január 10., péntek 11:52 keltezéssel, Laurent Pinchart <laurent.pinchart at 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 at 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
>