[libcamera-devel] [PATCH v2 2/2] android: Introduce PlatformFrameBufferAllocator

[Jacopo Mondi]

Hi Hiro,

On Thu, Nov 18, 2021 at 05:13:29PM +0900, Hirokazu Honda wrote:
> On Thu, Nov 18, 2021 at 3:02 AM Kieran Bingham
> <kieran.bingham at ideasonboard.com> wrote:
> >
> > Quoting Hirokazu Honda (2021-11-10 03:09:34)
> > > Hi Kieran,
> > >
> > > > In fact, it doesn't matter too much - but I still don't see why this
> > > > needs to be Extensible.
> > > >
> > > > The implementation for PlatformFrameBufferAllocator can have an
> > > > identically named class in both
> > > >         src/android/mm/cros_frame_buffer_allocator.cpp
> > > >         src/android/mm/generic_frame_buffer_allocator.cpp
> > > >
> > > > And the one that gets used will be the one that is compiled in...?
> > > >
> > >
> > > That's a good point. I couldn't find a reason for this.
> > > If this is applicable for PlatformFrameBufferAllocator, I think we can
> > > change CameraBuffer too.
> >
> > Sure, if it simplifies things. The class is not a public API, and
> > doesn't need to be 'extensible' with a private implementation as far as
> > I am aware. In this case, it's just that there are two possible
> > implementations which are selected at compile time anyway.
> >
>
> +Laurent Pinchart , can I implement so?
> Why is CameraBuffer designed so?
>

At the time I designed CameraBuffer using Extensible seemed like a
good idea. I had probably been lazy as there might be more opportune
patterns.

What I was looking for was a mechanism that

- Allows to define a standard and unique interface for the
  CameraManager to interface with
- Had a compile-time selectable implementation
- Allowed to not pollute the shared interface with details of the
  implementation

I evaluated making use of a more canonical class hierarchy, and I
considered:

a) A single .h file that defines the interface, multiple .cpp that
  implement it selected at compile time.

  I considered that Extensible would have left more room for
  implementation-specificities in the Private class interface,
  allowing more flexibility and wouldn't have requried
  implementation-specific details to surface in the interface. This
  mostly apply to the implementation-specific data, which Extensible
  allows to easy model in the Private subclass.

  Different .h is not an option and keeping them in sync with the
  common user would be painful

b) A canonical class hierarchy where the 'right' sub-class would be
  instantiated at run time, but I ditched that as it would have
  required #ifdefs in the code which I would prefer not to.

There surely are better solution, as your implementation shows
details and requirement of a specific implementation are surfacing to
the interface as well, so this can be surely improved with some more
opportune design pattern.

That said, I won't be too much concerned. The pattern is there and is
in use, simply replacing it with a) won't bring much value, so I would
say do whatever is faster for you and if we find a more elegant
solution we'll change both BufferAllocator and CameraBuffer in one
go.

-

> -Hiro
> > --
> > Kieran