[libcamera-devel] [PATCH v2 6/9] android: camera_device: Maintain a vector of CameraStream
Jacopo Mondi
jacopo at jmondi.org
Fri Jul 3 12:30:45 CEST 2020
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
> >
> >> 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 ?
> >
> >> 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
>
>
>
> >
> >> }
> >>
> >> 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
More information about the libcamera-devel
mailing list