[libcamera-devel] [PATCH v3 6/8] android: camera_device: Maintain a vector of CameraStream

Mon Jul 6 16:16:30 CEST 2020

On Mon, Jul 06, 2020 at 02:05:04PM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 06/07/2020 09:22, Jacopo Mondi wrote:
> > Hi Kieran
> >    sorry, I'm going to be picky.
> >
> > On Fri, Jul 03, 2020 at 01:39:17PM +0100, Kieran Bingham wrote:
> >> Introduce a vector storing a CameraStream to track and maintain
> >> state between an Android stream (camera3_stream_t) and a libcamera
> >> Stream.
> >>
> >> Only the index of the libcamera stream is stored, to facilitate identifying
> >> the correct index for both the StreamConfiguration and Stream vectors.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >> ---
> >>  src/android/camera_device.cpp | 22 ++++++++++++++++++++--
> >>  src/android/camera_device.h   | 10 ++++++++++
> >>  2 files changed, 30 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> >> index 4e77a92dbea5..28334751a26e 100644
> >> --- a/src/android/camera_device.cpp
> >> +++ b/src/android/camera_device.cpp
> >> @@ -952,6 +952,20 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >>  		return -EINVAL;
> >>  	}
> >>
> >> +	/*
> >> +	 * Clear and remove any existing configuration from previous calls, and
> >> +	 * ensure the required entries are available without further
> >> +	 * re-allcoation.
> >> +	 */
> >> +	streams_.clear();
> >> +	streams_.reserve(stream_list->num_streams);
> >> +
> >> +	/*
> >> +	 * Track actually created streams, as there may not be a 1:1 mapping of
> >> +	 * camera3 streams to libcamera streams.
> >> +	 */
> >> +	unsigned int streamIndex = 0;
> >> +
> >>  	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
> >>  		camera3_stream_t *stream = stream_list->streams[i];
> >>
> >> @@ -974,6 +988,9 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >>  		streamConfiguration.pixelFormat = format;
> >>
> >>  		config_->addConfiguration(streamConfiguration);
> >> +
> >> +		/* Maintain internal state of all stream mappings. */
> >
> > I don't want to be rude, but these are the comments I would prefer not
> > to see in code, and I'm the first one with a tendency to over-comment
>
>
> Don't worry - it's fine. I normally write comments before code, so
> sometimes indeed I'll end up with possibly inappropriate comments left
> that I haven't yet removed.
>
> And in this series, these code blocks have bounced around so much in the
> patches they might think they're on a trampoline ;)
>
>
> Indeed, this code block started out with storing much more content and
> internal state for JPEG, which has then been simplified to try to get
> this multi-stream series separated...
>
>
> > trivial operations (like recording an index) with comments that might
> > even be misleading.  "maintain the internal state of all stream
> > mappings" makes me look around for some kind of 'internal state' until
> > I don't realize we're just recording an index for later use. I would
> > prefer to know where the index is use and why, with a slightly longer:
>
> It's been simplified to a single index (for now).
>
>
> >
> >                 /*
> >                  * Record the index of the StreamConfiguration
> >                  * associated to the camera3 stream. As not all
> >                  * camera3 streams maps to a libcamera stream, the
> >                  * index might be repeated.
> >                  */
>
> Isn't that what's stated above at declaration of streamIndex?
>
> The comment for /* maintain internal state of all stream mappings */
> helps later on when there are several lines here, and it becomes a
> distinct code block, but it has been simplified to a single line for
> this patch.
>
> I'll just remove the line, there's no need to add a big block again. The
> single line was more of a separator...
>
>
> And in fact, I later found out I had to split the block anyway, as I can
> only increment the index after adding the Configuration, where JPEG
> streams that don't create a configuration break out of the loop earlier,
> so they have to update their internal streams_[i] structure before.
>
>
> >> +		streams_[i].index = streamIndex++;
> >>  	}
> >>
> >>  	switch (config_->validate()) {
> >> @@ -991,10 +1008,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];
> >> -		StreamConfiguration &streamConfiguration = config_->at(i);
> >> +		CameraStream *cameraStream = &streams_[i];
> >> +		StreamConfiguration &cfg = config_->at(cameraStream->index);
> >>
> >>  		/* Use the bufferCount confirmed by the validation process. */
> >> -		stream->max_buffers = streamConfiguration.bufferCount;
> >> +		stream->max_buffers = cfg.bufferCount;
> >>  	}
> >>
> >>  	/*
> >> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> >> index d7834d94f439..8e306c1f6d8b 100644
> >> --- a/src/android/camera_device.h
> >> +++ b/src/android/camera_device.h
> >> @@ -25,6 +25,15 @@
> >>
> >>  class CameraMetadata;
> >>
> >> +struct CameraStream {
> >> +	/*
> >> +	 * The index represents the index for libcamera stream parameters. This
> >
> > What are "stream parameters" ?
>
> StreamConfigurations, and Streams ... but I think actually the Stream*
> is obtained from the StreamConfiguration anyway, so really it's just the
> StreamConfigurations
>
> >
> >> +	 * will differ from any index of the halStream, particularly for HAL
> >
> > I'm not sure I get what "from any index of the halStream" means.
> > I also think you removed 'halStream' from the patch
>
> Yes, I have now removed halStream, and I don't think I'll add it back
> later any more.
>
>
> > I would:
> > "Index of the libcamera StreamConfiguration associated with an Android
> > requested stream. Not all streams requested by the Android framework
> > are 1-to-1 mapped to a libcamera Stream as some of them, such as JPEG,
> > might be produced by the libcamera HAL by processing data produced by
> > an existing libcamera Stream. This implies different Android stream
> > might be associated with the same libcamera StreamConfiguration index."
> >
> > Feel free to adjust if you think it's worth taking this in.
>
> I've changed this to:
>
>  /*
>   * 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.
>   */

Sounds perfect!
Thanks

>
> >
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
> Thanks,
>
>
> >
> > Thanks
> >   j
> >
> >
> >> +	 * only streams such as MJPEG.
> >> +	 */
> >> +	unsigned int index;
> >> +};
> >> +
> >>  class CameraDevice : protected libcamera::Loggable
> >>  {
> >>  public:
> >> @@ -90,6 +99,7 @@ private:
> >>
> >>  	std::vector<Camera3StreamConfiguration> streamConfigurations_;
> >>  	std::map<int, libcamera::PixelFormat> formatsMap_;
> >> +	std::vector<CameraStream> streams_;
> >>
> >>  	int facing_;
> >>  	int orientation_;
> >> --
> >> 2.25.1
> >>
> >> _______________________________________________
> >> libcamera-devel mailing list
> >> libcamera-devel at lists.libcamera.org
> >> https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards
> --
> Kieran