<div dir="ltr">Thanks Laurent!<div><br></div><div>Sorry I forgot to send this reply before going on vacation...</div><div><br></div><div>> 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?</div><div>Sure. I named it AndroidFrameBuffer simply because it's under src/android directory. Updated.</div><div><br></div><div>> 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).<br></div><div>I'm actually curious what's the style here, as clang-format and ./utils/checkstyle.py suggest me to use the style I adopted...</div><div>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...?)</div><div><br></div><div>Please check if I miss anything.</div><div><br></div><div>BR,</div><div>Harvey</div><div><br></div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Apr 27, 2022 at 8:11 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 Tue, Apr 26, 2022 at 09:14:07AM +0000, Harvey Yang via libcamera-devel wrote:<br>
> AndroidFrameBuffer is derived from FrameBuffer with access to<br>
> buffer_handle_t, which is needed for JEA usage.<br>
<br>
Given that we have an implement for Chrome OS and a "generic Android"<br>
implementation, would it be better (and more in line with the HAL naming<br>
conventions) to call the class HALFrameBuffer instead of<br>
AndroidFrameBuffer ?<br>
<br>
> Signed-off-by: Harvey Yang <chenghaoyang at <a href="http://chromium.org" rel="noreferrer" target="_blank">chromium.org</a>><br>
> ---<br>
> src/android/android_framebuffer.cpp | 32 +++++++++++++++++++<br>
> src/android/android_framebuffer.h | 28 ++++++++++++++++<br>
> src/android/camera_device.cpp | 3 +-<br>
> src/android/frame_buffer_allocator.h | 7 ++--<br>
> src/android/meson.build | 1 +<br>
> .../mm/cros_frame_buffer_allocator.cpp | 13 +++++---<br>
> .../mm/generic_frame_buffer_allocator.cpp | 11 ++++---<br>
> 7 files changed, 81 insertions(+), 14 deletions(-)<br>
> create mode 100644 src/android/android_framebuffer.cpp<br>
> create mode 100644 src/android/android_framebuffer.h<br>
> <br>
> diff --git a/src/android/android_framebuffer.cpp b/src/android/android_framebuffer.cpp<br>
> new file mode 100644<br>
> index 00000000..1ff7018e<br>
> --- /dev/null<br>
> +++ b/src/android/android_framebuffer.cpp<br>
> @@ -0,0 +1,32 @@<br>
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> +/*<br>
> + * Copyright (C) 2022, Google Inc.<br>
> + *<br>
> + * android_framebuffer.cpp - Android Frame buffer handling<br>
<br>
s/buffer handling/Buffer Handling/<br>
<br>
Same below.<br>
<br>
> + */<br>
> +<br>
> +#include "android_framebuffer.h"<br>
> +<br>
> +#include <hardware/camera3.h><br>
> +<br>
> +AndroidFrameBuffer::AndroidFrameBuffer(<br>
> + buffer_handle_t handle,<br>
> + std::unique_ptr<Private> d,<br>
> + const std::vector<Plane> &planes,<br>
> + unsigned int cookie)<br>
<br>
AndroidFrameBuffer::AndroidFrameBuffer(buffer_handle_t handle,<br>
std::unique_ptr<Private> d,<br>
const std::vector<Plane> &planes,<br>
unsigned int cookie)<br>
<br>
This isn't a rule that is universally enforced when it would result in<br>
really long lines (and I wonder if we wouldn't be better off with the<br>
coding style you've used, but that should then be changed globally).<br>
<br>
> + : FrameBuffer(std::move(d), planes, cookie), handle_(handle)<br>
> +{<br>
> +}<br>
> +<br>
> +AndroidFrameBuffer::AndroidFrameBuffer(<br>
> + buffer_handle_t handle,<br>
> + const std::vector<Plane> &planes,<br>
> + unsigned int cookie)<br>
<br>
Same here.<br>
<br>
> + : FrameBuffer(planes, cookie), handle_(handle)<br>
> +{<br>
> +}<br>
> +<br>
> +buffer_handle_t AndroidFrameBuffer::getHandle() const<br>
> +{<br>
> + return handle_;<br>
> +}<br>
> diff --git a/src/android/android_framebuffer.h b/src/android/android_framebuffer.h<br>
> new file mode 100644<br>
> index 00000000..49df9756<br>
> --- /dev/null<br>
> +++ b/src/android/android_framebuffer.h<br>
> @@ -0,0 +1,28 @@<br>
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> +/*<br>
> + * Copyright (C) 2022, Google Inc.<br>
> + *<br>
> + * android_framebuffer.h - Android Frame buffer handling<br>
> + */<br>
> +<br>
> +#pragma once<br>
> +<br>
> +#include "libcamera/internal/framebuffer.h"<br>
> +<br>
> +#include <hardware/camera3.h><br>
> +<br>
> +class AndroidFrameBuffer final : public libcamera::FrameBuffer<br>
> +{<br>
> +public:<br>
> + AndroidFrameBuffer(<br>
> + buffer_handle_t handle, std::unique_ptr<Private> d,<br>
<br>
Please make the Private pointer the first argument.<br>
<br>
> + const std::vector<Plane> &planes,<br>
> + unsigned int cookie = 0);<br>
<br>
The cookie argument is never used, I'd drop it. Same below.<br>
<br>
Same comment regarding the alignment.<br>
<br>
> + AndroidFrameBuffer(buffer_handle_t handle,<br>
> + const std::vector<Plane> &planes,<br>
> + unsigned int cookie = 0);<br>
<br>
Please add a blank line here.<br>
<br>
> + buffer_handle_t getHandle() const;<br>
<br>
You can inline this function, and rename it to just handle().<br>
<br>
> +<br>
> +private:<br>
> + buffer_handle_t handle_ = nullptr;<br>
<br>
No need for = nullptr, as the two constructors set the handle<br>
explicitly.<br>
<br>
> +};<br>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp<br>
> index 00d48471..643b4dee 100644<br>
> --- a/src/android/camera_device.cpp<br>
> +++ b/src/android/camera_device.cpp<br>
> @@ -25,6 +25,7 @@<br>
> <br>
> #include "system/graphics.h"<br>
> <br>
> +#include "android_framebuffer.h"<br>
> #include "camera_buffer.h"<br>
> #include "camera_hal_config.h"<br>
> #include "camera_ops.h"<br>
> @@ -754,7 +755,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<AndroidFrameBuffer>(camera3buffer, planes);<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..d7b2118e 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 "android_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<AndroidFrameBuffer> 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<AndroidFrameBuffer> \<br>
> PlatformFrameBufferAllocator::allocate(int halPixelFormat, \<br>
> const libcamera::Size &size, \<br>
> uint32_t usage) \<br>
> diff --git a/src/android/meson.build b/src/android/meson.build<br>
> index 75b4bf20..27be27bb 100644<br>
> --- a/src/android/meson.build<br>
> +++ b/src/android/meson.build<br>
> @@ -38,6 +38,7 @@ endif<br>
> android_deps += [libyuv_dep]<br>
> <br>
> android_hal_sources = files([<br>
> + 'android_framebuffer.cpp',<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 52e8c180..163c5d75 100644<br>
> --- a/src/android/mm/cros_frame_buffer_allocator.cpp<br>
> +++ b/src/android/mm/cros_frame_buffer_allocator.cpp<br>
> @@ -14,6 +14,7 @@<br>
> <br>
> #include "libcamera/internal/framebuffer.h"<br>
> <br>
> +#include "../android_framebuffer.h"<br>
> #include "../camera_device.h"<br>
> #include "../frame_buffer_allocator.h"<br>
> #include "cros-camera/camera_buffer_manager.h"<br>
> @@ -47,11 +48,11 @@ public:<br>
> {<br>
> }<br>
> <br>
> - std::unique_ptr<libcamera::FrameBuffer><br>
> + std::unique_ptr<AndroidFrameBuffer><br>
> allocate(int halPixelFormat, const libcamera::Size &size, uint32_t usage);<br>
> };<br>
> <br>
> -std::unique_ptr<libcamera::FrameBuffer><br>
> +std::unique_ptr<AndroidFrameBuffer><br>
> PlatformFrameBufferAllocator::Private::allocate(int halPixelFormat,<br>
> const libcamera::Size &size,<br>
> uint32_t usage)<br>
> @@ -80,9 +81,11 @@ 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)),<br>
> - planes);<br>
> + auto fb = std::make_unique<AndroidFrameBuffer>(handle,<br>
> + std::make_unique<CrosFrameBufferData>(std::move(scopedHandle)),<br>
> + planes);<br>
> +<br>
> + return fb;<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 acb2fa2b..c79b7b10 100644<br>
> --- a/src/android/mm/generic_frame_buffer_allocator.cpp<br>
> +++ b/src/android/mm/generic_frame_buffer_allocator.cpp<br>
> @@ -18,6 +18,7 @@<br>
> #include <hardware/gralloc.h><br>
> #include <hardware/hardware.h><br>
> <br>
> +#include "../android_framebuffer.h"<br>
> #include "../camera_device.h"<br>
> #include "../frame_buffer_allocator.h"<br>
> <br>
> @@ -77,7 +78,7 @@ public:<br>
> <br>
> ~Private() override;<br>
> <br>
> - std::unique_ptr<libcamera::FrameBuffer><br>
> + std::unique_ptr<AndroidFrameBuffer><br>
> allocate(int halPixelFormat, const libcamera::Size &size, uint32_t usage);<br>
> <br>
> private:<br>
> @@ -92,7 +93,7 @@ PlatformFrameBufferAllocator::Private::~Private()<br>
> gralloc_close(allocDevice_);<br>
> }<br>
> <br>
> -std::unique_ptr<libcamera::FrameBuffer><br>
> +std::unique_ptr<AndroidFrameBuffer><br>
> PlatformFrameBufferAllocator::Private::allocate(int halPixelFormat,<br>
> const libcamera::Size &size,<br>
> uint32_t usage)<br>
> @@ -135,9 +136,9 @@ PlatformFrameBufferAllocator::Private::allocate(int halPixelFormat,<br>
> offset += planeSize;<br>
> }<br>
> <br>
> - return std::make_unique<FrameBuffer>(<br>
> - std::make_unique<GenericFrameBufferData>(allocDevice_, handle),<br>
> - planes);<br>
> + return std::make_unique<AndroidFrameBuffer>(handle,<br>
> + std::make_unique<GenericFrameBufferData>(allocDevice_, handle),<br>
> + planes);<br>
> }<br>
> <br>
> PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div>