[libcamera-devel] [PATCH v2 1/2] libcamera: framebuffer_allocator: Make allocate() accept count

[Nícolas F. R. A. Prado]

Em 2021-04-19 03:07, Laurent Pinchart escreveu:

> Hello,
>
> On Mon, Apr 19, 2021 at 02:56:33PM +0900, Hirokazu Honda wrote:
> > Hi Nicolas, thanks for the patch.
> > 
> > On Sat, Apr 17, 2021 at 7:29 AM Nícolas F. R. A. Prado wrote:
> > >
> > > Add a 'count' argument to FrameBufferAllocator::allocate() so that a
> > > custom amount of buffers can be allocated. If 0 is passed, the pipeline
> > > handler allocates the default amount, which is the configured
> > > bufferCount.
> > >
> > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado at collabora.com>
> > > ---
> > >  include/libcamera/camera.h                         | 3 ++-
> > >  include/libcamera/framebuffer_allocator.h          | 2 +-
> > >  include/libcamera/internal/pipeline_handler.h      | 3 ++-
> > >  src/cam/capture.cpp                                | 2 +-
> > >  src/lc-compliance/simple_capture.cpp               | 8 ++++----
> > >  src/lc-compliance/simple_capture.h                 | 2 +-
> > >  src/libcamera/camera.cpp                           | 5 +++--
> > >  src/libcamera/framebuffer_allocator.cpp            | 5 +++--
> > >  src/libcamera/pipeline/ipu3/ipu3.cpp               | 9 ++++++---
> > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 9 ++++++---
> > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp           | 9 ++++++---
> > >  src/libcamera/pipeline/simple/simple.cpp           | 9 ++++++---
> > >  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp       | 9 ++++++---
> > >  src/libcamera/pipeline/vimc/vimc.cpp               | 9 ++++++---
> > >  src/libcamera/pipeline_handler.cpp                 | 1 +
> > >  src/qcam/main_window.cpp                           | 2 +-
> > >  test/camera/capture.cpp                            | 2 +-
> > >  test/camera/statemachine.cpp                       | 2 +-
> > >  test/mapped-buffer.cpp                             | 2 +-
> > >  19 files changed, 58 insertions(+), 35 deletions(-)
> > >
> > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > > index 326b14d0ca01..ef6113113b6c 100644
> > > --- a/include/libcamera/camera.h
> > > +++ b/include/libcamera/camera.h
> > > @@ -116,7 +116,8 @@ private:
> > >
> > >         friend class FrameBufferAllocator;
> > >         int exportFrameBuffers(Stream *stream,
> > > -                              std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> > > +                              std::vector<std::unique_ptr<FrameBuffer>> *buffers,
> > > +                              unsigned int count);
> > >  };
> > 
> > In my humble opinion, giving some number a special meaning is not a good design.
> > I would like to ask others' opinions.
> > Alternative (better) design is change the type of count to std::optional.
>
> std::optional isn't an option, as it's a C++17 feature, and the public
> API has to stay compatible with C++14 to support Chromium (the browser,
> not the OS).
>
> How about making the buffer count mandatory, always passing a value
> explicitly ?

Sounds good to me.

Given that this makes the configuration's bufferCount pointless, I should add a
patch removing it in this series, right?

Thanks,
Nícolas