[libcamera-devel,v9,3/4] cam: sdl_sink: Add SDL sink with initial YUYV support

15998 diff mbox series

This adds more portability to existing cam sinks. You can pass a
YUYV camera buffer and SDL will handle the pixel buffer conversion
to the display. 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 has not been
tested. Also tested on simpledrm Raspberry Pi 4 framebuffer
successfully where existing kms sink did not work. Can also be used as
kmsdrm sink. Only supports one camera stream at present.

Signed-off-by: Eric Curtin <ecurtin@redhat.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Tested-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/cam/camera_session.cpp   |   8 ++
 src/cam/main.cpp             |   4 +
 src/cam/main.h               |   1 +
 src/cam/meson.build          |  12 +++
 src/cam/sdl_sink.cpp         | 194 +++++++++++++++++++++++++++++++++++
 src/cam/sdl_sink.h           |  49 +++++++++
 src/cam/sdl_texture.cpp      |  37 +++++++
 src/cam/sdl_texture.h        |  29 ++++++
 src/cam/sdl_texture_yuyv.cpp |  20 ++++
 src/cam/sdl_texture_yuyv.h   |  17 +++
 10 files changed, 371 insertions(+)
 create mode 100644 src/cam/sdl_sink.cpp
 create mode 100644 src/cam/sdl_sink.h
 create mode 100644 src/cam/sdl_texture.cpp
 create mode 100644 src/cam/sdl_texture.h
 create mode 100644 src/cam/sdl_texture_yuyv.cpp
 create mode 100644 src/cam/sdl_texture_yuyv.h

