| Message ID | 20220527093440.953377-2-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul, Thank you for the patch On 5/27/22 11:34, Paul Elder via libcamera-devel wrote: > From: Hirokazu Honda <hiroh@chromium.org> > > Add to the CameraStream class a sourceStream field, which for streams > of type Mapped contains a reference to the stream which produces the > actual image data. I think some re-pharsing would be nice to better comprehend the changes. "Add a sourceStream field to the CameraStream class, meant to contain a reference of a direct stream (which produces actual image data) for CameraStream::Mapped stream types." > > The sourceStream of mapped streams will be used in later patches to make > sure for each Mapped stream at least one libcamera::Stream is queued to > the libcamera::Camera. > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > Changes in v2: > - fix whitespace > --- > src/android/camera_device.cpp | 8 +++++++- > src/android/camera_stream.cpp | 6 ++++-- > src/android/camera_stream.h | 6 +++++- > 3 files changed, 16 insertions(+), 4 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 8e804d4d..20599665 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -681,10 +681,16 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > for (const auto &streamConfig : streamConfigs) { > config->addConfiguration(streamConfig.config); > > + CameraStream *sourceStream = nullptr; > for (auto &stream : streamConfig.streams) { > streams_.emplace_back(this, config.get(), stream.type, > - stream.stream, config->size() - 1); > + stream.stream, sourceStream, > + config->size() - 1); > stream.stream->priv = static_cast<void *>(&streams_.back()); > + > + /* Mapped streams are always associated with a Direct one. */ > + if (stream.type == CameraStream::Type::Direct) This doesn't look right to me, why are we setting sourceStream field for a direct stream type? Shouldn't be it something like: + if (stream.type == CameraStream::Type::Mapped) + sourceStream = .... instead? > + sourceStream = &streams_.back(); > } > } > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp > index 94f1884c..154e088e 100644 > --- a/src/android/camera_stream.cpp > +++ b/src/android/camera_stream.cpp > @@ -52,9 +52,11 @@ LOG_DECLARE_CATEGORY(HAL) > > CameraStream::CameraStream(CameraDevice *const cameraDevice, > CameraConfiguration *config, Type type, > - camera3_stream_t *camera3Stream, unsigned int index) > + camera3_stream_t *camera3Stream, > + CameraStream *const sourceStream, unsigned int index) > : cameraDevice_(cameraDevice), config_(config), type_(type), > - camera3Stream_(camera3Stream), index_(index) > + camera3Stream_(camera3Stream), sourceStream_(sourceStream), > + index_(index) > { > } > > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h > index f9504462..4c5078b2 100644 > --- a/src/android/camera_stream.h > +++ b/src/android/camera_stream.h > @@ -114,7 +114,9 @@ public: > }; > CameraStream(CameraDevice *const cameraDevice, > libcamera::CameraConfiguration *config, Type type, > - camera3_stream_t *camera3Stream, unsigned int index); > + camera3_stream_t *camera3Stream, > + CameraStream *const sourceStream, > + unsigned int index); > CameraStream(CameraStream &&other); > ~CameraStream(); > > @@ -122,6 +124,7 @@ public: > camera3_stream_t *camera3Stream() const { return camera3Stream_; } > const libcamera::StreamConfiguration &configuration() const; > libcamera::Stream *stream() const; > + CameraStream *sourceStream() const { return sourceStream_; } > > int configure(); > int process(Camera3RequestDescriptor::StreamBuffer *streamBuffer); > @@ -167,6 +170,7 @@ private: > const libcamera::CameraConfiguration *config_; > const Type type_; > camera3_stream_t *camera3Stream_; > + CameraStream *const sourceStream_; > const unsigned int index_; > > std::unique_ptr<PlatformFrameBufferAllocator> allocator_;
Hi Umang, On Sun, May 29, 2022 at 11:44:14AM +0200, Umang Jain via libcamera-devel wrote: > Hi Paul, > > Thank you for the patch > > On 5/27/22 11:34, Paul Elder via libcamera-devel wrote: > > From: Hirokazu Honda <hiroh@chromium.org> > > > > Add to the CameraStream class a sourceStream field, which for streams > > of type Mapped contains a reference to the stream which produces the > > actual image data. > > > I think some re-pharsing would be nice to better comprehend the changes. > > "Add a sourceStream field to the CameraStream class, meant to contain a > reference > of a direct stream (which produces actual image data) for > CameraStream::Mapped > stream types." > > > > > The sourceStream of mapped streams will be used in later patches to make > > sure for each Mapped stream at least one libcamera::Stream is queued to > > the libcamera::Camera. > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > --- > > Changes in v2: > > - fix whitespace > > --- > > src/android/camera_device.cpp | 8 +++++++- > > src/android/camera_stream.cpp | 6 ++++-- > > src/android/camera_stream.h | 6 +++++- > > 3 files changed, 16 insertions(+), 4 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index 8e804d4d..20599665 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -681,10 +681,16 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > > for (const auto &streamConfig : streamConfigs) { > > config->addConfiguration(streamConfig.config); > > + CameraStream *sourceStream = nullptr; > > for (auto &stream : streamConfig.streams) { > > streams_.emplace_back(this, config.get(), stream.type, > > - stream.stream, config->size() - 1); > > + stream.stream, sourceStream, > > + config->size() - 1); > > stream.stream->priv = static_cast<void *>(&streams_.back()); > > + > > + /* Mapped streams are always associated with a Direct one. */ > > + if (stream.type == CameraStream::Type::Direct) > > > This doesn't look right to me, why are we setting sourceStream field for a > direct stream type? > > Shouldn't be it something like: > > + if (stream.type == CameraStream::Type::Mapped) > + sourceStream = .... > > instead? > I think it's correct the way it is. We populate sourceStream for streams of type Mapped, and initialze it when the corresponding Direct stream is first found in the streamConfig.streams list. > > + sourceStream = &streams_.back(); > > } > > } > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp > > index 94f1884c..154e088e 100644 > > --- a/src/android/camera_stream.cpp > > +++ b/src/android/camera_stream.cpp > > @@ -52,9 +52,11 @@ LOG_DECLARE_CATEGORY(HAL) > > CameraStream::CameraStream(CameraDevice *const cameraDevice, > > CameraConfiguration *config, Type type, > > - camera3_stream_t *camera3Stream, unsigned int index) > > + camera3_stream_t *camera3Stream, > > + CameraStream *const sourceStream, unsigned int index) > > : cameraDevice_(cameraDevice), config_(config), type_(type), > > - camera3Stream_(camera3Stream), index_(index) > > + camera3Stream_(camera3Stream), sourceStream_(sourceStream), > > + index_(index) > > { > > } > > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h > > index f9504462..4c5078b2 100644 > > --- a/src/android/camera_stream.h > > +++ b/src/android/camera_stream.h > > @@ -114,7 +114,9 @@ public: > > }; > > CameraStream(CameraDevice *const cameraDevice, > > libcamera::CameraConfiguration *config, Type type, > > - camera3_stream_t *camera3Stream, unsigned int index); > > + camera3_stream_t *camera3Stream, > > + CameraStream *const sourceStream, > > + unsigned int index); > > CameraStream(CameraStream &&other); > > ~CameraStream(); > > @@ -122,6 +124,7 @@ public: > > camera3_stream_t *camera3Stream() const { return camera3Stream_; } > > const libcamera::StreamConfiguration &configuration() const; > > libcamera::Stream *stream() const; > > + CameraStream *sourceStream() const { return sourceStream_; } > > int configure(); > > int process(Camera3RequestDescriptor::StreamBuffer *streamBuffer); > > @@ -167,6 +170,7 @@ private: > > const libcamera::CameraConfiguration *config_; > > const Type type_; > > camera3_stream_t *camera3Stream_; > > + CameraStream *const sourceStream_; > > const unsigned int index_; > > std::unique_ptr<PlatformFrameBufferAllocator> allocator_;
Hi Jacopo On 5/30/22 11:52, Jacopo Mondi wrote: > Hi Umang, > > On Sun, May 29, 2022 at 11:44:14AM +0200, Umang Jain via libcamera-devel wrote: >> Hi Paul, >> >> Thank you for the patch >> >> On 5/27/22 11:34, Paul Elder via libcamera-devel wrote: >>> From: Hirokazu Honda <hiroh@chromium.org> >>> >>> Add to the CameraStream class a sourceStream field, which for streams >>> of type Mapped contains a reference to the stream which produces the >>> actual image data. >> >> I think some re-pharsing would be nice to better comprehend the changes. >> >> "Add a sourceStream field to the CameraStream class, meant to contain a >> reference >> of a direct stream (which produces actual image data) for >> CameraStream::Mapped >> stream types." >> >>> The sourceStream of mapped streams will be used in later patches to make >>> sure for each Mapped stream at least one libcamera::Stream is queued to >>> the libcamera::Camera. >>> >>> Signed-off-by: Hirokazu Honda <hiroh@chromium.org> >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> >>> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> >>> >>> --- >>> Changes in v2: >>> - fix whitespace >>> --- >>> src/android/camera_device.cpp | 8 +++++++- >>> src/android/camera_stream.cpp | 6 ++++-- >>> src/android/camera_stream.h | 6 +++++- >>> 3 files changed, 16 insertions(+), 4 deletions(-) >>> >>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >>> index 8e804d4d..20599665 100644 >>> --- a/src/android/camera_device.cpp >>> +++ b/src/android/camera_device.cpp >>> @@ -681,10 +681,16 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) >>> for (const auto &streamConfig : streamConfigs) { >>> config->addConfiguration(streamConfig.config); >>> + CameraStream *sourceStream = nullptr; >>> for (auto &stream : streamConfig.streams) { >>> streams_.emplace_back(this, config.get(), stream.type, >>> - stream.stream, config->size() - 1); >>> + stream.stream, sourceStream, >>> + config->size() - 1); >>> stream.stream->priv = static_cast<void *>(&streams_.back()); >>> + >>> + /* Mapped streams are always associated with a Direct one. */ >>> + if (stream.type == CameraStream::Type::Direct) >> >> This doesn't look right to me, why are we setting sourceStream field for a >> direct stream type? >> >> Shouldn't be it something like: >> >> + if (stream.type == CameraStream::Type::Mapped) >> + sourceStream = .... >> >> instead? >> > I think it's correct the way it is. > > We populate sourceStream for streams of type Mapped, and initialze it > when the corresponding Direct stream is first found in the > streamConfig.streams list. Ah yeah, I got it now. I failed to see the sourceStream is a single pointer for the entire inner loop. It's setting the sourceStream on a direct stream and then on next iteration (probably which is a mapped stream), the sourceStream is not-null and hence gets emplace_back corretly. Thanks for clarifying, With the commit message be re-worded a bit, Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > > >>> + sourceStream = &streams_.back(); >>> } >>> } >>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp >>> index 94f1884c..154e088e 100644 >>> --- a/src/android/camera_stream.cpp >>> +++ b/src/android/camera_stream.cpp >>> @@ -52,9 +52,11 @@ LOG_DECLARE_CATEGORY(HAL) >>> CameraStream::CameraStream(CameraDevice *const cameraDevice, >>> CameraConfiguration *config, Type type, >>> - camera3_stream_t *camera3Stream, unsigned int index) >>> + camera3_stream_t *camera3Stream, >>> + CameraStream *const sourceStream, unsigned int index) >>> : cameraDevice_(cameraDevice), config_(config), type_(type), >>> - camera3Stream_(camera3Stream), index_(index) >>> + camera3Stream_(camera3Stream), sourceStream_(sourceStream), >>> + index_(index) >>> { >>> } >>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h >>> index f9504462..4c5078b2 100644 >>> --- a/src/android/camera_stream.h >>> +++ b/src/android/camera_stream.h >>> @@ -114,7 +114,9 @@ public: >>> }; >>> CameraStream(CameraDevice *const cameraDevice, >>> libcamera::CameraConfiguration *config, Type type, >>> - camera3_stream_t *camera3Stream, unsigned int index); >>> + camera3_stream_t *camera3Stream, >>> + CameraStream *const sourceStream, >>> + unsigned int index); >>> CameraStream(CameraStream &&other); >>> ~CameraStream(); >>> @@ -122,6 +124,7 @@ public: >>> camera3_stream_t *camera3Stream() const { return camera3Stream_; } >>> const libcamera::StreamConfiguration &configuration() const; >>> libcamera::Stream *stream() const; >>> + CameraStream *sourceStream() const { return sourceStream_; } >>> int configure(); >>> int process(Camera3RequestDescriptor::StreamBuffer *streamBuffer); >>> @@ -167,6 +170,7 @@ private: >>> const libcamera::CameraConfiguration *config_; >>> const Type type_; >>> camera3_stream_t *camera3Stream_; >>> + CameraStream *const sourceStream_; >>> const unsigned int index_; >>> std::unique_ptr<PlatformFrameBufferAllocator> allocator_;
Hello, On Mon, May 30, 2022 at 12:37:14PM +0200, Umang Jain via libcamera-devel wrote: > On 5/30/22 11:52, Jacopo Mondi wrote: > > On Sun, May 29, 2022 at 11:44:14AM +0200, Umang Jain via libcamera-devel wrote: > >> On 5/27/22 11:34, Paul Elder via libcamera-devel wrote: > >>> From: Hirokazu Honda <hiroh@chromium.org> > >>> > >>> Add to the CameraStream class a sourceStream field, which for streams > >>> of type Mapped contains a reference to the stream which produces the > >>> actual image data. > >> > >> I think some re-pharsing would be nice to better comprehend the changes. > >> > >> "Add a sourceStream field to the CameraStream class, meant to contain a > >> reference > >> of a direct stream (which produces actual image data) for > >> CameraStream::Mapped > >> stream types." > >> > >>> The sourceStream of mapped streams will be used in later patches to make > >>> sure for each Mapped stream at least one libcamera::Stream is queued to > >>> the libcamera::Camera. > >>> > >>> Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >>> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > >>> > >>> --- > >>> Changes in v2: > >>> - fix whitespace > >>> --- > >>> src/android/camera_device.cpp | 8 +++++++- > >>> src/android/camera_stream.cpp | 6 ++++-- > >>> src/android/camera_stream.h | 6 +++++- > >>> 3 files changed, 16 insertions(+), 4 deletions(-) > >>> > >>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > >>> index 8e804d4d..20599665 100644 > >>> --- a/src/android/camera_device.cpp > >>> +++ b/src/android/camera_device.cpp > >>> @@ -681,10 +681,16 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > >>> for (const auto &streamConfig : streamConfigs) { > >>> config->addConfiguration(streamConfig.config); > >>> + CameraStream *sourceStream = nullptr; > >>> for (auto &stream : streamConfig.streams) { > >>> streams_.emplace_back(this, config.get(), stream.type, > >>> - stream.stream, config->size() - 1); > >>> + stream.stream, sourceStream, > >>> + config->size() - 1); > >>> stream.stream->priv = static_cast<void *>(&streams_.back()); > >>> + > >>> + /* Mapped streams are always associated with a Direct one. */ > >>> + if (stream.type == CameraStream::Type::Direct) > >> > >> This doesn't look right to me, why are we setting sourceStream field for a > >> direct stream type? > >> > >> Shouldn't be it something like: > >> > >> + if (stream.type == CameraStream::Type::Mapped) > >> + sourceStream = .... > >> > >> instead? > >> > > I think it's correct the way it is. > > > > We populate sourceStream for streams of type Mapped, and initialze it > > when the corresponding Direct stream is first found in the > > streamConfig.streams list. > > Ah yeah, I got it now. I failed to see the sourceStream is a single > pointer for the entire inner loop. It's setting the sourceStream on a > direct stream and then on next iteration (probably which is a mapped > stream), the sourceStream is not-null and hence gets emplace_back corretly. I think this deserved to be captured in the comment, especially given that we rely on the direct stream being present in the streams list before any mapped stream. /* * The streamConfig.streams vector contains as its first * element a Direct (or Internal) stream, and then an * optional set Mapped streams derived from the * Direct streams. Cache the Direct stream pointer, to * be used when constructing the subsequent mapped * streams. */ > Thanks for clarifying, > > With the commit message be re-worded a bit, > > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > > >>> + sourceStream = &streams_.back(); > >>> } > >>> } > >>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp > >>> index 94f1884c..154e088e 100644 > >>> --- a/src/android/camera_stream.cpp > >>> +++ b/src/android/camera_stream.cpp > >>> @@ -52,9 +52,11 @@ LOG_DECLARE_CATEGORY(HAL) > >>> CameraStream::CameraStream(CameraDevice *const cameraDevice, > >>> CameraConfiguration *config, Type type, > >>> - camera3_stream_t *camera3Stream, unsigned int index) > >>> + camera3_stream_t *camera3Stream, > >>> + CameraStream *const sourceStream, unsigned int index) > >>> : cameraDevice_(cameraDevice), config_(config), type_(type), > >>> - camera3Stream_(camera3Stream), index_(index) > >>> + camera3Stream_(camera3Stream), sourceStream_(sourceStream), > >>> + index_(index) > >>> { > >>> } > >>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h > >>> index f9504462..4c5078b2 100644 > >>> --- a/src/android/camera_stream.h > >>> +++ b/src/android/camera_stream.h > >>> @@ -114,7 +114,9 @@ public: > >>> }; > >>> CameraStream(CameraDevice *const cameraDevice, > >>> libcamera::CameraConfiguration *config, Type type, > >>> - camera3_stream_t *camera3Stream, unsigned int index); > >>> + camera3_stream_t *camera3Stream, > >>> + CameraStream *const sourceStream, > >>> + unsigned int index); > >>> CameraStream(CameraStream &&other); > >>> ~CameraStream(); > >>> @@ -122,6 +124,7 @@ public: > >>> camera3_stream_t *camera3Stream() const { return camera3Stream_; } > >>> const libcamera::StreamConfiguration &configuration() const; > >>> libcamera::Stream *stream() const; > >>> + CameraStream *sourceStream() const { return sourceStream_; } > >>> int configure(); > >>> int process(Camera3RequestDescriptor::StreamBuffer *streamBuffer); > >>> @@ -167,6 +170,7 @@ private: > >>> const libcamera::CameraConfiguration *config_; > >>> const Type type_; > >>> camera3_stream_t *camera3Stream_; > >>> + CameraStream *const sourceStream_; Not a candidate for this patch, but it's time to document this class. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>> const unsigned int index_; > >>> std::unique_ptr<PlatformFrameBufferAllocator> allocator_;
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 8e804d4d..20599665 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -681,10 +681,16 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) for (const auto &streamConfig : streamConfigs) { config->addConfiguration(streamConfig.config); + CameraStream *sourceStream = nullptr; for (auto &stream : streamConfig.streams) { streams_.emplace_back(this, config.get(), stream.type, - stream.stream, config->size() - 1); + stream.stream, sourceStream, + config->size() - 1); stream.stream->priv = static_cast<void *>(&streams_.back()); + + /* Mapped streams are always associated with a Direct one. */ + if (stream.type == CameraStream::Type::Direct) + sourceStream = &streams_.back(); } } diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp index 94f1884c..154e088e 100644 --- a/src/android/camera_stream.cpp +++ b/src/android/camera_stream.cpp @@ -52,9 +52,11 @@ LOG_DECLARE_CATEGORY(HAL) CameraStream::CameraStream(CameraDevice *const cameraDevice, CameraConfiguration *config, Type type, - camera3_stream_t *camera3Stream, unsigned int index) + camera3_stream_t *camera3Stream, + CameraStream *const sourceStream, unsigned int index) : cameraDevice_(cameraDevice), config_(config), type_(type), - camera3Stream_(camera3Stream), index_(index) + camera3Stream_(camera3Stream), sourceStream_(sourceStream), + index_(index) { } diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h index f9504462..4c5078b2 100644 --- a/src/android/camera_stream.h +++ b/src/android/camera_stream.h @@ -114,7 +114,9 @@ public: }; CameraStream(CameraDevice *const cameraDevice, libcamera::CameraConfiguration *config, Type type, - camera3_stream_t *camera3Stream, unsigned int index); + camera3_stream_t *camera3Stream, + CameraStream *const sourceStream, + unsigned int index); CameraStream(CameraStream &&other); ~CameraStream(); @@ -122,6 +124,7 @@ public: camera3_stream_t *camera3Stream() const { return camera3Stream_; } const libcamera::StreamConfiguration &configuration() const; libcamera::Stream *stream() const; + CameraStream *sourceStream() const { return sourceStream_; } int configure(); int process(Camera3RequestDescriptor::StreamBuffer *streamBuffer); @@ -167,6 +170,7 @@ private: const libcamera::CameraConfiguration *config_; const Type type_; camera3_stream_t *camera3Stream_; + CameraStream *const sourceStream_; const unsigned int index_; std::unique_ptr<PlatformFrameBufferAllocator> allocator_;