[libcamera-devel] [PATCH v3 09/11] android: camera_device: Rework CameraStream handling
Hirokazu Honda
hiroh at chromium.org
Fri Sep 11 04:39:52 CEST 2020
Hi Jacopo, Thanks for the patch.
On Thu, Sep 10, 2020 at 8:11 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> 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>
Reviewed-by: Hirokazu Honda <hiroh at chromium.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
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list