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

[Jacopo Mondi]

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 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.


> > +				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_;