[libcamera-devel,v4] cam: sdl_sink: Add SDL sink

15580 diff mbox series

This adds more portability to existing cam sinks. You can pass a
YUYV camera buffer for example and SDL will handle the pixel buffer
conversion, although SDL does not support decompression for pixelformats
like MJPEG. This allows cam reference implementation to display images
on VMs, Mac M1, Raspberry Pi, etc. This also enables cam reference
implementation, to run as a desktop application in wayland or x11.
SDL also has support for Android and ChromeOS which I have not tested.
Also tested on simpledrm raspberry pi 4 framebuffer successfully where
existing kms sink did not work. Can also be used as kmsdrm sink.

Signed-off-by: Eric Curtin <ecurtin@redhat.com>
---
Changes in v2:
 - Remove hardcoded pixel format from SDL_CreateTexture call

Changes in v3:
 - Drop blank line
 - The contents of the if..endif are indented
 - Split configure function into start and configure
 - Add SDL_DestroyRenderer
 - Remove assign and test in the same statement

Changes in v4:
 - Add processSDLEvents as a timed event in the evloop
---
 src/cam/camera_session.cpp |   8 +++
 src/cam/event_loop.cpp     |  10 ++-
 src/cam/event_loop.h       |   1 +
 src/cam/main.cpp           |   5 ++
 src/cam/main.h             |   1 +
 src/cam/meson.build        |  10 +++
 src/cam/sdl_sink.cpp       | 142 +++++++++++++++++++++++++++++++++++++
 src/cam/sdl_sink.h         |  42 +++++++++++
 8 files changed, 218 insertions(+), 1 deletion(-)
 create mode 100644 src/cam/sdl_sink.cpp
 create mode 100644 src/cam/sdl_sink.h

Hi Eric,

Thank you for the patch.

