[libcamera-devel] [PATCH v3 2/4] Add AndroidFrameBuffer and replace FrameBuffer in src/android
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Apr 27 02:11:09 CEST 2022
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
More information about the libcamera-devel
mailing list