[libcamera-devel] [PATCH v3 2/4] Add AndroidFrameBuffer and replace FrameBuffer in src/android
Cheng-Hao Yang
chenghaoyang at chromium.org
Tue May 3 05:29:27 CEST 2022
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...
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...?)
Please check if I miss anything.
BR,
Harvey
On Wed, Apr 27, 2022 at 8:11 AM Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> Hi Harvey,
>
> Thank you for the patch.
>
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220503/0d02c55e/attachment-0001.htm>
More information about the libcamera-devel
mailing list