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

[Laurent Pinchart]

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 ?
:-)

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

-- 
Regards,

Laurent Pinchart