| Message ID | 20200702213654.2129054-7-kieran.bingham@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thanks for your work. On 2020-07-02 22:36:51 +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@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); Should you not also empty the vector before reserving memory? I'm thinking of if something below fails and exits we could be stuch with a streams_ that is half of B and half of A. Or maybe streams_ should be emptied on error instead? > + > + /* > + * 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]; > > @@ -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; > + > 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++; > } > > 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]; > + nit: I would drop the extra newline, with or without fixed, > + StreamConfiguration &cfg = config_->at(cameraStream->libcameraIndex); > > /* 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 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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Kieran, Thank you for the patch. On Fri, Jul 03, 2020 at 01:31:09AM +0200, Niklas Söderlund wrote: > On 2020-07-02 22:36:51 +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@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); > > Should you not also empty the vector before reserving memory? I'm > thinking of if something below fails and exits we could be stuch with a > streams_ that is half of B and half of A. Or maybe streams_ should be > emptied on error instead? reserve() will only reserve memory to avoid a memory allocation for every entry added to the vector, entries are still populated when adding them. Said different, reserve() doesn't change size(). > > + > > + /* > > + * 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]; > > > > @@ -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; > > + > > 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++; > > } > > > > 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]; > > + > > nit: I would drop the extra newline, with or without fixed, > > > + StreamConfiguration &cfg = config_->at(cameraStream->libcameraIndex); > > > > /* 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 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; I'd name this halStream, the rationale being that prefixing names with hal instead of android will keep them shorter. > > + unsigned int libcameraIndex; And I'd simply use index here (no prefix for the libcamera side), but I won't insist too much. The rationale is that otherwise I fear a libcamera prefix will spread in many places. A short comment along the lines of "The index of the stream in the CameraConfiguration" could also possibly be useful. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > +}; > > + > > 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_;
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@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 > 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 ? > } > > 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 ? > } > > /* > 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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi all, On 03/07/2020 01:37, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Fri, Jul 03, 2020 at 01:31:09AM +0200, Niklas Söderlund wrote: >> On 2020-07-02 22:36:51 +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@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); >> >> Should you not also empty the vector before reserving memory? I'm >> thinking of if something below fails and exits we could be stuch with a >> streams_ that is half of B and half of A. Or maybe streams_ should be >> emptied on error instead? > > reserve() will only reserve memory to avoid a memory allocation for > every entry added to the vector, entries are still populated when adding > them. Said different, reserve() doesn't change size(). > >>> + >>> + /* >>> + * 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]; >>> >>> @@ -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; >>> + >>> 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++; >>> } >>> >>> 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]; >>> + >> >> nit: I would drop the extra newline, with or without fixed, Niklas, was there a tag to go in here? :-D >> >>> + StreamConfiguration &cfg = config_->at(cameraStream->libcameraIndex); >>> >>> /* 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 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; > > I'd name this halStream, the rationale being that prefixing names with > hal instead of android will keep them shorter. I like halStream > >>> + unsigned int libcameraIndex; > > And I'd simply use index here (no prefix for the libcamera side), but I > won't insist too much. The rationale is that otherwise I fear a > libcamera prefix will spread in many places. A short comment along the > lines of "The index of the stream in the CameraConfiguration" could also > possibly be useful. And I'm happy with just index too, but it does /require/ the comment then, because the index is not required to match the index of the halStream. I'll update both. > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks, > >>> +}; >>> + >>> 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_; >
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@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? > >> 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. 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. Should that be added in a comment or is it ok? > >> } >> >> 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. > > >> } >> >> /* >> 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@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel
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@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@lists.libcamera.org > >> https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards > -- > Kieran
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@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@lists.libcamera.org >>>> https://lists.libcamera.org/listinfo/libcamera-devel >> >> -- >> Regards >> -- >> Kieran
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@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 ?
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@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)
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; + 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; + 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++; } 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; } /* 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_;
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@ideasonboard.com> --- src/android/camera_device.cpp | 18 ++++++++++++++++-- src/android/camera_device.h | 6 ++++++ 2 files changed, 22 insertions(+), 2 deletions(-)