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

[Laurent Pinchart]

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

> 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