On Fri, May 20, 2022 at 08:01:05PM +0100, Eric Curtin wrote:
> This adds more portability to existing cam sinks. You can pass a
> YUYV camera buffer and SDL will handle the pixel buffer conversion
> to the display. 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 has not been
> tested. Also tested on simpledrm Raspberry Pi 4 framebuffer
> successfully where existing kms sink did not work. Can also be used as
> kmsdrm sink. Only supports one camera stream at present.
> 
> Signed-off-by: Eric Curtin <ecurtin@redhat.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Tested-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/cam/camera_session.cpp   |   8 ++
>  src/cam/main.cpp             |   4 +
>  src/cam/main.h               |   1 +
>  src/cam/meson.build          |  12 +++
>  src/cam/sdl_sink.cpp         | 194 +++++++++++++++++++++++++++++++++++
>  src/cam/sdl_sink.h           |  49 +++++++++
>  src/cam/sdl_texture.cpp      |  37 +++++++
>  src/cam/sdl_texture.h        |  29 ++++++
>  src/cam/sdl_texture_yuyv.cpp |  20 ++++
>  src/cam/sdl_texture_yuyv.h   |  17 +++
>  10 files changed, 371 insertions(+)
>  create mode 100644 src/cam/sdl_sink.cpp
>  create mode 100644 src/cam/sdl_sink.h
>  create mode 100644 src/cam/sdl_texture.cpp
>  create mode 100644 src/cam/sdl_texture.h
>  create mode 100644 src/cam/sdl_texture_yuyv.cpp
>  create mode 100644 src/cam/sdl_texture_yuyv.h
> 
> diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp
> index efffafbf..336ae471 100644
> --- a/src/cam/camera_session.cpp
> +++ b/src/cam/camera_session.cpp
> @@ -20,6 +20,9 @@
>  #include "kms_sink.h"
>  #endif
>  #include "main.h"
> +#ifdef HAVE_SDL
> +#include "sdl_sink.h"
> +#endif
>  #include "stream_options.h"
>  
>  using namespace libcamera;
> @@ -186,6 +189,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/main.cpp b/src/cam/main.cpp
> index c7f664b9..962262a8 100644
> --- a/src/cam/main.cpp
> +++ b/src/cam/main.cpp
> @@ -147,6 +147,10 @@ int CamApp::parseOptions(int argc, char *argv[])
>  			 "The default file name is 'frame-#.bin'.",
>  			 "file", ArgumentOptional, "filename", false,
>  			 OptCamera);
> +#ifdef HAVE_SDL
> +	parser.addOption(OptSDL, OptionNone, "Display viewfinder through SDL",
> +			 "sdl", ArgumentNone, "", false, OptCamera);
> +#endif
>  	parser.addOption(OptStream, &streamKeyValue,
>  			 "Set configuration of a camera stream", "stream", true,
>  			 OptCamera);
> diff --git a/src/cam/main.h b/src/cam/main.h
> index 62f7bbc9..507a184f 100644
> --- a/src/cam/main.h
> +++ b/src/cam/main.h
> @@ -17,6 +17,7 @@ enum {
>  	OptList = 'l',
>  	OptListProperties = 'p',
>  	OptMonitor = 'm',
> +	OptSDL = 'S',
>  	OptStream = 's',
>  	OptListControls = 256,
>  	OptStrictFormats = 257,
> diff --git a/src/cam/meson.build b/src/cam/meson.build
> index 5bab8c9e..7d714152 100644
> --- a/src/cam/meson.build
> +++ b/src/cam/meson.build
> @@ -32,12 +32,24 @@ if libdrm.found()
>      ])
>  endif
>  
> +libsdl2 = dependency('SDL2', required : false)
> +
> +if libsdl2.found()
> +    cam_cpp_args += ['-DHAVE_SDL']
> +    cam_sources += files([
> +        'sdl_sink.cpp',
> +        'sdl_texture.cpp',
> +        'sdl_texture_yuyv.cpp'
> +    ])
> +endif
> +
>  cam  = executable('cam', cam_sources,
>                    dependencies : [
>                        libatomic,
>                        libcamera_public,
>                        libdrm,
>                        libevent,
> +                      libsdl2,
>                    ],
>                    cpp_args : cam_cpp_args,
>                    install : true)
> diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp
> new file mode 100644
> index 00000000..65430efb
> --- /dev/null
> +++ b/src/cam/sdl_sink.cpp
> @@ -0,0 +1,194 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2022, Ideas on Board Oy
> + *
> + * sdl_sink.h - 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"
> +#include "sdl_texture_yuyv.h"
> +
> +using namespace libcamera;
> +
> +using namespace std::chrono_literals;
> +
> +SDLSink::SDLSink()
> +	: window_(nullptr), renderer_(nullptr), rect_({}),
> +	  init_(false)
> +{
> +}
> +
> +SDLSink::~SDLSink()
> +{
> +	stop();
> +}
> +
> +int SDLSink::configure(const libcamera::CameraConfiguration &config)
> +{
> +	const int ret = FrameSink::configure(config);

No need to make ret const.

> +	if (ret < 0)
> +		return ret;
> +
> +	if (config.size() > 1) {
> +		std::cerr
> +			<< "SDL sink only supports one camera stream at present, streaming first camera stream"
> +			<< std::endl;
> +	} else if (config.empty()) {
> +		std::cerr << "Require at least one camera stream to process"
> +			  << std::endl;
> +		return -EINVAL;
> +	}
> +
> +	const libcamera::StreamConfiguration &cfg = config.at(0);
> +	rect_.w = cfg.size.width;
> +	rect_.h = cfg.size.height;
> +
> +	switch (cfg.pixelFormat) {
> +	case libcamera::formats::YUYV:
> +		texture_ = std::make_unique<SDLTextureYUYV>(rect_);
> +		break;
> +	default:
> +		std::cerr << "Unsupported pixel format "
> +			  << cfg.pixelFormat.toString() << std::endl;
> +		return -EINVAL;
> +	};
> +
> +	return 0;
> +}
> +
> +int SDLSink::start()
> +{
> +	int ret = SDL_Init(SDL_INIT_VIDEO);
> +	if (ret) {
> +		std::cerr << "Failed to initialize SDL: " << SDL_GetError()
> +			  << std::endl;
> +		return ret;
> +	}
> +
> +	init_ = true;
> +	window_ = SDL_CreateWindow("", SDL_WINDOWPOS_UNDEFINED,
> +				   SDL_WINDOWPOS_UNDEFINED, rect_.w,
> +				   rect_.h,
> +				   SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE);
> +	if (!window_) {
> +		std::cerr << "Failed to create SDL window: " << SDL_GetError()
> +			  << std::endl;
> +		return -EINVAL;
> +	}
> +
> +	renderer_ = SDL_CreateRenderer(window_, -1, 0);
> +	if (!renderer_) {
> +		std::cerr << "Failed to create SDL renderer: " << SDL_GetError()
> +			  << std::endl;
> +		return -EINVAL;
> +	}
> +
> +	ret = SDL_RenderSetLogicalSize(renderer_, rect_.w, rect_.h);
> +	if (ret) { /* Not critical to set, don't return in this case, set for scaling purposes */
> +		std::cerr << "Failed to set SDL render logical size: "
> +			  << SDL_GetError() << std::endl;
> +	}

	/*
	 * Not critical to set, don't return in this case, set for scaling
	 * purposes.
	 */
	if (ret)
		std::cerr << "Failed to set SDL render logical size: "
			  << SDL_GetError() << std::endl;

> +
> +	ret = texture_->create(renderer_);
> +	if (ret) {
> +		return ret;
> +	}
> +
> +	/* \todo Make the event cancellable to support stop/start cycles. */
> +	EventLoop::instance()->addTimerEvent(
> +		10ms, std::bind(&SDLSink::processSDLEvents, this));
> +
> +	return 0;
> +}
> +
> +int SDLSink::stop()
> +{
> +	if (texture_) {
> +		texture_.reset();
> +	}

This can be written as

	texture_.reset();

or

	texture_ = nullptr;

> +
> +	if (renderer_) {
> +		SDL_DestroyRenderer(renderer_);
> +		renderer_ = nullptr;
> +	}
> +
> +	if (window_) {
> +		SDL_DestroyWindow(window_);
> +		window_ = nullptr;
> +	}
> +
> +	if (init_) {
> +		SDL_Quit();
> +		init_ = false;
> +	}
> +
> +	return FrameSink::stop();
> +}
> +
> +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()) {
> +		renderBuffer(buffer);
> +		break; /* to be expanded to launch SDL window per buffer */
> +	}
> +
> +	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 */
> +			EventLoop::instance()->exit(0);
> +		}
> +	}
> +}
> +
> +void SDLSink::renderBuffer(FrameBuffer *buffer)
> +{
> +	Image *image = mappedBuffers_[buffer].get();
> +
> +	/* \todo Implement support for multi-planar formats. */
> +	const FrameMetadata::Plane &meta =
> +		buffer->metadata().planes()[0];

This holds on a single line.

> +
> +	Span<uint8_t> data = image->data(0);
> +	if (meta.bytesused > data.size())
> +		std::cerr << "payload size " << meta.bytesused
> +			  << " larger than plane size " << data.size()
> +			  << std::endl;
> +
> +	texture_->update(data);
> +
> +	SDL_RenderClear(renderer_);
> +	SDL_RenderCopy(renderer_, texture_->get(), nullptr, nullptr);
> +	SDL_RenderPresent(renderer_);
> +}
> diff --git a/src/cam/sdl_sink.h b/src/cam/sdl_sink.h
> new file mode 100644
> index 00000000..83171cca
> --- /dev/null
> +++ b/src/cam/sdl_sink.h
> @@ -0,0 +1,49 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2022, Ideas on Board Oy
> + *
> + * sdl_sink.h - SDL Sink
> + */
> +
> +#pragma once
> +
> +#include <map>
> +#include <memory>
> +
> +#include <libcamera/stream.h>
> +
> +#include <SDL2/SDL.h>
> +
> +#include "frame_sink.h"
> +
> +class Image;
> +class SDLTexture;
> +
> +class SDLSink : public FrameSink
> +{
> +public:
> +	SDLSink();
> +	~SDLSink();
> +
> +	int configure(const libcamera::CameraConfiguration &config) override;
> +	int start() override;
> +	int stop() override;
> +	void mapBuffer(libcamera::FrameBuffer *buffer) override;
> +
> +	bool processRequest(libcamera::Request *request) override;
> +
> +private:
> +	void renderBuffer(libcamera::FrameBuffer *buffer);
> +	void processSDLEvents();
> +
> +	std::map<libcamera::FrameBuffer *, std::unique_ptr<Image>>
> +		mappedBuffers_;
> +
> +	std::unique_ptr<SDLTexture> texture_;
> +
> +	SDL_Window *window_;
> +	SDL_Renderer *renderer_;
> +	SDL_Rect rect_;
> +	SDL_PixelFormatEnum pixelFormat_;
> +	bool init_;
> +};
> diff --git a/src/cam/sdl_texture.cpp b/src/cam/sdl_texture.cpp
> new file mode 100644
> index 00000000..ac355a97
> --- /dev/null
> +++ b/src/cam/sdl_texture.cpp
> @@ -0,0 +1,37 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2022, Ideas on Board Oy
> + *
> + * sdl_texture.cpp - SDL Texture
> + */
> +
> +#include "sdl_texture.h"
> +
> +#include <iostream>
> +
> +SDLTexture::SDLTexture(const SDL_Rect &rect, SDL_PixelFormatEnum pixelFormat,
> +		       const int pitch)
> +	: ptr_(nullptr), rect_(rect), pixelFormat_(pixelFormat), pitch_(pitch)
> +{
> +}
> +
> +SDLTexture::~SDLTexture()
> +{
> +	if (ptr_) {
> +		SDL_DestroyTexture(ptr_);
> +	}

No need for curly braces.

> +}
> +
> +int SDLTexture::create(SDL_Renderer *renderer_)
> +{
> +	ptr_ = SDL_CreateTexture(renderer_, pixelFormat_,
> +				 SDL_TEXTUREACCESS_STREAMING, rect_.w,
> +				 rect_.h);
> +	if (!ptr_) {
> +		std::cerr << "Failed to create SDL texture: " << SDL_GetError()
> +			  << std::endl;
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> diff --git a/src/cam/sdl_texture.h b/src/cam/sdl_texture.h
> new file mode 100644
> index 00000000..b04eece0
> --- /dev/null
> +++ b/src/cam/sdl_texture.h
> @@ -0,0 +1,29 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2022, Ideas on Board Oy
> + *
> + * sdl_texture.h - SDL Texture
> + */
> +
> +#pragma once
> +
> +#include <SDL2/SDL.h>
> +
> +#include "image.h"
> +
> +class SDLTexture
> +{
> +public:
> +	SDLTexture(const SDL_Rect &rect, SDL_PixelFormatEnum pixelFormat,
> +		   const int pitch);
> +	virtual ~SDLTexture();
> +	int create(SDL_Renderer *renderer_);

s/renderer_/renderer/

> +	virtual void update(const libcamera::Span<uint8_t> &data) = 0;
> +	SDL_Texture *get() const { return ptr_; }
> +
> +protected:
> +	SDL_Texture *ptr_;
> +	const SDL_Rect &rect_;

This should be a copy, not a reference, as the caller shouldn't need to
guarantee that the rectangle passed to the constructor will not be
destroyed as long as the texture exists.

These are small issues, I can fix them when applying.

> +	SDL_PixelFormatEnum pixelFormat_;
> +	const int pitch_;
> +};
> diff --git a/src/cam/sdl_texture_yuyv.cpp b/src/cam/sdl_texture_yuyv.cpp
> new file mode 100644
> index 00000000..a219097b
> --- /dev/null
> +++ b/src/cam/sdl_texture_yuyv.cpp
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2022, Ideas on Board Oy
> + *
> + * sdl_texture_yuyv.cpp - SDL Texture YUYV
> + */
> +
> +#include "sdl_texture_yuyv.h"
> +
> +using namespace libcamera;
> +
> +SDLTextureYUYV::SDLTextureYUYV(const SDL_Rect &sdlRect)
> +	: SDLTexture(sdlRect, SDL_PIXELFORMAT_YUY2, 4 * ((sdlRect.w + 1) / 2))
> +{
> +}
> +
> +void SDLTextureYUYV::update(const Span<uint8_t> &data)
> +{
> +	SDL_UpdateTexture(ptr_, &rect_, data.data(), pitch_);
> +}
> diff --git a/src/cam/sdl_texture_yuyv.h b/src/cam/sdl_texture_yuyv.h
> new file mode 100644
> index 00000000..38c51c74
> --- /dev/null
> +++ b/src/cam/sdl_texture_yuyv.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2022, Ideas on Board Oy
> + *
> + * sdl_texture_yuyv.h - SDL Texture YUYV
> + */
> +
> +#pragma once
> +
> +#include "sdl_texture.h"
> +
> +class SDLTextureYUYV : public SDLTexture
> +{
> +public:
> +	SDLTextureYUYV(const SDL_Rect &sdlRect);
> +	void update(const libcamera::Span<uint8_t> &data) override;
> +};

Another small comment.

On Sun, May 22, 2022 at 01:36:28PM +0300, Laurent Pinchart via libcamera-devel wrote:
> On Fri, May 20, 2022 at 08:01:05PM +0100, Eric Curtin wrote:
> > This adds more portability to existing cam sinks. You can pass a
> > YUYV camera buffer and SDL will handle the pixel buffer conversion
> > to the display. 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 has not been
> > tested. Also tested on simpledrm Raspberry Pi 4 framebuffer
> > successfully where existing kms sink did not work. Can also be used as
> > kmsdrm sink. Only supports one camera stream at present.
> > 
> > Signed-off-by: Eric Curtin <ecurtin@redhat.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Tested-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/cam/camera_session.cpp   |   8 ++
> >  src/cam/main.cpp             |   4 +
> >  src/cam/main.h               |   1 +
> >  src/cam/meson.build          |  12 +++
> >  src/cam/sdl_sink.cpp         | 194 +++++++++++++++++++++++++++++++++++
> >  src/cam/sdl_sink.h           |  49 +++++++++
> >  src/cam/sdl_texture.cpp      |  37 +++++++
> >  src/cam/sdl_texture.h        |  29 ++++++
> >  src/cam/sdl_texture_yuyv.cpp |  20 ++++
> >  src/cam/sdl_texture_yuyv.h   |  17 +++
> >  10 files changed, 371 insertions(+)
> >  create mode 100644 src/cam/sdl_sink.cpp
> >  create mode 100644 src/cam/sdl_sink.h
> >  create mode 100644 src/cam/sdl_texture.cpp
> >  create mode 100644 src/cam/sdl_texture.h
> >  create mode 100644 src/cam/sdl_texture_yuyv.cpp
> >  create mode 100644 src/cam/sdl_texture_yuyv.h
> > 
> > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp
> > index efffafbf..336ae471 100644
> > --- a/src/cam/camera_session.cpp
> > +++ b/src/cam/camera_session.cpp
> > @@ -20,6 +20,9 @@
> >  #include "kms_sink.h"
> >  #endif
> >  #include "main.h"
> > +#ifdef HAVE_SDL
> > +#include "sdl_sink.h"
> > +#endif
> >  #include "stream_options.h"
> >  
> >  using namespace libcamera;
> > @@ -186,6 +189,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/main.cpp b/src/cam/main.cpp
> > index c7f664b9..962262a8 100644
> > --- a/src/cam/main.cpp
> > +++ b/src/cam/main.cpp
> > @@ -147,6 +147,10 @@ int CamApp::parseOptions(int argc, char *argv[])
> >  			 "The default file name is 'frame-#.bin'.",
> >  			 "file", ArgumentOptional, "filename", false,
> >  			 OptCamera);
> > +#ifdef HAVE_SDL
> > +	parser.addOption(OptSDL, OptionNone, "Display viewfinder through SDL",
> > +			 "sdl", ArgumentNone, "", false, OptCamera);
> > +#endif
> >  	parser.addOption(OptStream, &streamKeyValue,
> >  			 "Set configuration of a camera stream", "stream", true,
> >  			 OptCamera);
> > diff --git a/src/cam/main.h b/src/cam/main.h
> > index 62f7bbc9..507a184f 100644
> > --- a/src/cam/main.h
> > +++ b/src/cam/main.h
> > @@ -17,6 +17,7 @@ enum {
> >  	OptList = 'l',
> >  	OptListProperties = 'p',
> >  	OptMonitor = 'm',
> > +	OptSDL = 'S',
> >  	OptStream = 's',
> >  	OptListControls = 256,
> >  	OptStrictFormats = 257,
> > diff --git a/src/cam/meson.build b/src/cam/meson.build
> > index 5bab8c9e..7d714152 100644
> > --- a/src/cam/meson.build
> > +++ b/src/cam/meson.build
> > @@ -32,12 +32,24 @@ if libdrm.found()
> >      ])
> >  endif
> >  
> > +libsdl2 = dependency('SDL2', required : false)
> > +
> > +if libsdl2.found()
> > +    cam_cpp_args += ['-DHAVE_SDL']
> > +    cam_sources += files([
> > +        'sdl_sink.cpp',
> > +        'sdl_texture.cpp',
> > +        'sdl_texture_yuyv.cpp'
> > +    ])
> > +endif
> > +
> >  cam  = executable('cam', cam_sources,
> >                    dependencies : [
> >                        libatomic,
> >                        libcamera_public,
> >                        libdrm,
> >                        libevent,
> > +                      libsdl2,
> >                    ],
> >                    cpp_args : cam_cpp_args,
> >                    install : true)
> > diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp
> > new file mode 100644
> > index 00000000..65430efb
> > --- /dev/null
> > +++ b/src/cam/sdl_sink.cpp
> > @@ -0,0 +1,194 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2022, Ideas on Board Oy
> > + *
> > + * sdl_sink.h - 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"
> > +#include "sdl_texture_yuyv.h"
> > +
> > +using namespace libcamera;
> > +
> > +using namespace std::chrono_literals;
> > +
> > +SDLSink::SDLSink()
> > +	: window_(nullptr), renderer_(nullptr), rect_({}),
> > +	  init_(false)
> > +{
> > +}
> > +
> > +SDLSink::~SDLSink()
> > +{
> > +	stop();
> > +}
> > +
> > +int SDLSink::configure(const libcamera::CameraConfiguration &config)
> > +{
> > +	const int ret = FrameSink::configure(config);
> 
> No need to make ret const.
> 
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (config.size() > 1) {
> > +		std::cerr
> > +			<< "SDL sink only supports one camera stream at present, streaming first camera stream"
> > +			<< std::endl;
> > +	} else if (config.empty()) {
> > +		std::cerr << "Require at least one camera stream to process"
> > +			  << std::endl;
> > +		return -EINVAL;
> > +	}
> > +
> > +	const libcamera::StreamConfiguration &cfg = config.at(0);
> > +	rect_.w = cfg.size.width;
> > +	rect_.h = cfg.size.height;
> > +
> > +	switch (cfg.pixelFormat) {
> > +	case libcamera::formats::YUYV:
> > +		texture_ = std::make_unique<SDLTextureYUYV>(rect_);
> > +		break;
> > +	default:
> > +		std::cerr << "Unsupported pixel format "
> > +			  << cfg.pixelFormat.toString() << std::endl;
> > +		return -EINVAL;
> > +	};
> > +
> > +	return 0;
> > +}
> > +
> > +int SDLSink::start()
> > +{
> > +	int ret = SDL_Init(SDL_INIT_VIDEO);
> > +	if (ret) {
> > +		std::cerr << "Failed to initialize SDL: " << SDL_GetError()
> > +			  << std::endl;
> > +		return ret;
> > +	}
> > +
> > +	init_ = true;
> > +	window_ = SDL_CreateWindow("", SDL_WINDOWPOS_UNDEFINED,
> > +				   SDL_WINDOWPOS_UNDEFINED, rect_.w,
> > +				   rect_.h,
> > +				   SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE);
> > +	if (!window_) {
> > +		std::cerr << "Failed to create SDL window: " << SDL_GetError()
> > +			  << std::endl;
> > +		return -EINVAL;
> > +	}
> > +
> > +	renderer_ = SDL_CreateRenderer(window_, -1, 0);
> > +	if (!renderer_) {
> > +		std::cerr << "Failed to create SDL renderer: " << SDL_GetError()
> > +			  << std::endl;
> > +		return -EINVAL;
> > +	}
> > +
> > +	ret = SDL_RenderSetLogicalSize(renderer_, rect_.w, rect_.h);
> > +	if (ret) { /* Not critical to set, don't return in this case, set for scaling purposes */
> > +		std::cerr << "Failed to set SDL render logical size: "
> > +			  << SDL_GetError() << std::endl;
> > +	}
> 
> 	/*
> 	 * Not critical to set, don't return in this case, set for scaling
> 	 * purposes.
> 	 */
> 	if (ret)
> 		std::cerr << "Failed to set SDL render logical size: "
> 			  << SDL_GetError() << std::endl;
> 
> > +
> > +	ret = texture_->create(renderer_);
> > +	if (ret) {
> > +		return ret;
> > +	}
> > +
> > +	/* \todo Make the event cancellable to support stop/start cycles. */
> > +	EventLoop::instance()->addTimerEvent(
> > +		10ms, std::bind(&SDLSink::processSDLEvents, this));
> > +
> > +	return 0;
> > +}
> > +
> > +int SDLSink::stop()
> > +{
> > +	if (texture_) {
> > +		texture_.reset();
> > +	}
> 
> This can be written as
> 
> 	texture_.reset();
> 
> or
> 
> 	texture_ = nullptr;
> 
> > +
> > +	if (renderer_) {
> > +		SDL_DestroyRenderer(renderer_);
> > +		renderer_ = nullptr;
> > +	}
> > +
> > +	if (window_) {
> > +		SDL_DestroyWindow(window_);
> > +		window_ = nullptr;
> > +	}
> > +
> > +	if (init_) {
> > +		SDL_Quit();
> > +		init_ = false;
> > +	}
> > +
> > +	return FrameSink::stop();
> > +}
> > +
> > +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()) {
> > +		renderBuffer(buffer);
> > +		break; /* to be expanded to launch SDL window per buffer */
> > +	}
> > +
> > +	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 */
> > +			EventLoop::instance()->exit(0);
> > +		}
> > +	}
> > +}
> > +
> > +void SDLSink::renderBuffer(FrameBuffer *buffer)
> > +{
> > +	Image *image = mappedBuffers_[buffer].get();
> > +
> > +	/* \todo Implement support for multi-planar formats. */
> > +	const FrameMetadata::Plane &meta =
> > +		buffer->metadata().planes()[0];
> 
> This holds on a single line.
> 
> > +
> > +	Span<uint8_t> data = image->data(0);
> > +	if (meta.bytesused > data.size())
> > +		std::cerr << "payload size " << meta.bytesused
> > +			  << " larger than plane size " << data.size()
> > +			  << std::endl;
> > +
> > +	texture_->update(data);
> > +
> > +	SDL_RenderClear(renderer_);
> > +	SDL_RenderCopy(renderer_, texture_->get(), nullptr, nullptr);
> > +	SDL_RenderPresent(renderer_);
> > +}
> > diff --git a/src/cam/sdl_sink.h b/src/cam/sdl_sink.h
> > new file mode 100644
> > index 00000000..83171cca
> > --- /dev/null
> > +++ b/src/cam/sdl_sink.h
> > @@ -0,0 +1,49 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2022, Ideas on Board Oy
> > + *
> > + * sdl_sink.h - SDL Sink
> > + */
> > +
> > +#pragma once
> > +
> > +#include <map>
> > +#include <memory>
> > +
> > +#include <libcamera/stream.h>
> > +
> > +#include <SDL2/SDL.h>
> > +
> > +#include "frame_sink.h"
> > +
> > +class Image;
> > +class SDLTexture;
> > +
> > +class SDLSink : public FrameSink
> > +{
> > +public:
> > +	SDLSink();
> > +	~SDLSink();
> > +
> > +	int configure(const libcamera::CameraConfiguration &config) override;
> > +	int start() override;
> > +	int stop() override;
> > +	void mapBuffer(libcamera::FrameBuffer *buffer) override;
> > +
> > +	bool processRequest(libcamera::Request *request) override;
> > +
> > +private:
> > +	void renderBuffer(libcamera::FrameBuffer *buffer);
> > +	void processSDLEvents();
> > +
> > +	std::map<libcamera::FrameBuffer *, std::unique_ptr<Image>>
> > +		mappedBuffers_;
> > +
> > +	std::unique_ptr<SDLTexture> texture_;
> > +
> > +	SDL_Window *window_;
> > +	SDL_Renderer *renderer_;
> > +	SDL_Rect rect_;
> > +	SDL_PixelFormatEnum pixelFormat_;

This isn't used and can be dropped.

> > +	bool init_;
> > +};
> > diff --git a/src/cam/sdl_texture.cpp b/src/cam/sdl_texture.cpp
> > new file mode 100644
> > index 00000000..ac355a97
> > --- /dev/null
> > +++ b/src/cam/sdl_texture.cpp
> > @@ -0,0 +1,37 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2022, Ideas on Board Oy
> > + *
> > + * sdl_texture.cpp - SDL Texture
> > + */
> > +
> > +#include "sdl_texture.h"
> > +
> > +#include <iostream>
> > +
> > +SDLTexture::SDLTexture(const SDL_Rect &rect, SDL_PixelFormatEnum pixelFormat,
> > +		       const int pitch)
> > +	: ptr_(nullptr), rect_(rect), pixelFormat_(pixelFormat), pitch_(pitch)
> > +{
> > +}
> > +
> > +SDLTexture::~SDLTexture()
> > +{
> > +	if (ptr_) {
> > +		SDL_DestroyTexture(ptr_);
> > +	}
> 
> No need for curly braces.
> 
> > +}
> > +
> > +int SDLTexture::create(SDL_Renderer *renderer_)
> > +{
> > +	ptr_ = SDL_CreateTexture(renderer_, pixelFormat_,
> > +				 SDL_TEXTUREACCESS_STREAMING, rect_.w,
> > +				 rect_.h);
> > +	if (!ptr_) {
> > +		std::cerr << "Failed to create SDL texture: " << SDL_GetError()
> > +			  << std::endl;
> > +		return -ENOMEM;
> > +	}
> > +
> > +	return 0;
> > +}
> > diff --git a/src/cam/sdl_texture.h b/src/cam/sdl_texture.h
> > new file mode 100644
> > index 00000000..b04eece0
> > --- /dev/null
> > +++ b/src/cam/sdl_texture.h
> > @@ -0,0 +1,29 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2022, Ideas on Board Oy
> > + *
> > + * sdl_texture.h - SDL Texture
> > + */
> > +
> > +#pragma once
> > +
> > +#include <SDL2/SDL.h>
> > +
> > +#include "image.h"
> > +
> > +class SDLTexture
> > +{
> > +public:
> > +	SDLTexture(const SDL_Rect &rect, SDL_PixelFormatEnum pixelFormat,
> > +		   const int pitch);
> > +	virtual ~SDLTexture();
> > +	int create(SDL_Renderer *renderer_);
> 
> s/renderer_/renderer/
> 
> > +	virtual void update(const libcamera::Span<uint8_t> &data) = 0;
> > +	SDL_Texture *get() const { return ptr_; }
> > +
> > +protected:
> > +	SDL_Texture *ptr_;
> > +	const SDL_Rect &rect_;
> 
> This should be a copy, not a reference, as the caller shouldn't need to
> guarantee that the rectangle passed to the constructor will not be
> destroyed as long as the texture exists.
> 
> These are small issues, I can fix them when applying.
> 
> > +	SDL_PixelFormatEnum pixelFormat_;
> > +	const int pitch_;
> > +};
> > diff --git a/src/cam/sdl_texture_yuyv.cpp b/src/cam/sdl_texture_yuyv.cpp
> > new file mode 100644
> > index 00000000..a219097b
> > --- /dev/null
> > +++ b/src/cam/sdl_texture_yuyv.cpp
> > @@ -0,0 +1,20 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2022, Ideas on Board Oy
> > + *
> > + * sdl_texture_yuyv.cpp - SDL Texture YUYV
> > + */
> > +
> > +#include "sdl_texture_yuyv.h"
> > +
> > +using namespace libcamera;
> > +
> > +SDLTextureYUYV::SDLTextureYUYV(const SDL_Rect &sdlRect)
> > +	: SDLTexture(sdlRect, SDL_PIXELFORMAT_YUY2, 4 * ((sdlRect.w + 1) / 2))
> > +{
> > +}
> > +
> > +void SDLTextureYUYV::update(const Span<uint8_t> &data)
> > +{
> > +	SDL_UpdateTexture(ptr_, &rect_, data.data(), pitch_);
> > +}
> > diff --git a/src/cam/sdl_texture_yuyv.h b/src/cam/sdl_texture_yuyv.h
> > new file mode 100644
> > index 00000000..38c51c74
> > --- /dev/null
> > +++ b/src/cam/sdl_texture_yuyv.h
> > @@ -0,0 +1,17 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2022, Ideas on Board Oy
> > + *
> > + * sdl_texture_yuyv.h - SDL Texture YUYV
> > + */
> > +
> > +#pragma once
> > +
> > +#include "sdl_texture.h"
> > +
> > +class SDLTextureYUYV : public SDLTexture
> > +{
> > +public:
> > +	SDLTextureYUYV(const SDL_Rect &sdlRect);
> > +	void update(const libcamera::Span<uint8_t> &data) override;
> > +};

