[libcamera-devel] [PATCH v3 09/11] android: camera_device: Rework CameraStream handling
Jacopo Mondi
jacopo at jmondi.org
Thu Sep 10 13:15:09 CEST 2020
Hi Niklas,
On Thu, Sep 10, 2020 at 01:00:06PM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your work.
>
> On 2020-09-08 15:41:40 +0200, 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:
> >
> > 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 all 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.
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > src/android/camera_device.cpp | 91 ++++++++++++++++++-----------------
> > src/android/camera_device.h | 18 ++++---
> > 2 files changed, 57 insertions(+), 52 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index eab01d808917..e0260c92246c 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)
> > {
> > }
> >
> > @@ -1190,6 +1190,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> > streams_.reserve(stream_list->num_streams);
> >
> > /* First handle all non-MJPEG streams. */
> > + camera3_stream_t *jpegStream = nullptr;
> > for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
> > camera3_stream_t *stream = stream_list->streams[i];
> > Size size(stream->width, stream->height);
> > @@ -1206,52 +1207,50 @@ 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)
> > + if (stream->format == HAL_PIXEL_FORMAT_BLOB) {
> > + jpegStream = stream;
> > continue;
> > + }
> >
> > StreamConfiguration streamConfiguration;
> > -
> > streamConfiguration.size = size;
> > streamConfiguration.pixelFormat = format;
> >
> > config_->addConfiguration(streamConfiguration);
> > - streams_[i].index = config_->size() - 1;
> > + unsigned int index = config_->size() - 1;
> > + streams_.emplace_back(format, size, index);
> > + stream->priv = static_cast<void *>(&streams_.back());
>
> Is this not dangerous? If the stremas_ vector is modified in any way the
> members may be reallocated and the pointer becomes invalid. I think this
> is what boost::container::stable_vector tries to solve for example.
We reserve space for all possible entries in advance to avoid
relocations.
>
> > }
> >
> > /* 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;
> > + if (jpegStream) {
> > + int index = -1;
> >
> > - /* Search for a compatible stream */
> > - for (unsigned int j = 0; j < config_->size(); j++) {
> > - StreamConfiguration &cfg = config_->at(j);
> > + /* Search for a compatible stream in the non-JPEG ones. */
> > + for (unsigned int i = 0; i < config_->size(); i++) {
> > + StreamConfiguration &cfg = config_->at(i);
> >
> > /*
> > * \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 != jpegStream->width ||
> > + cfg.size.height != jpegStream->height)
> > + continue;
> >
> > - match = true;
> > - streams_[i].index = j;
> > - }
> > + LOG(HAL, Info)
> > + << "Android JPEG stream mapped on stream " << i;
> > +
> > + index = i;
> > + break;
> > }
> >
> > /*
> > * Without a compatible match for JPEG encoding we must
> > * introduce a new stream to satisfy the request requirements.
> > */
> > - if (!match) {
> > + if (index < 0) {
> > StreamConfiguration streamConfiguration;
> >
> > /*
> > @@ -1260,15 +1259,31 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> > * handled, and should be considered as part of any
> > * stream configuration reworks.
> > */
> > - streamConfiguration.size.width = stream->width;
> > - streamConfiguration.size.height = stream->height;
> > + streamConfiguration.size.width = jpegStream->width;
> > + streamConfiguration.size.height = jpegStream->height;
> > streamConfiguration.pixelFormat = formats::NV12;
> >
> > LOG(HAL, Info) << "Adding " << streamConfiguration.toString()
> > << " for MJPEG support";
> >
> > config_->addConfiguration(streamConfiguration);
> > - streams_[i].index = config_->size() - 1;
> > + index = config_->size() - 1;
> > + }
> > +
> > + StreamConfiguration &cfg = config_->at(index);
> > + CameraStream &cameraStream =
> > + streams_.emplace_back(formats::MJPEG, cfg.size, index);
> > + jpegStream->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;
> > }
> > }
> >
> > @@ -1291,25 +1306,11 @@ 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);
> > + CameraStream *cameraStream = static_cast<CameraStream *>(stream->priv);
> > + 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;
> > - }
> > - }
> > }
> >
> > /*
> > @@ -1423,7 +1424,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);
> > @@ -1478,7 +1479,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 dc0ee664d443..f8f237203ce9 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
> > --
> > 2.28.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund
More information about the libcamera-devel
mailing list