| Message ID | 20200902130846.55910-6-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, On 02/09/2020 14:08, Jacopo Mondi wrote: > The CameraDevice::streams_ vector of CameraStream instances is > currently mostly accessed by index. The current implementation > creates all the CameraStream during the first loop that inspects the > camera3_stream instances, and the update the index of the > StreamConfiguration associated with the CameraStream during a second > loop that inspects MJPEG streams. A third loop creates the JPEG encoder > associated with CameraStreams that produce MJPEG format. > > As the index-based association is hard to follow and rather fragile, > rework the creation and handling of CameraStream: I recall it was necessary to split operations between things pre-validate, and post-validate though.. > > 1) Make the StreamConfiguration index a constructor parameter and a > private struct member. This disallow the creation of CameraStream > without a StreamConfiguration index assigned. I think your earlier index patches now make that possible (so I like this) > > 2) Create CameraStream only after the associated StreamConfiguration > has been identified. The first loop creates CameraStream for non-JPEG > streams, the second for the JPEG ones after having identified the > associated StreamConfiguration. Since we have just created the > CameraStream, create the JPEG encoder at the same time instead of > deferring it. > > This change removes most accesses by index to the CameraDevice::streams_ > vector. > > No functional changes intended, but this change aims to make the code > easier to follow and more robust. Sounds good - Need to check in detail, as earlier this is where I thought I saw a key functional change... diggging.... Nope - the change I had in mind was good... > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> I really like this. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/android/camera_device.cpp | 73 +++++++++++++++++++---------------- > src/android/camera_device.h | 18 +++++---- > 2 files changed, 50 insertions(+), 41 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index a917404016e7..15dc12556cf5 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -168,8 +168,8 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer, > } > } > > -CameraStream::CameraStream(PixelFormat f, Size s) > - : index(-1), format(f), size(s), jpeg(nullptr) > +CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i) > + : format(f), size(s), jpeg(nullptr), index_(i) > { > } > > @@ -1212,30 +1212,28 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > if (!format.isValid()) > return -EINVAL; > > - streams_.emplace_back(format, size); > - stream->priv = static_cast<void *>(&streams_[i]); > - > /* Defer handling of MJPEG streams until all others are known. */ > if (stream->format == HAL_PIXEL_FORMAT_BLOB) > continue; > > StreamConfiguration streamConfiguration; > - > streamConfiguration.size = size; > streamConfiguration.pixelFormat = format; > > - streams_[i].index = config_->addConfiguration(streamConfiguration); > + unsigned int index = config_->addConfiguration(streamConfiguration); > + CameraStream &cameraStream = streams_.emplace_back(format, size, index); > + stream->priv = static_cast<void *>(&cameraStream); ah yes, this is better indeed. That index from addConfiguration really helps out. > } > > /* Now handle MJPEG streams, adding a new stream if required. */ > for (unsigned int i = 0; i < stream_list->num_streams; ++i) { > camera3_stream_t *stream = stream_list->streams[i]; > - bool match = false; > - > if (stream->format != HAL_PIXEL_FORMAT_BLOB) > continue; > > - /* Search for a compatible stream */ > + /* Search for a compatible stream in the non-JPEG ones. */ > + bool match = false; > + unsigned int index; > for (unsigned int j = 0; j < config_->size(); j++) { > StreamConfiguration &cfg = config_->at(j); > > @@ -1243,13 +1241,15 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > * \todo The PixelFormat must also be compatible with > * the encoder. > */ > - if (cfg.size == streams_[i].size) { > - LOG(HAL, Info) << "Stream " << i > - << " using libcamera stream " << j; > + if (cfg.size.width != stream->width || > + cfg.size.height != stream->height) > + continue; > > - match = true; > - streams_[i].index = j; > - } > + LOG(HAL, Info) << "Stream " << i > + << " using libcamera stream " << j; > + > + index = j; > + match = true; > } > > /* Sigh - this is where I hate e-mail based reviews. This is missing what would be reallly helpful context right here ;-) > @@ -1272,7 +1272,26 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > LOG(HAL, Info) << "Adding " << streamConfiguration.toString() > << " for MJPEG support"; > > - streams_[i].index = config_->addConfiguration(streamConfiguration); > + index = config_->addConfiguration(streamConfiguration); > + } > + > + StreamConfiguration &cfg = config_->at(index); > + PixelFormat format = formats::MJPEG; I think your previous patches have rendered this format field unused? (But I 'like' having it ..., and I think it still has use/value when we deal with other stream conversions later ... we'll see) > + 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); > + if (ret) { > + LOG(HAL, Error) > + << "Failed to configure encoder"; > + return ret; Nice, I'm not sure why I had the encoder so late before, but I think it's fine/better here. Maybe it was so we only created the encoders after we've validated the configurations ? But maybe it's not a problem though. > } > } > > @@ -1296,24 +1315,10 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > for (unsigned int i = 0; i < stream_list->num_streams; ++i) { > camera3_stream_t *stream = stream_list->streams[i]; > CameraStream *cameraStream = &streams_[i]; > - StreamConfiguration &cfg = config_->at(cameraStream->index); > + StreamConfiguration &cfg = config_->at(cameraStream->index()); > > /* Use the bufferCount confirmed by the validation process. */ > stream->max_buffers = cfg.bufferCount; > - > - /* > - * Construct a software encoder for MJPEG streams from the > - * chosen libcamera source stream. > - */ > - if (cameraStream->format == formats::MJPEG) { > - cameraStream->jpeg = new EncoderLibJpeg(); > - int ret = cameraStream->jpeg->configure(cfg); > - if (ret) { > - LOG(HAL, Error) > - << "Failed to configure encoder"; > - return ret; > - } > - } > } > > /* > @@ -1427,7 +1432,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > } > descriptor->frameBuffers.emplace_back(buffer); > > - StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index); > + StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index()); > Stream *stream = streamConfiguration->stream(); > > request->addBuffer(stream, buffer); > @@ -1482,7 +1487,7 @@ void CameraDevice::requestComplete(Request *request) > continue; > } > > - StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index); > + StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index()); > Stream *stream = streamConfiguration->stream(); > FrameBuffer *buffer = request->findBuffer(stream); > if (!buffer) { > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > index 230e89b011e6..975c312c1c12 100644 > --- a/src/android/camera_device.h > +++ b/src/android/camera_device.h > @@ -28,20 +28,24 @@ > class CameraMetadata; > > struct CameraStream { > - CameraStream(libcamera::PixelFormat, libcamera::Size); > +public: > + CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i); > ~CameraStream(); > > - /* > - * The index of the libcamera StreamConfiguration as added during > - * configureStreams(). A single libcamera Stream may be used to deliver > - * one or more streams to the Android framework. > - */ > - unsigned int index; > + unsigned int index() const { return index_; } > > libcamera::PixelFormat format; > libcamera::Size size; > > Encoder *jpeg; > + > +private: > + /* > + * The index of the libcamera StreamConfiguration as added during > + * configureStreams(). A single libcamera Stream may be used to deliver > + * one or more streams to the Android framework. > + */ > + unsigned int index_; > }; > > class CameraDevice : protected libcamera::Loggable >
Hi Kieran, On Wed, Sep 02, 2020 at 02:41:44PM +0100, Kieran Bingham wrote: > Hi Jacopo, > > On 02/09/2020 14:08, Jacopo Mondi wrote: > > The CameraDevice::streams_ vector of CameraStream instances is > > currently mostly accessed by index. The current implementation > > creates all the CameraStream during the first loop that inspects the > > camera3_stream instances, and the update the index of the > > StreamConfiguration associated with the CameraStream during a second > > loop that inspects MJPEG streams. A third loop creates the JPEG encoder > > associated with CameraStreams that produce MJPEG format. > > > > As the index-based association is hard to follow and rather fragile, > > rework the creation and handling of CameraStream: > > I recall it was necessary to split operations between things > pre-validate, and post-validate though.. > > > > > > 1) Make the StreamConfiguration index a constructor parameter and a > > private struct member. This disallow the creation of CameraStream > > without a StreamConfiguration index assigned. > > I think your earlier index patches now make that possible (so I like this) > > > > > 2) Create CameraStream only after the associated StreamConfiguration > > has been identified. The first loop creates CameraStream for non-JPEG > > streams, the second for the JPEG ones after having identified the > > associated StreamConfiguration. Since we have just created the > > CameraStream, create the JPEG encoder at the same time instead of > > deferring it. > > > > This change removes most accesses by index to the CameraDevice::streams_ > > vector. > > > > No functional changes intended, but this change aims to make the code > > easier to follow and more robust. > > Sounds good - Need to check in detail, as earlier this is where I > thought I saw a key functional change... diggging.... > > Nope - the change I had in mind was good... > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > I really like this. > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Thanks! > > > --- > > src/android/camera_device.cpp | 73 +++++++++++++++++++---------------- > > src/android/camera_device.h | 18 +++++---- > > 2 files changed, 50 insertions(+), 41 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index a917404016e7..15dc12556cf5 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -168,8 +168,8 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer, > > } > > } > > > > -CameraStream::CameraStream(PixelFormat f, Size s) > > - : index(-1), format(f), size(s), jpeg(nullptr) > > +CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i) > > + : format(f), size(s), jpeg(nullptr), index_(i) > > { > > } > > > > @@ -1212,30 +1212,28 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > > if (!format.isValid()) > > return -EINVAL; > > > > - streams_.emplace_back(format, size); > > - stream->priv = static_cast<void *>(&streams_[i]); > > - > > /* Defer handling of MJPEG streams until all others are known. */ > > if (stream->format == HAL_PIXEL_FORMAT_BLOB) > > continue; > > > > StreamConfiguration streamConfiguration; > > - > > streamConfiguration.size = size; > > streamConfiguration.pixelFormat = format; > > > > - streams_[i].index = config_->addConfiguration(streamConfiguration); > > + unsigned int index = config_->addConfiguration(streamConfiguration); > > + CameraStream &cameraStream = streams_.emplace_back(format, size, index); > > + stream->priv = static_cast<void *>(&cameraStream); > > ah yes, this is better indeed. > > That index from addConfiguration really helps out. > > > > } > > > > /* Now handle MJPEG streams, adding a new stream if required. */ > > for (unsigned int i = 0; i < stream_list->num_streams; ++i) { > > camera3_stream_t *stream = stream_list->streams[i]; > > - bool match = false; > > - > > if (stream->format != HAL_PIXEL_FORMAT_BLOB) > > continue; > > > > - /* Search for a compatible stream */ > > + /* Search for a compatible stream in the non-JPEG ones. */ > > + bool match = false; > > + unsigned int index; > > for (unsigned int j = 0; j < config_->size(); j++) { > > StreamConfiguration &cfg = config_->at(j); > > > > @@ -1243,13 +1241,15 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > > * \todo The PixelFormat must also be compatible with > > * the encoder. > > */ > > - if (cfg.size == streams_[i].size) { > > - LOG(HAL, Info) << "Stream " << i > > - << " using libcamera stream " << j; > > + if (cfg.size.width != stream->width || > > + cfg.size.height != stream->height) > > + continue; > > > > - match = true; > > - streams_[i].index = j; > > - } > > + LOG(HAL, Info) << "Stream " << i > > + << " using libcamera stream " << j; > > + > > + index = j; > > + match = true; > > } > > > > /* > > > Sigh - this is where I hate e-mail based reviews. This is missing what > would be reallly helpful context right here ;-) > > > > @@ -1272,7 +1272,26 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > > LOG(HAL, Info) << "Adding " << streamConfiguration.toString() > > << " for MJPEG support"; > > > > - streams_[i].index = config_->addConfiguration(streamConfiguration); > > + index = config_->addConfiguration(streamConfiguration); > > + } > > + > > + StreamConfiguration &cfg = config_->at(index); > > + PixelFormat format = formats::MJPEG; > > I think your previous patches have rendered this format field unused? > > (But I 'like' having it ..., and I think it still has use/value when we > deal with other stream conversions later ... we'll see) Yes indeed, we can use formats::MJPEG directly as CameraStream constructor paramter. > > > + 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); > > + if (ret) { Leaking the encoder ? > > + LOG(HAL, Error) > > + << "Failed to configure encoder"; > > + return ret; > > Nice, I'm not sure why I had the encoder so late before, but I think > it's fine/better here. > > Maybe it was so we only created the encoders after we've validated the > configurations ? Which makes sense, and actually I wonder if I'm now leaking the Encoder. and yes I am in the error path here above. After it gets assinged to the CameraStream instance we should be safe, as long as all CameraStream instances are correctly deleted (see the comment below). > > But maybe it's not a problem though. > > > > > } > > } > > > > @@ -1296,24 +1315,10 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > > for (unsigned int i = 0; i < stream_list->num_streams; ++i) { > > camera3_stream_t *stream = stream_list->streams[i]; > > CameraStream *cameraStream = &streams_[i]; Please note this can also be replaced with CameraStream *cameraStream = static_cast<CameraStream *>(stream->priv); So all the index-based accesses to streams_ are gone, which I think it's nice. So the only use of streams_ is now to store the CameraStream instances so that they can easily be destroyed during the next call to configureStreams() with streams_.clear() (or at CameraDevice destruction time). I wonder if we can disallow future index-based accesses more strictly than just pointing that out at code review time.. Thanks j > > - StreamConfiguration &cfg = config_->at(cameraStream->index); > > + StreamConfiguration &cfg = config_->at(cameraStream->index()); > > > > /* Use the bufferCount confirmed by the validation process. */ > > stream->max_buffers = cfg.bufferCount; > > - > > - /* > > - * Construct a software encoder for MJPEG streams from the > > - * chosen libcamera source stream. > > - */ > > - if (cameraStream->format == formats::MJPEG) { > > - cameraStream->jpeg = new EncoderLibJpeg(); > > - int ret = cameraStream->jpeg->configure(cfg); > > - if (ret) { > > - LOG(HAL, Error) > > - << "Failed to configure encoder"; > > - return ret; > > - } > > - } > > } > > > > /* > > @@ -1427,7 +1432,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > } > > descriptor->frameBuffers.emplace_back(buffer); > > > > - StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index); > > + StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index()); > > Stream *stream = streamConfiguration->stream(); > > > > request->addBuffer(stream, buffer); > > @@ -1482,7 +1487,7 @@ void CameraDevice::requestComplete(Request *request) > > continue; > > } > > > > - StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index); > > + StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index()); > > Stream *stream = streamConfiguration->stream(); > > FrameBuffer *buffer = request->findBuffer(stream); > > if (!buffer) { > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > > index 230e89b011e6..975c312c1c12 100644 > > --- a/src/android/camera_device.h > > +++ b/src/android/camera_device.h > > @@ -28,20 +28,24 @@ > > class CameraMetadata; > > > > struct CameraStream { > > - CameraStream(libcamera::PixelFormat, libcamera::Size); > > +public: > > + CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i); > > ~CameraStream(); > > > > - /* > > - * The index of the libcamera StreamConfiguration as added during > > - * configureStreams(). A single libcamera Stream may be used to deliver > > - * one or more streams to the Android framework. > > - */ > > - unsigned int index; > > + unsigned int index() const { return index_; } > > > > libcamera::PixelFormat format; > > libcamera::Size size; > > > > Encoder *jpeg; > > + > > +private: > > + /* > > + * The index of the libcamera StreamConfiguration as added during > > + * configureStreams(). A single libcamera Stream may be used to deliver > > + * one or more streams to the Android framework. > > + */ > > + unsigned int index_; > > }; > > > > class CameraDevice : protected libcamera::Loggable > > > > -- > Regards > -- > Kieran
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index a917404016e7..15dc12556cf5 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -168,8 +168,8 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer, } } -CameraStream::CameraStream(PixelFormat f, Size s) - : index(-1), format(f), size(s), jpeg(nullptr) +CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i) + : format(f), size(s), jpeg(nullptr), index_(i) { } @@ -1212,30 +1212,28 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) if (!format.isValid()) return -EINVAL; - streams_.emplace_back(format, size); - stream->priv = static_cast<void *>(&streams_[i]); - /* Defer handling of MJPEG streams until all others are known. */ if (stream->format == HAL_PIXEL_FORMAT_BLOB) continue; StreamConfiguration streamConfiguration; - streamConfiguration.size = size; streamConfiguration.pixelFormat = format; - streams_[i].index = config_->addConfiguration(streamConfiguration); + unsigned int index = config_->addConfiguration(streamConfiguration); + CameraStream &cameraStream = streams_.emplace_back(format, size, index); + stream->priv = static_cast<void *>(&cameraStream); } /* Now handle MJPEG streams, adding a new stream if required. */ for (unsigned int i = 0; i < stream_list->num_streams; ++i) { camera3_stream_t *stream = stream_list->streams[i]; - bool match = false; - if (stream->format != HAL_PIXEL_FORMAT_BLOB) continue; - /* Search for a compatible stream */ + /* Search for a compatible stream in the non-JPEG ones. */ + bool match = false; + unsigned int index; for (unsigned int j = 0; j < config_->size(); j++) { StreamConfiguration &cfg = config_->at(j); @@ -1243,13 +1241,15 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) * \todo The PixelFormat must also be compatible with * the encoder. */ - if (cfg.size == streams_[i].size) { - LOG(HAL, Info) << "Stream " << i - << " using libcamera stream " << j; + if (cfg.size.width != stream->width || + cfg.size.height != stream->height) + continue; - match = true; - streams_[i].index = j; - } + LOG(HAL, Info) << "Stream " << i + << " using libcamera stream " << j; + + index = j; + match = true; } /* @@ -1272,7 +1272,26 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) LOG(HAL, Info) << "Adding " << streamConfiguration.toString() << " for MJPEG support"; - streams_[i].index = config_->addConfiguration(streamConfiguration); + index = config_->addConfiguration(streamConfiguration); + } + + StreamConfiguration &cfg = config_->at(index); + 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); + if (ret) { + LOG(HAL, Error) + << "Failed to configure encoder"; + return ret; } } @@ -1296,24 +1315,10 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) for (unsigned int i = 0; i < stream_list->num_streams; ++i) { camera3_stream_t *stream = stream_list->streams[i]; CameraStream *cameraStream = &streams_[i]; - StreamConfiguration &cfg = config_->at(cameraStream->index); + StreamConfiguration &cfg = config_->at(cameraStream->index()); /* Use the bufferCount confirmed by the validation process. */ stream->max_buffers = cfg.bufferCount; - - /* - * Construct a software encoder for MJPEG streams from the - * chosen libcamera source stream. - */ - if (cameraStream->format == formats::MJPEG) { - cameraStream->jpeg = new EncoderLibJpeg(); - int ret = cameraStream->jpeg->configure(cfg); - if (ret) { - LOG(HAL, Error) - << "Failed to configure encoder"; - return ret; - } - } } /* @@ -1427,7 +1432,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques } descriptor->frameBuffers.emplace_back(buffer); - StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index); + StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index()); Stream *stream = streamConfiguration->stream(); request->addBuffer(stream, buffer); @@ -1482,7 +1487,7 @@ void CameraDevice::requestComplete(Request *request) continue; } - StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index); + StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index()); Stream *stream = streamConfiguration->stream(); FrameBuffer *buffer = request->findBuffer(stream); if (!buffer) { diff --git a/src/android/camera_device.h b/src/android/camera_device.h index 230e89b011e6..975c312c1c12 100644 --- a/src/android/camera_device.h +++ b/src/android/camera_device.h @@ -28,20 +28,24 @@ class CameraMetadata; struct CameraStream { - CameraStream(libcamera::PixelFormat, libcamera::Size); +public: + CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i); ~CameraStream(); - /* - * The index of the libcamera StreamConfiguration as added during - * configureStreams(). A single libcamera Stream may be used to deliver - * one or more streams to the Android framework. - */ - unsigned int index; + unsigned int index() const { return index_; } libcamera::PixelFormat format; libcamera::Size size; Encoder *jpeg; + +private: + /* + * The index of the libcamera StreamConfiguration as added during + * configureStreams(). A single libcamera Stream may be used to deliver + * one or more streams to the Android framework. + */ + unsigned int index_; }; class CameraDevice : protected libcamera::Loggable
The CameraDevice::streams_ vector of CameraStream instances is currently mostly accessed by index. The current implementation creates all the CameraStream during the first loop that inspects the camera3_stream instances, and the update the index of the StreamConfiguration associated with the CameraStream during a second loop that inspects MJPEG streams. A third loop creates the JPEG encoder associated with CameraStreams that produce MJPEG format. As the index-based association is hard to follow and rather fragile, rework the creation and handling of CameraStream: 1) Make the StreamConfiguration index a constructor parameter and a private struct member. This disallow the creation of CameraStream without a StreamConfiguration index assigned. 2) Create CameraStream only after the associated StreamConfiguration has been identified. The first loop creates CameraStream for non-JPEG streams, the second for the JPEG ones after having identified the associated StreamConfiguration. Since we have just created the CameraStream, create the JPEG encoder at the same time instead of deferring it. This change removes most accesses by index to the CameraDevice::streams_ vector. No functional changes intended, but this change aims to make the code easier to follow and more robust. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/android/camera_device.cpp | 73 +++++++++++++++++++---------------- src/android/camera_device.h | 18 +++++---- 2 files changed, 50 insertions(+), 41 deletions(-)