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

[Umang Jain]

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

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_;
>>>    	const unsigned int index_;
>>>    	std::unique_ptr<PlatformFrameBufferAllocator> allocator_;