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

[Jacopo Mondi]

Hi Kieran

On Fri, Jul 03, 2020 at 11:36:56AM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 03/07/2020 11:30, Jacopo Mondi wrote:
> > Hi Kieran
> >
> > On Fri, Jul 03, 2020 at 10:55:22AM +0100, Kieran Bingham wrote:
> >> Hi Jacopo,
> >>
> >> On 03/07/2020 10:35, Jacopo Mondi wrote:
> >>> Hi Kieran,
> >>>
> >>> On Thu, Jul 02, 2020 at 10:36:51PM +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>
> >>>> ---
> >>>>  src/android/camera_device.cpp | 18 ++++++++++++++++--
> >>>>  src/android/camera_device.h   |  6 ++++++
> >>>>  2 files changed, 22 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> >>>> index 77083219d8a1..fc3962dac230 100644
> >>>> --- a/src/android/camera_device.cpp
> >>>> +++ b/src/android/camera_device.cpp
> >>>> @@ -952,6 +952,14 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >>>>  		return -EINVAL;
> >>>>  	}
> >>>>
> >>>> +	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;
> >>>> +
> >>>
> >>> I would drop this one
> >>
> >> Drop the newline? Or something else?
> >>
> >
> > yeah, new line
>
>
> Can I disagree here? - I put that there intentionally to not 'hide' the
> streamIndex variable against the for loop, and not associate the comment
> added with it to the loop.
>
> The comment applies to the variable, not the loop.

Your code, your choice :D

>
>
> >
> >>>
> >>>>  	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
> >>>>  		camera3_stream_t *stream = stream_list->streams[i];
> >>>>
> >>>> @@ -967,6 +975,9 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >>>>  		if (!format.isValid())
> >>>>  			return -EINVAL;
> >>>>
> >>>> +		/* Maintain internal state of all stream mappings. */
> >>>> +		streams_[i].androidStream = stream;
> >>>> +
> >>>
> >>> Am I mistaken, or looking at the following patches, this is not used ?
>
>
> Hrm, you might be right. Perhaps maintaining the correct indexing for
> the streams_[] to camera3_stream_t, and (later) putting the pointer in
> is enough.
>
> Wow - this struct is certainly getting small for now then, just storing
> a single index ...
>
> And yet, I still want to make it a class... hrm ... maybe class can come
> later.

Surely a class for single unsigned int would be 'slightly' and
overkill :)

>
>
>
> >>>
> >>>>  		StreamConfiguration streamConfiguration;
> >>>>
> >>>>  		streamConfiguration.size.width = stream->width;
> >>>> @@ -974,6 +985,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >>>>  		streamConfiguration.pixelFormat = format;
> >>>>
> >>>>  		config_->addConfiguration(streamConfiguration);
> >>>> +		streams_[i].libcameraIndex = streamIndex++;
> >>>
> >>> In that case and the androidStream field is not used, we would just
> >>> need to store a pointer to the StreamConfiguration associated to an
> >>> android stream, don't we ?
> >>
> >>
> >> No, we can't store a pointer to the StreamConfiguration, because it's
> >> not valid to do so.
> >>
> >>
> >> At this point, we have 'added' the configuration to the config_, but it
> >> makes a copy, so /this/ streamConfiguration is the wrong pointer to store.
> >>
> >
> > Right, indeed stream configurations are copied into the camera.
> > Didn't you have a patch to make CameraConfiguration::addConfiguration
> > return a pointer to the stored StreamConfiguration ?
> >
> >
> >> Further more, it could then be suggested that we just obtain the
> >> 'correct' pointer - by using config_->at(n);.
> >>
> >> But we can't do that either, because we are in a loop, adding configs to
> >> a vector, and the vector can re-allocate - so the pointers could change.
> >>
> >> Thus, I am storing an index.
> >
> > Makes sense, then please ignore my comment.
> >
> >>
> >>
> >> Should that be added in a comment or is it ok?
> >
> > If you can capture this in a few lines, why not.
> >
> > My question on the usage of .androidStream remains though
>
> I could store just the index in stream->priv as I think Laurent
> suggested (with casting) at this stage, and later add a struct, but I
> think I'll keep the struct for now.
>

If "later" is after this patch series, I tend to disagree, but I won't
bother too much

>
> >>>>  	}
> >>>>
> >>>>  	switch (config_->validate()) {
> >>>> @@ -991,10 +1003,12 @@ 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->libcameraIndex);
> >>>>
> >>>>  		/* Use the bufferCount confirmed by the validation process. */
> >>>> -		stream->max_buffers = streamConfiguration.bufferCount;
> >>>> +		stream->max_buffers = cfg.bufferCount;
> >>>
> >>> I'm not sure I get the purpose of this hunk.
> >>>
> >>> If you're preparing to have less StreamConfiguration than android
> >>> streams (as some streams, as JPEG one, might be hal-only streams),
> >>> why don't you just iterate the camera configuration, as the only
> >>> purpose here is to collect the maximum number of buffers ?
> >>
> >> Because even the HAL only streams need to have the stream->max_buffers
> >> populated ? In the case of a hal-only stream, it gets populated with the
> >> max_buffers of the relevant /source/ stream which will feed that hal
> >> only stream:
> >>
> >>  Stream #1 YUV : MaxBuffers=4
> >>  Stream #2 MJPEG : MaxBuffers=maxBuffersOfStream(1);
> >>
> >> If we iterate over the camera configuration, we would not set the
> >> max_buffers for the hal-only streams, nor perform any other
> >> per-android-stream actions that may be required post-validation.
> >
> > Right, we need to iterate over all the android provided streams to
> > fill their max_buffers field.
> >
> > Now that I look at the code again, I see that in the first loop we store
> > an increasing streamIndex, which is exactly equal to i.
> >
> >
> > 	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
> >                 ...
> >
> > 		config_->addConfiguration(streamConfiguration);
> > 		streams_[i].libcameraIndex = streamIndex++;
> > 	}
> >
> >         ...
> >
> > In the second loop we use that index, which is exactly equal to the
> > loop counter
> >
> > 	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->libcameraIndex);
> >
> > 		/* Use the bufferCount confirmed by the validation process. */
> > 		stream->max_buffers = cfg.bufferCount;
> > 	}
> >
> > Re-phrasing: why do you need that libcamerIdex at all now ?
> >
> > How do you plan to map HAL-only streams to the streamIndex ? There
> > will be one index in the stream_ vector for each of the android
> > stream, will some entries be repeated ? As HAL-only streams will
> > 'point' to the StreamConfiguration of the libcamera Stream that feeds
> > them ?
> >

Missed this part ?