[libcamera-devel] [PATCH v3 6/8] android: camera_device: Maintain a vector of CameraStream
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Jul 6 15:05:04 CEST 2020
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.
*/
>
> 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
More information about the libcamera-devel
mailing list