[libcamera-devel] [PATCH v2 10/12] android: camera_device: Rework CameraStream handling
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Sep 7 15:28:06 CEST 2020
Hi Jacopo,
On Mon, Sep 07, 2020 at 10:53:33AM +0200, Jacopo Mondi wrote:
> On Sun, Sep 06, 2020 at 01:05:24AM +0300, Laurent Pinchart wrote:
> > On Wed, Sep 02, 2020 at 05:22:34PM +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 | 75 +++++++++++++++++++----------------
> > > src/android/camera_device.h | 18 +++++----
> > > 2 files changed, 51 insertions(+), 42 deletions(-)
> > >
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index 3630e87e8814..9bcd1d993c17 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);
> >
> > This is problematic. std::vector::emplace_back may cause a reallocation,
>
> We have reserved space for all the possible cameraStream instances
> before entering this loop, didn't we ? It should save all relocations,
> or don't you think it's enough ?
You're right, I had forgotten about that.
> > which invalidates all iterators, rendering the pointer invalid. One
> > option would be to turn streams_ into an std::list, another option to
> > fill the priv pointers in a separate loop after populating streams_.
> >
> > > }
> > >
> > > /* 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;
> >
> > Drive-by comment for a possible future simplification, can't we rely on
> > the fact that only a single JPEG stream will be requested ? If so, we
> > could store a pointer to the JPEG camera3_stream_configuration_t in the
> > previous loop, and have a
> >
> > if (jpegStream) {
> > ...
> > }
> >
> > here instead of a loop.
>
> Ah yes, we can save this external loop indeed!
> Thanks ;)
>
> > > - /* 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
> >
> > s/Stream/Android stream/ ?
> >
> > > + << " using libcamera stream " << j;
> > > +
> > > + index = j;
> > > + match = true;
> >
> > Should you break ?
>
> Ups, I should indeed
>
> > > }
> > >
> > > /*
> > > @@ -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);
> >
> > No need for the format and size local variables, you can write
> >
> > CameraStream &cameraStream =
> > streams_.emplace_back(formats::MJPEG, cfg.size, index);
>
> Kieran had a similar comment iirc. I'll change this!
>
> > > + 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";
> >
> > This holds on a single line.
> >
> > LOG(HAL, Error) << "Failed to configure encoder";
>
> Right!
>
> > > + return ret;
> > > }
> > > }
> > >
> > > @@ -1295,25 +1314,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;
> > > - }
> > > - }
> > > }
> > >
> > > /*
> > > @@ -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 dc0ee664d443..f8f237203ce9 100644
> > > --- a/src/android/camera_device.h
> > > +++ b/src/android/camera_device.h
> > > @@ -28,20 +28,24 @@
> > > class CameraMetadata;
> > >
> > > struct CameraStream {
> >
> > Can you turn it into a class while at it ?
>
> See the end of the series :)
>
> > Also clearly not a candidate for this patch, do you think it would be
> > useful to rename the HAL-related classes with a HAL prefix (e.g.
> > HALCamera instead of CameraDevice, HALStream instead of CameraStream) to
> > avoid confusing them with the libcamera classes ? We're familiar with
> > this code so it's probably not too confusing for us, but it may be for a
> > newcomer.
>
> Indeed, I'm looking forward to get to a point where we can break
> camera_device.cpp in multiple files, and possible re-name them as
> well.
>
> I tried using the camera3 prefix for all Android-things to keep them
> separate from our ones, but indeed we have confusing names for some
> classes.
>
> > > - CameraStream(libcamera::PixelFormat, libcamera::Size);
> > > +public:
> > > + CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i);
> >
> > i is not a very clear name, index would be better. I would also name the
> > existing parameters (that could be a separate change).
>
> I think this happens later all in one go
>
> > > ~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.
> > > + */
> >
> > Should this be moved to the .cpp file ? Documentation rules are a bit
> > more relaxed here though, as it's internal doc.
>
> well, having them in the cpp file makes most sense for the purpose of
> generating documentation, which doesn't happen for the HAL. This
> comment alone, not anchored to the field declaration loses a bit of
> value.
>
> I thinks going forward we want to document more formally this part
> though.
>
> > > + unsigned int index_;
> > > };
> > >
> > > class CameraDevice : protected libcamera::Loggable
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list