[libcamera-devel] [PATCH v2 6/9] android: camera_device: Maintain a vector of CameraStream
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Jul 3 13:03:16 CEST 2020
Hi Jacopo,
On 03/07/2020 11:47, Jacopo Mondi wrote:
> 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 :)
Of course, but this 'object' is the HAL Stream representation, and will
soon have an optional JPEG compressor... (or be subclassed to a JPEG
stream?, or .... we'll see).
And I don't know what will happen for RAW ... I hope Niklas has some
ideas forming...
>>>>>
>>>>>> 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 ?
Yes.
The {streamIndex,libcameraIndex,index} stored for each
{camera3_stream_t,CameraStream} points to the relevant index that can
correctly map to a libcamera StreamConfiguration and thus a Stream.
If only a JPEG stream is requested, then an NV12 stream will be added to
the libcamera config_, and the HAL stream #1 will use libcamera stream #1.
If an NV12 and MJPEG stream are requested (and they are the same size),
then MJPEG will use the NV12 stream index.
If the NV12 and MJPEG streams are not compatible, a new stream will be
added to libcamera:
JPEG only:
HAL libcamera
Stream #1 JPEG - Stream #1 NV12 (640x320)
JPEG+YUV(SameSize):
HAL libcamera
Stream #1 NV12 - Stream #1 NV12 (640x320)
Stream #2 JPEG - Stream #1 ^^^^ (640x320)
JPEG+YUV(DifferentSize, or incompatible format):
HAL libcamera
Stream #1 NV12 - Stream #1 NV12 (640x320)
Stream #2 JPEG - Stream #2 NV12 (1920x1080)
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list