On Sun, 22 May 2022 at 18:32, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Another small comment.
>
> On Sun, May 22, 2022 at 01:36:28PM +0300, Laurent Pinchart via libcamera-devel wrote:
> > On Fri, May 20, 2022 at 08:01:05PM +0100, Eric Curtin wrote:
> > > This adds more portability to existing cam sinks. You can pass a
> > > YUYV camera buffer and SDL will handle the pixel buffer conversion
> > > to the display. 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 has not been
> > > tested. Also tested on simpledrm Raspberry Pi 4 framebuffer
> > > successfully where existing kms sink did not work. Can also be used as
> > > kmsdrm sink. Only supports one camera stream at present.
> > >
> > > Signed-off-by: Eric Curtin <ecurtin@redhat.com>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > Tested-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  src/cam/camera_session.cpp   |   8 ++
> > >  src/cam/main.cpp             |   4 +
> > >  src/cam/main.h               |   1 +
> > >  src/cam/meson.build          |  12 +++
> > >  src/cam/sdl_sink.cpp         | 194 +++++++++++++++++++++++++++++++++++
> > >  src/cam/sdl_sink.h           |  49 +++++++++
> > >  src/cam/sdl_texture.cpp      |  37 +++++++
> > >  src/cam/sdl_texture.h        |  29 ++++++
> > >  src/cam/sdl_texture_yuyv.cpp |  20 ++++
> > >  src/cam/sdl_texture_yuyv.h   |  17 +++
> > >  10 files changed, 371 insertions(+)
> > >  create mode 100644 src/cam/sdl_sink.cpp
> > >  create mode 100644 src/cam/sdl_sink.h
> > >  create mode 100644 src/cam/sdl_texture.cpp
> > >  create mode 100644 src/cam/sdl_texture.h
> > >  create mode 100644 src/cam/sdl_texture_yuyv.cpp
> > >  create mode 100644 src/cam/sdl_texture_yuyv.h
> > >
> > > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp
> > > index efffafbf..336ae471 100644
> > > --- a/src/cam/camera_session.cpp
> > > +++ b/src/cam/camera_session.cpp
> > > @@ -20,6 +20,9 @@
> > >  #include "kms_sink.h"
> > >  #endif
> > >  #include "main.h"
> > > +#ifdef HAVE_SDL
> > > +#include "sdl_sink.h"
> > > +#endif
> > >  #include "stream_options.h"
> > >
> > >  using namespace libcamera;
> > > @@ -186,6 +189,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/main.cpp b/src/cam/main.cpp
> > > index c7f664b9..962262a8 100644
> > > --- a/src/cam/main.cpp
> > > +++ b/src/cam/main.cpp
> > > @@ -147,6 +147,10 @@ int CamApp::parseOptions(int argc, char *argv[])
> > >                      "The default file name is 'frame-#.bin'.",
> > >                      "file", ArgumentOptional, "filename", false,
> > >                      OptCamera);
> > > +#ifdef HAVE_SDL
> > > +   parser.addOption(OptSDL, OptionNone, "Display viewfinder through SDL",
> > > +                    "sdl", ArgumentNone, "", false, OptCamera);
> > > +#endif
> > >     parser.addOption(OptStream, &streamKeyValue,
> > >                      "Set configuration of a camera stream", "stream", true,
> > >                      OptCamera);
> > > diff --git a/src/cam/main.h b/src/cam/main.h
> > > index 62f7bbc9..507a184f 100644
> > > --- a/src/cam/main.h
> > > +++ b/src/cam/main.h
> > > @@ -17,6 +17,7 @@ enum {
> > >     OptList = 'l',
> > >     OptListProperties = 'p',
> > >     OptMonitor = 'm',
> > > +   OptSDL = 'S',
> > >     OptStream = 's',
> > >     OptListControls = 256,
> > >     OptStrictFormats = 257,
> > > diff --git a/src/cam/meson.build b/src/cam/meson.build
> > > index 5bab8c9e..7d714152 100644
> > > --- a/src/cam/meson.build
> > > +++ b/src/cam/meson.build
> > > @@ -32,12 +32,24 @@ if libdrm.found()
> > >      ])
> > >  endif
> > >
> > > +libsdl2 = dependency('SDL2', required : false)
> > > +
> > > +if libsdl2.found()
> > > +    cam_cpp_args += ['-DHAVE_SDL']
> > > +    cam_sources += files([
> > > +        'sdl_sink.cpp',
> > > +        'sdl_texture.cpp',
> > > +        'sdl_texture_yuyv.cpp'
> > > +    ])
> > > +endif
> > > +
> > >  cam  = executable('cam', cam_sources,
> > >                    dependencies : [
> > >                        libatomic,
> > >                        libcamera_public,
> > >                        libdrm,
> > >                        libevent,
> > > +                      libsdl2,
> > >                    ],
> > >                    cpp_args : cam_cpp_args,
> > >                    install : true)
> > > diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp
> > > new file mode 100644
> > > index 00000000..65430efb
> > > --- /dev/null
> > > +++ b/src/cam/sdl_sink.cpp
> > > @@ -0,0 +1,194 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > +/*
> > > + * Copyright (C) 2022, Ideas on Board Oy
> > > + *
> > > + * sdl_sink.h - 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"
> > > +#include "sdl_texture_yuyv.h"
> > > +
> > > +using namespace libcamera;
> > > +
> > > +using namespace std::chrono_literals;
> > > +
> > > +SDLSink::SDLSink()
> > > +   : window_(nullptr), renderer_(nullptr), rect_({}),
> > > +     init_(false)
> > > +{
> > > +}
> > > +
> > > +SDLSink::~SDLSink()
> > > +{
> > > +   stop();
> > > +}
> > > +
> > > +int SDLSink::configure(const libcamera::CameraConfiguration &config)
> > > +{
> > > +   const int ret = FrameSink::configure(config);
> >
> > No need to make ret const.
> >
> > > +   if (ret < 0)
> > > +           return ret;
> > > +
> > > +   if (config.size() > 1) {
> > > +           std::cerr
> > > +                   << "SDL sink only supports one camera stream at present, streaming first camera stream"
> > > +                   << std::endl;
> > > +   } else if (config.empty()) {
> > > +           std::cerr << "Require at least one camera stream to process"
> > > +                     << std::endl;
> > > +           return -EINVAL;
> > > +   }
> > > +
> > > +   const libcamera::StreamConfiguration &cfg = config.at(0);
> > > +   rect_.w = cfg.size.width;
> > > +   rect_.h = cfg.size.height;
> > > +
> > > +   switch (cfg.pixelFormat) {
> > > +   case libcamera::formats::YUYV:
> > > +           texture_ = std::make_unique<SDLTextureYUYV>(rect_);
> > > +           break;
> > > +   default:
> > > +           std::cerr << "Unsupported pixel format "
> > > +                     << cfg.pixelFormat.toString() << std::endl;
> > > +           return -EINVAL;
> > > +   };
> > > +
> > > +   return 0;
> > > +}
> > > +
> > > +int SDLSink::start()
> > > +{
> > > +   int ret = SDL_Init(SDL_INIT_VIDEO);
> > > +   if (ret) {
> > > +           std::cerr << "Failed to initialize SDL: " << SDL_GetError()
> > > +                     << std::endl;
> > > +           return ret;
> > > +   }
> > > +
> > > +   init_ = true;
> > > +   window_ = SDL_CreateWindow("", SDL_WINDOWPOS_UNDEFINED,
> > > +                              SDL_WINDOWPOS_UNDEFINED, rect_.w,
> > > +                              rect_.h,
> > > +                              SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE);
> > > +   if (!window_) {
> > > +           std::cerr << "Failed to create SDL window: " << SDL_GetError()
> > > +                     << std::endl;
> > > +           return -EINVAL;
> > > +   }
> > > +
> > > +   renderer_ = SDL_CreateRenderer(window_, -1, 0);
> > > +   if (!renderer_) {
> > > +           std::cerr << "Failed to create SDL renderer: " << SDL_GetError()
> > > +                     << std::endl;
> > > +           return -EINVAL;
> > > +   }
> > > +
> > > +   ret = SDL_RenderSetLogicalSize(renderer_, rect_.w, rect_.h);
> > > +   if (ret) { /* Not critical to set, don't return in this case, set for scaling purposes */
> > > +           std::cerr << "Failed to set SDL render logical size: "
> > > +                     << SDL_GetError() << std::endl;
> > > +   }
> >
> >       /*
> >        * Not critical to set, don't return in this case, set for scaling
> >        * purposes.
> >        */
> >       if (ret)
> >               std::cerr << "Failed to set SDL render logical size: "
> >                         << SDL_GetError() << std::endl;
> >
> > > +
> > > +   ret = texture_->create(renderer_);
> > > +   if (ret) {
> > > +           return ret;
> > > +   }
> > > +
> > > +   /* \todo Make the event cancellable to support stop/start cycles. */
> > > +   EventLoop::instance()->addTimerEvent(
> > > +           10ms, std::bind(&SDLSink::processSDLEvents, this));
> > > +
> > > +   return 0;
> > > +}
> > > +
> > > +int SDLSink::stop()
> > > +{
> > > +   if (texture_) {
> > > +           texture_.reset();
> > > +   }
> >
> > This can be written as
> >
> >       texture_.reset();
> >
> > or
> >
> >       texture_ = nullptr;
> >
> > > +
> > > +   if (renderer_) {
> > > +           SDL_DestroyRenderer(renderer_);
> > > +           renderer_ = nullptr;
> > > +   }
> > > +
> > > +   if (window_) {
> > > +           SDL_DestroyWindow(window_);
> > > +           window_ = nullptr;
> > > +   }
> > > +
> > > +   if (init_) {
> > > +           SDL_Quit();
> > > +           init_ = false;
> > > +   }
> > > +
> > > +   return FrameSink::stop();
> > > +}
> > > +
> > > +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()) {
> > > +           renderBuffer(buffer);
> > > +           break; /* to be expanded to launch SDL window per buffer */
> > > +   }
> > > +
> > > +   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 */
> > > +                   EventLoop::instance()->exit(0);
> > > +           }
> > > +   }
> > > +}
> > > +
> > > +void SDLSink::renderBuffer(FrameBuffer *buffer)
> > > +{
> > > +   Image *image = mappedBuffers_[buffer].get();
> > > +
> > > +   /* \todo Implement support for multi-planar formats. */
> > > +   const FrameMetadata::Plane &meta =
> > > +           buffer->metadata().planes()[0];
> >
> > This holds on a single line.
> >
> > > +
> > > +   Span<uint8_t> data = image->data(0);
> > > +   if (meta.bytesused > data.size())
> > > +           std::cerr << "payload size " << meta.bytesused
> > > +                     << " larger than plane size " << data.size()
> > > +                     << std::endl;
> > > +
> > > +   texture_->update(data);
> > > +
> > > +   SDL_RenderClear(renderer_);
> > > +   SDL_RenderCopy(renderer_, texture_->get(), nullptr, nullptr);
> > > +   SDL_RenderPresent(renderer_);
> > > +}
> > > diff --git a/src/cam/sdl_sink.h b/src/cam/sdl_sink.h
> > > new file mode 100644
> > > index 00000000..83171cca
> > > --- /dev/null
> > > +++ b/src/cam/sdl_sink.h
> > > @@ -0,0 +1,49 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > +/*
> > > + * Copyright (C) 2022, Ideas on Board Oy
> > > + *
> > > + * sdl_sink.h - SDL Sink
> > > + */
> > > +
> > > +#pragma once
> > > +
> > > +#include <map>
> > > +#include <memory>
> > > +
> > > +#include <libcamera/stream.h>
> > > +
> > > +#include <SDL2/SDL.h>
> > > +
> > > +#include "frame_sink.h"
> > > +
> > > +class Image;
> > > +class SDLTexture;
> > > +
> > > +class SDLSink : public FrameSink
> > > +{
> > > +public:
> > > +   SDLSink();
> > > +   ~SDLSink();
> > > +
> > > +   int configure(const libcamera::CameraConfiguration &config) override;
> > > +   int start() override;
> > > +   int stop() override;
> > > +   void mapBuffer(libcamera::FrameBuffer *buffer) override;
> > > +
> > > +   bool processRequest(libcamera::Request *request) override;
> > > +
> > > +private:
> > > +   void renderBuffer(libcamera::FrameBuffer *buffer);
> > > +   void processSDLEvents();
> > > +
> > > +   std::map<libcamera::FrameBuffer *, std::unique_ptr<Image>>
> > > +           mappedBuffers_;
> > > +
> > > +   std::unique_ptr<SDLTexture> texture_;
> > > +
> > > +   SDL_Window *window_;
> > > +   SDL_Renderer *renderer_;
> > > +   SDL_Rect rect_;
> > > +   SDL_PixelFormatEnum pixelFormat_;
>
> This isn't used and can be dropped.
>
> > > +   bool init_;
> > > +};
> > > diff --git a/src/cam/sdl_texture.cpp b/src/cam/sdl_texture.cpp
> > > new file mode 100644
> > > index 00000000..ac355a97
> > > --- /dev/null
> > > +++ b/src/cam/sdl_texture.cpp
> > > @@ -0,0 +1,37 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > +/*
> > > + * Copyright (C) 2022, Ideas on Board Oy
> > > + *
> > > + * sdl_texture.cpp - SDL Texture
> > > + */
> > > +
> > > +#include "sdl_texture.h"
> > > +
> > > +#include <iostream>
> > > +
> > > +SDLTexture::SDLTexture(const SDL_Rect &rect, SDL_PixelFormatEnum pixelFormat,
> > > +                  const int pitch)
> > > +   : ptr_(nullptr), rect_(rect), pixelFormat_(pixelFormat), pitch_(pitch)
> > > +{
> > > +}
> > > +
> > > +SDLTexture::~SDLTexture()
> > > +{
> > > +   if (ptr_) {
> > > +           SDL_DestroyTexture(ptr_);
> > > +   }
> >
> > No need for curly braces.
> >
> > > +}
> > > +
> > > +int SDLTexture::create(SDL_Renderer *renderer_)
> > > +{
> > > +   ptr_ = SDL_CreateTexture(renderer_, pixelFormat_,
> > > +                            SDL_TEXTUREACCESS_STREAMING, rect_.w,
> > > +                            rect_.h);
> > > +   if (!ptr_) {
> > > +           std::cerr << "Failed to create SDL texture: " << SDL_GetError()
> > > +                     << std::endl;
> > > +           return -ENOMEM;
> > > +   }
> > > +
> > > +   return 0;
> > > +}
> > > diff --git a/src/cam/sdl_texture.h b/src/cam/sdl_texture.h
> > > new file mode 100644
> > > index 00000000..b04eece0
> > > --- /dev/null
> > > +++ b/src/cam/sdl_texture.h
> > > @@ -0,0 +1,29 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > +/*
> > > + * Copyright (C) 2022, Ideas on Board Oy
> > > + *
> > > + * sdl_texture.h - SDL Texture
> > > + */
> > > +
> > > +#pragma once
> > > +
> > > +#include <SDL2/SDL.h>
> > > +
> > > +#include "image.h"
> > > +
> > > +class SDLTexture
> > > +{
> > > +public:
> > > +   SDLTexture(const SDL_Rect &rect, SDL_PixelFormatEnum pixelFormat,
> > > +              const int pitch);
> > > +   virtual ~SDLTexture();
> > > +   int create(SDL_Renderer *renderer_);
> >
> > s/renderer_/renderer/
> >
> > > +   virtual void update(const libcamera::Span<uint8_t> &data) = 0;
> > > +   SDL_Texture *get() const { return ptr_; }
> > > +
> > > +protected:
> > > +   SDL_Texture *ptr_;
> > > +   const SDL_Rect &rect_;
> >
> > This should be a copy, not a reference, as the caller shouldn't need to
> > guarantee that the rectangle passed to the constructor will not be
> > destroyed as long as the texture exists.
> >
> > These are small issues, I can fix them when applying.
> >
> > > +   SDL_PixelFormatEnum pixelFormat_;
> > > +   const int pitch_;
> > > +};
> > > diff --git a/src/cam/sdl_texture_yuyv.cpp b/src/cam/sdl_texture_yuyv.cpp
> > > new file mode 100644
> > > index 00000000..a219097b
> > > --- /dev/null
> > > +++ b/src/cam/sdl_texture_yuyv.cpp
> > > @@ -0,0 +1,20 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > +/*
> > > + * Copyright (C) 2022, Ideas on Board Oy
> > > + *
> > > + * sdl_texture_yuyv.cpp - SDL Texture YUYV
> > > + */
> > > +
> > > +#include "sdl_texture_yuyv.h"
> > > +
> > > +using namespace libcamera;
> > > +
> > > +SDLTextureYUYV::SDLTextureYUYV(const SDL_Rect &sdlRect)
> > > +   : SDLTexture(sdlRect, SDL_PIXELFORMAT_YUY2, 4 * ((sdlRect.w + 1) / 2))
> > > +{
> > > +}
> > > +
> > > +void SDLTextureYUYV::update(const Span<uint8_t> &data)
> > > +{
> > > +   SDL_UpdateTexture(ptr_, &rect_, data.data(), pitch_);
> > > +}
> > > diff --git a/src/cam/sdl_texture_yuyv.h b/src/cam/sdl_texture_yuyv.h
> > > new file mode 100644
> > > index 00000000..38c51c74
> > > --- /dev/null
> > > +++ b/src/cam/sdl_texture_yuyv.h
> > > @@ -0,0 +1,17 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > +/*
> > > + * Copyright (C) 2022, Ideas on Board Oy
> > > + *
> > > + * sdl_texture_yuyv.h - SDL Texture YUYV
> > > + */
> > > +
> > > +#pragma once
> > > +
> > > +#include "sdl_texture.h"
> > > +
> > > +class SDLTextureYUYV : public SDLTexture
> > > +{
> > > +public:
> > > +   SDLTextureYUYV(const SDL_Rect &sdlRect);
> > > +   void update(const libcamera::Span<uint8_t> &data) override;
> > > +};
>
> --
> Regards,
>
> Laurent Pinchart
>

