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

[Hirokazu Honda]

Hi Jacopo,

On Thu, Nov 18, 2021 at 5:43 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> 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.
>

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.

-Hiro
> -
>
> > -Hiro
> > > --
> > > Kieran