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

[Laurent Pinchart]

Hi Umang,

On Wed, Aug 04, 2021 at 01:20:09PM +0530, Umang Jain wrote:
> On 8/3/21 6:29 PM, Laurent Pinchart wrote:
> > 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 :)

It's a good point, I'll add that.

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

-- 
Regards,

Laurent Pinchart