[libcamera-devel] [PATCH v2 6/9] android: camera_device: Maintain a vector of CameraStream
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Jul 3 12:36:56 CEST 2020
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.
>
>>>
>>>> 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.
>>>
>>>> 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.
--
Kieran
>>>> }
>>>>
>>>> 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 ?
>
>>
>>
>>>
>>>
>>>> }
>>>>
>>>> /*
>>>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
>>>> index 5bd6cf416156..275760f0aa26 100644
>>>> --- a/src/android/camera_device.h
>>>> +++ b/src/android/camera_device.h
>>>> @@ -25,6 +25,11 @@
>>>>
>>>> class CameraMetadata;
>>>>
>>>> +struct CameraStream {
>>>> + camera3_stream *androidStream;
>>>> + unsigned int libcameraIndex;
>>>> +};
>>>> +
>>>> class CameraDevice : protected libcamera::Loggable
>>>> {
>>>> public:
>>>> @@ -89,6 +94,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
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list