<div dir="ltr">Thanks for the review!<br><div><br></div><div>Updated the above in the new patch.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Dec 7, 2022 at 11:26 AM Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Harvey,<br>
<br>
Thank you for the patch.<br>
<br>
On Thu, Dec 01, 2022 at 09:27:29AM +0000, Harvey Yang via libcamera-devel wrote:<br>
> From: Harvey Yang <<a href="mailto:chenghaoyang@chromium.org" target="_blank">chenghaoyang@chromium.org</a>><br>
> <br>
> AndroidFrameBuffer is derived from FrameBuffer with access to<br>
<br>
s/AndroidFrameBuffer/HALFrameBuffer/<br>
<br>
> buffer_handle_t, which is needed for JEA usage.<br>
> <br>
> Signed-off-by: Harvey Yang <<a href="mailto:chenghaoyang@chromium.org" target="_blank">chenghaoyang@chromium.org</a>><br>
> ---<br>
> src/android/camera_device.cpp | 3 ++-<br>
> src/android/frame_buffer_allocator.h | 7 ++---<br>
> src/android/hal_framebuffer.cpp | 22 ++++++++++++++++<br>
> src/android/hal_framebuffer.h | 26 +++++++++++++++++++<br>
> src/android/meson.build | 1 +<br>
> .../mm/cros_frame_buffer_allocator.cpp | 9 ++++---<br>
> .../mm/generic_frame_buffer_allocator.cpp | 11 +++++---<br>
> 7 files changed, 67 insertions(+), 12 deletions(-)<br>
> create mode 100644 src/android/hal_framebuffer.cpp<br>
> create mode 100644 src/android/hal_framebuffer.h<br>
> <br>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp<br>
> index b20e389b..872161ba 100644<br>
> --- a/src/android/camera_device.cpp<br>
> +++ b/src/android/camera_device.cpp<br>
> @@ -30,6 +30,7 @@<br>
> #include "camera_hal_config.h"<br>
> #include "camera_ops.h"<br>
> #include "camera_request.h"<br>
> +#include "hal_framebuffer.h"<br>
> <br>
> using namespace libcamera;<br>
> <br>
> @@ -794,7 +795,7 @@ CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer,<br>
> planes[i].length = buf.size(i);<br>
> }<br>
> <br>
> - return std::make_unique<FrameBuffer>(planes);<br>
> + return std::make_unique<HALFrameBuffer>(planes, camera3buffer);<br>
> }<br>
> <br>
> int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)<br>
> diff --git a/src/android/frame_buffer_allocator.h b/src/android/frame_buffer_allocator.h<br>
> index 5d2eeda1..e5c94922 100644<br>
> --- a/src/android/frame_buffer_allocator.h<br>
> +++ b/src/android/frame_buffer_allocator.h<br>
> @@ -13,9 +13,10 @@<br>
> #include <libcamera/base/class.h><br>
> <br>
> #include <libcamera/camera.h><br>
> -#include <libcamera/framebuffer.h><br>
> #include <libcamera/geometry.h><br>
> <br>
> +#include "hal_framebuffer.h"<br>
> +<br>
> class CameraDevice;<br>
> <br>
> class PlatformFrameBufferAllocator : libcamera::Extensible<br>
> @@ -31,7 +32,7 @@ public:<br>
> * Note: The returned FrameBuffer needs to be destroyed before<br>
> * PlatformFrameBufferAllocator is destroyed.<br>
> */<br>
> - std::unique_ptr<libcamera::FrameBuffer> allocate(<br>
> + std::unique_ptr<HALFrameBuffer> allocate(<br>
> int halPixelFormat, const libcamera::Size &size, uint32_t usage);<br>
> };<br>
> <br>
> @@ -44,7 +45,7 @@ PlatformFrameBufferAllocator::PlatformFrameBufferAllocator( \<br>
> PlatformFrameBufferAllocator::~PlatformFrameBufferAllocator() \<br>
> { \<br>
> } \<br>
> -std::unique_ptr<libcamera::FrameBuffer> \<br>
> +std::unique_ptr<HALFrameBuffer> \<br>
> PlatformFrameBufferAllocator::allocate(int halPixelFormat, \<br>
> const libcamera::Size &size, \<br>
> uint32_t usage) \<br>
> diff --git a/src/android/hal_framebuffer.cpp b/src/android/hal_framebuffer.cpp<br>
> new file mode 100644<br>
> index 00000000..4ac5a820<br>
> --- /dev/null<br>
> +++ b/src/android/hal_framebuffer.cpp<br>
> @@ -0,0 +1,22 @@<br>
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> +/*<br>
> + * Copyright (C) 2022, Google Inc.<br>
> + *<br>
> + * hal_framebuffer.cpp - Android Frame Buffer Handling<br>
<br>
s/Android/HAL/<br>
<br>
> + */<br>
> +<br>
> +#include "hal_framebuffer.h"<br>
> +<br>
> +#include <hardware/camera3.h><br>
> +<br>
> +HALFrameBuffer::HALFrameBuffer(std::unique_ptr<Private> d,<br>
> + buffer_handle_t handle)<br>
> + : FrameBuffer(std::move(d)), handle_(handle)<br>
> +{<br>
> +}<br>
> +<br>
> +HALFrameBuffer::HALFrameBuffer(const std::vector<Plane> &planes,<br>
> + buffer_handle_t handle)<br>
> + : FrameBuffer(planes), handle_(handle)<br>
> +{<br>
> +}<br>
> diff --git a/src/android/hal_framebuffer.h b/src/android/hal_framebuffer.h<br>
> new file mode 100644<br>
> index 00000000..ec737e70<br>
> --- /dev/null<br>
> +++ b/src/android/hal_framebuffer.h<br>
> @@ -0,0 +1,26 @@<br>
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> +/*<br>
> + * Copyright (C) 2022, Google Inc.<br>
> + *<br>
> + * hal_framebuffer.h - Android Frame Buffer Handling<br>
<br>
s/Android/HAL/<br>
<br>
> + */<br>
> +<br>
> +#pragma once<br>
> +<br>
> +#include "libcamera/internal/framebuffer.h"<br>
> +<br>
> +#include <hardware/camera3.h><br>
> +<br>
> +class HALFrameBuffer final : public libcamera::FrameBuffer<br>
> +{<br>
> +public:<br>
> + HALFrameBuffer(std::unique_ptr<Private> d,<br>
> + buffer_handle_t handle);<br>
> + HALFrameBuffer(const std::vector<Plane> &planes,<br>
> + buffer_handle_t handle);<br>
> +<br>
> + buffer_handle_t handle() const { return handle_; }<br>
> +<br>
> +private:<br>
> + buffer_handle_t handle_;<br>
> +};<br>
> diff --git a/src/android/meson.build b/src/android/meson.build<br>
> index 1bba54de..c2773f9e 100644<br>
> --- a/src/android/meson.build<br>
> +++ b/src/android/meson.build<br>
> @@ -37,6 +37,7 @@ endif<br>
> android_deps += [libyuv_dep]<br>
> <br>
> android_hal_sources = files([<br>
> + 'hal_framebuffer.cpp',<br>
<br>
Alphabetical order please.<br>
<br>
Reviewed-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
<br>
> 'camera3_hal.cpp',<br>
> 'camera_capabilities.cpp',<br>
> 'camera_device.cpp',<br>
> diff --git a/src/android/mm/cros_frame_buffer_allocator.cpp b/src/android/mm/cros_frame_buffer_allocator.cpp<br>
> index 0665c77b..0a5c59f2 100644<br>
> --- a/src/android/mm/cros_frame_buffer_allocator.cpp<br>
> +++ b/src/android/mm/cros_frame_buffer_allocator.cpp<br>
> @@ -16,6 +16,7 @@<br>
> <br>
> #include "../camera_device.h"<br>
> #include "../frame_buffer_allocator.h"<br>
> +#include "../hal_framebuffer.h"<br>
> #include "cros-camera/camera_buffer_manager.h"<br>
> <br>
> using namespace libcamera;<br>
> @@ -48,11 +49,11 @@ public:<br>
> {<br>
> }<br>
> <br>
> - std::unique_ptr<libcamera::FrameBuffer><br>
> + std::unique_ptr<HALFrameBuffer><br>
> allocate(int halPixelFormat, const libcamera::Size &size, uint32_t usage);<br>
> };<br>
> <br>
> -std::unique_ptr<libcamera::FrameBuffer><br>
> +std::unique_ptr<HALFrameBuffer><br>
> PlatformFrameBufferAllocator::Private::allocate(int halPixelFormat,<br>
> const libcamera::Size &size,<br>
> uint32_t usage)<br>
> @@ -81,8 +82,8 @@ PlatformFrameBufferAllocator::Private::allocate(int halPixelFormat,<br>
> plane.length = cros::CameraBufferManager::GetPlaneSize(handle, i);<br>
> }<br>
> <br>
> - return std::make_unique<FrameBuffer>(<br>
> - std::make_unique<CrosFrameBufferData>(std::move(scopedHandle), planes));<br>
> + return std::make_unique<HALFrameBuffer>(<br>
> + std::make_unique<CrosFrameBufferData>(std::move(scopedHandle), planes), handle);<br>
> }<br>
> <br>
> PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION<br>
> diff --git a/src/android/mm/generic_frame_buffer_allocator.cpp b/src/android/mm/generic_frame_buffer_allocator.cpp<br>
> index 956623df..3750e1bf 100644<br>
> --- a/src/android/mm/generic_frame_buffer_allocator.cpp<br>
> +++ b/src/android/mm/generic_frame_buffer_allocator.cpp<br>
> @@ -20,6 +20,7 @@<br>
> <br>
> #include "../camera_device.h"<br>
> #include "../frame_buffer_allocator.h"<br>
> +#include "../hal_framebuffer.h"<br>
> <br>
> using namespace libcamera;<br>
> <br>
> @@ -79,7 +80,7 @@ public:<br>
> <br>
> ~Private() override;<br>
> <br>
> - std::unique_ptr<libcamera::FrameBuffer><br>
> + std::unique_ptr<HALFrameBuffer><br>
> allocate(int halPixelFormat, const libcamera::Size &size, uint32_t usage);<br>
> <br>
> private:<br>
> @@ -94,7 +95,7 @@ PlatformFrameBufferAllocator::Private::~Private()<br>
> gralloc_close(allocDevice_);<br>
> }<br>
> <br>
> -std::unique_ptr<libcamera::FrameBuffer><br>
> +std::unique_ptr<HALFrameBuffer><br>
> PlatformFrameBufferAllocator::Private::allocate(int halPixelFormat,<br>
> const libcamera::Size &size,<br>
> uint32_t usage)<br>
> @@ -137,8 +138,10 @@ PlatformFrameBufferAllocator::Private::allocate(int halPixelFormat,<br>
> offset += planeSize;<br>
> }<br>
> <br>
> - return std::make_unique<FrameBuffer>(<br>
> - std::make_unique<GenericFrameBufferData>(allocDevice_, handle, planes));<br>
> + return std::make_unique<HALFrameBuffer>(<br>
> + std::make_unique<GenericFrameBufferData>(<br>
> + allocDevice_, handle, planes),<br>
> + handle);<br>
> }<br>
> <br>
> PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div>