[libcamera-devel] [PATCH v2 1/5] android: camera_stream: Add sourceStream

[Laurent Pinchart]

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 at 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 at chromium.org>
> >>> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> >>> Signed-off-by: Paul Elder <paul.elder at 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 at 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 at ideasonboard.com>

> >>>    	const unsigned int index_;
> >>>    	std::unique_ptr<PlatformFrameBufferAllocator> allocator_;

-- 
Regards,

Laurent Pinchart