[libcamera-devel] [PATCH v2 2/8] cam: Add FrameSink base class

[Umang Jain]

Hi Laurent,

On 8/3/21 6:29 PM, Laurent Pinchart wrote:
> Hi Kieran,
>
> On Tue, Aug 03, 2021 at 01:55:38PM +0100, Kieran Bingham wrote:
>> On 03/08/2021 13:44, Laurent Pinchart wrote:
>>> On Tue, Aug 03, 2021 at 11:46:55AM +0100, Kieran Bingham wrote:
>>>> On 30/07/2021 02:03, Laurent Pinchart wrote:
>>>>> The FrameSink class serves as a base to implement components that
>>>>> consume frames. This allows handling frame sinks in a generic way,
>>>>> independent of their nature. The BufferWrite class will be ported to
>>>>> FrameSink in a second step.
>>>>>
>>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>>>> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
>>>>> ---
>>>>>   src/cam/frame_sink.cpp | 31 +++++++++++++++++++++++++++++++
>>>>>   src/cam/frame_sink.h   | 34 ++++++++++++++++++++++++++++++++++
>>>>>   src/cam/meson.build    |  1 +
>>>>>   3 files changed, 66 insertions(+)
>>>>>   create mode 100644 src/cam/frame_sink.cpp
>>>>>   create mode 100644 src/cam/frame_sink.h
>>>>>
>>>>> diff --git a/src/cam/frame_sink.cpp b/src/cam/frame_sink.cpp
>>>>> new file mode 100644
>>>>> index 000000000000..6e15c1007f12
>>>>> --- /dev/null
>>>>> +++ b/src/cam/frame_sink.cpp
>>>>> @@ -0,0 +1,31 @@
>>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>>> +/*
>>>>> + * Copyright (C) 2020, Ideas on Board Oy
>>>> 2021 now?
>>>>
>>>>> + *
>>>>> + * frame_sink.cpp - Base Frame Sink Class
>>>>> + */
>>>>> +
>>>>> +#include "frame_sink.h"
>>>>> +
>>>>> +FrameSink::~FrameSink()
>>>>> +{
>>>>> +}
>>>>> +
>>>>> +int FrameSink::configure([[maybe_unused]] const libcamera::CameraConfiguration &config)
>>>> I wonder if a FrameSink should be configured based on a
>>>> StreamConfiguration rather than a CameraConfiguration ...
>>>>
>>>> Presumably each FrameSink represents a single stream - not the full
>>>> camera outputs...
>>> FrameSink models a sink for a complete request, not for a single buffer.
>> Then should it be a RequestSink ... not a FrameSink?
>>
>> To me a FrameSink - sinks a single frame ... that's why I would have
>> expected this to be modelled around a single stream.
>>
>> I would have thought that one FrameSink would be created per stream, and
>> when a request completes, if there is a sink configured for the streams
>> it has - it can process the Frames through the FrameSink ...?
> I'm not sure exactly how it would be architectured, there will be
> details to handle, those are usually annoying. I hope it's not a blocker
> :-)
>
> RequestSink sounds a bit weird as a name, even if I've thought about it
> as well when replying to your previous e-mails. Maybe, for the meantime,
> we could think about FrameSink as an abstract concept, like a dishwasher
> doesn't have to be called a disheswasher to wash more than one dish ?
> :-)

Usually there is one / two line description of the class (especially 
parent classes) at the top of the file, after the licensing. It's 
"nice-to-have" else one can dig into git log though, to know about the 
specifics. Since as you say, this is an "abstract" concept (maybe an 
abstract class as well?), I got reminded that these the brief 
descriptions of the class are useful :)

Anyways,

Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>

>
>>> It could be interesting to change that, for instance to write some
>>> streams to disk and display other streams on the screen (not entirely
>>> sure about the use cases though). However, even in that case, we'll have
>>> to bundle multiple streams together somehow, as the KMS sink would need
>>> to update all planes at once, not sequentially and in isolation. I'd
>>> consider this a topic for future research :-)
>> afterall we're just re-implementing gstreamer anyway ;-)
> My thoughts as well :-)
>
>>>> But lets see - maybe that's handled differently later.
>>>>
>>>> Perhaps tentatively:
>>>>
>>>> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>>>
>>>>> +{
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +void FrameSink::mapBuffer([[maybe_unused]] libcamera::FrameBuffer *buffer)
>>>>> +{
>>>>> +}
>>>>> +
>>>>> +int FrameSink::start()
>>>>> +{
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +int FrameSink::stop()
>>>>> +{
>>>>> +	return 0;
>>>>> +}
>>>>> diff --git a/src/cam/frame_sink.h b/src/cam/frame_sink.h
>>>>> new file mode 100644
>>>>> index 000000000000..116e5267bebe
>>>>> --- /dev/null
>>>>> +++ b/src/cam/frame_sink.h
>>>>> @@ -0,0 +1,34 @@
>>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>>> +/*
>>>>> + * Copyright (C) 2020, Ideas on Board Oy
>>>> Same,
>>>>
>>>>> + *
>>>>> + * frame_sink.h - Base Frame Sink Class
>>>>> + */
>>>>> +#ifndef __CAM_FRAME_SINK_H__
>>>>> +#define __CAM_FRAME_SINK_H__
>>>>> +
>>>>> +#include <libcamera/base/signal.h>
>>>>> +
>>>>> +namespace libcamera {
>>>>> +class CameraConfiguration;
>>>>> +class FrameBuffer;
>>>>> +class Request;
>>>>> +} /* namespace libcamera */
>>>>> +
>>>>> +class FrameSink
>>>>> +{
>>>>> +public:
>>>>> +	virtual ~FrameSink();
>>>>> +
>>>>> +	virtual int configure(const libcamera::CameraConfiguration &config);
>>>>> +
>>>>> +	virtual void mapBuffer(libcamera::FrameBuffer *buffer);
>>>>> +
>>>>> +	virtual int start();
>>>>> +	virtual int stop();
>>>>> +
>>>>> +	virtual bool consumeRequest(libcamera::Request *request) = 0;
>>>>> +	libcamera::Signal<libcamera::Request *> requestReleased;
>>>>> +};
>>>>> +
>>>>> +#endif /* __CAM_FRAME_SINK_H__ */
>>>>> diff --git a/src/cam/meson.build b/src/cam/meson.build
>>>>> index 1e90ee521f74..649cc990d867 100644
>>>>> --- a/src/cam/meson.build
>>>>> +++ b/src/cam/meson.build
>>>>> @@ -13,6 +13,7 @@ cam_sources = files([
>>>>>       'buffer_writer.cpp',
>>>>>       'camera_session.cpp',
>>>>>       'event_loop.cpp',
>>>>> +    'frame_sink.cpp',
>>>>>       'main.cpp',
>>>>>       'options.cpp',
>>>>>       'stream_options.cpp',