| Message ID | 20221214093330.3345421-1-chenghaoyang@google.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Harvey,
Could you add the post-commit checkstyle hooks so you get notified about
checkstyle issues please?
cp utils/hooks/post-commit .git/hooks/
I think the PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION indendation
we'd ignore, but the others in "Add JEA implementation" look appropriate
to fix.
--
Kieran
-------------------------------------------------------------------------
01b369fa09fb79aaca3c9a5e915e3652ae7d32df Allow inheritance of FrameBuffer
-------------------------------------------------------------------------
No issue detected
--------------------------------------------------------------------------------------------------
3458f84f864921cb45ca8e3a0d7e9e347a3b59a7 Add HALFrameBuffer and replace FrameBuffer in src/android
--------------------------------------------------------------------------------------------------
--- src/android/frame_buffer_allocator.h
+++ src/android/frame_buffer_allocator.h
@@ -36,21 +36,21 @@
int halPixelFormat, const libcamera::Size &size, uint32_t usage);
};
-#define PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION \
-PlatformFrameBufferAllocator::PlatformFrameBufferAllocator( \
- CameraDevice *const cameraDevice) \
- : Extensible(std::make_unique<Private>(cameraDevice)) \
-{ \
-} \
-PlatformFrameBufferAllocator::~PlatformFrameBufferAllocator() \
-{ \
-} \
-std::unique_ptr<HALFrameBuffer> \
-PlatformFrameBufferAllocator::allocate(int halPixelFormat, \
- const libcamera::Size &size, \
- uint32_t usage) \
-{ \
- return _d()->allocate(halPixelFormat, size, usage); \
-}
+#define PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION \
+ PlatformFrameBufferAllocator::PlatformFrameBufferAllocator( \
+ CameraDevice *const cameraDevice) \
+ : Extensible(std::make_unique<Private>(cameraDevice)) \
+ { \
+ } \
+ PlatformFrameBufferAllocator::~PlatformFrameBufferAllocator() \
+ { \
+ } \
+ std::unique_ptr<HALFrameBuffer> \
+ PlatformFrameBufferAllocator::allocate(int halPixelFormat, \
+ const libcamera::Size &size, \
+ uint32_t usage) \
+ { \
+ return _d()->allocate(halPixelFormat, size, usage); \
+ }
#endif /* __ANDROID_FRAME_BUFFER_ALLOCATOR_H__ */
---
1 potential issue detected, please review
----------------------------------------------------------------------------
214944ac387b6f22e30e16ea7950a4f40899047b Add meson.build in src/android/jpeg
----------------------------------------------------------------------------
No issue detected
----------------------------------------------------------------------------------------
2ed81d92f699b2c62c61e808066520f544fa5e40 Add an internal Encoder class in EncoderLibJpeg
----------------------------------------------------------------------------------------
--- src/android/jpeg/encoder_libjpeg.cpp
+++ src/android/jpeg/encoder_libjpeg.cpp
@@ -201,8 +201,8 @@
}
int EncoderLibJpeg::encode(const std::vector<Span<uint8_t>> &src,
- Span<uint8_t> dest, Span<const uint8_t> exifData,
- unsigned int quality)
+ Span<uint8_t> dest, Span<const uint8_t> exifData,
+ unsigned int quality)
{
return encoder_.encode(src, std::move(dest), std::move(exifData), quality);
}
---
1 potential issue detected, please review
-------------------------------------------------------------------------------------------------
f115685e544741d3084a603794328c4beb8f608c Move generateThumbnail from PostProcessorJpeg to Encoder
-------------------------------------------------------------------------------------------------
No issue detected
------------------------------------------------------------------------------
3113d70138d5204fd956ae1e0fc496a106a3e96f Pass StreamBuffer to Encoder::encoder
------------------------------------------------------------------------------
No issue detected
---------------------------------------------------------------
72e0597c7e3af7ac9e80ee2db9b7c212ab1172d8 Add JEA implementation
---------------------------------------------------------------
--- src/android/jpeg/encoder_jea.cpp
+++ src/android/jpeg/encoder_jea.cpp
@@ -56,9 +56,9 @@
}
void EncoderJea::generateThumbnail(const libcamera::FrameBuffer &source,
- const libcamera::Size &targetSize,
- unsigned int quality,
- std::vector<unsigned char> *thumbnail)
+ const libcamera::Size &targetSize,
+ unsigned int quality,
+ std::vector<unsigned char> *thumbnail)
{
if (!jpegCompressor_)
return;
@@ -71,11 +71,11 @@
// JEA needs consecutive memory.
unsigned long size = 0, index = 0;
- for (const auto& plane : frame.planes())
+ for (const auto &plane : frame.planes())
size += plane.size();
std::vector<uint8_t> data(size);
- for (const auto& plane : frame.planes()) {
+ for (const auto &plane : frame.planes()) {
memcpy(&data[index], plane.data(), plane.size());
index += plane.size();
}
---
2 potential issues detected, please review
Quoting Harvey Yang via libcamera-devel (2022-12-14 09:33:23)
> Hi all,
>
> Updated based on Laurent's comments, and fixed a memory flaky
> issue in JEA's generateThumbnail function.
>
> Sorry that the issue took me so long, with other issues in CrOS camera
> service mixed together :p
>
> BR,
> Harvey
>
> Harvey Yang (7):
> Allow inheritance of FrameBuffer
> Add HALFrameBuffer and replace FrameBuffer in src/android
> Add meson.build in src/android/jpeg
> Add an internal Encoder class in EncoderLibJpeg
> Move generateThumbnail from PostProcessorJpeg to Encoder
> Pass StreamBuffer to Encoder::encoder
> Add JEA implementation
>
> include/libcamera/framebuffer.h | 3 +-
> src/android/camera_device.cpp | 5 +-
> src/android/camera_device.h | 3 +-
> src/android/camera_request.h | 3 +-
> src/android/cros/camera3_hal.cpp | 4 +-
> src/android/cros_mojo_token.h | 12 ++
> src/android/frame_buffer_allocator.h | 7 +-
> src/android/hal_framebuffer.cpp | 22 ++++
> src/android/hal_framebuffer.h | 26 +++++
> src/android/jpeg/encoder.h | 9 +-
> src/android/jpeg/encoder_jea.cpp | 105 ++++++++++++++++++
> src/android/jpeg/encoder_jea.h | 35 ++++++
> src/android/jpeg/encoder_libjpeg.cpp | 85 ++++++++++++--
> src/android/jpeg/encoder_libjpeg.h | 46 +++++---
> src/android/jpeg/meson.build | 17 +++
> src/android/jpeg/post_processor_jpeg.cpp | 63 ++---------
> src/android/jpeg/post_processor_jpeg.h | 11 +-
> src/android/meson.build | 6 +-
> .../mm/cros_frame_buffer_allocator.cpp | 9 +-
> .../mm/generic_frame_buffer_allocator.cpp | 11 +-
> 20 files changed, 373 insertions(+), 109 deletions(-)
> create mode 100644 src/android/cros_mojo_token.h
> create mode 100644 src/android/hal_framebuffer.cpp
> create mode 100644 src/android/hal_framebuffer.h
> create mode 100644 src/android/jpeg/encoder_jea.cpp
> create mode 100644 src/android/jpeg/encoder_jea.h
> create mode 100644 src/android/jpeg/meson.build
>
> --
> 2.39.0.rc1.256.g54fd8350bd-goog
>
Right. Thanks Kieran for catching that! Added the post-commit script. Will try to prevent it from happening again. I'll wait a bit for other comments/reviews before sending another version of patches, to avoid the spamming. BR, Harvey On Wed, Dec 14, 2022 at 7:10 PM Kieran Bingham < kieran.bingham@ideasonboard.com> wrote: > Hi Harvey, > > Could you add the post-commit checkstyle hooks so you get notified about > checkstyle issues please? > > cp utils/hooks/post-commit .git/hooks/ > > > I think the PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION indendation > we'd ignore, but the others in "Add JEA implementation" look appropriate > to fix. > > -- > Kieran > > > > ------------------------------------------------------------------------- > 01b369fa09fb79aaca3c9a5e915e3652ae7d32df Allow inheritance of FrameBuffer > ------------------------------------------------------------------------- > No issue detected > > > -------------------------------------------------------------------------------------------------- > 3458f84f864921cb45ca8e3a0d7e9e347a3b59a7 Add HALFrameBuffer and replace > FrameBuffer in src/android > > -------------------------------------------------------------------------------------------------- > --- src/android/frame_buffer_allocator.h > +++ src/android/frame_buffer_allocator.h > @@ -36,21 +36,21 @@ > int halPixelFormat, const libcamera::Size &size, uint32_t > usage); > }; > > -#define PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION \ > -PlatformFrameBufferAllocator::PlatformFrameBufferAllocator( \ > - CameraDevice *const cameraDevice) \ > - : Extensible(std::make_unique<Private>(cameraDevice)) \ > -{ \ > -} \ > -PlatformFrameBufferAllocator::~PlatformFrameBufferAllocator() \ > -{ \ > -} \ > -std::unique_ptr<HALFrameBuffer> \ > -PlatformFrameBufferAllocator::allocate(int halPixelFormat, \ > - const libcamera::Size &size, \ > - uint32_t usage) \ > -{ \ > - return _d()->allocate(halPixelFormat, size, usage); \ > -} > +#define PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION > \ > + PlatformFrameBufferAllocator::PlatformFrameBufferAllocator( > \ > + CameraDevice *const cameraDevice) > \ > + : Extensible(std::make_unique<Private>(cameraDevice)) > \ > + { > \ > + } > \ > + PlatformFrameBufferAllocator::~PlatformFrameBufferAllocator() > \ > + { > \ > + } > \ > + std::unique_ptr<HALFrameBuffer> > \ > + PlatformFrameBufferAllocator::allocate(int halPixelFormat, > \ > + const libcamera::Size > &size, \ > + uint32_t usage) > \ > + { > \ > + return _d()->allocate(halPixelFormat, size, usage); > \ > + } > > #endif /* __ANDROID_FRAME_BUFFER_ALLOCATOR_H__ */ > --- > 1 potential issue detected, please review > > > ---------------------------------------------------------------------------- > 214944ac387b6f22e30e16ea7950a4f40899047b Add meson.build in > src/android/jpeg > > ---------------------------------------------------------------------------- > No issue detected > > > ---------------------------------------------------------------------------------------- > 2ed81d92f699b2c62c61e808066520f544fa5e40 Add an internal Encoder class in > EncoderLibJpeg > > ---------------------------------------------------------------------------------------- > --- src/android/jpeg/encoder_libjpeg.cpp > +++ src/android/jpeg/encoder_libjpeg.cpp > @@ -201,8 +201,8 @@ > } > > int EncoderLibJpeg::encode(const std::vector<Span<uint8_t>> &src, > - Span<uint8_t> dest, Span<const > uint8_t> exifData, > - unsigned int quality) > + Span<uint8_t> dest, Span<const uint8_t> > exifData, > + unsigned int quality) > { > return encoder_.encode(src, std::move(dest), std::move(exifData), > quality); > } > --- > 1 potential issue detected, please review > > > ------------------------------------------------------------------------------------------------- > f115685e544741d3084a603794328c4beb8f608c Move generateThumbnail from > PostProcessorJpeg to Encoder > > ------------------------------------------------------------------------------------------------- > No issue detected > > > ------------------------------------------------------------------------------ > 3113d70138d5204fd956ae1e0fc496a106a3e96f Pass StreamBuffer to > Encoder::encoder > > ------------------------------------------------------------------------------ > No issue detected > > --------------------------------------------------------------- > 72e0597c7e3af7ac9e80ee2db9b7c212ab1172d8 Add JEA implementation > --------------------------------------------------------------- > --- src/android/jpeg/encoder_jea.cpp > +++ src/android/jpeg/encoder_jea.cpp > @@ -56,9 +56,9 @@ > } > > void EncoderJea::generateThumbnail(const libcamera::FrameBuffer &source, > - const libcamera::Size &targetSize, > - unsigned int quality, > - std::vector<unsigned char> *thumbnail) > + const libcamera::Size &targetSize, > + unsigned int quality, > + std::vector<unsigned char> *thumbnail) > { > if (!jpegCompressor_) > return; > @@ -71,11 +71,11 @@ > > // JEA needs consecutive memory. > unsigned long size = 0, index = 0; > - for (const auto& plane : frame.planes()) > + for (const auto &plane : frame.planes()) > size += plane.size(); > > std::vector<uint8_t> data(size); > - for (const auto& plane : frame.planes()) { > + for (const auto &plane : frame.planes()) { > memcpy(&data[index], plane.data(), plane.size()); > index += plane.size(); > } > --- > 2 potential issues detected, please review > > > > > Quoting Harvey Yang via libcamera-devel (2022-12-14 09:33:23) > > Hi all, > > > > Updated based on Laurent's comments, and fixed a memory flaky > > issue in JEA's generateThumbnail function. > > > > Sorry that the issue took me so long, with other issues in CrOS camera > > service mixed together :p > > > > BR, > > Harvey > > > > Harvey Yang (7): > > Allow inheritance of FrameBuffer > > Add HALFrameBuffer and replace FrameBuffer in src/android > > Add meson.build in src/android/jpeg > > Add an internal Encoder class in EncoderLibJpeg > > Move generateThumbnail from PostProcessorJpeg to Encoder > > Pass StreamBuffer to Encoder::encoder > > Add JEA implementation > > > > include/libcamera/framebuffer.h | 3 +- > > src/android/camera_device.cpp | 5 +- > > src/android/camera_device.h | 3 +- > > src/android/camera_request.h | 3 +- > > src/android/cros/camera3_hal.cpp | 4 +- > > src/android/cros_mojo_token.h | 12 ++ > > src/android/frame_buffer_allocator.h | 7 +- > > src/android/hal_framebuffer.cpp | 22 ++++ > > src/android/hal_framebuffer.h | 26 +++++ > > src/android/jpeg/encoder.h | 9 +- > > src/android/jpeg/encoder_jea.cpp | 105 ++++++++++++++++++ > > src/android/jpeg/encoder_jea.h | 35 ++++++ > > src/android/jpeg/encoder_libjpeg.cpp | 85 ++++++++++++-- > > src/android/jpeg/encoder_libjpeg.h | 46 +++++--- > > src/android/jpeg/meson.build | 17 +++ > > src/android/jpeg/post_processor_jpeg.cpp | 63 ++--------- > > src/android/jpeg/post_processor_jpeg.h | 11 +- > > src/android/meson.build | 6 +- > > .../mm/cros_frame_buffer_allocator.cpp | 9 +- > > .../mm/generic_frame_buffer_allocator.cpp | 11 +- > > 20 files changed, 373 insertions(+), 109 deletions(-) > > create mode 100644 src/android/cros_mojo_token.h > > create mode 100644 src/android/hal_framebuffer.cpp > > create mode 100644 src/android/hal_framebuffer.h > > create mode 100644 src/android/jpeg/encoder_jea.cpp > > create mode 100644 src/android/jpeg/encoder_jea.h > > create mode 100644 src/android/jpeg/meson.build > > > > -- > > 2.39.0.rc1.256.g54fd8350bd-goog > > >