[libcamera-devel] [RFC 5/7] android: camera_device: Rework CameraStream handling

[Jacopo Mondi]

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 at jmondi.org>
>
> I really like this.
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham at 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