On Tue, Mar 29, 2022 at 12:17:25PM +0100, Eric Curtin via libcamera-devel wrote:
> This adds more portability to existing cam sinks. You can pass a
> YUYV camera buffer for example and SDL will handle the pixel buffer
> conversion, although SDL does not support decompression for pixelformats
> like MJPEG. This allows cam reference implementation to display images
> on VMs, Mac M1, Raspberry Pi, etc. This also enables cam reference
> implementation, to run as a desktop application in wayland or x11.
> SDL also has support for Android and ChromeOS which I have not tested.
> Also tested on simpledrm raspberry pi 4 framebuffer successfully where
> existing kms sink did not work. Can also be used as kmsdrm sink.
> 
> Signed-off-by: Eric Curtin <ecurtin@redhat.com>
> ---
> Changes in v2:
>  - Remove hardcoded pixel format from SDL_CreateTexture call
> 
> Changes in v3:
>  - Drop blank line
>  - The contents of the if..endif are indented
>  - Split configure function into start and configure
>  - Add SDL_DestroyRenderer
>  - Remove assign and test in the same statement
> 
> Changes in v4:
>  - Add processSDLEvents as a timed event in the evloop
> ---
>  src/cam/camera_session.cpp |   8 +++
>  src/cam/event_loop.cpp     |  10 ++-
>  src/cam/event_loop.h       |   1 +
>  src/cam/main.cpp           |   5 ++
>  src/cam/main.h             |   1 +
>  src/cam/meson.build        |  10 +++
>  src/cam/sdl_sink.cpp       | 142 +++++++++++++++++++++++++++++++++++++
>  src/cam/sdl_sink.h         |  42 +++++++++++
>  8 files changed, 218 insertions(+), 1 deletion(-)
>  create mode 100644 src/cam/sdl_sink.cpp
>  create mode 100644 src/cam/sdl_sink.h
> 
> diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp
> index 0428b538..30162dbd 100644
> --- a/src/cam/camera_session.cpp
> +++ b/src/cam/camera_session.cpp
> @@ -19,6 +19,9 @@
>  #ifdef HAVE_KMS
>  #include "kms_sink.h"
>  #endif
> +#ifdef HAVE_SDL
> +#include "sdl_sink.h"
> +#endif
>  #include "main.h"
>  #include "stream_options.h"
>  
> @@ -187,6 +190,11 @@ int CameraSession::start()
>  		sink_ = std::make_unique<KMSSink>(options_[OptDisplay].toString());
>  #endif
>  
> +#ifdef HAVE_SDL
> +	if (options_.isSet(OptSDL))
> +		sink_ = std::make_unique<SDLSink>();
> +#endif
> +
>  	if (options_.isSet(OptFile)) {
>  		if (!options_[OptFile].toString().empty())
>  			sink_ = std::make_unique<FileSink>(streamNames_,
> diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp
> index e25784c0..41339d4e 100644
> --- a/src/cam/event_loop.cpp
> +++ b/src/cam/event_loop.cpp
> @@ -75,7 +75,15 @@ void EventLoop::addEvent(int fd, EventType type,
>  		return;
>  	}
>  
> -	int ret = event_add(event->event_, nullptr);
> +	struct timeval *tp = nullptr;
> +	struct timeval tv;
> +	if (events == EV_PERSIST) {
> +		tp = &tv;
> +		tv.tv_sec = 0;
> +		tv.tv_usec = 10000; /* every 10 ms */
> +	}

Could you please split the change to the event loop to a separate patch,
with an explanation of what it does in the commit message ? This isn't
trivial to understand.

> +
> +	int ret = event_add(event->event_, tp);
>  	if (ret < 0) {
>  		std::cerr << "Failed to add event for fd " << fd << std::endl;
>  		return;
> diff --git a/src/cam/event_loop.h b/src/cam/event_loop.h
> index a4613eb2..8fd9bd20 100644
> --- a/src/cam/event_loop.h
> +++ b/src/cam/event_loop.h
> @@ -20,6 +20,7 @@ class EventLoop
>  {
>  public:
>  	enum EventType {
> +		Default = 0, /* the event can be triggered only by a timeout or by manual activation */
>  		Read = 1,
>  		Write = 2,
>  	};
> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> index c7f664b9..1d62a64a 100644
> --- a/src/cam/main.cpp
> +++ b/src/cam/main.cpp
> @@ -137,6 +137,11 @@ int CamApp::parseOptions(int argc, char *argv[])
>  			 "Display viewfinder through DRM/KMS on specified connector",
>  			 "display", ArgumentOptional, "connector", false,
>  			 OptCamera);
> +#endif
> +#ifdef HAVE_SDL
> +	parser.addOption(OptSDL, OptionNone,
> +			 "Display viewfinder through SDL",
> +			 "sdl", ArgumentNone, "", false, OptCamera);
>  #endif
>  	parser.addOption(OptFile, OptionString,
>  			 "Write captured frames to disk\n"
> diff --git a/src/cam/main.h b/src/cam/main.h
> index 62f7bbc9..a64f95a0 100644
> --- a/src/cam/main.h
> +++ b/src/cam/main.h
> @@ -11,6 +11,7 @@ enum {
>  	OptCamera = 'c',
>  	OptCapture = 'C',
>  	OptDisplay = 'D',
> +	OptSDL = 'S',

Let's keep the options sorted.

>  	OptFile = 'F',
>  	OptHelp = 'h',
>  	OptInfo = 'I',
> diff --git a/src/cam/meson.build b/src/cam/meson.build
> index e8e2ae57..bd536c5b 100644
> --- a/src/cam/meson.build
> +++ b/src/cam/meson.build
> @@ -32,11 +32,21 @@ cam_sources += files([
>  ])
>  endif
>  
> +libsdl2 = dependency('SDL2', required : false)
> +
> +if libsdl2.found()
> +    cam_cpp_args += [ '-DHAVE_SDL' ]

No space within the [ ].

> +    cam_sources += files([
> +        'sdl_sink.cpp'
> +    ])
> +endif
> +
>  cam  = executable('cam', cam_sources,
>                    dependencies : [
>                        libatomic,
>                        libcamera_public,
>                        libdrm,
> +                      libsdl2,
>                        libevent,
>                    ],
>                    cpp_args : cam_cpp_args,
> diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp
> new file mode 100644
> index 00000000..6c6e37d0
> --- /dev/null
> +++ b/src/cam/sdl_sink.cpp
> @@ -0,0 +1,142 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * sdl_sink.cpp - SDL Sink
> + */
> +
> +#include "sdl_sink.h"
> +
> +#include <assert.h>
> +#include <fcntl.h>
> +#include <iomanip>
> +#include <iostream>
> +#include <signal.h>
> +#include <sstream>
> +#include <string.h>
> +#include <unistd.h>
> +
> +#include <libcamera/camera.h>
> +#include <libcamera/formats.h>
> +
> +#include "event_loop.h"
> +#include "image.h"
> +
> +using namespace libcamera;
> +
> +SDLSink::SDLSink()
> +	: sdlRenderer_(0)

It's a pointer, should be initialized to nullptr, not 0.

> +{
> +	memset(&sdlRect_, 0, sizeof(sdlRect_));

This could be initialized with

	sdlRect_({})

in the constructor member initializers list.

> +}
> +
> +SDLSink::~SDLSink()
> +{
> +	if (sdlRenderer_)
> +		SDL_DestroyRenderer(sdlRenderer_);
> +	SDL_Quit();

Shouldn't the cleanup go to stop(), now that you have the initialization
in start() ?

No need for any cleanup on sdlScreen_ and sdlTexture_ ?

> +}
> +
> +int SDLSink::configure(const libcamera::CameraConfiguration &cfg)
> +{
> +	int ret = FrameSink::configure(cfg);
> +	if (ret < 0)
> +		return ret;
> +
> +	const libcamera::StreamConfiguration &sCfg = cfg.at(0);
> +	pf = (SDL_PixelFormatEnum)sCfg.pixelFormat.fourcc();
> +	if (pf == SDL_DEFINE_PIXELFOURCC('Y', 'U', 'Y', 'V')) {
> +		pf = SDL_PIXELFORMAT_YUY2;
> +	} else if (int ne = strcmp(SDL_GetPixelFormatName(pf), "SDL_PIXELFORMAT_UNKNOWN"); !ne) {

No variable declaration in an if () statement.

> +		std::cerr << "SDL_GetPixelFormatName error - exiting: SDL_PIXELFORMAT_UNKNOWN, no " << sCfg.pixelFormat.toString() << " support\n";

Those two lines are too long.

Replace \n with std::endl

> +		return -EINVAL;
> +	}

I don't think this is right. SDL pixel formats don't map directly to the
4CC values used by libcamera. You need a translation table.

> +
> +	sdlRect_.w = sCfg.size.width;
> +	sdlRect_.h = sCfg.size.height;
> +
> +	return 0;
> +}
> +
> +int SDLSink::start()
> +{
> +	int ret = SDL_Init(SDL_INIT_VIDEO);
> +	if (ret) {
> +		std::cerr << "SDL_Init error - exiting: " << SDL_GetError() << std::endl;

		std::cerr << "Failed to initialize SDL: " << SDL_GetError()
			  << std::endl;

and similarly below.

> +		return ret;
> +	}
> +
> +	sdlScreen_ = SDL_CreateWindow("", SDL_WINDOWPOS_UNDEFINED,
> +				      SDL_WINDOWPOS_UNDEFINED, sdlRect_.w,
> +				      sdlRect_.h, SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE);
> +	if (!sdlScreen_) {
> +		std::cerr << "SDL_CreateWindow error - exiting: " << SDL_GetError() << std::endl;
> +		return -EINVAL;
> +	}
> +
> +	sdlRenderer_ = SDL_CreateRenderer(
> +		sdlScreen_, -1, 0);
> +	if (!sdlRenderer_) {
> +		std::cerr << "SDL_CreateRenderer error - exiting: " << SDL_GetError() << std::endl;
> +		return -EINVAL;
> +	}
> +
> +	SDL_RenderSetLogicalSize(sdlRenderer_, sdlRect_.w,
> +				 sdlRect_.h);
> +
> +	sdlTexture_ =
> +		SDL_CreateTexture(sdlRenderer_, pf,
> +				  SDL_TEXTUREACCESS_STREAMING, sdlRect_.w,
> +				  sdlRect_.h);
> +	EventLoop::instance()->addEvent(-1, EventLoop::Default,
> +					std::bind(&SDLSink::processSDLEvents, this));
> +
> +	return 0;
> +}
> +
> +void SDLSink::mapBuffer(FrameBuffer *buffer)
> +{
> +	std::unique_ptr<Image> image =
> +		Image::fromFrameBuffer(buffer, Image::MapMode::ReadOnly);
> +	assert(image != nullptr);
> +
> +	mappedBuffers_[buffer] = std::move(image);
> +}
> +
> +bool SDLSink::processRequest(Request *request)
> +{
> +	for (auto [stream, buffer] : request->buffers())
> +		writeBuffer(buffer);

Will this really work properly if the request contains multiple buffers
?

> +
> +	return true;
> +}
> +
> +/*
> + * Process SDL events, required for things like window resize and quit button
> + */
> +void SDLSink::processSDLEvents()
> +{
> +	for (SDL_Event e; SDL_PollEvent(&e);) {
> +		if (e.type == SDL_QUIT) { /* click close icon then quit */
> +			kill(getpid(), SIGINT);

That's too harsh. Calling the exit() function of the event loop should
be enough.

> +		}
> +	}
> +}
> +
> +void SDLSink::writeBuffer(FrameBuffer *buffer)

This function doesn't "write" a buffer, it should be named differently.

> +{
> +	Image *image = mappedBuffers_[buffer].get();
> +
> +	for (unsigned int i = 0; i < buffer->planes().size(); ++i) {
> +		const FrameMetadata::Plane &meta = buffer->metadata().planes()[i];
> +
> +		Span<uint8_t> data = image->data(i);
> +		if (meta.bytesused > data.size())
> +			std::cerr << "payload size " << meta.bytesused
> +				  << " larger than plane size " << data.size()
> +				  << std::endl;
> +
> +		SDL_UpdateTexture(sdlTexture_, &sdlRect_, data.data(), sdlRect_.w * 2);

Is the last argument format-dependent ?

> +		SDL_RenderClear(sdlRenderer_);
> +		SDL_RenderCopy(sdlRenderer_, sdlTexture_, NULL, NULL);

s/NULL/nullptr/

> +		SDL_RenderPresent(sdlRenderer_);
> +	}
> +}
> diff --git a/src/cam/sdl_sink.h b/src/cam/sdl_sink.h
> new file mode 100644
> index 00000000..6f20e2d6
> --- /dev/null
> +++ b/src/cam/sdl_sink.h
> @@ -0,0 +1,42 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * sdl_sink.h - SDL Sink
> + */
> +
> +#pragma once
> +
> +#include <map>
> +#include <memory>
> +#include <string>
> +
> +#include <libcamera/stream.h>
> +
> +#include <SDL2/SDL.h>
> +
> +#include "frame_sink.h"
> +
> +class Image;
> +
> +class SDLSink : public FrameSink
> +{
> +public:
> +	SDLSink();
> +	~SDLSink();
> +
> +	int configure(const libcamera::CameraConfiguration &cfg) override;
> +	int start() override;
> +	void mapBuffer(libcamera::FrameBuffer *buffer) override;
> +
> +	bool processRequest(libcamera::Request *request) override;
> +
> +private:
> +	void writeBuffer(libcamera::FrameBuffer *buffer);
> +	void processSDLEvents();
> +
> +	std::map<libcamera::FrameBuffer *, std::unique_ptr<Image>> mappedBuffers_;

I'd add a blank line here.

> +	SDL_Window *sdlScreen_;
> +	SDL_Renderer *sdlRenderer_;
> +	SDL_Texture *sdlTexture_;
> +	SDL_Rect sdlRect_;
> +	SDL_PixelFormatEnum pf;

s/pf/pixelFormat_/

> +};

On Tue, 29 Mar 2022 at 16:10, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Eric,
>
> Thank you for the patch.
>
> On Tue, Mar 29, 2022 at 12:17:25PM +0100, Eric Curtin via libcamera-devel wrote:
> > This adds more portability to existing cam sinks. You can pass a
> > YUYV camera buffer for example and SDL will handle the pixel buffer
> > conversion, although SDL does not support decompression for pixelformats
> > like MJPEG. This allows cam reference implementation to display images
> > on VMs, Mac M1, Raspberry Pi, etc. This also enables cam reference
> > implementation, to run as a desktop application in wayland or x11.
> > SDL also has support for Android and ChromeOS which I have not tested.
> > Also tested on simpledrm raspberry pi 4 framebuffer successfully where
> > existing kms sink did not work. Can also be used as kmsdrm sink.
> >
> > Signed-off-by: Eric Curtin <ecurtin@redhat.com>
> > ---
> > Changes in v2:
> >  - Remove hardcoded pixel format from SDL_CreateTexture call
> >
> > Changes in v3:
> >  - Drop blank line
> >  - The contents of the if..endif are indented
> >  - Split configure function into start and configure
> >  - Add SDL_DestroyRenderer
> >  - Remove assign and test in the same statement
> >
> > Changes in v4:
> >  - Add processSDLEvents as a timed event in the evloop
> > ---
> >  src/cam/camera_session.cpp |   8 +++
> >  src/cam/event_loop.cpp     |  10 ++-
> >  src/cam/event_loop.h       |   1 +
> >  src/cam/main.cpp           |   5 ++
> >  src/cam/main.h             |   1 +
> >  src/cam/meson.build        |  10 +++
> >  src/cam/sdl_sink.cpp       | 142 +++++++++++++++++++++++++++++++++++++
> >  src/cam/sdl_sink.h         |  42 +++++++++++
> >  8 files changed, 218 insertions(+), 1 deletion(-)
> >  create mode 100644 src/cam/sdl_sink.cpp
> >  create mode 100644 src/cam/sdl_sink.h
> >
> > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp
> > index 0428b538..30162dbd 100644
> > --- a/src/cam/camera_session.cpp
> > +++ b/src/cam/camera_session.cpp
> > @@ -19,6 +19,9 @@
> >  #ifdef HAVE_KMS
> >  #include "kms_sink.h"
> >  #endif
> > +#ifdef HAVE_SDL
> > +#include "sdl_sink.h"
> > +#endif
> >  #include "main.h"
> >  #include "stream_options.h"
> >
> > @@ -187,6 +190,11 @@ int CameraSession::start()
> >               sink_ = std::make_unique<KMSSink>(options_[OptDisplay].toString());
> >  #endif
> >
> > +#ifdef HAVE_SDL
> > +     if (options_.isSet(OptSDL))
> > +             sink_ = std::make_unique<SDLSink>();
> > +#endif
> > +
> >       if (options_.isSet(OptFile)) {
> >               if (!options_[OptFile].toString().empty())
> >                       sink_ = std::make_unique<FileSink>(streamNames_,
> > diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp
> > index e25784c0..41339d4e 100644
> > --- a/src/cam/event_loop.cpp
> > +++ b/src/cam/event_loop.cpp
> > @@ -75,7 +75,15 @@ void EventLoop::addEvent(int fd, EventType type,
> >               return;
> >       }
> >
> > -     int ret = event_add(event->event_, nullptr);
> > +     struct timeval *tp = nullptr;
> > +     struct timeval tv;
> > +     if (events == EV_PERSIST) {
> > +             tp = &tv;
> > +             tv.tv_sec = 0;
> > +             tv.tv_usec = 10000; /* every 10 ms */
> > +     }
>
> Could you please split the change to the event loop to a separate patch,
> with an explanation of what it does in the commit message ? This isn't
> trivial to understand.

Ok.

>
> > +
> > +     int ret = event_add(event->event_, tp);
> >       if (ret < 0) {
> >               std::cerr << "Failed to add event for fd " << fd << std::endl;
> >               return;
> > diff --git a/src/cam/event_loop.h b/src/cam/event_loop.h
> > index a4613eb2..8fd9bd20 100644
> > --- a/src/cam/event_loop.h
> > +++ b/src/cam/event_loop.h
> > @@ -20,6 +20,7 @@ class EventLoop
> >  {
> >  public:
> >       enum EventType {
> > +             Default = 0, /* the event can be triggered only by a timeout or by manual activation */
> >               Read = 1,
> >               Write = 2,
> >       };
> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> > index c7f664b9..1d62a64a 100644
> > --- a/src/cam/main.cpp
> > +++ b/src/cam/main.cpp
> > @@ -137,6 +137,11 @@ int CamApp::parseOptions(int argc, char *argv[])
> >                        "Display viewfinder through DRM/KMS on specified connector",
> >                        "display", ArgumentOptional, "connector", false,
> >                        OptCamera);
> > +#endif
> > +#ifdef HAVE_SDL
> > +     parser.addOption(OptSDL, OptionNone,
> > +                      "Display viewfinder through SDL",
> > +                      "sdl", ArgumentNone, "", false, OptCamera);
> >  #endif
> >       parser.addOption(OptFile, OptionString,
> >                        "Write captured frames to disk\n"
> > diff --git a/src/cam/main.h b/src/cam/main.h
> > index 62f7bbc9..a64f95a0 100644
> > --- a/src/cam/main.h
> > +++ b/src/cam/main.h
> > @@ -11,6 +11,7 @@ enum {
> >       OptCamera = 'c',
> >       OptCapture = 'C',
> >       OptDisplay = 'D',
> > +     OptSDL = 'S',
>
> Let's keep the options sorted.

Ok.

>
> >       OptFile = 'F',
> >       OptHelp = 'h',
> >       OptInfo = 'I',
> > diff --git a/src/cam/meson.build b/src/cam/meson.build
> > index e8e2ae57..bd536c5b 100644
> > --- a/src/cam/meson.build
> > +++ b/src/cam/meson.build
> > @@ -32,11 +32,21 @@ cam_sources += files([
> >  ])
> >  endif
> >
> > +libsdl2 = dependency('SDL2', required : false)
> > +
> > +if libsdl2.found()
> > +    cam_cpp_args += [ '-DHAVE_SDL' ]
>
> No space within the [ ].

Ok.

>
> > +    cam_sources += files([
> > +        'sdl_sink.cpp'
> > +    ])
> > +endif
> > +
> >  cam  = executable('cam', cam_sources,
> >                    dependencies : [
> >                        libatomic,
> >                        libcamera_public,
> >                        libdrm,
> > +                      libsdl2,
> >                        libevent,
> >                    ],
> >                    cpp_args : cam_cpp_args,
> > diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp
> > new file mode 100644
> > index 00000000..6c6e37d0
> > --- /dev/null
> > +++ b/src/cam/sdl_sink.cpp
> > @@ -0,0 +1,142 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * sdl_sink.cpp - SDL Sink
> > + */
> > +
> > +#include "sdl_sink.h"
> > +
> > +#include <assert.h>
> > +#include <fcntl.h>
> > +#include <iomanip>
> > +#include <iostream>
> > +#include <signal.h>
> > +#include <sstream>
> > +#include <string.h>
> > +#include <unistd.h>
> > +
> > +#include <libcamera/camera.h>
> > +#include <libcamera/formats.h>
> > +
> > +#include "event_loop.h"
> > +#include "image.h"
> > +
> > +using namespace libcamera;
> > +
> > +SDLSink::SDLSink()
> > +     : sdlRenderer_(0)
>
> It's a pointer, should be initialized to nullptr, not 0.

Ok.

>
> > +{
> > +     memset(&sdlRect_, 0, sizeof(sdlRect_));
>
> This could be initialized with
>
>         sdlRect_({})
>
> in the constructor member initializers list.

Ok.

>
> > +}
> > +
> > +SDLSink::~SDLSink()
> > +{
> > +     if (sdlRenderer_)
> > +             SDL_DestroyRenderer(sdlRenderer_);
> > +     SDL_Quit();
>
> Shouldn't the cleanup go to stop(), now that you have the initialization
> in start() ?

Yes thanks for spotting.

>
> No need for any cleanup on sdlScreen_ and sdlTexture_ ?

No need.

>
> > +}
> > +
> > +int SDLSink::configure(const libcamera::CameraConfiguration &cfg)
> > +{
> > +     int ret = FrameSink::configure(cfg);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     const libcamera::StreamConfiguration &sCfg = cfg.at(0);
> > +     pf = (SDL_PixelFormatEnum)sCfg.pixelFormat.fourcc();
> > +     if (pf == SDL_DEFINE_PIXELFOURCC('Y', 'U', 'Y', 'V')) {
> > +             pf = SDL_PIXELFORMAT_YUY2;
> > +     } else if (int ne = strcmp(SDL_GetPixelFormatName(pf), "SDL_PIXELFORMAT_UNKNOWN"); !ne) {
>
> No variable declaration in an if () statement.

Ok.

>
> > +             std::cerr << "SDL_GetPixelFormatName error - exiting: SDL_PIXELFORMAT_UNKNOWN, no " << sCfg.pixelFormat.toString() << " support\n";
>
> Those two lines are too long.

Ok.

>
> Replace \n with std::endl
>
> > +             return -EINVAL;
> > +     }
>
> I don't think this is right. SDL pixel formats don't map directly to the
> 4CC values used by libcamera. You need a translation table.

I can start one, I won't be able to test it much as I don't have the
hardware to do that but I can start.

>
> > +
> > +     sdlRect_.w = sCfg.size.width;
> > +     sdlRect_.h = sCfg.size.height;
> > +
> > +     return 0;
> > +}
> > +
> > +int SDLSink::start()
> > +{
> > +     int ret = SDL_Init(SDL_INIT_VIDEO);
> > +     if (ret) {
> > +             std::cerr << "SDL_Init error - exiting: " << SDL_GetError() << std::endl;
>
>                 std::cerr << "Failed to initialize SDL: " << SDL_GetError()
>                           << std::endl;
>
> and similarly below.

Ok

>
> > +             return ret;
> > +     }
> > +
> > +     sdlScreen_ = SDL_CreateWindow("", SDL_WINDOWPOS_UNDEFINED,
> > +                                   SDL_WINDOWPOS_UNDEFINED, sdlRect_.w,
> > +                                   sdlRect_.h, SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE);
> > +     if (!sdlScreen_) {
> > +             std::cerr << "SDL_CreateWindow error - exiting: " << SDL_GetError() << std::endl;
> > +             return -EINVAL;
> > +     }
> > +
> > +     sdlRenderer_ = SDL_CreateRenderer(
> > +             sdlScreen_, -1, 0);
> > +     if (!sdlRenderer_) {
> > +             std::cerr << "SDL_CreateRenderer error - exiting: " << SDL_GetError() << std::endl;
> > +             return -EINVAL;
> > +     }
> > +
> > +     SDL_RenderSetLogicalSize(sdlRenderer_, sdlRect_.w,
> > +                              sdlRect_.h);
> > +
> > +     sdlTexture_ =
> > +             SDL_CreateTexture(sdlRenderer_, pf,
> > +                               SDL_TEXTUREACCESS_STREAMING, sdlRect_.w,
> > +                               sdlRect_.h);
> > +     EventLoop::instance()->addEvent(-1, EventLoop::Default,
> > +                                     std::bind(&SDLSink::processSDLEvents, this));
> > +
> > +     return 0;
> > +}
> > +
> > +void SDLSink::mapBuffer(FrameBuffer *buffer)
> > +{
> > +     std::unique_ptr<Image> image =
> > +             Image::fromFrameBuffer(buffer, Image::MapMode::ReadOnly);
> > +     assert(image != nullptr);
> > +
> > +     mappedBuffers_[buffer] = std::move(image);
> > +}
> > +
> > +bool SDLSink::processRequest(Request *request)
> > +{
> > +     for (auto [stream, buffer] : request->buffers())
> > +             writeBuffer(buffer);
>
> Will this really work properly if the request contains multiple buffers
> ?

I will write more code to solve this.

>
> > +
> > +     return true;
> > +}
> > +
> > +/*
> > + * Process SDL events, required for things like window resize and quit button
> > + */
> > +void SDLSink::processSDLEvents()
> > +{
> > +     for (SDL_Event e; SDL_PollEvent(&e);) {
> > +             if (e.type == SDL_QUIT) { /* click close icon then quit */
> > +                     kill(getpid(), SIGINT);
>
> That's too harsh. Calling the exit() function of the event loop should
> be enough.

I'll try that, my first thought was to try and make it behave like
when you type Ctrl-C as much as possible, but yeah it's weird this
way, a process calling it's own signal handler.

>
> > +             }
> > +     }
> > +}
> > +
> > +void SDLSink::writeBuffer(FrameBuffer *buffer)
>
> This function doesn't "write" a buffer, it should be named differently.

renderBuffer maybe?

>
> > +{
> > +     Image *image = mappedBuffers_[buffer].get();
> > +
> > +     for (unsigned int i = 0; i < buffer->planes().size(); ++i) {
> > +             const FrameMetadata::Plane &meta = buffer->metadata().planes()[i];
> > +
> > +             Span<uint8_t> data = image->data(i);
> > +             if (meta.bytesused > data.size())
> > +                     std::cerr << "payload size " << meta.bytesused
> > +                               << " larger than plane size " << data.size()
> > +                               << std::endl;
> > +
> > +             SDL_UpdateTexture(sdlTexture_, &sdlRect_, data.data(), sdlRect_.w * 2);
>
> Is the last argument format-dependent ?

I'll check, lacking test hardware of course though, I only have UVC
YUYV cameras which isn't ideal.

>
> > +             SDL_RenderClear(sdlRenderer_);
> > +             SDL_RenderCopy(sdlRenderer_, sdlTexture_, NULL, NULL);
>
> s/NULL/nullptr/

Ok

>
> > +             SDL_RenderPresent(sdlRenderer_);
> > +     }
> > +}
> > diff --git a/src/cam/sdl_sink.h b/src/cam/sdl_sink.h
> > new file mode 100644
> > index 00000000..6f20e2d6
> > --- /dev/null
> > +++ b/src/cam/sdl_sink.h
> > @@ -0,0 +1,42 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * sdl_sink.h - SDL Sink
> > + */
> > +
> > +#pragma once
> > +
> > +#include <map>
> > +#include <memory>
> > +#include <string>
> > +
> > +#include <libcamera/stream.h>
> > +
> > +#include <SDL2/SDL.h>
> > +
> > +#include "frame_sink.h"
> > +
> > +class Image;
> > +
> > +class SDLSink : public FrameSink
> > +{
> > +public:
> > +     SDLSink();
> > +     ~SDLSink();
> > +
> > +     int configure(const libcamera::CameraConfiguration &cfg) override;
> > +     int start() override;
> > +     void mapBuffer(libcamera::FrameBuffer *buffer) override;
> > +
> > +     bool processRequest(libcamera::Request *request) override;
> > +
> > +private:
> > +     void writeBuffer(libcamera::FrameBuffer *buffer);
> > +     void processSDLEvents();
> > +
> > +     std::map<libcamera::FrameBuffer *, std::unique_ptr<Image>> mappedBuffers_;
>
> I'd add a blank line here.

Ok

>
> > +     SDL_Window *sdlScreen_;
> > +     SDL_Renderer *sdlRenderer_;
> > +     SDL_Texture *sdlTexture_;
> > +     SDL_Rect sdlRect_;
> > +     SDL_PixelFormatEnum pf;
>
> s/pf/pixelFormat_/

Ok.

>
> > +};
>
> --
> Regards,
>
> Laurent Pinchart
>

Hi Eric,

On Tue, Mar 29, 2022 at 05:45:57PM +0100, Eric Curtin wrote:
> On Tue, 29 Mar 2022 at 16:10, Laurent Pinchart wrote:
> > On Tue, Mar 29, 2022 at 12:17:25PM +0100, Eric Curtin via libcamera-devel wrote:
> > > This adds more portability to existing cam sinks. You can pass a
> > > YUYV camera buffer for example and SDL will handle the pixel buffer
> > > conversion, although SDL does not support decompression for pixelformats
> > > like MJPEG. This allows cam reference implementation to display images
> > > on VMs, Mac M1, Raspberry Pi, etc. This also enables cam reference
> > > implementation, to run as a desktop application in wayland or x11.
> > > SDL also has support for Android and ChromeOS which I have not tested.
> > > Also tested on simpledrm raspberry pi 4 framebuffer successfully where
> > > existing kms sink did not work. Can also be used as kmsdrm sink.
> > >
> > > Signed-off-by: Eric Curtin <ecurtin@redhat.com>
> > > ---
> > > Changes in v2:
> > >  - Remove hardcoded pixel format from SDL_CreateTexture call
> > >
> > > Changes in v3:
> > >  - Drop blank line
> > >  - The contents of the if..endif are indented
> > >  - Split configure function into start and configure
> > >  - Add SDL_DestroyRenderer
> > >  - Remove assign and test in the same statement
> > >
> > > Changes in v4:
> > >  - Add processSDLEvents as a timed event in the evloop
> > > ---
> > >  src/cam/camera_session.cpp |   8 +++
> > >  src/cam/event_loop.cpp     |  10 ++-
> > >  src/cam/event_loop.h       |   1 +
> > >  src/cam/main.cpp           |   5 ++
> > >  src/cam/main.h             |   1 +
> > >  src/cam/meson.build        |  10 +++
> > >  src/cam/sdl_sink.cpp       | 142 +++++++++++++++++++++++++++++++++++++
> > >  src/cam/sdl_sink.h         |  42 +++++++++++
> > >  8 files changed, 218 insertions(+), 1 deletion(-)
> > >  create mode 100644 src/cam/sdl_sink.cpp
> > >  create mode 100644 src/cam/sdl_sink.h
> > >
> > > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp
> > > index 0428b538..30162dbd 100644
> > > --- a/src/cam/camera_session.cpp
> > > +++ b/src/cam/camera_session.cpp
> > > @@ -19,6 +19,9 @@
> > >  #ifdef HAVE_KMS
> > >  #include "kms_sink.h"
> > >  #endif
> > > +#ifdef HAVE_SDL
> > > +#include "sdl_sink.h"
> > > +#endif
> > >  #include "main.h"
> > >  #include "stream_options.h"
> > >
> > > @@ -187,6 +190,11 @@ int CameraSession::start()
> > >               sink_ = std::make_unique<KMSSink>(options_[OptDisplay].toString());
> > >  #endif
> > >
> > > +#ifdef HAVE_SDL
> > > +     if (options_.isSet(OptSDL))
> > > +             sink_ = std::make_unique<SDLSink>();
> > > +#endif
> > > +
> > >       if (options_.isSet(OptFile)) {
> > >               if (!options_[OptFile].toString().empty())
> > >                       sink_ = std::make_unique<FileSink>(streamNames_,
> > > diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp
> > > index e25784c0..41339d4e 100644
> > > --- a/src/cam/event_loop.cpp
> > > +++ b/src/cam/event_loop.cpp
> > > @@ -75,7 +75,15 @@ void EventLoop::addEvent(int fd, EventType type,
> > >               return;
> > >       }
> > >
> > > -     int ret = event_add(event->event_, nullptr);
> > > +     struct timeval *tp = nullptr;
> > > +     struct timeval tv;
> > > +     if (events == EV_PERSIST) {
> > > +             tp = &tv;
> > > +             tv.tv_sec = 0;
> > > +             tv.tv_usec = 10000; /* every 10 ms */
> > > +     }
> >
> > Could you please split the change to the event loop to a separate patch,
> > with an explanation of what it does in the commit message ? This isn't
> > trivial to understand.
> 
> Ok.
> 
> > > +
> > > +     int ret = event_add(event->event_, tp);
> > >       if (ret < 0) {
> > >               std::cerr << "Failed to add event for fd " << fd << std::endl;
> > >               return;
> > > diff --git a/src/cam/event_loop.h b/src/cam/event_loop.h
> > > index a4613eb2..8fd9bd20 100644
> > > --- a/src/cam/event_loop.h
> > > +++ b/src/cam/event_loop.h
> > > @@ -20,6 +20,7 @@ class EventLoop
> > >  {
> > >  public:
> > >       enum EventType {
> > > +             Default = 0, /* the event can be triggered only by a timeout or by manual activation */
> > >               Read = 1,
> > >               Write = 2,
> > >       };
> > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> > > index c7f664b9..1d62a64a 100644
> > > --- a/src/cam/main.cpp
> > > +++ b/src/cam/main.cpp
> > > @@ -137,6 +137,11 @@ int CamApp::parseOptions(int argc, char *argv[])
> > >                        "Display viewfinder through DRM/KMS on specified connector",
> > >                        "display", ArgumentOptional, "connector", false,
> > >                        OptCamera);
> > > +#endif
> > > +#ifdef HAVE_SDL
> > > +     parser.addOption(OptSDL, OptionNone,
> > > +                      "Display viewfinder through SDL",
> > > +                      "sdl", ArgumentNone, "", false, OptCamera);
> > >  #endif
> > >       parser.addOption(OptFile, OptionString,
> > >                        "Write captured frames to disk\n"
> > > diff --git a/src/cam/main.h b/src/cam/main.h
> > > index 62f7bbc9..a64f95a0 100644
> > > --- a/src/cam/main.h
> > > +++ b/src/cam/main.h
> > > @@ -11,6 +11,7 @@ enum {
> > >       OptCamera = 'c',
> > >       OptCapture = 'C',
> > >       OptDisplay = 'D',
> > > +     OptSDL = 'S',
> >
> > Let's keep the options sorted.
> 
> Ok.
> 
> > >       OptFile = 'F',
> > >       OptHelp = 'h',
> > >       OptInfo = 'I',
> > > diff --git a/src/cam/meson.build b/src/cam/meson.build
> > > index e8e2ae57..bd536c5b 100644
> > > --- a/src/cam/meson.build
> > > +++ b/src/cam/meson.build
> > > @@ -32,11 +32,21 @@ cam_sources += files([
> > >  ])
> > >  endif
> > >
> > > +libsdl2 = dependency('SDL2', required : false)
> > > +
> > > +if libsdl2.found()
> > > +    cam_cpp_args += [ '-DHAVE_SDL' ]
> >
> > No space within the [ ].
> 
> Ok.
> 
> > > +    cam_sources += files([
> > > +        'sdl_sink.cpp'
> > > +    ])
> > > +endif
> > > +
> > >  cam  = executable('cam', cam_sources,
> > >                    dependencies : [
> > >                        libatomic,
> > >                        libcamera_public,
> > >                        libdrm,
> > > +                      libsdl2,
> > >                        libevent,
> > >                    ],
> > >                    cpp_args : cam_cpp_args,
> > > diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp
> > > new file mode 100644
> > > index 00000000..6c6e37d0
> > > --- /dev/null
> > > +++ b/src/cam/sdl_sink.cpp
> > > @@ -0,0 +1,142 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > +/*
> > > + * sdl_sink.cpp - SDL Sink
> > > + */
> > > +
> > > +#include "sdl_sink.h"
> > > +
> > > +#include <assert.h>
> > > +#include <fcntl.h>
> > > +#include <iomanip>
> > > +#include <iostream>
> > > +#include <signal.h>
> > > +#include <sstream>
> > > +#include <string.h>
> > > +#include <unistd.h>
> > > +
> > > +#include <libcamera/camera.h>
> > > +#include <libcamera/formats.h>
> > > +
> > > +#include "event_loop.h"
> > > +#include "image.h"
> > > +
> > > +using namespace libcamera;
> > > +
> > > +SDLSink::SDLSink()
> > > +     : sdlRenderer_(0)
> >
> > It's a pointer, should be initialized to nullptr, not 0.
> 
> Ok.
> 
> > > +{
> > > +     memset(&sdlRect_, 0, sizeof(sdlRect_));
> >
> > This could be initialized with
> >
> >         sdlRect_({})
> >
> > in the constructor member initializers list.
> 
> Ok.
> 
> > > +}
> > > +
> > > +SDLSink::~SDLSink()
> > > +{
> > > +     if (sdlRenderer_)
> > > +             SDL_DestroyRenderer(sdlRenderer_);
> > > +     SDL_Quit();
> >
> > Shouldn't the cleanup go to stop(), now that you have the initialization
> > in start() ?
> 
> Yes thanks for spotting.
> 
> > No need for any cleanup on sdlScreen_ and sdlTexture_ ?
> 
> No need.

Maybe a comment would be nice to state this, so that it looks less like
it has been forgotten to someone not familiar with SDL.

> > > +}
> > > +
> > > +int SDLSink::configure(const libcamera::CameraConfiguration &cfg)
> > > +{
> > > +     int ret = FrameSink::configure(cfg);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     const libcamera::StreamConfiguration &sCfg = cfg.at(0);
> > > +     pf = (SDL_PixelFormatEnum)sCfg.pixelFormat.fourcc();
> > > +     if (pf == SDL_DEFINE_PIXELFOURCC('Y', 'U', 'Y', 'V')) {
> > > +             pf = SDL_PIXELFORMAT_YUY2;
> > > +     } else if (int ne = strcmp(SDL_GetPixelFormatName(pf), "SDL_PIXELFORMAT_UNKNOWN"); !ne) {
> >
> > No variable declaration in an if () statement.
> 
> Ok.
> 
> > > +             std::cerr << "SDL_GetPixelFormatName error - exiting: SDL_PIXELFORMAT_UNKNOWN, no " << sCfg.pixelFormat.toString() << " support\n";
> >
> > Those two lines are too long.
> 
> Ok.
> 
> > Replace \n with std::endl
> >
> > > +             return -EINVAL;
> > > +     }
> >
> > I don't think this is right. SDL pixel formats don't map directly to the
> > 4CC values used by libcamera. You need a translation table.
> 
> I can start one, I won't be able to test it much as I don't have the
> hardware to do that but I can start.

You don't have to handle all formats, you can limit the translation to
the format(s) you can test (or possibly adding a few of the most common
other formats). More formats can always be added later.

> > > +
> > > +     sdlRect_.w = sCfg.size.width;
> > > +     sdlRect_.h = sCfg.size.height;
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +int SDLSink::start()
> > > +{
> > > +     int ret = SDL_Init(SDL_INIT_VIDEO);
> > > +     if (ret) {
> > > +             std::cerr << "SDL_Init error - exiting: " << SDL_GetError() << std::endl;
> >
> >                 std::cerr << "Failed to initialize SDL: " << SDL_GetError()
> >                           << std::endl;
> >
> > and similarly below.
> 
> Ok
> 
> > > +             return ret;
> > > +     }
> > > +
> > > +     sdlScreen_ = SDL_CreateWindow("", SDL_WINDOWPOS_UNDEFINED,
> > > +                                   SDL_WINDOWPOS_UNDEFINED, sdlRect_.w,
> > > +                                   sdlRect_.h, SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE);
> > > +     if (!sdlScreen_) {
> > > +             std::cerr << "SDL_CreateWindow error - exiting: " << SDL_GetError() << std::endl;
> > > +             return -EINVAL;
> > > +     }
> > > +
> > > +     sdlRenderer_ = SDL_CreateRenderer(
> > > +             sdlScreen_, -1, 0);
> > > +     if (!sdlRenderer_) {
> > > +             std::cerr << "SDL_CreateRenderer error - exiting: " << SDL_GetError() << std::endl;
> > > +             return -EINVAL;
> > > +     }
> > > +
> > > +     SDL_RenderSetLogicalSize(sdlRenderer_, sdlRect_.w,
> > > +                              sdlRect_.h);
> > > +
> > > +     sdlTexture_ =
> > > +             SDL_CreateTexture(sdlRenderer_, pf,
> > > +                               SDL_TEXTUREACCESS_STREAMING, sdlRect_.w,
> > > +                               sdlRect_.h);
> > > +     EventLoop::instance()->addEvent(-1, EventLoop::Default,
> > > +                                     std::bind(&SDLSink::processSDLEvents, this));
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +void SDLSink::mapBuffer(FrameBuffer *buffer)
> > > +{
> > > +     std::unique_ptr<Image> image =
> > > +             Image::fromFrameBuffer(buffer, Image::MapMode::ReadOnly);
> > > +     assert(image != nullptr);
> > > +
> > > +     mappedBuffers_[buffer] = std::move(image);
> > > +}
> > > +
> > > +bool SDLSink::processRequest(Request *request)
> > > +{
> > > +     for (auto [stream, buffer] : request->buffers())
> > > +             writeBuffer(buffer);
> >
> > Will this really work properly if the request contains multiple buffers
> > ?
> 
> I will write more code to solve this.

The KMS sink doesn't support this either, so a simple option would be to
handle the first stream only, or to reject multi-stream configurations
in the configure() function. Another option could be to create multiple
SDL windows, one for each stream, but that will become complicated I
suppose.

> > > +
> > > +     return true;
> > > +}
> > > +
> > > +/*
> > > + * Process SDL events, required for things like window resize and quit button
> > > + */
> > > +void SDLSink::processSDLEvents()
> > > +{
> > > +     for (SDL_Event e; SDL_PollEvent(&e);) {
> > > +             if (e.type == SDL_QUIT) { /* click close icon then quit */
> > > +                     kill(getpid(), SIGINT);
> >
> > That's too harsh. Calling the exit() function of the event loop should
> > be enough.
> 
> I'll try that, my first thought was to try and make it behave like
> when you type Ctrl-C as much as possible, but yeah it's weird this
> way, a process calling it's own signal handler.
> 
> > > +             }
> > > +     }
> > > +}
> > > +
> > > +void SDLSink::writeBuffer(FrameBuffer *buffer)
> >
> > This function doesn't "write" a buffer, it should be named differently.
> 
> renderBuffer maybe?

Sounds good to me.

> > > +{
> > > +     Image *image = mappedBuffers_[buffer].get();
> > > +
> > > +     for (unsigned int i = 0; i < buffer->planes().size(); ++i) {
> > > +             const FrameMetadata::Plane &meta = buffer->metadata().planes()[i];
> > > +
> > > +             Span<uint8_t> data = image->data(i);
> > > +             if (meta.bytesused > data.size())
> > > +                     std::cerr << "payload size " << meta.bytesused
> > > +                               << " larger than plane size " << data.size()
> > > +                               << std::endl;
> > > +
> > > +             SDL_UpdateTexture(sdlTexture_, &sdlRect_, data.data(), sdlRect_.w * 2);
> >
> > Is the last argument format-dependent ?
> 
> I'll check, lacking test hardware of course though, I only have UVC
> YUYV cameras which isn't ideal.
> 
> > > +             SDL_RenderClear(sdlRenderer_);
> > > +             SDL_RenderCopy(sdlRenderer_, sdlTexture_, NULL, NULL);
> >
> > s/NULL/nullptr/
> 
> Ok
> 
> > > +             SDL_RenderPresent(sdlRenderer_);
> > > +     }
> > > +}
> > > diff --git a/src/cam/sdl_sink.h b/src/cam/sdl_sink.h
> > > new file mode 100644
> > > index 00000000..6f20e2d6
> > > --- /dev/null
> > > +++ b/src/cam/sdl_sink.h
> > > @@ -0,0 +1,42 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > +/*
> > > + * sdl_sink.h - SDL Sink
> > > + */
> > > +
> > > +#pragma once
> > > +
> > > +#include <map>
> > > +#include <memory>
> > > +#include <string>
> > > +
> > > +#include <libcamera/stream.h>
> > > +
> > > +#include <SDL2/SDL.h>
> > > +
> > > +#include "frame_sink.h"
> > > +
> > > +class Image;
> > > +
> > > +class SDLSink : public FrameSink
> > > +{
> > > +public:
> > > +     SDLSink();
> > > +     ~SDLSink();
> > > +
> > > +     int configure(const libcamera::CameraConfiguration &cfg) override;
> > > +     int start() override;
> > > +     void mapBuffer(libcamera::FrameBuffer *buffer) override;
> > > +
> > > +     bool processRequest(libcamera::Request *request) override;
> > > +
> > > +private:
> > > +     void writeBuffer(libcamera::FrameBuffer *buffer);
> > > +     void processSDLEvents();
> > > +
> > > +     std::map<libcamera::FrameBuffer *, std::unique_ptr<Image>> mappedBuffers_;
> >
> > I'd add a blank line here.
> 
> Ok
> 
> > > +     SDL_Window *sdlScreen_;
> > > +     SDL_Renderer *sdlRenderer_;
> > > +     SDL_Texture *sdlTexture_;
> > > +     SDL_Rect sdlRect_;
> > > +     SDL_PixelFormatEnum pf;
> >
> > s/pf/pixelFormat_/
> 
> Ok.
> 
> > > +};

On Tue, 29 Mar 2022 at 21:09, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Eric,
>
> On Tue, Mar 29, 2022 at 05:45:57PM +0100, Eric Curtin wrote:
> > On Tue, 29 Mar 2022 at 16:10, Laurent Pinchart wrote:
> > > On Tue, Mar 29, 2022 at 12:17:25PM +0100, Eric Curtin via libcamera-devel wrote:
> > > > This adds more portability to existing cam sinks. You can pass a
> > > > YUYV camera buffer for example and SDL will handle the pixel buffer
> > > > conversion, although SDL does not support decompression for pixelformats
> > > > like MJPEG. This allows cam reference implementation to display images
> > > > on VMs, Mac M1, Raspberry Pi, etc. This also enables cam reference
> > > > implementation, to run as a desktop application in wayland or x11.
> > > > SDL also has support for Android and ChromeOS which I have not tested.
> > > > Also tested on simpledrm raspberry pi 4 framebuffer successfully where
> > > > existing kms sink did not work. Can also be used as kmsdrm sink.
> > > >
> > > > Signed-off-by: Eric Curtin <ecurtin@redhat.com>
> > > > ---
> > > > Changes in v2:
> > > >  - Remove hardcoded pixel format from SDL_CreateTexture call
> > > >
> > > > Changes in v3:
> > > >  - Drop blank line
> > > >  - The contents of the if..endif are indented
> > > >  - Split configure function into start and configure
> > > >  - Add SDL_DestroyRenderer
> > > >  - Remove assign and test in the same statement
> > > >
> > > > Changes in v4:
> > > >  - Add processSDLEvents as a timed event in the evloop
> > > > ---
> > > >  src/cam/camera_session.cpp |   8 +++
> > > >  src/cam/event_loop.cpp     |  10 ++-
> > > >  src/cam/event_loop.h       |   1 +
> > > >  src/cam/main.cpp           |   5 ++
> > > >  src/cam/main.h             |   1 +
> > > >  src/cam/meson.build        |  10 +++
> > > >  src/cam/sdl_sink.cpp       | 142 +++++++++++++++++++++++++++++++++++++
> > > >  src/cam/sdl_sink.h         |  42 +++++++++++
> > > >  8 files changed, 218 insertions(+), 1 deletion(-)
> > > >  create mode 100644 src/cam/sdl_sink.cpp
> > > >  create mode 100644 src/cam/sdl_sink.h
> > > >
> > > > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp
> > > > index 0428b538..30162dbd 100644
> > > > --- a/src/cam/camera_session.cpp
> > > > +++ b/src/cam/camera_session.cpp
> > > > @@ -19,6 +19,9 @@
> > > >  #ifdef HAVE_KMS
> > > >  #include "kms_sink.h"
> > > >  #endif
> > > > +#ifdef HAVE_SDL
> > > > +#include "sdl_sink.h"
> > > > +#endif
> > > >  #include "main.h"
> > > >  #include "stream_options.h"
> > > >
> > > > @@ -187,6 +190,11 @@ int CameraSession::start()
> > > >               sink_ = std::make_unique<KMSSink>(options_[OptDisplay].toString());
> > > >  #endif
> > > >
> > > > +#ifdef HAVE_SDL
> > > > +     if (options_.isSet(OptSDL))
> > > > +             sink_ = std::make_unique<SDLSink>();
> > > > +#endif
> > > > +
> > > >       if (options_.isSet(OptFile)) {
> > > >               if (!options_[OptFile].toString().empty())
> > > >                       sink_ = std::make_unique<FileSink>(streamNames_,
> > > > diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp
> > > > index e25784c0..41339d4e 100644
> > > > --- a/src/cam/event_loop.cpp
> > > > +++ b/src/cam/event_loop.cpp
> > > > @@ -75,7 +75,15 @@ void EventLoop::addEvent(int fd, EventType type,
> > > >               return;
> > > >       }
> > > >
> > > > -     int ret = event_add(event->event_, nullptr);
> > > > +     struct timeval *tp = nullptr;
> > > > +     struct timeval tv;
> > > > +     if (events == EV_PERSIST) {
> > > > +             tp = &tv;
> > > > +             tv.tv_sec = 0;
> > > > +             tv.tv_usec = 10000; /* every 10 ms */
> > > > +     }
> > >
> > > Could you please split the change to the event loop to a separate patch,
> > > with an explanation of what it does in the commit message ? This isn't
> > > trivial to understand.
> >
> > Ok.
> >
> > > > +
> > > > +     int ret = event_add(event->event_, tp);
> > > >       if (ret < 0) {
> > > >               std::cerr << "Failed to add event for fd " << fd << std::endl;
> > > >               return;
> > > > diff --git a/src/cam/event_loop.h b/src/cam/event_loop.h
> > > > index a4613eb2..8fd9bd20 100644
> > > > --- a/src/cam/event_loop.h
> > > > +++ b/src/cam/event_loop.h
> > > > @@ -20,6 +20,7 @@ class EventLoop
> > > >  {
> > > >  public:
> > > >       enum EventType {
> > > > +             Default = 0, /* the event can be triggered only by a timeout or by manual activation */
> > > >               Read = 1,
> > > >               Write = 2,
> > > >       };
> > > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> > > > index c7f664b9..1d62a64a 100644
> > > > --- a/src/cam/main.cpp
> > > > +++ b/src/cam/main.cpp
> > > > @@ -137,6 +137,11 @@ int CamApp::parseOptions(int argc, char *argv[])
> > > >                        "Display viewfinder through DRM/KMS on specified connector",
> > > >                        "display", ArgumentOptional, "connector", false,
> > > >                        OptCamera);
> > > > +#endif
> > > > +#ifdef HAVE_SDL
> > > > +     parser.addOption(OptSDL, OptionNone,
> > > > +                      "Display viewfinder through SDL",
> > > > +                      "sdl", ArgumentNone, "", false, OptCamera);
> > > >  #endif
> > > >       parser.addOption(OptFile, OptionString,
> > > >                        "Write captured frames to disk\n"
> > > > diff --git a/src/cam/main.h b/src/cam/main.h
> > > > index 62f7bbc9..a64f95a0 100644
> > > > --- a/src/cam/main.h
> > > > +++ b/src/cam/main.h
> > > > @@ -11,6 +11,7 @@ enum {
> > > >       OptCamera = 'c',
> > > >       OptCapture = 'C',
> > > >       OptDisplay = 'D',
> > > > +     OptSDL = 'S',
> > >
> > > Let's keep the options sorted.
> >
> > Ok.
> >
> > > >       OptFile = 'F',
> > > >       OptHelp = 'h',
> > > >       OptInfo = 'I',
> > > > diff --git a/src/cam/meson.build b/src/cam/meson.build
> > > > index e8e2ae57..bd536c5b 100644
> > > > --- a/src/cam/meson.build
> > > > +++ b/src/cam/meson.build
> > > > @@ -32,11 +32,21 @@ cam_sources += files([
> > > >  ])
> > > >  endif
> > > >
> > > > +libsdl2 = dependency('SDL2', required : false)
> > > > +
> > > > +if libsdl2.found()
> > > > +    cam_cpp_args += [ '-DHAVE_SDL' ]
> > >
> > > No space within the [ ].
> >
> > Ok.
> >
> > > > +    cam_sources += files([
> > > > +        'sdl_sink.cpp'
> > > > +    ])
> > > > +endif
> > > > +
> > > >  cam  = executable('cam', cam_sources,
> > > >                    dependencies : [
> > > >                        libatomic,
> > > >                        libcamera_public,
> > > >                        libdrm,
> > > > +                      libsdl2,
> > > >                        libevent,
> > > >                    ],
> > > >                    cpp_args : cam_cpp_args,
> > > > diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp
> > > > new file mode 100644
> > > > index 00000000..6c6e37d0
> > > > --- /dev/null
> > > > +++ b/src/cam/sdl_sink.cpp
> > > > @@ -0,0 +1,142 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > > +/*
> > > > + * sdl_sink.cpp - SDL Sink
> > > > + */
> > > > +
> > > > +#include "sdl_sink.h"
> > > > +
> > > > +#include <assert.h>
> > > > +#include <fcntl.h>
> > > > +#include <iomanip>
> > > > +#include <iostream>
> > > > +#include <signal.h>
> > > > +#include <sstream>
> > > > +#include <string.h>
> > > > +#include <unistd.h>
> > > > +
> > > > +#include <libcamera/camera.h>
> > > > +#include <libcamera/formats.h>
> > > > +
> > > > +#include "event_loop.h"
> > > > +#include "image.h"
> > > > +
> > > > +using namespace libcamera;
> > > > +
> > > > +SDLSink::SDLSink()
> > > > +     : sdlRenderer_(0)
> > >
> > > It's a pointer, should be initialized to nullptr, not 0.
> >
> > Ok.
> >
> > > > +{
> > > > +     memset(&sdlRect_, 0, sizeof(sdlRect_));
> > >
> > > This could be initialized with
> > >
> > >         sdlRect_({})
> > >
> > > in the constructor member initializers list.
> >
> > Ok.
> >
> > > > +}
> > > > +
> > > > +SDLSink::~SDLSink()
> > > > +{
> > > > +     if (sdlRenderer_)
> > > > +             SDL_DestroyRenderer(sdlRenderer_);
> > > > +     SDL_Quit();
> > >
> > > Shouldn't the cleanup go to stop(), now that you have the initialization
> > > in start() ?
> >
> > Yes thanks for spotting.
> >
> > > No need for any cleanup on sdlScreen_ and sdlTexture_ ?
> >
> > No need.
>
> Maybe a comment would be nice to state this, so that it looks less like
> it has been forgotten to someone not familiar with SDL.

It's funny actually, was trying different combinations today with
valgrind, address sanitizer, the only cleanup function you actually
need is SDL_Quit, it seems to clean up the rest automatically. I think
I'll put them all in explicitly though, just to alleviate future
readers getting worried. Some SDL docs call them all, some don't.

>
> > > > +}
> > > > +
> > > > +int SDLSink::configure(const libcamera::CameraConfiguration &cfg)
> > > > +{
> > > > +     int ret = FrameSink::configure(cfg);
> > > > +     if (ret < 0)
> > > > +             return ret;
> > > > +
> > > > +     const libcamera::StreamConfiguration &sCfg = cfg.at(0);
> > > > +     pf = (SDL_PixelFormatEnum)sCfg.pixelFormat.fourcc();
> > > > +     if (pf == SDL_DEFINE_PIXELFOURCC('Y', 'U', 'Y', 'V')) {
> > > > +             pf = SDL_PIXELFORMAT_YUY2;
> > > > +     } else if (int ne = strcmp(SDL_GetPixelFormatName(pf), "SDL_PIXELFORMAT_UNKNOWN"); !ne) {
> > >
> > > No variable declaration in an if () statement.
> >
> > Ok.
> >
> > > > +             std::cerr << "SDL_GetPixelFormatName error - exiting: SDL_PIXELFORMAT_UNKNOWN, no " << sCfg.pixelFormat.toString() << " support\n";
> > >
> > > Those two lines are too long.
> >
> > Ok.
> >
> > > Replace \n with std::endl
> > >
> > > > +             return -EINVAL;
> > > > +     }
> > >
> > > I don't think this is right. SDL pixel formats don't map directly to the
> > > 4CC values used by libcamera. You need a translation table.
> >
> > I can start one, I won't be able to test it much as I don't have the
> > hardware to do that but I can start.
>
> You don't have to handle all formats, you can limit the translation to
> the format(s) you can test (or possibly adding a few of the most common
> other formats). More formats can always be added later.
>
> > > > +
> > > > +     sdlRect_.w = sCfg.size.width;
> > > > +     sdlRect_.h = sCfg.size.height;
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +int SDLSink::start()
> > > > +{
> > > > +     int ret = SDL_Init(SDL_INIT_VIDEO);
> > > > +     if (ret) {
> > > > +             std::cerr << "SDL_Init error - exiting: " << SDL_GetError() << std::endl;
> > >
> > >                 std::cerr << "Failed to initialize SDL: " << SDL_GetError()
> > >                           << std::endl;
> > >
> > > and similarly below.
> >
> > Ok
> >
> > > > +             return ret;
> > > > +     }
> > > > +
> > > > +     sdlScreen_ = SDL_CreateWindow("", SDL_WINDOWPOS_UNDEFINED,
> > > > +                                   SDL_WINDOWPOS_UNDEFINED, sdlRect_.w,
> > > > +                                   sdlRect_.h, SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE);
> > > > +     if (!sdlScreen_) {
> > > > +             std::cerr << "SDL_CreateWindow error - exiting: " << SDL_GetError() << std::endl;
> > > > +             return -EINVAL;
> > > > +     }
> > > > +
> > > > +     sdlRenderer_ = SDL_CreateRenderer(
> > > > +             sdlScreen_, -1, 0);
> > > > +     if (!sdlRenderer_) {
> > > > +             std::cerr << "SDL_CreateRenderer error - exiting: " << SDL_GetError() << std::endl;
> > > > +             return -EINVAL;
> > > > +     }
> > > > +
> > > > +     SDL_RenderSetLogicalSize(sdlRenderer_, sdlRect_.w,
> > > > +                              sdlRect_.h);
> > > > +
> > > > +     sdlTexture_ =
> > > > +             SDL_CreateTexture(sdlRenderer_, pf,
> > > > +                               SDL_TEXTUREACCESS_STREAMING, sdlRect_.w,
> > > > +                               sdlRect_.h);
> > > > +     EventLoop::instance()->addEvent(-1, EventLoop::Default,
> > > > +                                     std::bind(&SDLSink::processSDLEvents, this));
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +void SDLSink::mapBuffer(FrameBuffer *buffer)
> > > > +{
> > > > +     std::unique_ptr<Image> image =
> > > > +             Image::fromFrameBuffer(buffer, Image::MapMode::ReadOnly);
> > > > +     assert(image != nullptr);
> > > > +
> > > > +     mappedBuffers_[buffer] = std::move(image);
> > > > +}
> > > > +
> > > > +bool SDLSink::processRequest(Request *request)
> > > > +{
> > > > +     for (auto [stream, buffer] : request->buffers())
> > > > +             writeBuffer(buffer);
> > >
> > > Will this really work properly if the request contains multiple buffers
> > > ?
> >
> > I will write more code to solve this.
>
> The KMS sink doesn't support this either, so a simple option would be to
> handle the first stream only, or to reject multi-stream configurations
> in the configure() function. Another option could be to create multiple
> SDL windows, one for each stream, but that will become complicated I
> suppose.
>
> > > > +
> > > > +     return true;
> > > > +}
> > > > +
> > > > +/*
> > > > + * Process SDL events, required for things like window resize and quit button
> > > > + */
> > > > +void SDLSink::processSDLEvents()
> > > > +{
> > > > +     for (SDL_Event e; SDL_PollEvent(&e);) {
> > > > +             if (e.type == SDL_QUIT) { /* click close icon then quit */
> > > > +                     kill(getpid(), SIGINT);
> > >
> > > That's too harsh. Calling the exit() function of the event loop should
> > > be enough.
> >
> > I'll try that, my first thought was to try and make it behave like
> > when you type Ctrl-C as much as possible, but yeah it's weird this
> > way, a process calling it's own signal handler.
> >
> > > > +             }
> > > > +     }
> > > > +}
> > > > +
> > > > +void SDLSink::writeBuffer(FrameBuffer *buffer)
> > >
> > > This function doesn't "write" a buffer, it should be named differently.
> >
> > renderBuffer maybe?
>
> Sounds good to me.
>
> > > > +{
> > > > +     Image *image = mappedBuffers_[buffer].get();
> > > > +
> > > > +     for (unsigned int i = 0; i < buffer->planes().size(); ++i) {
> > > > +             const FrameMetadata::Plane &meta = buffer->metadata().planes()[i];
> > > > +
> > > > +             Span<uint8_t> data = image->data(i);
> > > > +             if (meta.bytesused > data.size())
> > > > +                     std::cerr << "payload size " << meta.bytesused
> > > > +                               << " larger than plane size " << data.size()
> > > > +                               << std::endl;
> > > > +
> > > > +             SDL_UpdateTexture(sdlTexture_, &sdlRect_, data.data(), sdlRect_.w * 2);
> > >
> > > Is the last argument format-dependent ?
> >
> > I'll check, lacking test hardware of course though, I only have UVC
> > YUYV cameras which isn't ideal.
> >
> > > > +             SDL_RenderClear(sdlRenderer_);
> > > > +             SDL_RenderCopy(sdlRenderer_, sdlTexture_, NULL, NULL);
> > >
> > > s/NULL/nullptr/
> >
> > Ok
> >
> > > > +             SDL_RenderPresent(sdlRenderer_);
> > > > +     }
> > > > +}
> > > > diff --git a/src/cam/sdl_sink.h b/src/cam/sdl_sink.h
> > > > new file mode 100644
> > > > index 00000000..6f20e2d6
> > > > --- /dev/null
> > > > +++ b/src/cam/sdl_sink.h
> > > > @@ -0,0 +1,42 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > > +/*
> > > > + * sdl_sink.h - SDL Sink
> > > > + */
> > > > +
> > > > +#pragma once
> > > > +
> > > > +#include <map>
> > > > +#include <memory>
> > > > +#include <string>
> > > > +
> > > > +#include <libcamera/stream.h>
> > > > +
> > > > +#include <SDL2/SDL.h>
> > > > +
> > > > +#include "frame_sink.h"
> > > > +
> > > > +class Image;
> > > > +
> > > > +class SDLSink : public FrameSink
> > > > +{
> > > > +public:
> > > > +     SDLSink();
> > > > +     ~SDLSink();
> > > > +
> > > > +     int configure(const libcamera::CameraConfiguration &cfg) override;
> > > > +     int start() override;
> > > > +     void mapBuffer(libcamera::FrameBuffer *buffer) override;
> > > > +
> > > > +     bool processRequest(libcamera::Request *request) override;
> > > > +
> > > > +private:
> > > > +     void writeBuffer(libcamera::FrameBuffer *buffer);
> > > > +     void processSDLEvents();
> > > > +
> > > > +     std::map<libcamera::FrameBuffer *, std::unique_ptr<Image>> mappedBuffers_;
> > >
> > > I'd add a blank line here.
> >
> > Ok
> >
> > > > +     SDL_Window *sdlScreen_;
> > > > +     SDL_Renderer *sdlRenderer_;
> > > > +     SDL_Texture *sdlTexture_;
> > > > +     SDL_Rect sdlRect_;
> > > > +     SDL_PixelFormatEnum pf;
> > >
> > > s/pf/pixelFormat_/
> >
> > Ok.
> >
> > > > +};
>
> --
> Regards,
>
> Laurent Pinchart
>