Message ID | 20200902130846.55910-7-jacopo@jmondi.org |
---|---|
State | Accepted |
Headers | show |
Series |
|
Related | show |
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@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@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 >
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); ~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
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. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/android/camera_device.cpp | 22 +++++++++++----------- src/android/camera_device.h | 7 ++++--- 2 files changed, 15 insertions(+), 14 deletions(-)