[libcamera-devel] [PATCH v2 10/12] android: camera_device: Rework CameraStream handling
Hirokazu Honda
hiroh at chromium.org
Tue Sep 8 07:17:46 CEST 2020
On Mon, Sep 7, 2020 at 10:28 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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.
>
Huge nit: [This is my preference, please feel free to ignore]
I would write
streams_.push_back(format, size, index);
stream->priv = static_cast<void*>(&streams_.back());
> > > 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
> >
Nit: I would save match by setting index to -1 initially, or use std::optional.
> > > > }
> > > >
> > > > /*
> > > > @@ -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
> _______________________________________________
> 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