[libcamera-devel] [RFC 6/7] android: camera_device: Set Encoder at construction
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Sep 2 16:08:26 CEST 2020
Hi Jacopo,
On 02/09/2020 14:08, Jacopo Mondi wrote:
> Make the CameraStream Encoder * a private struct member and require its
> initialization at construction time.
>
> This change dis-allow creating a CameraStream and set the Encoder later,
> which shall not happen now that we create CameraStream once we have all
> the required information in place.
>
> No functional changes intended but this change aims to make the code
> more robust enforcing a stricter CameraStream interface.
Excellent plan ;-)
>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> src/android/camera_device.cpp | 22 +++++++++++-----------
> src/android/camera_device.h | 7 ++++---
> 2 files changed, 15 insertions(+), 14 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 15dc12556cf5..aef9a6fb4be1 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -168,14 +168,14 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,
> }
> }
>
> -CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i)
> - : format(f), size(s), jpeg(nullptr), index_(i)
> +CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i, Encoder *encoder)
> + : format(f), size(s), index_(i), encoder_(encoder)
> {
> }
>
> CameraStream::~CameraStream()
> {
> - delete jpeg;
> + delete encoder_;
> };
>
> /*
> @@ -1279,20 +1279,20 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> PixelFormat format = formats::MJPEG;
> Size size = cfg.size;
>
> - CameraStream &cameraStream = streams_.emplace_back(format, size, index);
> - stream->priv = static_cast<void *>(&cameraStream);
> -
> /*
> * Construct a software encoder for MJPEG streams from the
> * chosen libcamera source stream.
> */
> - cameraStream.jpeg = new EncoderLibJpeg();
> - int ret = cameraStream.jpeg->configure(cfg);
> + Encoder *encoder = new EncoderLibJpeg();
> + int ret = encoder->configure(cfg);
> if (ret) {
> - LOG(HAL, Error)
> - << "Failed to configure encoder";
> + LOG(HAL, Error) << "Failed to configure encoder";
> return ret;
> }
> +
> + CameraStream &cameraStream = streams_.emplace_back(format, size,
> + index, encoder);
> + stream->priv = static_cast<void *>(&cameraStream);
> }
>
> switch (config_->validate()) {
> @@ -1481,7 +1481,7 @@ void CameraDevice::requestComplete(Request *request)
> if (cameraStream->format != formats::MJPEG)
> continue;
>
> - Encoder *encoder = cameraStream->jpeg;
> + Encoder *encoder = cameraStream->encoder();
> if (!encoder) {
> LOG(HAL, Error) << "Failed to identify encoder";
> continue;
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 975c312c1c12..da9d5c0435f0 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -29,16 +29,16 @@ class CameraMetadata;
>
> struct CameraStream {
> public:
> - CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i);
> + CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i,
> + Encoder *encoder = nullptr);
Aha - there it is, the encoder is optional.
That's why I couldn't see an update to the non-jpeg streams.
Ok - this is fine with me, I fear there will be further refactoring
later when we need CameraStreams to do differnet things like
pixel-format conversions or rescales. At that point, I imagine seeing
the JPEG encoder being constructed inside CameraStream.cpp perhaps - but
that's a tomorrow issue.
Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ~CameraStream();
>
> unsigned int index() const { return index_; }
> + Encoder *encoder() const { return encoder_; }
>
> libcamera::PixelFormat format;
> libcamera::Size size;
>
> - Encoder *jpeg;
> -
> private:
> /*
> * The index of the libcamera StreamConfiguration as added during
> @@ -46,6 +46,7 @@ private:
> * one or more streams to the Android framework.
> */
> unsigned int index_;
> + Encoder *encoder_;
> };
>
> class CameraDevice : protected libcamera::Loggable
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list