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

[Kieran Bingham]

Hi Laurent,

On 03/08/2021 13:44, Laurent Pinchart wrote:
> Hi Kieran,
> 
> 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 ...?

> 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 ;-)


>> 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',
>