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

[Laurent Pinchart]

Hello,

On Thu, Nov 18, 2021 at 05:50:15PM +0900, Hirokazu Honda wrote:
> On Thu, Nov 18, 2021 at 5:43 PM Jacopo Mondi wrote:
> > On Thu, Nov 18, 2021 at 05:13:29PM +0900, Hirokazu Honda wrote:
> > > On Thu, Nov 18, 2021 at 3:02 AM Kieran Bingham 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.

Usage of the Extensible class for CameraBuffer was a bit of a hack I
think. In particular, I'm not fond of the
PUBLIC_CAMERA_BUFFER_IMPLEMENTATION macro. I haven't taken the time to
try and find a better solution.

> > 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.
> 
> Ah, it is a good point that with Extensible we can have the platform
> dependent class like allocDevice_ in the Extensible class.
> So I would go along with Extensible.

-- 
Regards,

Laurent Pinchart