[libcamera-devel] [PATCH v3 2/4] Add AndroidFrameBuffer and replace FrameBuffer in src/android
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Sep 1 22:05:46 CEST 2022
Hi Harvey,
On Tue, May 03, 2022 at 11:29:27AM +0800, Cheng-Hao Yang wrote:
> Thanks Laurent!
>
> Sorry I forgot to send this reply before going on vacation...
>
> > Given that we have an implement for Chrome OS and a "generic Android"
> > implementation, would it be better (and more in line with the HAL naming
> > conventions) to call the class HALFrameBuffer instead of AndroidFrameBuffer?
>
> Sure. I named it AndroidFrameBuffer simply because it's under src/android
> directory. Updated.
>
> > This isn't a rule that is universally enforced when it would result in
> > really long lines (and I wonder if we wouldn't be better off with the
> > coding style you've used, but that should then be changed globally).
>
> I'm actually curious what's the style here, as clang-format and
> ./utils/checkstyle.py suggest me to use the style I adopted...
It's hard to get clang-format to do exactly what we want. It's close,
but not a 100% match. checkstyle.py is based on clang-format, so the two
produce the same output :-) (checkstyle.py has a number of other checks
though).
> Updated with the class name changed. Hope that still fits the style you
> suggested. (While I'm not sure how many indents/spaces for the header
> file's c'tor...?)
I'll check in the latest version.
> Please check if I miss anything.
>
> On Wed, Apr 27, 2022 at 8:11 AM Laurent Pinchart wrote:
> > On Tue, Apr 26, 2022 at 09:14:07AM +0000, Harvey Yang via libcamera-devel wrote:
> > > AndroidFrameBuffer is derived from FrameBuffer with access to
> > > buffer_handle_t, which is needed for JEA usage.
> >
> > Given that we have an implement for Chrome OS and a "generic Android"
> > implementation, would it be better (and more in line with the HAL naming
> > conventions) to call the class HALFrameBuffer instead of
> > AndroidFrameBuffer ?
> >
> > > Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> > > ---
> > > src/android/android_framebuffer.cpp | 32 +++++++++++++++++++
> > > src/android/android_framebuffer.h | 28 ++++++++++++++++
> > > src/android/camera_device.cpp | 3 +-
> > > src/android/frame_buffer_allocator.h | 7 ++--
> > > src/android/meson.build | 1 +
> > > .../mm/cros_frame_buffer_allocator.cpp | 13 +++++---
> > > .../mm/generic_frame_buffer_allocator.cpp | 11 ++++---
> > > 7 files changed, 81 insertions(+), 14 deletions(-)
> > > create mode 100644 src/android/android_framebuffer.cpp
> > > create mode 100644 src/android/android_framebuffer.h
> > >
> > > diff --git a/src/android/android_framebuffer.cpp
> > b/src/android/android_framebuffer.cpp
> > > new file mode 100644
> > > index 00000000..1ff7018e
> > > --- /dev/null
> > > +++ b/src/android/android_framebuffer.cpp
> > > @@ -0,0 +1,32 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2022, Google Inc.
> > > + *
> > > + * android_framebuffer.cpp - Android Frame buffer handling
> >
> > s/buffer handling/Buffer Handling/
> >
> > Same below.
> >
> > > + */
> > > +
> > > +#include "android_framebuffer.h"
> > > +
> > > +#include <hardware/camera3.h>
> > > +
> > > +AndroidFrameBuffer::AndroidFrameBuffer(
> > > + buffer_handle_t handle,
> > > + std::unique_ptr<Private> d,
> > > + const std::vector<Plane> &planes,
> > > + unsigned int cookie)
> >
> > AndroidFrameBuffer::AndroidFrameBuffer(buffer_handle_t handle,
> > std::unique_ptr<Private> d,
> > const std::vector<Plane> &planes,
> > unsigned int cookie)
> >
> > This isn't a rule that is universally enforced when it would result in
> > really long lines (and I wonder if we wouldn't be better off with the
> > coding style you've used, but that should then be changed globally).
> >
> > > + : FrameBuffer(std::move(d), planes, cookie), handle_(handle)
> > > +{
> > > +}
> > > +
> > > +AndroidFrameBuffer::AndroidFrameBuffer(
> > > + buffer_handle_t handle,
> > > + const std::vector<Plane> &planes,
> > > + unsigned int cookie)
> >
> > Same here.
> >
> > > + : FrameBuffer(planes, cookie), handle_(handle)
> > > +{
> > > +}
> > > +
> > > +buffer_handle_t AndroidFrameBuffer::getHandle() const
> > > +{
> > > + return handle_;
> > > +}
> > > diff --git a/src/android/android_framebuffer.h
> > b/src/android/android_framebuffer.h
> > > new file mode 100644
> > > index 00000000..49df9756
> > > --- /dev/null
> > > +++ b/src/android/android_framebuffer.h
> > > @@ -0,0 +1,28 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2022, Google Inc.
> > > + *
> > > + * android_framebuffer.h - Android Frame buffer handling
> > > + */
> > > +
> > > +#pragma once
> > > +
> > > +#include "libcamera/internal/framebuffer.h"
> > > +
> > > +#include <hardware/camera3.h>
> > > +
> > > +class AndroidFrameBuffer final : public libcamera::FrameBuffer
> > > +{
> > > +public:
> > > + AndroidFrameBuffer(
> > > + buffer_handle_t handle, std::unique_ptr<Private> d,
> >
> > Please make the Private pointer the first argument.
> >
> > > + const std::vector<Plane> &planes,
> > > + unsigned int cookie = 0);
> >
> > The cookie argument is never used, I'd drop it. Same below.
> >
> > Same comment regarding the alignment.
> >
> > > + AndroidFrameBuffer(buffer_handle_t handle,
> > > + const std::vector<Plane> &planes,
> > > + unsigned int cookie = 0);
> >
> > Please add a blank line here.
> >
> > > + buffer_handle_t getHandle() const;
> >
> > You can inline this function, and rename it to just handle().
> >
> > > +
> > > +private:
> > > + buffer_handle_t handle_ = nullptr;
> >
> > No need for = nullptr, as the two constructors set the handle
> > explicitly.
> >
> > > +};
> > > diff --git a/src/android/camera_device.cpp
> > b/src/android/camera_device.cpp
> > > index 00d48471..643b4dee 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -25,6 +25,7 @@
> > >
> > > #include "system/graphics.h"
> > >
> > > +#include "android_framebuffer.h"
> > > #include "camera_buffer.h"
> > > #include "camera_hal_config.h"
> > > #include "camera_ops.h"
> > > @@ -754,7 +755,7 @@ CameraDevice::createFrameBuffer(const
> > buffer_handle_t camera3buffer,
> > > planes[i].length = buf.size(i);
> > > }
> > >
> > > - return std::make_unique<FrameBuffer>(planes);
> > > + return std::make_unique<AndroidFrameBuffer>(camera3buffer, planes);
> > > }
> > >
> > > int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)
> > > diff --git a/src/android/frame_buffer_allocator.h
> > b/src/android/frame_buffer_allocator.h
> > > index 5d2eeda1..d7b2118e 100644
> > > --- a/src/android/frame_buffer_allocator.h
> > > +++ b/src/android/frame_buffer_allocator.h
> > > @@ -13,9 +13,10 @@
> > > #include <libcamera/base/class.h>
> > >
> > > #include <libcamera/camera.h>
> > > -#include <libcamera/framebuffer.h>
> > > #include <libcamera/geometry.h>
> > >
> > > +#include "android_framebuffer.h"
> > > +
> > > class CameraDevice;
> > >
> > > class PlatformFrameBufferAllocator : libcamera::Extensible
> > > @@ -31,7 +32,7 @@ public:
> > > * Note: The returned FrameBuffer needs to be destroyed before
> > > * PlatformFrameBufferAllocator is destroyed.
> > > */
> > > - std::unique_ptr<libcamera::FrameBuffer> allocate(
> > > + std::unique_ptr<AndroidFrameBuffer> allocate(
> > > int halPixelFormat, const libcamera::Size &size, uint32_t
> > usage);
> > > };
> > >
> > > @@ -44,7 +45,7 @@
> > PlatformFrameBufferAllocator::PlatformFrameBufferAllocator( \
> > > PlatformFrameBufferAllocator::~PlatformFrameBufferAllocator()
> > \
> > > { \
> > > } \
> > > -std::unique_ptr<libcamera::FrameBuffer>
> > \
> > > +std::unique_ptr<AndroidFrameBuffer>
> > \
> > > PlatformFrameBufferAllocator::allocate(int halPixelFormat, \
> > > const libcamera::Size &size, \
> > > uint32_t usage) \
> > > diff --git a/src/android/meson.build b/src/android/meson.build
> > > index 75b4bf20..27be27bb 100644
> > > --- a/src/android/meson.build
> > > +++ b/src/android/meson.build
> > > @@ -38,6 +38,7 @@ endif
> > > android_deps += [libyuv_dep]
> > >
> > > android_hal_sources = files([
> > > + 'android_framebuffer.cpp',
> > > 'camera3_hal.cpp',
> > > 'camera_capabilities.cpp',
> > > 'camera_device.cpp',
> > > diff --git a/src/android/mm/cros_frame_buffer_allocator.cpp
> > b/src/android/mm/cros_frame_buffer_allocator.cpp
> > > index 52e8c180..163c5d75 100644
> > > --- a/src/android/mm/cros_frame_buffer_allocator.cpp
> > > +++ b/src/android/mm/cros_frame_buffer_allocator.cpp
> > > @@ -14,6 +14,7 @@
> > >
> > > #include "libcamera/internal/framebuffer.h"
> > >
> > > +#include "../android_framebuffer.h"
> > > #include "../camera_device.h"
> > > #include "../frame_buffer_allocator.h"
> > > #include "cros-camera/camera_buffer_manager.h"
> > > @@ -47,11 +48,11 @@ public:
> > > {
> > > }
> > >
> > > - std::unique_ptr<libcamera::FrameBuffer>
> > > + std::unique_ptr<AndroidFrameBuffer>
> > > allocate(int halPixelFormat, const libcamera::Size &size, uint32_t
> > usage);
> > > };
> > >
> > > -std::unique_ptr<libcamera::FrameBuffer>
> > > +std::unique_ptr<AndroidFrameBuffer>
> > > PlatformFrameBufferAllocator::Private::allocate(int halPixelFormat,
> > > const libcamera::Size
> > &size,
> > > uint32_t usage)
> > > @@ -80,9 +81,11 @@ PlatformFrameBufferAllocator::Private::allocate(int
> > halPixelFormat,
> > > plane.length =
> > cros::CameraBufferManager::GetPlaneSize(handle, i);
> > > }
> > >
> > > - return std::make_unique<FrameBuffer>(
> > > -
> > std::make_unique<CrosFrameBufferData>(std::move(scopedHandle)),
> > > - planes);
> > > + auto fb = std::make_unique<AndroidFrameBuffer>(handle,
> > > +
> > std::make_unique<CrosFrameBufferData>(std::move(scopedHandle)),
> > > + planes);
> > > +
> > > + return fb;
> > > }
> > >
> > > PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION
> > > diff --git a/src/android/mm/generic_frame_buffer_allocator.cpp
> > b/src/android/mm/generic_frame_buffer_allocator.cpp
> > > index acb2fa2b..c79b7b10 100644
> > > --- a/src/android/mm/generic_frame_buffer_allocator.cpp
> > > +++ b/src/android/mm/generic_frame_buffer_allocator.cpp
> > > @@ -18,6 +18,7 @@
> > > #include <hardware/gralloc.h>
> > > #include <hardware/hardware.h>
> > >
> > > +#include "../android_framebuffer.h"
> > > #include "../camera_device.h"
> > > #include "../frame_buffer_allocator.h"
> > >
> > > @@ -77,7 +78,7 @@ public:
> > >
> > > ~Private() override;
> > >
> > > - std::unique_ptr<libcamera::FrameBuffer>
> > > + std::unique_ptr<AndroidFrameBuffer>
> > > allocate(int halPixelFormat, const libcamera::Size &size, uint32_t
> > usage);
> > >
> > > private:
> > > @@ -92,7 +93,7 @@ PlatformFrameBufferAllocator::Private::~Private()
> > > gralloc_close(allocDevice_);
> > > }
> > >
> > > -std::unique_ptr<libcamera::FrameBuffer>
> > > +std::unique_ptr<AndroidFrameBuffer>
> > > PlatformFrameBufferAllocator::Private::allocate(int halPixelFormat,
> > > const libcamera::Size
> > &size,
> > > uint32_t usage)
> > > @@ -135,9 +136,9 @@ PlatformFrameBufferAllocator::Private::allocate(int
> > halPixelFormat,
> > > offset += planeSize;
> > > }
> > >
> > > - return std::make_unique<FrameBuffer>(
> > > - std::make_unique<GenericFrameBufferData>(allocDevice_,
> > handle),
> > > - planes);
> > > + return std::make_unique<AndroidFrameBuffer>(handle,
> > > +
> > std::make_unique<GenericFrameBufferData>(allocDevice_, handle),
> > > + planes);
> > > }
> > >
> > > PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list