| Message ID | 20220426091409.1352047-3-chenghaoyang@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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
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@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 >
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
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 + */ + +#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) + : FrameBuffer(std::move(d), planes, cookie), handle_(handle) +{ +} + +AndroidFrameBuffer::AndroidFrameBuffer( + buffer_handle_t handle, + const std::vector<Plane> &planes, + unsigned int cookie) + : 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, + const std::vector<Plane> &planes, + unsigned int cookie = 0); + AndroidFrameBuffer(buffer_handle_t handle, + const std::vector<Plane> &planes, + unsigned int cookie = 0); + buffer_handle_t getHandle() const; + +private: + buffer_handle_t handle_ = nullptr; +}; 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
AndroidFrameBuffer is derived from FrameBuffer with access to buffer_handle_t, which is needed for JEA usage. 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