There is an extra "SDL_PixelFormatEnum pixelFormat_;" sorry, if you'd
like to make changes no problem on my end. The main thing for me is to
have this merged and working for YUYV and MJPG and build on it from
there.

Hi Eric,

On Sun, May 22, 2022 at 09:08:09PM +0100, Eric Curtin wrote:
> On Sun, 22 May 2022 at 18:32, Laurent Pinchart wrote:
> >
> > Another small comment.
> >
> > On Sun, May 22, 2022 at 01:36:28PM +0300, Laurent Pinchart via libcamera-devel wrote:
> > > On Fri, May 20, 2022 at 08:01:05PM +0100, Eric Curtin wrote:
> > > > This adds more portability to existing cam sinks. You can pass a
> > > > YUYV camera buffer and SDL will handle the pixel buffer conversion
> > > > to the display. 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 has not been
> > > > tested. Also tested on simpledrm Raspberry Pi 4 framebuffer
> > > > successfully where existing kms sink did not work. Can also be used as
> > > > kmsdrm sink. Only supports one camera stream at present.
> > > >
> > > > Signed-off-by: Eric Curtin <ecurtin@redhat.com>
> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > Tested-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > ---
> > > >  src/cam/camera_session.cpp   |   8 ++
> > > >  src/cam/main.cpp             |   4 +
> > > >  src/cam/main.h               |   1 +
> > > >  src/cam/meson.build          |  12 +++
> > > >  src/cam/sdl_sink.cpp         | 194 +++++++++++++++++++++++++++++++++++
> > > >  src/cam/sdl_sink.h           |  49 +++++++++
> > > >  src/cam/sdl_texture.cpp      |  37 +++++++
> > > >  src/cam/sdl_texture.h        |  29 ++++++
> > > >  src/cam/sdl_texture_yuyv.cpp |  20 ++++
> > > >  src/cam/sdl_texture_yuyv.h   |  17 +++
> > > >  10 files changed, 371 insertions(+)
> > > >  create mode 100644 src/cam/sdl_sink.cpp
> > > >  create mode 100644 src/cam/sdl_sink.h
> > > >  create mode 100644 src/cam/sdl_texture.cpp
> > > >  create mode 100644 src/cam/sdl_texture.h
> > > >  create mode 100644 src/cam/sdl_texture_yuyv.cpp
> > > >  create mode 100644 src/cam/sdl_texture_yuyv.h
> > > >
> > > > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp
> > > > index efffafbf..336ae471 100644
> > > > --- a/src/cam/camera_session.cpp
> > > > +++ b/src/cam/camera_session.cpp
> > > > @@ -20,6 +20,9 @@
> > > >  #include "kms_sink.h"
> > > >  #endif
> > > >  #include "main.h"
> > > > +#ifdef HAVE_SDL
> > > > +#include "sdl_sink.h"
> > > > +#endif
> > > >  #include "stream_options.h"
> > > >
> > > >  using namespace libcamera;
> > > > @@ -186,6 +189,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/main.cpp b/src/cam/main.cpp
> > > > index c7f664b9..962262a8 100644
> > > > --- a/src/cam/main.cpp
> > > > +++ b/src/cam/main.cpp
> > > > @@ -147,6 +147,10 @@ int CamApp::parseOptions(int argc, char *argv[])
> > > >                      "The default file name is 'frame-#.bin'.",
> > > >                      "file", ArgumentOptional, "filename", false,
> > > >                      OptCamera);
> > > > +#ifdef HAVE_SDL
> > > > +   parser.addOption(OptSDL, OptionNone, "Display viewfinder through SDL",
> > > > +                    "sdl", ArgumentNone, "", false, OptCamera);
> > > > +#endif
> > > >     parser.addOption(OptStream, &streamKeyValue,
> > > >                      "Set configuration of a camera stream", "stream", true,
> > > >                      OptCamera);
> > > > diff --git a/src/cam/main.h b/src/cam/main.h
> > > > index 62f7bbc9..507a184f 100644
> > > > --- a/src/cam/main.h
> > > > +++ b/src/cam/main.h
> > > > @@ -17,6 +17,7 @@ enum {
> > > >     OptList = 'l',
> > > >     OptListProperties = 'p',
> > > >     OptMonitor = 'm',
> > > > +   OptSDL = 'S',
> > > >     OptStream = 's',
> > > >     OptListControls = 256,
> > > >     OptStrictFormats = 257,
> > > > diff --git a/src/cam/meson.build b/src/cam/meson.build
> > > > index 5bab8c9e..7d714152 100644
> > > > --- a/src/cam/meson.build
> > > > +++ b/src/cam/meson.build
> > > > @@ -32,12 +32,24 @@ if libdrm.found()
> > > >      ])
> > > >  endif
> > > >
> > > > +libsdl2 = dependency('SDL2', required : false)
> > > > +
> > > > +if libsdl2.found()
> > > > +    cam_cpp_args += ['-DHAVE_SDL']
> > > > +    cam_sources += files([
> > > > +        'sdl_sink.cpp',
> > > > +        'sdl_texture.cpp',
> > > > +        'sdl_texture_yuyv.cpp'
> > > > +    ])
> > > > +endif
> > > > +
> > > >  cam  = executable('cam', cam_sources,
> > > >                    dependencies : [
> > > >                        libatomic,
> > > >                        libcamera_public,
> > > >                        libdrm,
> > > >                        libevent,
> > > > +                      libsdl2,
> > > >                    ],
> > > >                    cpp_args : cam_cpp_args,
> > > >                    install : true)
> > > > diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp
> > > > new file mode 100644
> > > > index 00000000..65430efb
> > > > --- /dev/null
> > > > +++ b/src/cam/sdl_sink.cpp
> > > > @@ -0,0 +1,194 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > > +/*
> > > > + * Copyright (C) 2022, Ideas on Board Oy
> > > > + *
> > > > + * sdl_sink.h - 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"
> > > > +#include "sdl_texture_yuyv.h"
> > > > +
> > > > +using namespace libcamera;
> > > > +
> > > > +using namespace std::chrono_literals;
> > > > +
> > > > +SDLSink::SDLSink()
> > > > +   : window_(nullptr), renderer_(nullptr), rect_({}),
> > > > +     init_(false)
> > > > +{
> > > > +}
> > > > +
> > > > +SDLSink::~SDLSink()
> > > > +{
> > > > +   stop();
> > > > +}
> > > > +
> > > > +int SDLSink::configure(const libcamera::CameraConfiguration &config)
> > > > +{
> > > > +   const int ret = FrameSink::configure(config);
> > >
> > > No need to make ret const.
> > >
> > > > +   if (ret < 0)
> > > > +           return ret;
> > > > +
> > > > +   if (config.size() > 1) {
> > > > +           std::cerr
> > > > +                   << "SDL sink only supports one camera stream at present, streaming first camera stream"
> > > > +                   << std::endl;
> > > > +   } else if (config.empty()) {
> > > > +           std::cerr << "Require at least one camera stream to process"
> > > > +                     << std::endl;
> > > > +           return -EINVAL;
> > > > +   }
> > > > +
> > > > +   const libcamera::StreamConfiguration &cfg = config.at(0);
> > > > +   rect_.w = cfg.size.width;
> > > > +   rect_.h = cfg.size.height;
> > > > +
> > > > +   switch (cfg.pixelFormat) {
> > > > +   case libcamera::formats::YUYV:
> > > > +           texture_ = std::make_unique<SDLTextureYUYV>(rect_);
> > > > +           break;
> > > > +   default:
> > > > +           std::cerr << "Unsupported pixel format "
> > > > +                     << cfg.pixelFormat.toString() << std::endl;
> > > > +           return -EINVAL;
> > > > +   };
> > > > +
> > > > +   return 0;
> > > > +}
> > > > +
> > > > +int SDLSink::start()
> > > > +{
> > > > +   int ret = SDL_Init(SDL_INIT_VIDEO);
> > > > +   if (ret) {
> > > > +           std::cerr << "Failed to initialize SDL: " << SDL_GetError()
> > > > +                     << std::endl;
> > > > +           return ret;
> > > > +   }
> > > > +
> > > > +   init_ = true;
> > > > +   window_ = SDL_CreateWindow("", SDL_WINDOWPOS_UNDEFINED,
> > > > +                              SDL_WINDOWPOS_UNDEFINED, rect_.w,
> > > > +                              rect_.h,
> > > > +                              SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE);
> > > > +   if (!window_) {
> > > > +           std::cerr << "Failed to create SDL window: " << SDL_GetError()
> > > > +                     << std::endl;
> > > > +           return -EINVAL;
> > > > +   }
> > > > +
> > > > +   renderer_ = SDL_CreateRenderer(window_, -1, 0);
> > > > +   if (!renderer_) {
> > > > +           std::cerr << "Failed to create SDL renderer: " << SDL_GetError()
> > > > +                     << std::endl;
> > > > +           return -EINVAL;
> > > > +   }
> > > > +
> > > > +   ret = SDL_RenderSetLogicalSize(renderer_, rect_.w, rect_.h);
> > > > +   if (ret) { /* Not critical to set, don't return in this case, set for scaling purposes */
> > > > +           std::cerr << "Failed to set SDL render logical size: "
> > > > +                     << SDL_GetError() << std::endl;
> > > > +   }
> > >
> > >       /*
> > >        * Not critical to set, don't return in this case, set for scaling
> > >        * purposes.
> > >        */
> > >       if (ret)
> > >               std::cerr << "Failed to set SDL render logical size: "
> > >                         << SDL_GetError() << std::endl;
> > >
> > > > +
> > > > +   ret = texture_->create(renderer_);
> > > > +   if (ret) {
> > > > +           return ret;
> > > > +   }
> > > > +
> > > > +   /* \todo Make the event cancellable to support stop/start cycles. */
> > > > +   EventLoop::instance()->addTimerEvent(
> > > > +           10ms, std::bind(&SDLSink::processSDLEvents, this));
> > > > +
> > > > +   return 0;
> > > > +}
> > > > +
> > > > +int SDLSink::stop()
> > > > +{
> > > > +   if (texture_) {
> > > > +           texture_.reset();
> > > > +   }
> > >
> > > This can be written as
> > >
> > >       texture_.reset();
> > >
> > > or
> > >
> > >       texture_ = nullptr;
> > >
> > > > +
> > > > +   if (renderer_) {
> > > > +           SDL_DestroyRenderer(renderer_);
> > > > +           renderer_ = nullptr;
> > > > +   }
> > > > +
> > > > +   if (window_) {
> > > > +           SDL_DestroyWindow(window_);
> > > > +           window_ = nullptr;
> > > > +   }
> > > > +
> > > > +   if (init_) {
> > > > +           SDL_Quit();
> > > > +           init_ = false;
> > > > +   }
> > > > +
> > > > +   return FrameSink::stop();
> > > > +}
> > > > +
> > > > +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()) {
> > > > +           renderBuffer(buffer);
> > > > +           break; /* to be expanded to launch SDL window per buffer */
> > > > +   }
> > > > +
> > > > +   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 */
> > > > +                   EventLoop::instance()->exit(0);
> > > > +           }
> > > > +   }
> > > > +}
> > > > +
> > > > +void SDLSink::renderBuffer(FrameBuffer *buffer)
> > > > +{
> > > > +   Image *image = mappedBuffers_[buffer].get();
> > > > +
> > > > +   /* \todo Implement support for multi-planar formats. */
> > > > +   const FrameMetadata::Plane &meta =
> > > > +           buffer->metadata().planes()[0];
> > >
> > > This holds on a single line.
> > >
> > > > +
> > > > +   Span<uint8_t> data = image->data(0);
> > > > +   if (meta.bytesused > data.size())
> > > > +           std::cerr << "payload size " << meta.bytesused
> > > > +                     << " larger than plane size " << data.size()
> > > > +                     << std::endl;
> > > > +
> > > > +   texture_->update(data);
> > > > +
> > > > +   SDL_RenderClear(renderer_);
> > > > +   SDL_RenderCopy(renderer_, texture_->get(), nullptr, nullptr);
> > > > +   SDL_RenderPresent(renderer_);
> > > > +}
> > > > diff --git a/src/cam/sdl_sink.h b/src/cam/sdl_sink.h
> > > > new file mode 100644
> > > > index 00000000..83171cca
> > > > --- /dev/null
> > > > +++ b/src/cam/sdl_sink.h
> > > > @@ -0,0 +1,49 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > > +/*
> > > > + * Copyright (C) 2022, Ideas on Board Oy
> > > > + *
> > > > + * sdl_sink.h - SDL Sink
> > > > + */
> > > > +
> > > > +#pragma once
> > > > +
> > > > +#include <map>
> > > > +#include <memory>
> > > > +
> > > > +#include <libcamera/stream.h>
> > > > +
> > > > +#include <SDL2/SDL.h>
> > > > +
> > > > +#include "frame_sink.h"
> > > > +
> > > > +class Image;
> > > > +class SDLTexture;
> > > > +
> > > > +class SDLSink : public FrameSink
> > > > +{
> > > > +public:
> > > > +   SDLSink();
> > > > +   ~SDLSink();
> > > > +
> > > > +   int configure(const libcamera::CameraConfiguration &config) override;
> > > > +   int start() override;
> > > > +   int stop() override;
> > > > +   void mapBuffer(libcamera::FrameBuffer *buffer) override;
> > > > +
> > > > +   bool processRequest(libcamera::Request *request) override;
> > > > +
> > > > +private:
> > > > +   void renderBuffer(libcamera::FrameBuffer *buffer);
> > > > +   void processSDLEvents();
> > > > +
> > > > +   std::map<libcamera::FrameBuffer *, std::unique_ptr<Image>>
> > > > +           mappedBuffers_;
> > > > +
> > > > +   std::unique_ptr<SDLTexture> texture_;
> > > > +
> > > > +   SDL_Window *window_;
> > > > +   SDL_Renderer *renderer_;
> > > > +   SDL_Rect rect_;
> > > > +   SDL_PixelFormatEnum pixelFormat_;
> >
> > This isn't used and can be dropped.
> >
> > > > +   bool init_;
> > > > +};
> > > > diff --git a/src/cam/sdl_texture.cpp b/src/cam/sdl_texture.cpp
> > > > new file mode 100644
> > > > index 00000000..ac355a97
> > > > --- /dev/null
> > > > +++ b/src/cam/sdl_texture.cpp
> > > > @@ -0,0 +1,37 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > > +/*
> > > > + * Copyright (C) 2022, Ideas on Board Oy
> > > > + *
> > > > + * sdl_texture.cpp - SDL Texture
> > > > + */
> > > > +
> > > > +#include "sdl_texture.h"
> > > > +
> > > > +#include <iostream>
> > > > +
> > > > +SDLTexture::SDLTexture(const SDL_Rect &rect, SDL_PixelFormatEnum pixelFormat,
> > > > +                  const int pitch)
> > > > +   : ptr_(nullptr), rect_(rect), pixelFormat_(pixelFormat), pitch_(pitch)
> > > > +{
> > > > +}
> > > > +
> > > > +SDLTexture::~SDLTexture()
> > > > +{
> > > > +   if (ptr_) {
> > > > +           SDL_DestroyTexture(ptr_);
> > > > +   }
> > >
> > > No need for curly braces.
> > >
> > > > +}
> > > > +
> > > > +int SDLTexture::create(SDL_Renderer *renderer_)
> > > > +{
> > > > +   ptr_ = SDL_CreateTexture(renderer_, pixelFormat_,
> > > > +                            SDL_TEXTUREACCESS_STREAMING, rect_.w,
> > > > +                            rect_.h);
> > > > +   if (!ptr_) {
> > > > +           std::cerr << "Failed to create SDL texture: " << SDL_GetError()
> > > > +                     << std::endl;
> > > > +           return -ENOMEM;
> > > > +   }
> > > > +
> > > > +   return 0;
> > > > +}
> > > > diff --git a/src/cam/sdl_texture.h b/src/cam/sdl_texture.h
> > > > new file mode 100644
> > > > index 00000000..b04eece0
> > > > --- /dev/null
> > > > +++ b/src/cam/sdl_texture.h
> > > > @@ -0,0 +1,29 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > > +/*
> > > > + * Copyright (C) 2022, Ideas on Board Oy
> > > > + *
> > > > + * sdl_texture.h - SDL Texture
> > > > + */
> > > > +
> > > > +#pragma once
> > > > +
> > > > +#include <SDL2/SDL.h>
> > > > +
> > > > +#include "image.h"
> > > > +
> > > > +class SDLTexture
> > > > +{
> > > > +public:
> > > > +   SDLTexture(const SDL_Rect &rect, SDL_PixelFormatEnum pixelFormat,
> > > > +              const int pitch);
> > > > +   virtual ~SDLTexture();
> > > > +   int create(SDL_Renderer *renderer_);
> > >
> > > s/renderer_/renderer/
> > >
> > > > +   virtual void update(const libcamera::Span<uint8_t> &data) = 0;
> > > > +   SDL_Texture *get() const { return ptr_; }
> > > > +
> > > > +protected:
> > > > +   SDL_Texture *ptr_;
> > > > +   const SDL_Rect &rect_;
> > >
> > > This should be a copy, not a reference, as the caller shouldn't need to
> > > guarantee that the rectangle passed to the constructor will not be
> > > destroyed as long as the texture exists.
> > >
> > > These are small issues, I can fix them when applying.
> > >
> > > > +   SDL_PixelFormatEnum pixelFormat_;
> > > > +   const int pitch_;
> > > > +};
> > > > diff --git a/src/cam/sdl_texture_yuyv.cpp b/src/cam/sdl_texture_yuyv.cpp
> > > > new file mode 100644
> > > > index 00000000..a219097b
> > > > --- /dev/null
> > > > +++ b/src/cam/sdl_texture_yuyv.cpp
> > > > @@ -0,0 +1,20 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > > +/*
> > > > + * Copyright (C) 2022, Ideas on Board Oy
> > > > + *
> > > > + * sdl_texture_yuyv.cpp - SDL Texture YUYV
> > > > + */
> > > > +
> > > > +#include "sdl_texture_yuyv.h"
> > > > +
> > > > +using namespace libcamera;
> > > > +
> > > > +SDLTextureYUYV::SDLTextureYUYV(const SDL_Rect &sdlRect)
> > > > +   : SDLTexture(sdlRect, SDL_PIXELFORMAT_YUY2, 4 * ((sdlRect.w + 1) / 2))
> > > > +{
> > > > +}
> > > > +
> > > > +void SDLTextureYUYV::update(const Span<uint8_t> &data)
> > > > +{
> > > > +   SDL_UpdateTexture(ptr_, &rect_, data.data(), pitch_);
> > > > +}
> > > > diff --git a/src/cam/sdl_texture_yuyv.h b/src/cam/sdl_texture_yuyv.h
> > > > new file mode 100644
> > > > index 00000000..38c51c74
> > > > --- /dev/null
> > > > +++ b/src/cam/sdl_texture_yuyv.h
> > > > @@ -0,0 +1,17 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > > +/*
> > > > + * Copyright (C) 2022, Ideas on Board Oy
> > > > + *
> > > > + * sdl_texture_yuyv.h - SDL Texture YUYV
> > > > + */
> > > > +
> > > > +#pragma once
> > > > +
> > > > +#include "sdl_texture.h"
> > > > +
> > > > +class SDLTextureYUYV : public SDLTexture
> > > > +{
> > > > +public:
> > > > +   SDLTextureYUYV(const SDL_Rect &sdlRect);
> > > > +   void update(const libcamera::Span<uint8_t> &data) override;
> > > > +};
> 
> There is an extra "SDL_PixelFormatEnum pixelFormat_;" sorry, if you'd
> like to make changes no problem on my end. The main thing for me is to
> have this merged and working for YUYV and MJPG and build on it from
> there.

