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

[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 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.

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);

-- 
Regards,

Laurent Pinchart