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

13147 diff mbox series

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@ideasonboard.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@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

Hi Laurent,

On Fri, Jul 30, 2021 at 04:03:00AM +0300, 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@ideasonboard.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  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
> + *
> + * frame_sink.cpp - Base Frame Sink Class
> + */
> +
> +#include "frame_sink.h"
> +
> +FrameSink::~FrameSink()
> +{
> +}
> +
> +int FrameSink::configure([[maybe_unused]] const libcamera::CameraConfiguration &config)
> +{
> +	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
> + *
> + * 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
>

Hi Laurent,

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@ideasonboard.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@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...

But lets see - maybe that's handled differently later.

Perhaps tentatively:

Reviewed-by: Kieran Bingham <kieran.bingham@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',
>

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@ideasonboard.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@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@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',

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@ideasonboard.com>
>>> Reviewed-by: Niklas Söderlund <niklas.soderlund@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@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',
>

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@ideasonboard.com>
> >>> Reviewed-by: Niklas Söderlund <niklas.soderlund@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@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',
> >

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@ideasonboard.com>
>>>>> Reviewed-by: Niklas Söderlund <niklas.soderlund@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@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@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',

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@ideasonboard.com>
> >>>>> Reviewed-by: Niklas Söderlund <niklas.soderlund@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@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@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',