OK. Could you reply to the question about the race condition fix ?
That's the only outstanding issue, I can then merge the patches.

On Mon, 23 May 2022 at 09:25, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Eric,
>
> On Sun, May 22, 2022 at 09:08:09PM +0100, Eric Curtin wrote:
> > On Sun, 22 May 2022 at 18:32, Laurent Pinchart wrote:
> > >
> > > Another small comment.
> > >
> > > On Sun, May 22, 2022 at 01:36:28PM +0300, Laurent Pinchart via libcamera-devel wrote:
> > > > On Fri, May 20, 2022 at 08:01:05PM +0100, Eric Curtin wrote:
> > > > > This adds more portability to existing cam sinks. You can pass a
> > > > > YUYV camera buffer and SDL will handle the pixel buffer conversion
> > > > > to the display. 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 has not been
> > > > > tested. Also tested on simpledrm Raspberry Pi 4 framebuffer
> > > > > successfully where existing kms sink did not work. Can also be used as
> > > > > kmsdrm sink. Only supports one camera stream at present.
> > > > >
> > > > > Signed-off-by: Eric Curtin <ecurtin@redhat.com>
> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > > Tested-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > > ---
> > > > >  src/cam/camera_session.cpp   |   8 ++
> > > > >  src/cam/main.cpp             |   4 +
> > > > >  src/cam/main.h               |   1 +
> > > > >  src/cam/meson.build          |  12 +++
> > > > >  src/cam/sdl_sink.cpp         | 194 +++++++++++++++++++++++++++++++++++
> > > > >  src/cam/sdl_sink.h           |  49 +++++++++
> > > > >  src/cam/sdl_texture.cpp      |  37 +++++++
> > > > >  src/cam/sdl_texture.h        |  29 ++++++
> > > > >  src/cam/sdl_texture_yuyv.cpp |  20 ++++
> > > > >  src/cam/sdl_texture_yuyv.h   |  17 +++
> > > > >  10 files changed, 371 insertions(+)
> > > > >  create mode 100644 src/cam/sdl_sink.cpp
> > > > >  create mode 100644 src/cam/sdl_sink.h
> > > > >  create mode 100644 src/cam/sdl_texture.cpp
> > > > >  create mode 100644 src/cam/sdl_texture.h
> > > > >  create mode 100644 src/cam/sdl_texture_yuyv.cpp
> > > > >  create mode 100644 src/cam/sdl_texture_yuyv.h
> > > > >
> > > > > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp
> > > > > index efffafbf..336ae471 100644
> > > > > --- a/src/cam/camera_session.cpp
> > > > > +++ b/src/cam/camera_session.cpp
> > > > > @@ -20,6 +20,9 @@
> > > > >  #include "kms_sink.h"
> > > > >  #endif
> > > > >  #include "main.h"
> > > > > +#ifdef HAVE_SDL
> > > > > +#include "sdl_sink.h"
> > > > > +#endif
> > > > >  #include "stream_options.h"
> > > > >
> > > > >  using namespace libcamera;
> > > > > @@ -186,6 +189,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/main.cpp b/src/cam/main.cpp
> > > > > index c7f664b9..962262a8 100644
> > > > > --- a/src/cam/main.cpp
> > > > > +++ b/src/cam/main.cpp
> > > > > @@ -147,6 +147,10 @@ int CamApp::parseOptions(int argc, char *argv[])
> > > > >                      "The default file name is 'frame-#.bin'.",
> > > > >                      "file", ArgumentOptional, "filename", false,
> > > > >                      OptCamera);
> > > > > +#ifdef HAVE_SDL
> > > > > +   parser.addOption(OptSDL, OptionNone, "Display viewfinder through SDL",
> > > > > +                    "sdl", ArgumentNone, "", false, OptCamera);
> > > > > +#endif
> > > > >     parser.addOption(OptStream, &streamKeyValue,
> > > > >                      "Set configuration of a camera stream", "stream", true,
> > > > >                      OptCamera);
> > > > > diff --git a/src/cam/main.h b/src/cam/main.h
> > > > > index 62f7bbc9..507a184f 100644
> > > > > --- a/src/cam/main.h
> > > > > +++ b/src/cam/main.h
> > > > > @@ -17,6 +17,7 @@ enum {
> > > > >     OptList = 'l',
> > > > >     OptListProperties = 'p',
> > > > >     OptMonitor = 'm',
> > > > > +   OptSDL = 'S',
> > > > >     OptStream = 's',
> > > > >     OptListControls = 256,
> > > > >     OptStrictFormats = 257,
> > > > > diff --git a/src/cam/meson.build b/src/cam/meson.build
> > > > > index 5bab8c9e..7d714152 100644
> > > > > --- a/src/cam/meson.build
> > > > > +++ b/src/cam/meson.build
> > > > > @@ -32,12 +32,24 @@ if libdrm.found()
> > > > >      ])
> > > > >  endif
> > > > >
> > > > > +libsdl2 = dependency('SDL2', required : false)
> > > > > +
> > > > > +if libsdl2.found()
> > > > > +    cam_cpp_args += ['-DHAVE_SDL']
> > > > > +    cam_sources += files([
> > > > > +        'sdl_sink.cpp',
> > > > > +        'sdl_texture.cpp',
> > > > > +        'sdl_texture_yuyv.cpp'
> > > > > +    ])
> > > > > +endif
> > > > > +
> > > > >  cam  = executable('cam', cam_sources,
> > > > >                    dependencies : [
> > > > >                        libatomic,
> > > > >                        libcamera_public,
> > > > >                        libdrm,
> > > > >                        libevent,
> > > > > +                      libsdl2,
> > > > >                    ],
> > > > >                    cpp_args : cam_cpp_args,
> > > > >                    install : true)
> > > > > diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp
> > > > > new file mode 100644
> > > > > index 00000000..65430efb
> > > > > --- /dev/null
> > > > > +++ b/src/cam/sdl_sink.cpp
> > > > > @@ -0,0 +1,194 @@
> > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > > > +/*
> > > > > + * Copyright (C) 2022, Ideas on Board Oy
> > > > > + *
> > > > > + * sdl_sink.h - 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"
> > > > > +#include "sdl_texture_yuyv.h"
> > > > > +
> > > > > +using namespace libcamera;
> > > > > +
> > > > > +using namespace std::chrono_literals;
> > > > > +
> > > > > +SDLSink::SDLSink()
> > > > > +   : window_(nullptr), renderer_(nullptr), rect_({}),
> > > > > +     init_(false)
> > > > > +{
> > > > > +}
> > > > > +
> > > > > +SDLSink::~SDLSink()
> > > > > +{
> > > > > +   stop();
> > > > > +}
> > > > > +
> > > > > +int SDLSink::configure(const libcamera::CameraConfiguration &config)
> > > > > +{
> > > > > +   const int ret = FrameSink::configure(config);
> > > >
> > > > No need to make ret const.
> > > >
> > > > > +   if (ret < 0)
> > > > > +           return ret;
> > > > > +
> > > > > +   if (config.size() > 1) {
> > > > > +           std::cerr
> > > > > +                   << "SDL sink only supports one camera stream at present, streaming first camera stream"
> > > > > +                   << std::endl;
> > > > > +   } else if (config.empty()) {
> > > > > +           std::cerr << "Require at least one camera stream to process"
> > > > > +                     << std::endl;
> > > > > +           return -EINVAL;
> > > > > +   }
> > > > > +
> > > > > +   const libcamera::StreamConfiguration &cfg = config.at(0);
> > > > > +   rect_.w = cfg.size.width;
> > > > > +   rect_.h = cfg.size.height;
> > > > > +
> > > > > +   switch (cfg.pixelFormat) {
> > > > > +   case libcamera::formats::YUYV:
> > > > > +           texture_ = std::make_unique<SDLTextureYUYV>(rect_);
> > > > > +           break;
> > > > > +   default:
> > > > > +           std::cerr << "Unsupported pixel format "
> > > > > +                     << cfg.pixelFormat.toString() << std::endl;
> > > > > +           return -EINVAL;
> > > > > +   };
> > > > > +
> > > > > +   return 0;
> > > > > +}
> > > > > +
> > > > > +int SDLSink::start()
> > > > > +{
> > > > > +   int ret = SDL_Init(SDL_INIT_VIDEO);
> > > > > +   if (ret) {
> > > > > +           std::cerr << "Failed to initialize SDL: " << SDL_GetError()
> > > > > +                     << std::endl;
> > > > > +           return ret;
> > > > > +   }
> > > > > +
> > > > > +   init_ = true;
> > > > > +   window_ = SDL_CreateWindow("", SDL_WINDOWPOS_UNDEFINED,
> > > > > +                              SDL_WINDOWPOS_UNDEFINED, rect_.w,
> > > > > +                              rect_.h,
> > > > > +                              SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE);
> > > > > +   if (!window_) {
> > > > > +           std::cerr << "Failed to create SDL window: " << SDL_GetError()
> > > > > +                     << std::endl;
> > > > > +           return -EINVAL;
> > > > > +   }
> > > > > +
> > > > > +   renderer_ = SDL_CreateRenderer(window_, -1, 0);
> > > > > +   if (!renderer_) {
> > > > > +           std::cerr << "Failed to create SDL renderer: " << SDL_GetError()
> > > > > +                     << std::endl;
> > > > > +           return -EINVAL;
> > > > > +   }
> > > > > +
> > > > > +   ret = SDL_RenderSetLogicalSize(renderer_, rect_.w, rect_.h);
> > > > > +   if (ret) { /* Not critical to set, don't return in this case, set for scaling purposes */
> > > > > +           std::cerr << "Failed to set SDL render logical size: "
> > > > > +                     << SDL_GetError() << std::endl;
> > > > > +   }
> > > >
> > > >       /*
> > > >        * Not critical to set, don't return in this case, set for scaling
> > > >        * purposes.
> > > >        */
> > > >       if (ret)
> > > >               std::cerr << "Failed to set SDL render logical size: "
> > > >                         << SDL_GetError() << std::endl;
> > > >
> > > > > +
> > > > > +   ret = texture_->create(renderer_);
> > > > > +   if (ret) {
> > > > > +           return ret;
> > > > > +   }
> > > > > +
> > > > > +   /* \todo Make the event cancellable to support stop/start cycles. */
> > > > > +   EventLoop::instance()->addTimerEvent(
> > > > > +           10ms, std::bind(&SDLSink::processSDLEvents, this));
> > > > > +
> > > > > +   return 0;
> > > > > +}
> > > > > +
> > > > > +int SDLSink::stop()
> > > > > +{
> > > > > +   if (texture_) {
> > > > > +           texture_.reset();
> > > > > +   }
> > > >
> > > > This can be written as
> > > >
> > > >       texture_.reset();
> > > >
> > > > or
> > > >
> > > >       texture_ = nullptr;
> > > >
> > > > > +
> > > > > +   if (renderer_) {
> > > > > +           SDL_DestroyRenderer(renderer_);
> > > > > +           renderer_ = nullptr;
> > > > > +   }
> > > > > +
> > > > > +   if (window_) {
> > > > > +           SDL_DestroyWindow(window_);
> > > > > +           window_ = nullptr;
> > > > > +   }
> > > > > +
> > > > > +   if (init_) {
> > > > > +           SDL_Quit();
> > > > > +           init_ = false;
> > > > > +   }
> > > > > +
> > > > > +   return FrameSink::stop();
> > > > > +}
> > > > > +
> > > > > +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()) {
> > > > > +           renderBuffer(buffer);
> > > > > +           break; /* to be expanded to launch SDL window per buffer */
> > > > > +   }
> > > > > +
> > > > > +   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 */
> > > > > +                   EventLoop::instance()->exit(0);
> > > > > +           }
> > > > > +   }
> > > > > +}
> > > > > +
> > > > > +void SDLSink::renderBuffer(FrameBuffer *buffer)
> > > > > +{
> > > > > +   Image *image = mappedBuffers_[buffer].get();
> > > > > +
> > > > > +   /* \todo Implement support for multi-planar formats. */
> > > > > +   const FrameMetadata::Plane &meta =
> > > > > +           buffer->metadata().planes()[0];
> > > >
> > > > This holds on a single line.
> > > >
> > > > > +
> > > > > +   Span<uint8_t> data = image->data(0);
> > > > > +   if (meta.bytesused > data.size())
> > > > > +           std::cerr << "payload size " << meta.bytesused
> > > > > +                     << " larger than plane size " << data.size()
> > > > > +                     << std::endl;
> > > > > +
> > > > > +   texture_->update(data);
> > > > > +
> > > > > +   SDL_RenderClear(renderer_);
> > > > > +   SDL_RenderCopy(renderer_, texture_->get(), nullptr, nullptr);
> > > > > +   SDL_RenderPresent(renderer_);
> > > > > +}
> > > > > diff --git a/src/cam/sdl_sink.h b/src/cam/sdl_sink.h
> > > > > new file mode 100644
> > > > > index 00000000..83171cca
> > > > > --- /dev/null
> > > > > +++ b/src/cam/sdl_sink.h
> > > > > @@ -0,0 +1,49 @@
> > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > > > +/*
> > > > > + * Copyright (C) 2022, Ideas on Board Oy
> > > > > + *
> > > > > + * sdl_sink.h - SDL Sink
> > > > > + */
> > > > > +
> > > > > +#pragma once
> > > > > +
> > > > > +#include <map>
> > > > > +#include <memory>
> > > > > +
> > > > > +#include <libcamera/stream.h>
> > > > > +
> > > > > +#include <SDL2/SDL.h>
> > > > > +
> > > > > +#include "frame_sink.h"
> > > > > +
> > > > > +class Image;
> > > > > +class SDLTexture;
> > > > > +
> > > > > +class SDLSink : public FrameSink
> > > > > +{
> > > > > +public:
> > > > > +   SDLSink();
> > > > > +   ~SDLSink();
> > > > > +
> > > > > +   int configure(const libcamera::CameraConfiguration &config) override;
> > > > > +   int start() override;
> > > > > +   int stop() override;
> > > > > +   void mapBuffer(libcamera::FrameBuffer *buffer) override;
> > > > > +
> > > > > +   bool processRequest(libcamera::Request *request) override;
> > > > > +
> > > > > +private:
> > > > > +   void renderBuffer(libcamera::FrameBuffer *buffer);
> > > > > +   void processSDLEvents();
> > > > > +
> > > > > +   std::map<libcamera::FrameBuffer *, std::unique_ptr<Image>>
> > > > > +           mappedBuffers_;
> > > > > +
> > > > > +   std::unique_ptr<SDLTexture> texture_;
> > > > > +
> > > > > +   SDL_Window *window_;
> > > > > +   SDL_Renderer *renderer_;
> > > > > +   SDL_Rect rect_;
> > > > > +   SDL_PixelFormatEnum pixelFormat_;
> > >
> > > This isn't used and can be dropped.
> > >
> > > > > +   bool init_;
> > > > > +};
> > > > > diff --git a/src/cam/sdl_texture.cpp b/src/cam/sdl_texture.cpp
> > > > > new file mode 100644
> > > > > index 00000000..ac355a97
> > > > > --- /dev/null
> > > > > +++ b/src/cam/sdl_texture.cpp
> > > > > @@ -0,0 +1,37 @@
> > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > > > +/*
> > > > > + * Copyright (C) 2022, Ideas on Board Oy
> > > > > + *
> > > > > + * sdl_texture.cpp - SDL Texture
> > > > > + */
> > > > > +
> > > > > +#include "sdl_texture.h"
> > > > > +
> > > > > +#include <iostream>
> > > > > +
> > > > > +SDLTexture::SDLTexture(const SDL_Rect &rect, SDL_PixelFormatEnum pixelFormat,
> > > > > +                  const int pitch)
> > > > > +   : ptr_(nullptr), rect_(rect), pixelFormat_(pixelFormat), pitch_(pitch)
> > > > > +{
> > > > > +}
> > > > > +
> > > > > +SDLTexture::~SDLTexture()
> > > > > +{
> > > > > +   if (ptr_) {
> > > > > +           SDL_DestroyTexture(ptr_);
> > > > > +   }
> > > >
> > > > No need for curly braces.
> > > >
> > > > > +}
> > > > > +
> > > > > +int SDLTexture::create(SDL_Renderer *renderer_)
> > > > > +{
> > > > > +   ptr_ = SDL_CreateTexture(renderer_, pixelFormat_,
> > > > > +                            SDL_TEXTUREACCESS_STREAMING, rect_.w,
> > > > > +                            rect_.h);
> > > > > +   if (!ptr_) {
> > > > > +           std::cerr << "Failed to create SDL texture: " << SDL_GetError()
> > > > > +                     << std::endl;
> > > > > +           return -ENOMEM;
> > > > > +   }
> > > > > +
> > > > > +   return 0;
> > > > > +}
> > > > > diff --git a/src/cam/sdl_texture.h b/src/cam/sdl_texture.h
> > > > > new file mode 100644
> > > > > index 00000000..b04eece0
> > > > > --- /dev/null
> > > > > +++ b/src/cam/sdl_texture.h
> > > > > @@ -0,0 +1,29 @@
> > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > > > +/*
> > > > > + * Copyright (C) 2022, Ideas on Board Oy
> > > > > + *
> > > > > + * sdl_texture.h - SDL Texture
> > > > > + */
> > > > > +
> > > > > +#pragma once
> > > > > +
> > > > > +#include <SDL2/SDL.h>
> > > > > +
> > > > > +#include "image.h"
> > > > > +
> > > > > +class SDLTexture
> > > > > +{
> > > > > +public:
> > > > > +   SDLTexture(const SDL_Rect &rect, SDL_PixelFormatEnum pixelFormat,
> > > > > +              const int pitch);
> > > > > +   virtual ~SDLTexture();
> > > > > +   int create(SDL_Renderer *renderer_);
> > > >
> > > > s/renderer_/renderer/
> > > >
> > > > > +   virtual void update(const libcamera::Span<uint8_t> &data) = 0;
> > > > > +   SDL_Texture *get() const { return ptr_; }
> > > > > +
> > > > > +protected:
> > > > > +   SDL_Texture *ptr_;
> > > > > +   const SDL_Rect &rect_;
> > > >
> > > > This should be a copy, not a reference, as the caller shouldn't need to
> > > > guarantee that the rectangle passed to the constructor will not be
> > > > destroyed as long as the texture exists.
> > > >
> > > > These are small issues, I can fix them when applying.
> > > >
> > > > > +   SDL_PixelFormatEnum pixelFormat_;
> > > > > +   const int pitch_;
> > > > > +};
> > > > > diff --git a/src/cam/sdl_texture_yuyv.cpp b/src/cam/sdl_texture_yuyv.cpp
> > > > > new file mode 100644
> > > > > index 00000000..a219097b
> > > > > --- /dev/null
> > > > > +++ b/src/cam/sdl_texture_yuyv.cpp
> > > > > @@ -0,0 +1,20 @@
> > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > > > +/*
> > > > > + * Copyright (C) 2022, Ideas on Board Oy
> > > > > + *
> > > > > + * sdl_texture_yuyv.cpp - SDL Texture YUYV
> > > > > + */
> > > > > +
> > > > > +#include "sdl_texture_yuyv.h"
> > > > > +
> > > > > +using namespace libcamera;
> > > > > +
> > > > > +SDLTextureYUYV::SDLTextureYUYV(const SDL_Rect &sdlRect)
> > > > > +   : SDLTexture(sdlRect, SDL_PIXELFORMAT_YUY2, 4 * ((sdlRect.w + 1) / 2))
> > > > > +{
> > > > > +}
> > > > > +
> > > > > +void SDLTextureYUYV::update(const Span<uint8_t> &data)
> > > > > +{
> > > > > +   SDL_UpdateTexture(ptr_, &rect_, data.data(), pitch_);
> > > > > +}
> > > > > diff --git a/src/cam/sdl_texture_yuyv.h b/src/cam/sdl_texture_yuyv.h
> > > > > new file mode 100644
> > > > > index 00000000..38c51c74
> > > > > --- /dev/null
> > > > > +++ b/src/cam/sdl_texture_yuyv.h
> > > > > @@ -0,0 +1,17 @@
> > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > > > +/*
> > > > > + * Copyright (C) 2022, Ideas on Board Oy
> > > > > + *
> > > > > + * sdl_texture_yuyv.h - SDL Texture YUYV
> > > > > + */
> > > > > +
> > > > > +#pragma once
> > > > > +
> > > > > +#include "sdl_texture.h"
> > > > > +
> > > > > +class SDLTextureYUYV : public SDLTexture
> > > > > +{
> > > > > +public:
> > > > > +   SDLTextureYUYV(const SDL_Rect &sdlRect);
> > > > > +   void update(const libcamera::Span<uint8_t> &data) override;
> > > > > +};
> >
> > There is an extra "SDL_PixelFormatEnum pixelFormat_;" sorry, if you'd
> > like to make changes no problem on my end. The main thing for me is to
> > have this merged and working for YUYV and MJPG and build on it from
> > there.
>
> OK. Could you reply to the question about the race condition fix ?
> That's the only outstanding issue, I can then merge the patches.

There was no identified race condition, you could remove that.

>
> --
> Regards,
>
> Laurent Pinchart
>