[{"id":23022,"web_url":"https://patchwork.libcamera.org/comment/23022/","msgid":"<165283070083.2416244.15906036641040599419@Monstersaurus>","date":"2022-05-17T23:38:20","subject":"Re: [libcamera-devel] [PATCH v8 3/4] cam: sdl_sink: Add SDL sink\n\twith initial YUYV support","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Eric Curtin via libcamera-devel (2022-05-05 16:18:50)\n> This adds more portability to existing cam sinks. You can pass a\n> YUYV camera buffer and SDL will handle the pixel buffer conversion\n> to the display. This allows cam reference implementation to display\n> images on VMs, Mac M1, Raspberry Pi, etc. This also enables cam\n> reference implementation, to run as a desktop application in wayland or\n> x11. SDL also has support for Android and ChromeOS which has not been\n> tested. Also tested on simpledrm raspberry pi 4 framebuffer\n> successfully where existing kms sink did not work. Can also be used as\n> kmsdrm sink. Only supports one camera stream at present.\n> \n> Signed-off-by: Eric Curtin \n> ---\n> src/cam/camera_session.cpp | 8 ++\n> src/cam/main.cpp | 4 +\n> src/cam/main.h | 1 +\n> src/cam/meson.build | 12 +++\n> src/cam/sdl_sink.cpp | 192 +++++++++++++++++++++++++++++++++++\n> src/cam/sdl_sink.h | 48 +++++++++\n> src/cam/sdl_texture.cpp | 32 ++++++\n> src/cam/sdl_texture.h | 23 +++++\n> src/cam/sdl_texture_yuyv.cpp | 13 +++\n> src/cam/sdl_texture_yuyv.h | 10 ++\n\nGiven the number of sdl_ files here I wonder if they should be under\nsrc/cam/sdl/.... \n\nBut not crucial to this patch.\n\nI can't test this right now - but I'm looking forward to seeing a\npreview on cam ... so I'll hopefully test this tomorrow.\n\n\n> 10 files changed, 343 insertions(+)\n> create mode 100644 src/cam/sdl_sink.cpp\n> create mode 100644 src/cam/sdl_sink.h\n> create mode 100644 src/cam/sdl_texture.cpp\n> create mode 100644 src/cam/sdl_texture.h\n> create mode 100644 src/cam/sdl_texture_yuyv.cpp\n> create mode 100644 src/cam/sdl_texture_yuyv.h\n> \n> diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp\n> index efffafbf..71e6bd60 100644\n> --- a/src/cam/camera_session.cpp\n> +++ b/src/cam/camera_session.cpp\n> @@ -19,6 +19,9 @@\n> #ifdef HAVE_KMS\n> #include \"kms_sink.h\"\n> #endif\n> +#ifdef HAVE_SDL\n> +#include \"sdl_sink.h\"\n> +#endif\n> #include \"main.h\"\n> #include \"stream_options.h\"\n> \n> @@ -186,6 +189,11 @@ int CameraSession::start()\n> sink_ = std::make_unique(options_[OptDisplay].toString());\n> #endif\n> \n> +#ifdef HAVE_SDL\n> + if (options_.isSet(OptSDL))\n> + sink_ = std::make_unique();\n> +#endif\n> +\n\nIt looks like the precedence of which sink really gets used if multiples\nare set is simply the order in this file that they get set. But I think\nthat's ok for the moment. Not sure we could do much else right now\nwithout extending to support multiple sinks which is out of scope here.\n\nReviewed-by: Kieran Bingham \n\n\n> if (options_.isSet(OptFile)) {\n> if (!options_[OptFile].toString().empty())\n> sink_ = std::make_unique(streamNames_,\n> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> index c7f664b9..fd3108b0 100644\n> --- a/src/cam/main.cpp\n> +++ b/src/cam/main.cpp\n> @@ -137,6 +137,10 @@ int CamApp::parseOptions(int argc, char *argv[])\n> \"Display viewfinder through DRM/KMS on specified connector\",\n> \"display\", ArgumentOptional, \"connector\", false,\n> OptCamera);\n> +#endif\n> +#ifdef HAVE_SDL\n> + parser.addOption(OptSDL, OptionNone, \"Display viewfinder through SDL\",\n> + \"sdl\", ArgumentNone, \"\", false, OptCamera);\n> #endif\n> parser.addOption(OptFile, OptionString,\n> \"Write captured frames to disk\\n\"\n> diff --git a/src/cam/main.h b/src/cam/main.h\n> index 62f7bbc9..2b285808 100644\n> --- a/src/cam/main.h\n> +++ b/src/cam/main.h\n> @@ -18,6 +18,7 @@ enum {\n> OptListProperties = 'p',\n> OptMonitor = 'm',\n> OptStream = 's',\n> + OptSDL = 'S',\n> OptListControls = 256,\n> OptStrictFormats = 257,\n> OptMetadata = 258,\n> diff --git a/src/cam/meson.build b/src/cam/meson.build\n> index 5bab8c9e..3032730b 100644\n> --- a/src/cam/meson.build\n> +++ b/src/cam/meson.build\n> @@ -32,11 +32,23 @@ if libdrm.found()\n> ])\n> endif\n> \n> +libsdl2 = dependency('SDL2', required : false)\n> +\n> +if libsdl2.found()\n> + cam_cpp_args += ['-DHAVE_SDL']\n> + cam_sources += files([\n> + 'sdl_sink.cpp',\n> + 'sdl_texture.cpp',\n> + 'sdl_texture_yuyv.cpp'\n> + ])\n> +endif\n> +\n> cam = executable('cam', cam_sources,\n> dependencies : [\n> libatomic,\n> libcamera_public,\n> libdrm,\n> + libsdl2,\n> libevent,\n> ],\n> cpp_args : cam_cpp_args,\n> diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp\n> new file mode 100644\n> index 00000000..6fbeaf56\n> --- /dev/null\n> +++ b/src/cam/sdl_sink.cpp\n> @@ -0,0 +1,192 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * sdl_sink.cpp - SDL Sink\n> + */\n> +\n> +#include \"sdl_sink.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \"sdl_texture_yuyv.h\"\n> +\n> +#include \"event_loop.h\"\n> +#include \"image.h\"\n> +\n> +using namespace libcamera;\n> +\n> +SDLSink::SDLSink()\n> + : sdlWindow_(nullptr), sdlRenderer_(nullptr), sdlRect_({}),\n> + sdlInit_(false)\n> +{\n> +}\n> +\n> +SDLSink::~SDLSink()\n> +{\n> + stop();\n> +}\n> +\n> +int SDLSink::configure(const libcamera::CameraConfiguration &config)\n> +{\n> + int ret = FrameSink::configure(config);\n> + if (ret < 0)\n> + return ret;\n> +\n> + if (config.size() > 1) {\n> + std::cerr\n> + << \"SDL sink only supports one camera stream at present, streaming first camera stream\"\n> + << std::endl;\n> + } else if (config.empty()) {\n> + std::cerr << \"Require at least one camera stream to process\"\n> + << std::endl;\n> + return -EINVAL;\n> + }\n> +\n> + const libcamera::StreamConfiguration &cfg = config.at(0);\n> + sdlRect_.w = cfg.size.width;\n> + sdlRect_.h = cfg.size.height;\n> +\n> + switch (cfg.pixelFormat) {\n> + case libcamera::formats::YUYV:\n> + sdlTexture_ = std::make_unique(sdlRect_);\n> + break;\n> + default:\n> + std::cerr << \"Unsupported pixel format \"\n> + << cfg.pixelFormat.toString() << std::endl;\n> + return -EINVAL;\n> + };\n> +\n> + return 0;\n> +}\n> +\n> +int SDLSink::start()\n> +{\n> + int ret = SDL_Init(SDL_INIT_VIDEO);\n> + if (ret) {\n> + std::cerr << \"Failed to initialize SDL: \" << SDL_GetError()\n> + << std::endl;\n> + return ret;\n> + }\n> +\n> + sdlInit_ = true;\n> + sdlWindow_ = SDL_CreateWindow(\"\", SDL_WINDOWPOS_UNDEFINED,\n> + SDL_WINDOWPOS_UNDEFINED, sdlRect_.w,\n> + sdlRect_.h,\n> + SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE);\n> + if (!sdlWindow_) {\n> + std::cerr << \"Failed to create SDL window: \" << SDL_GetError()\n> + << std::endl;\n> + return -EINVAL;\n> + }\n> +\n> + sdlRenderer_ = SDL_CreateRenderer(sdlWindow_, -1, 0);\n> + if (!sdlRenderer_) {\n> + std::cerr << \"Failed to create SDL renderer: \" << SDL_GetError()\n> + << std::endl;\n> + return -EINVAL;\n> + }\n> +\n> + ret = SDL_RenderSetLogicalSize(sdlRenderer_, sdlRect_.w, sdlRect_.h);\n> + if (ret) { /* Not critical to set, don't return in this case, set for scaling purposes */\n> + std::cerr << \"Failed to set SDL render logical size: \"\n> + << SDL_GetError() << std::endl;\n> + }\n> +\n> + ret = sdlTexture_->create(sdlRenderer_);\n> + if (ret) {\n> + return ret;\n> + }\n> +\n> + EventLoop::instance()->addTimerEvent(\n> + 10ms, std::bind(&SDLSink::processSDLEvents, this));\n> +\n> + return 0;\n> +}\n> +\n> +int SDLSink::stop()\n> +{\n> + if (sdlTexture_) {\n> + sdlTexture_->destroy();\n> + sdlTexture_ = nullptr;\n> + }\n> +\n> + if (sdlRenderer_) {\n> + SDL_DestroyRenderer(sdlRenderer_);\n> + sdlRenderer_ = nullptr;\n> + }\n> +\n> + if (sdlWindow_) {\n> + SDL_DestroyWindow(sdlWindow_);\n> + sdlWindow_ = nullptr;\n> + }\n> +\n> + if (sdlInit_) {\n> + SDL_Quit();\n> + sdlInit_ = false;\n> + }\n> +\n> + return FrameSink::stop();\n> +}\n> +\n> +void SDLSink::mapBuffer(FrameBuffer *buffer)\n> +{\n> + std::unique_ptr image =\n> + Image::fromFrameBuffer(buffer, Image::MapMode::ReadOnly);\n> + assert(image != nullptr);\n> +\n> + mappedBuffers_[buffer] = std::move(image);\n> +}\n> +\n> +bool SDLSink::processRequest(Request *request)\n> +{\n> + for (auto [stream, buffer] : request->buffers()) {\n> + renderBuffer(buffer);\n> + break; /* to be expanded to launch SDL window per buffer */\n> + }\n> +\n> + return true;\n> +}\n> +\n> +/*\n> + * Process SDL events, required for things like window resize and quit button\n> + */\n> +void SDLSink::processSDLEvents()\n> +{\n> + for (SDL_Event e; SDL_PollEvent(&e);) {\n> + if (e.type == SDL_QUIT) {\n> + /* Click close icon then quit */\n> + EventLoop::instance()->exit(0);\n> + }\n> + }\n> +}\n> +\n> +void SDLSink::renderBuffer(FrameBuffer *buffer)\n> +{\n> + Image *image = mappedBuffers_[buffer].get();\n> +\n> + for (unsigned int i = 0; i < buffer->planes().size(); ++i) {\n> + const FrameMetadata::Plane &meta =\n> + buffer->metadata().planes()[i];\n> +\n> + Span data = image->data(i);\n> + if (meta.bytesused > data.size())\n> + std::cerr << \"payload size \" << meta.bytesused\n> + << \" larger than plane size \" << data.size()\n> + << std::endl;\n> +\n> + sdlTexture_->update(data);\n> + }\n> +\n> + SDL_RenderClear(sdlRenderer_);\n> + SDL_RenderCopy(sdlRenderer_, sdlTexture_->ptr_, nullptr, nullptr);\n> + SDL_RenderPresent(sdlRenderer_);\n> +}\n> diff --git a/src/cam/sdl_sink.h b/src/cam/sdl_sink.h\n> new file mode 100644\n> index 00000000..37dd9f7e\n> --- /dev/null\n> +++ b/src/cam/sdl_sink.h\n> @@ -0,0 +1,48 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * sdl_sink.h - SDL Sink\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \n> +\n> +#include \"frame_sink.h\"\n> +#include \"sdl_texture.h\"\n> +\n> +class Image;\n> +\n> +class SDLSink : public FrameSink\n> +{\n> +public:\n> + SDLSink();\n> + ~SDLSink();\n> +\n> + int configure(const libcamera::CameraConfiguration &config) override;\n> + int start() override;\n> + int stop() override;\n> + void mapBuffer(libcamera::FrameBuffer *buffer) override;\n> +\n> + bool processRequest(libcamera::Request *request) override;\n> +\n> +private:\n> + void renderBuffer(libcamera::FrameBuffer *buffer);\n> + void processSDLEvents();\n> +\n> + std::map>\n> + mappedBuffers_;\n> +\n> + std::unique_ptr sdlTexture_;\n> +\n> + SDL_Window *sdlWindow_;\n> + SDL_Renderer *sdlRenderer_;\n> + SDL_Rect sdlRect_;\n> + SDL_PixelFormatEnum pixelFormat_;\n> + bool sdlInit_;\n> +};\n> diff --git a/src/cam/sdl_texture.cpp b/src/cam/sdl_texture.cpp\n> new file mode 100644\n> index 00000000..b39080c9\n> --- /dev/null\n> +++ b/src/cam/sdl_texture.cpp\n> @@ -0,0 +1,32 @@\n> +#include \"sdl_texture.h\"\n> +\n> +#include \n> +\n> +SDLTexture::SDLTexture(const SDL_Rect &sdlRect, SDL_PixelFormatEnum pixelFormat,\n> + const int pitch)\n> + : sdlRect_(sdlRect), pixelFormat_(pixelFormat), pitch_(pitch)\n> +{\n> +}\n> +\n> +SDLTexture::~SDLTexture()\n> +{\n> +}\n> +\n> +int SDLTexture::create(SDL_Renderer *sdlRenderer_)\n> +{\n> + ptr_ = SDL_CreateTexture(sdlRenderer_, pixelFormat_,\n> + SDL_TEXTUREACCESS_STREAMING, sdlRect_.w,\n> + sdlRect_.h);\n> + if (!ptr_) {\n> + std::cerr << \"Failed to create SDL texture: \" << SDL_GetError()\n> + << std::endl;\n> + return -ENOMEM;\n> + }\n> +\n> + return 0;\n> +}\n> +\n> +void SDLTexture::destroy()\n> +{\n> + SDL_DestroyTexture(ptr_);\n> +}\n> diff --git a/src/cam/sdl_texture.h b/src/cam/sdl_texture.h\n> new file mode 100644\n> index 00000000..f9525f7f\n> --- /dev/null\n> +++ b/src/cam/sdl_texture.h\n> @@ -0,0 +1,23 @@\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \"image.h\"\n> +\n> +class SDLTexture\n> +{\n> +public:\n> + SDLTexture(const SDL_Rect &sdlRect, SDL_PixelFormatEnum pixelFormat,\n> + const int pitch);\n> + virtual ~SDLTexture();\n> + int create(SDL_Renderer *sdlRenderer_);\n> + void destroy();\n> + virtual void update(const libcamera::Span &data) = 0;\n> +\n> + SDL_Texture *ptr_;\n> +\n> +protected:\n> + const SDL_Rect &sdlRect_;\n> + SDL_PixelFormatEnum pixelFormat_;\n> + const int pitch_;\n> +};\n> diff --git a/src/cam/sdl_texture_yuyv.cpp b/src/cam/sdl_texture_yuyv.cpp\n> new file mode 100644\n> index 00000000..cf519a5d\n> --- /dev/null\n> +++ b/src/cam/sdl_texture_yuyv.cpp\n> @@ -0,0 +1,13 @@\n> +#include \"sdl_texture_yuyv.h\"\n> +\n> +using namespace libcamera;\n> +\n> +SDLTextureYUYV::SDLTextureYUYV(const SDL_Rect &sdlRect)\n> + : SDLTexture(sdlRect, SDL_PIXELFORMAT_YUY2, 4 * ((sdlRect.w + 1) / 2))\n> +{\n> +}\n> +\n> +void SDLTextureYUYV::update(const Span &data)\n> +{\n> + SDL_UpdateTexture(ptr_, &sdlRect_, data.data(), pitch_);\n> +}\n> diff --git a/src/cam/sdl_texture_yuyv.h b/src/cam/sdl_texture_yuyv.h\n> new file mode 100644\n> index 00000000..ad70f14f\n> --- /dev/null\n> +++ b/src/cam/sdl_texture_yuyv.h\n> @@ -0,0 +1,10 @@\n> +#pragma once\n> +\n> +#include \"sdl_texture.h\"\n> +\n> +class SDLTextureYUYV : public SDLTexture\n> +{\n> +public:\n> + SDLTextureYUYV(const SDL_Rect &sdlRect);\n> + void update(const libcamera::Span &data) override;\n> +};\n> -- \n> 2.35.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D51FFC3256\n\tfor ;\n\tTue, 17 May 2022 23:38:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2BBBB65656;\n\tWed, 18 May 2022 01:38:25 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CC2486041D\n\tfor ;\n\tWed, 18 May 2022 01:38:23 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5687E4A8;\n\tWed, 18 May 2022 01:38:23 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1652830705;\n\tbh=8oSaIA/UNcI93lacrouttadX92juiaOj+J5jRo8vqrs=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=fNGGX9gsYp8ogeTTo7rZAePZuWapXobbEdYLVFXFeM7pxtlfYlXE6zz4OvXNjnZfO\n\tyLXnJ6IGFTxNeWqZbPTkPI/Oza8RPuKiQwmogNJCVC1cYItc1FgKCABDyqQBzS7BMA\n\tlVvgpCEhs0wzh52ljUQCSlUvNGrTdW7oTrbjFTGSZrESUpi3JsYxzJSnNjoS5z0ccK\n\t+qTykqNh49LrqdZRDnGBmzfCzAPYMB2zFCIPWUfwm2BfAFR3SeXzvm0u0F7w7DLlQJ\n\tdWGnvj1q1N3XYjasHu0XlO5tD/CGT/uaNbguKg3CUJl4lmr6zKRB7dt3TUQhIBm1AG\n\tipsxL43StsXHQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1652830703;\n\tbh=8oSaIA/UNcI93lacrouttadX92juiaOj+J5jRo8vqrs=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=RSoiQCXaf0nCAObd8lIxZOJu0k3LbgTiG5+dFkC7iPhHqG36czFpOLC6sMARcTbh9\n\tZ5jjy7LmykUSl173KdOKUVfSDPtzr1GSyxFA3VAOLBgeTnLYP712Tx9KgesOONq2Gp\n\tdXGy6awInkTl/4jLNe3/vzGZUb3vHwcJb6TfZGeg="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"RSoiQCXa\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220505151851.2421-3-ecurtin@redhat.com>","References":"<20220505151851.2421-1-ecurtin@redhat.com>\n\t<20220505151851.2421-3-ecurtin@redhat.com>","To":"Eric Curtin , libcamera-devel@lists.libcamera.org","Date":"Wed, 18 May 2022 00:38:20 +0100","Message-ID":"<165283070083.2416244.15906036641040599419@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v8 3/4] cam: sdl_sink: Add SDL sink\n\twith initial YUYV support","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23075,"web_url":"https://patchwork.libcamera.org/comment/23075/","msgid":"","date":"2022-05-18T20:06:10","subject":"Re: [libcamera-devel] [PATCH v8 3/4] cam: sdl_sink: Add SDL sink\n\twith initial YUYV support","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Eric,\n\nThank you for the patch.\n\nOn Wed, May 18, 2022 at 12:38:20AM +0100, Kieran Bingham via libcamera-devel wrote:\n> Quoting Eric Curtin via libcamera-devel (2022-05-05 16:18:50)\n> > This adds more portability to existing cam sinks. You can pass a\n> > YUYV camera buffer and SDL will handle the pixel buffer conversion\n> > to the display. This allows cam reference implementation to display\n> > images on VMs, Mac M1, Raspberry Pi, etc. This also enables cam\n> > reference implementation, to run as a desktop application in wayland or\n\ns/wayland/Wayland/\n\n> > x11. SDL also has support for Android and ChromeOS which has not been\n\ns/x11/X11/\n\n> > tested. Also tested on simpledrm raspberry pi 4 framebuffer\n\ns/raspberry pi/Raspberry Pi/\n\n> > successfully where existing kms sink did not work. Can also be used as\n> > kmsdrm sink. Only supports one camera stream at present.\n> > \n> > Signed-off-by: Eric Curtin \n> > ---\n> > src/cam/camera_session.cpp | 8 ++\n> > src/cam/main.cpp | 4 +\n> > src/cam/main.h | 1 +\n> > src/cam/meson.build | 12 +++\n> > src/cam/sdl_sink.cpp | 192 +++++++++++++++++++++++++++++++++++\n> > src/cam/sdl_sink.h | 48 +++++++++\n> > src/cam/sdl_texture.cpp | 32 ++++++\n> > src/cam/sdl_texture.h | 23 +++++\n> > src/cam/sdl_texture_yuyv.cpp | 13 +++\n> > src/cam/sdl_texture_yuyv.h | 10 ++\n> \n> Given the number of sdl_ files here I wonder if they should be under\n> src/cam/sdl/.... \n> \n> But not crucial to this patch.\n> \n> I can't test this right now - but I'm looking forward to seeing a\n> preview on cam ... so I'll hopefully test this tomorrow.\n> \n> > 10 files changed, 343 insertions(+)\n> > create mode 100644 src/cam/sdl_sink.cpp\n> > create mode 100644 src/cam/sdl_sink.h\n> > create mode 100644 src/cam/sdl_texture.cpp\n> > create mode 100644 src/cam/sdl_texture.h\n> > create mode 100644 src/cam/sdl_texture_yuyv.cpp\n> > create mode 100644 src/cam/sdl_texture_yuyv.h\n> > \n> > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp\n> > index efffafbf..71e6bd60 100644\n> > --- a/src/cam/camera_session.cpp\n> > +++ b/src/cam/camera_session.cpp\n> > @@ -19,6 +19,9 @@\n> > #ifdef HAVE_KMS\n> > #include \"kms_sink.h\"\n> > #endif\n> > +#ifdef HAVE_SDL\n> > +#include \"sdl_sink.h\"\n> > +#endif\n\nPlease move this below in alphabetical order.\n\n> > #include \"main.h\"\n> > #include \"stream_options.h\"\n> > \n> > @@ -186,6 +189,11 @@ int CameraSession::start()\n> > sink_ = std::make_unique(options_[OptDisplay].toString());\n> > #endif\n> > \n> > +#ifdef HAVE_SDL\n> > + if (options_.isSet(OptSDL))\n> > + sink_ = std::make_unique();\n> > +#endif\n> > +\n> \n> It looks like the precedence of which sink really gets used if multiples\n> are set is simply the order in this file that they get set. But I think\n> that's ok for the moment. Not sure we could do much else right now\n> without extending to support multiple sinks which is out of scope here.\n> \n> Reviewed-by: Kieran Bingham \n> \n> \n> > if (options_.isSet(OptFile)) {\n> > if (!options_[OptFile].toString().empty())\n> > sink_ = std::make_unique(streamNames_,\n> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> > index c7f664b9..fd3108b0 100644\n> > --- a/src/cam/main.cpp\n> > +++ b/src/cam/main.cpp\n> > @@ -137,6 +137,10 @@ int CamApp::parseOptions(int argc, char *argv[])\n> > \"Display viewfinder through DRM/KMS on specified connector\",\n> > \"display\", ArgumentOptional, \"connector\", false,\n> > OptCamera);\n> > +#endif\n> > +#ifdef HAVE_SDL\n> > + parser.addOption(OptSDL, OptionNone, \"Display viewfinder through SDL\",\n> > + \"sdl\", ArgumentNone, \"\", false, OptCamera);\n> > #endif\n\nI wonder if we should merge all sink arguments into one, but that can be\ndone on top (the cam arguments syntax isn't stable yet).\n\n> > parser.addOption(OptFile, OptionString,\n> > \"Write captured frames to disk\\n\"\n> > diff --git a/src/cam/main.h b/src/cam/main.h\n> > index 62f7bbc9..2b285808 100644\n> > --- a/src/cam/main.h\n> > +++ b/src/cam/main.h\n> > @@ -18,6 +18,7 @@ enum {\n> > OptListProperties = 'p',\n> > OptMonitor = 'm',\n> > OptStream = 's',\n> > + OptSDL = 'S',\n> > OptListControls = 256,\n> > OptStrictFormats = 257,\n> > OptMetadata = 258,\n> > diff --git a/src/cam/meson.build b/src/cam/meson.build\n> > index 5bab8c9e..3032730b 100644\n> > --- a/src/cam/meson.build\n> > +++ b/src/cam/meson.build\n> > @@ -32,11 +32,23 @@ if libdrm.found()\n> > ])\n> > endif\n> > \n> > +libsdl2 = dependency('SDL2', required : false)\n> > +\n> > +if libsdl2.found()\n> > + cam_cpp_args += ['-DHAVE_SDL']\n> > + cam_sources += files([\n> > + 'sdl_sink.cpp',\n> > + 'sdl_texture.cpp',\n> > + 'sdl_texture_yuyv.cpp'\n> > + ])\n> > +endif\n> > +\n> > cam = executable('cam', cam_sources,\n> > dependencies : [\n> > libatomic,\n> > libcamera_public,\n> > libdrm,\n> > + libsdl2,\n\nPlease keep the list alphabetically ordered.\n\n> > libevent,\n> > ],\n> > cpp_args : cam_cpp_args,\n> > diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp\n> > new file mode 100644\n> > index 00000000..6fbeaf56\n> > --- /dev/null\n> > +++ b/src/cam/sdl_sink.cpp\n> > @@ -0,0 +1,192 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * sdl_sink.cpp - SDL Sink\n> > + */\n> > +\n> > +#include \"sdl_sink.h\"\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \"sdl_texture_yuyv.h\"\n> > +\n> > +#include \"event_loop.h\"\n> > +#include \"image.h\"\n\nI don't think there's a need to split sdl_texture_yuyv.h and the next\ntwo headers in two groups.\n\n> > +\n> > +using namespace libcamera;\n> > +\n> > +SDLSink::SDLSink()\n> > + : sdlWindow_(nullptr), sdlRenderer_(nullptr), sdlRect_({}),\n> > + sdlInit_(false)\n> > +{\n> > +}\n> > +\n> > +SDLSink::~SDLSink()\n> > +{\n> > + stop();\n> > +}\n> > +\n> > +int SDLSink::configure(const libcamera::CameraConfiguration &config)\n> > +{\n> > + int ret = FrameSink::configure(config);\n> > + if (ret < 0)\n> > + return ret;\n> > +\n> > + if (config.size() > 1) {\n> > + std::cerr\n> > + << \"SDL sink only supports one camera stream at present, streaming first camera stream\"\n> > + << std::endl;\n> > + } else if (config.empty()) {\n> > + std::cerr << \"Require at least one camera stream to process\"\n> > + << std::endl;\n> > + return -EINVAL;\n> > + }\n> > +\n> > + const libcamera::StreamConfiguration &cfg = config.at(0);\n> > + sdlRect_.w = cfg.size.width;\n> > + sdlRect_.h = cfg.size.height;\n> > +\n> > + switch (cfg.pixelFormat) {\n> > + case libcamera::formats::YUYV:\n> > + sdlTexture_ = std::make_unique(sdlRect_);\n> > + break;\n> > + default:\n> > + std::cerr << \"Unsupported pixel format \"\n> > + << cfg.pixelFormat.toString() << std::endl;\n> > + return -EINVAL;\n> > + };\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +int SDLSink::start()\n> > +{\n> > + int ret = SDL_Init(SDL_INIT_VIDEO);\n> > + if (ret) {\n> > + std::cerr << \"Failed to initialize SDL: \" << SDL_GetError()\n> > + << std::endl;\n> > + return ret;\n> > + }\n> > +\n> > + sdlInit_ = true;\n> > + sdlWindow_ = SDL_CreateWindow(\"\", SDL_WINDOWPOS_UNDEFINED,\n> > + SDL_WINDOWPOS_UNDEFINED, sdlRect_.w,\n> > + sdlRect_.h,\n> > + SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE);\n> > + if (!sdlWindow_) {\n> > + std::cerr << \"Failed to create SDL window: \" << SDL_GetError()\n> > + << std::endl;\n> > + return -EINVAL;\n> > + }\n> > +\n> > + sdlRenderer_ = SDL_CreateRenderer(sdlWindow_, -1, 0);\n> > + if (!sdlRenderer_) {\n> > + std::cerr << \"Failed to create SDL renderer: \" << SDL_GetError()\n> > + << std::endl;\n> > + return -EINVAL;\n> > + }\n> > +\n> > + ret = SDL_RenderSetLogicalSize(sdlRenderer_, sdlRect_.w, sdlRect_.h);\n> > + if (ret) { /* Not critical to set, don't return in this case, set for scaling purposes */\n> > + std::cerr << \"Failed to set SDL render logical size: \"\n> > + << SDL_GetError() << std::endl;\n> > + }\n\n\t/*\n\t * Not critical to set, don't return in this case, set for scaling\n\t * purposes.\n\t */\n\tif (ret)\n\t\tstd::cerr << \"Failed to set SDL render logical size: \"\n\t\t\t << SDL_GetError() << std::endl;\n\n> > +\n> > + ret = sdlTexture_->create(sdlRenderer_);\n> > + if (ret) {\n> > + return ret;\n> > + }\n> > +\n> > + EventLoop::instance()->addTimerEvent(\n> > + 10ms, std::bind(&SDLSink::processSDLEvents, this));\n\nI think I mentioned before that this won't work nicely if we stop and\nrestart. cam doesn't do so at the moment, so it doesn't have to be fixed\nyet, but a todo comment would be good:\n\n\t/* \\todo Make the event cancellable to support stop/start cycles. */\n\tEventLoop::instance()->addTimerEvent(\n\t\t10ms, std::bind(&SDLSink::processSDLEvents, this));\n\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +int SDLSink::stop()\n> > +{\n> > + if (sdlTexture_) {\n> > + sdlTexture_->destroy();\n\nCan you turn SDLTexture::destroy() into a destructor ? Then you could\nsimply write\n\n\tsdlTexture_ = nullptr;\n\nor\n\n\tsdlTexture_.reset();\n\n> > + sdlTexture_ = nullptr;\n> > + }\n> > +\n> > + if (sdlRenderer_) {\n> > + SDL_DestroyRenderer(sdlRenderer_);\n> > + sdlRenderer_ = nullptr;\n> > + }\n> > +\n> > + if (sdlWindow_) {\n> > + SDL_DestroyWindow(sdlWindow_);\n> > + sdlWindow_ = nullptr;\n> > + }\n> > +\n> > + if (sdlInit_) {\n> > + SDL_Quit();\n> > + sdlInit_ = false;\n> > + }\n> > +\n> > + return FrameSink::stop();\n> > +}\n> > +\n> > +void SDLSink::mapBuffer(FrameBuffer *buffer)\n> > +{\n> > + std::unique_ptr image =\n> > + Image::fromFrameBuffer(buffer, Image::MapMode::ReadOnly);\n> > + assert(image != nullptr);\n> > +\n> > + mappedBuffers_[buffer] = std::move(image);\n> > +}\n> > +\n> > +bool SDLSink::processRequest(Request *request)\n> > +{\n> > + for (auto [stream, buffer] : request->buffers()) {\n> > + renderBuffer(buffer);\n> > + break; /* to be expanded to launch SDL window per buffer */\n> > + }\n> > +\n> > + return true;\n> > +}\n> > +\n> > +/*\n> > + * Process SDL events, required for things like window resize and quit button\n> > + */\n> > +void SDLSink::processSDLEvents()\n> > +{\n> > + for (SDL_Event e; SDL_PollEvent(&e);) {\n> > + if (e.type == SDL_QUIT) {\n> > + /* Click close icon then quit */\n> > + EventLoop::instance()->exit(0);\n> > + }\n> > + }\n> > +}\n> > +\n> > +void SDLSink::renderBuffer(FrameBuffer *buffer)\n> > +{\n> > + Image *image = mappedBuffers_[buffer].get();\n> > +\n> > + for (unsigned int i = 0; i < buffer->planes().size(); ++i) {\n> > + const FrameMetadata::Plane &meta =\n> > + buffer->metadata().planes()[i];\n> > +\n> > + Span data = image->data(i);\n> > + if (meta.bytesused > data.size())\n> > + std::cerr << \"payload size \" << meta.bytesused\n> > + << \" larger than plane size \" << data.size()\n> > + << std::endl;\n> > +\n> > + sdlTexture_->update(data);\n\nI really don't think this will do what you expect for multi-planar\nformats. As multi-planar formats are not supported yet, I'd replace the\nloop by a single pass over plane 0, and add a\n\n\t/* \\todo Implement support for multi-planar formats. */\n\n> > + }\n> > +\n> > + SDL_RenderClear(sdlRenderer_);\n> > + SDL_RenderCopy(sdlRenderer_, sdlTexture_->ptr_, nullptr, nullptr);\n> > + SDL_RenderPresent(sdlRenderer_);\n> > +}\n> > diff --git a/src/cam/sdl_sink.h b/src/cam/sdl_sink.h\n> > new file mode 100644\n> > index 00000000..37dd9f7e\n> > --- /dev/null\n> > +++ b/src/cam/sdl_sink.h\n> > @@ -0,0 +1,48 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * sdl_sink.h - SDL Sink\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +#include \n> > +#include \n\nI think you can drop the string header.\n\n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"frame_sink.h\"\n> > +#include \"sdl_texture.h\"\n\nYou could also forward-declare the SDLTexture class and drop this\nheader.\n\n> > +\n> > +class Image;\n> > +\n> > +class SDLSink : public FrameSink\n> > +{\n> > +public:\n> > + SDLSink();\n> > + ~SDLSink();\n> > +\n> > + int configure(const libcamera::CameraConfiguration &config) override;\n> > + int start() override;\n> > + int stop() override;\n> > + void mapBuffer(libcamera::FrameBuffer *buffer) override;\n> > +\n> > + bool processRequest(libcamera::Request *request) override;\n> > +\n> > +private:\n> > + void renderBuffer(libcamera::FrameBuffer *buffer);\n> > + void processSDLEvents();\n> > +\n> > + std::map>\n> > + mappedBuffers_;\n> > +\n> > + std::unique_ptr sdlTexture_;\n> > +\n> > + SDL_Window *sdlWindow_;\n> > + SDL_Renderer *sdlRenderer_;\n> > + SDL_Rect sdlRect_;\n\nI'm tempted to drop the sdl prefix from variable names, up to you.\n\n> > + SDL_PixelFormatEnum pixelFormat_;\n> > + bool sdlInit_;\n> > +};\n> > diff --git a/src/cam/sdl_texture.cpp b/src/cam/sdl_texture.cpp\n> > new file mode 100644\n> > index 00000000..b39080c9\n> > --- /dev/null\n> > +++ b/src/cam/sdl_texture.cpp\n> > @@ -0,0 +1,32 @@\n\nMissing SPDX tag and copyright header. Same below.\n\n> > +#include \"sdl_texture.h\"\n> > +\n> > +#include \n> > +\n> > +SDLTexture::SDLTexture(const SDL_Rect &sdlRect, SDL_PixelFormatEnum pixelFormat,\n> > + const int pitch)\n> > + : sdlRect_(sdlRect), pixelFormat_(pixelFormat), pitch_(pitch)\n> > +{\n> > +}\n> > +\n> > +SDLTexture::~SDLTexture()\n> > +{\n> > +}\n> > +\n> > +int SDLTexture::create(SDL_Renderer *sdlRenderer_)\n> > +{\n> > + ptr_ = SDL_CreateTexture(sdlRenderer_, pixelFormat_,\n> > + SDL_TEXTUREACCESS_STREAMING, sdlRect_.w,\n> > + sdlRect_.h);\n> > + if (!ptr_) {\n> > + std::cerr << \"Failed to create SDL texture: \" << SDL_GetError()\n> > + << std::endl;\n> > + return -ENOMEM;\n> > + }\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +void SDLTexture::destroy()\n> > +{\n> > + SDL_DestroyTexture(ptr_);\n> > +}\n> > diff --git a/src/cam/sdl_texture.h b/src/cam/sdl_texture.h\n> > new file mode 100644\n> > index 00000000..f9525f7f\n> > --- /dev/null\n> > +++ b/src/cam/sdl_texture.h\n> > @@ -0,0 +1,23 @@\n> > +#pragma once\n> > +\n> > +#include \n> > +\n> > +#include \"image.h\"\n> > +\n> > +class SDLTexture\n> > +{\n> > +public:\n> > + SDLTexture(const SDL_Rect &sdlRect, SDL_PixelFormatEnum pixelFormat,\n> > + const int pitch);\n> > + virtual ~SDLTexture();\n> > + int create(SDL_Renderer *sdlRenderer_);\n\ns/sdlRenderer_/sdlRenderer/\n\nAnd same here (and with sdlRect in the previous function) about the sdl\nprefix.\n\n> > + void destroy();\n> > + virtual void update(const libcamera::Span &data) = 0;\n> > +\n> > + SDL_Texture *ptr_;\n\nIs it worth making this protected and adding an accessor function ?\n\n> > +\n> > +protected:\n> > + const SDL_Rect &sdlRect_;\n\nSame here, rect_.\n\n> > + SDL_PixelFormatEnum pixelFormat_;\n> > + const int pitch_;\n> > +};\n> > diff --git a/src/cam/sdl_texture_yuyv.cpp b/src/cam/sdl_texture_yuyv.cpp\n> > new file mode 100644\n> > index 00000000..cf519a5d\n> > --- /dev/null\n> > +++ b/src/cam/sdl_texture_yuyv.cpp\n> > @@ -0,0 +1,13 @@\n> > +#include \"sdl_texture_yuyv.h\"\n> > +\n> > +using namespace libcamera;\n> > +\n> > +SDLTextureYUYV::SDLTextureYUYV(const SDL_Rect &sdlRect)\n> > + : SDLTexture(sdlRect, SDL_PIXELFORMAT_YUY2, 4 * ((sdlRect.w + 1) / 2))\n> > +{\n> > +}\n> > +\n> > +void SDLTextureYUYV::update(const Span &data)\n> > +{\n> > + SDL_UpdateTexture(ptr_, &sdlRect_, data.data(), pitch_);\n> > +}\n> > diff --git a/src/cam/sdl_texture_yuyv.h b/src/cam/sdl_texture_yuyv.h\n> > new file mode 100644\n> > index 00000000..ad70f14f\n> > --- /dev/null\n> > +++ b/src/cam/sdl_texture_yuyv.h\n> > @@ -0,0 +1,10 @@\n> > +#pragma once\n> > +\n> > +#include \"sdl_texture.h\"\n> > +\n> > +class SDLTextureYUYV : public SDLTexture\n> > +{\n> > +public:\n> > + SDLTextureYUYV(const SDL_Rect &sdlRect);\n> > + void update(const libcamera::Span &data) override;\n> > +};","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 650C9C0F2A\n\tfor ;\n\tWed, 18 May 2022 20:06:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8566965659;\n\tWed, 18 May 2022 22:06:20 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6707465656\n\tfor ;\n\tWed, 18 May 2022 22:06:19 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [45.131.31.124])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 732444A8;\n\tWed, 18 May 2022 22:06:18 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1652904380;\n\tbh=i56F+YyEn70/8jIWyQIuqhSrDA+oftiFoD2gpMHoba8=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=pZdDIhLP/e046MqaNA3tASTjKSFCICEsHGz+CWfV8w6xgM6I43vLHFUvBqqayyHFR\n\ttNEmESNXXfiNC3ei16ZokXacA847H83VlI6htaPT/thK6caVKnwIbClUB3Dno8FNZQ\n\tYLcMqD6Ae8x5PFvhAwBnsKuaAupRekLVVXZp/KKAPFKPvHZNphKCJd3GaGLCGzL4VH\n\t/8zE50GFjkRIYVhU/MEMnOQJaAtgo8cSdDNYlkm6xq+aNQDScUXNk25i6V7eCBg0aL\n\tqnXH6ox7xsvHLTyQFjOlSUftIBslGIkWHqlHL9xM4Qf5api9W0sR7oajvYiYe+gsDX\n\tAoiAoET/E7UWA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1652904379;\n\tbh=i56F+YyEn70/8jIWyQIuqhSrDA+oftiFoD2gpMHoba8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fg6Xi8zClGnnN0lYcdsaWg/RWJ5IjSC5DYFS12q1HR23jcBaiC3MF8z4OTZhavp2Q\n\t6FAzymMRIgydJJQ2pY+jPCneTHveSG+6JYY1Nh3NBYNXH9XdtQBeYpxsH7PY6j8n/H\n\thUOlpKpTSsCMrB4X5gpieeb/CerjwUapWKLR1FhM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"fg6Xi8zC\"; dkim-atps=neutral","Date":"Wed, 18 May 2022 23:06:10 +0300","To":"Eric Curtin ","Message-ID":"","References":"<20220505151851.2421-1-ecurtin@redhat.com>\n\t<20220505151851.2421-3-ecurtin@redhat.com>\n\t<165283070083.2416244.15906036641040599419@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<165283070083.2416244.15906036641040599419@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v8 3/4] cam: sdl_sink: Add SDL sink\n\twith initial YUYV support","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23095,"web_url":"https://patchwork.libcamera.org/comment/23095/","msgid":"<165297736139.368702.12158192629293893246@Monstersaurus>","date":"2022-05-19T16:22:41","subject":"Re: [libcamera-devel] [PATCH v8 3/4] cam: sdl_sink: Add SDL sink\n\twith initial YUYV support","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2022-05-18 21:06:10)\n> Hi Eric,\n> \n> Thank you for the patch.\n> \n> On Wed, May 18, 2022 at 12:38:20AM +0100, Kieran Bingham via libcamera-devel wrote:\n> > Quoting Eric Curtin via libcamera-devel (2022-05-05 16:18:50)\n> > > This adds more portability to existing cam sinks. You can pass a\n> > > YUYV camera buffer and SDL will handle the pixel buffer conversion\n> > > to the display. This allows cam reference implementation to display\n> > > images on VMs, Mac M1, Raspberry Pi, etc. This also enables cam\n> > > reference implementation, to run as a desktop application in wayland or\n> \n> s/wayland/Wayland/\n> \n> > > x11. SDL also has support for Android and ChromeOS which has not been\n> \n> s/x11/X11/\n> \n> > > tested. Also tested on simpledrm raspberry pi 4 framebuffer\n> \n> s/raspberry pi/Raspberry Pi/\n> \n> > > successfully where existing kms sink did not work. Can also be used as\n> > > kmsdrm sink. Only supports one camera stream at present.\n> > > \n> > > Signed-off-by: Eric Curtin \n> > > ---\n> > > src/cam/camera_session.cpp | 8 ++\n> > > src/cam/main.cpp | 4 +\n> > > src/cam/main.h | 1 +\n> > > src/cam/meson.build | 12 +++\n> > > src/cam/sdl_sink.cpp | 192 +++++++++++++++++++++++++++++++++++\n> > > src/cam/sdl_sink.h | 48 +++++++++\n> > > src/cam/sdl_texture.cpp | 32 ++++++\n> > > src/cam/sdl_texture.h | 23 +++++\n> > > src/cam/sdl_texture_yuyv.cpp | 13 +++\n> > > src/cam/sdl_texture_yuyv.h | 10 ++\n> > \n> > Given the number of sdl_ files here I wonder if they should be under\n> > src/cam/sdl/.... \n> > \n> > But not crucial to this patch.\n> > \n> > I can't test this right now - but I'm looking forward to seeing a\n> > preview on cam ... so I'll hopefully test this tomorrow.\n> > \n> > > 10 files changed, 343 insertions(+)\n> > > create mode 100644 src/cam/sdl_sink.cpp\n> > > create mode 100644 src/cam/sdl_sink.h\n> > > create mode 100644 src/cam/sdl_texture.cpp\n> > > create mode 100644 src/cam/sdl_texture.h\n> > > create mode 100644 src/cam/sdl_texture_yuyv.cpp\n> > > create mode 100644 src/cam/sdl_texture_yuyv.h\n> > > \n> > > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp\n> > > index efffafbf..71e6bd60 100644\n> > > --- a/src/cam/camera_session.cpp\n> > > +++ b/src/cam/camera_session.cpp\n> > > @@ -19,6 +19,9 @@\n> > > #ifdef HAVE_KMS\n> > > #include \"kms_sink.h\"\n> > > #endif\n> > > +#ifdef HAVE_SDL\n> > > +#include \"sdl_sink.h\"\n> > > +#endif\n> \n> Please move this below in alphabetical order.\n> \n> > > #include \"main.h\"\n> > > #include \"stream_options.h\"\n> > > \n> > > @@ -186,6 +189,11 @@ int CameraSession::start()\n> > > sink_ = std::make_unique(options_[OptDisplay].toString());\n> > > #endif\n> > > \n> > > +#ifdef HAVE_SDL\n> > > + if (options_.isSet(OptSDL))\n> > > + sink_ = std::make_unique();\n> > > +#endif\n> > > +\n> > \n> > It looks like the precedence of which sink really gets used if multiples\n> > are set is simply the order in this file that they get set. But I think\n> > that's ok for the moment. Not sure we could do much else right now\n> > without extending to support multiple sinks which is out of scope here.\n> > \n> > Reviewed-by: Kieran Bingham \n> > \n> > \n> > > if (options_.isSet(OptFile)) {\n> > > if (!options_[OptFile].toString().empty())\n> > > sink_ = std::make_unique(streamNames_,\n> > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> > > index c7f664b9..fd3108b0 100644\n> > > --- a/src/cam/main.cpp\n> > > +++ b/src/cam/main.cpp\n> > > @@ -137,6 +137,10 @@ int CamApp::parseOptions(int argc, char *argv[])\n> > > \"Display viewfinder through DRM/KMS on specified connector\",\n> > > \"display\", ArgumentOptional, \"connector\", false,\n> > > OptCamera);\n> > > +#endif\n> > > +#ifdef HAVE_SDL\n> > > + parser.addOption(OptSDL, OptionNone, \"Display viewfinder through SDL\",\n> > > + \"sdl\", ArgumentNone, \"\", false, OptCamera);\n> > > #endif\n> \n> I wonder if we should merge all sink arguments into one, but that can be\n> done on top (the cam arguments syntax isn't stable yet).\n> \n> > > parser.addOption(OptFile, OptionString,\n> > > \"Write captured frames to disk\\n\"\n> > > diff --git a/src/cam/main.h b/src/cam/main.h\n> > > index 62f7bbc9..2b285808 100644\n> > > --- a/src/cam/main.h\n> > > +++ b/src/cam/main.h\n> > > @@ -18,6 +18,7 @@ enum {\n> > > OptListProperties = 'p',\n> > > OptMonitor = 'm',\n> > > OptStream = 's',\n> > > + OptSDL = 'S',\n> > > OptListControls = 256,\n> > > OptStrictFormats = 257,\n> > > OptMetadata = 258,\n> > > diff --git a/src/cam/meson.build b/src/cam/meson.build\n> > > index 5bab8c9e..3032730b 100644\n> > > --- a/src/cam/meson.build\n> > > +++ b/src/cam/meson.build\n> > > @@ -32,11 +32,23 @@ if libdrm.found()\n> > > ])\n> > > endif\n> > > \n> > > +libsdl2 = dependency('SDL2', required : false)\n\nI installed libsdl2-dev but got failures around SDL2/SDL_image.h\n\nOn ubuntu I installed libsdl2-image-dev, and then rebuilt. That found\nthe header, but then caused a link error...\n\nDoes this also need to have some 'SDL2-Image' dependency check?\n\n\n> > > +\n> > > +if libsdl2.found()\n> > > + cam_cpp_args += ['-DHAVE_SDL']\n> > > + cam_sources += files([\n> > > + 'sdl_sink.cpp',\n> > > + 'sdl_texture.cpp',\n> > > + 'sdl_texture_yuyv.cpp'\n> > > + ])\n> > > +endif\n> > > +\n> > > cam = executable('cam', cam_sources,\n> > > dependencies : [\n> > > libatomic,\n> > > libcamera_public,\n> > > libdrm,\n> > > + libsdl2,\n> \n> Please keep the list alphabetically ordered.\n> \n> > > libevent,\n> > > ],\n> > > cpp_args : cam_cpp_args,\n> > > diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp\n> > > new file mode 100644\n> > > index 00000000..6fbeaf56\n> > > --- /dev/null\n> > > +++ b/src/cam/sdl_sink.cpp\n> > > @@ -0,0 +1,192 @@\n> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > +/*\n> > > + * sdl_sink.cpp - SDL Sink\n> > > + */\n> > > +\n> > > +#include \"sdl_sink.h\"\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \"sdl_texture_yuyv.h\"\n> > > +\n> > > +#include \"event_loop.h\"\n> > > +#include \"image.h\"\n> \n> I don't think there's a need to split sdl_texture_yuyv.h and the next\n> two headers in two groups.\n> \n> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +SDLSink::SDLSink()\n> > > + : sdlWindow_(nullptr), sdlRenderer_(nullptr), sdlRect_({}),\n> > > + sdlInit_(false)\n> > > +{\n> > > +}\n> > > +\n> > > +SDLSink::~SDLSink()\n> > > +{\n> > > + stop();\n> > > +}\n> > > +\n> > > +int SDLSink::configure(const libcamera::CameraConfiguration &config)\n> > > +{\n> > > + int ret = FrameSink::configure(config);\n> > > + if (ret < 0)\n> > > + return ret;\n> > > +\n> > > + if (config.size() > 1) {\n> > > + std::cerr\n> > > + << \"SDL sink only supports one camera stream at present, streaming first camera stream\"\n> > > + << std::endl;\n> > > + } else if (config.empty()) {\n> > > + std::cerr << \"Require at least one camera stream to process\"\n> > > + << std::endl;\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + const libcamera::StreamConfiguration &cfg = config.at(0);\n> > > + sdlRect_.w = cfg.size.width;\n> > > + sdlRect_.h = cfg.size.height;\n> > > +\n> > > + switch (cfg.pixelFormat) {\n> > > + case libcamera::formats::YUYV:\n> > > + sdlTexture_ = std::make_unique(sdlRect_);\n> > > + break;\n> > > + default:\n> > > + std::cerr << \"Unsupported pixel format \"\n> > > + << cfg.pixelFormat.toString() << std::endl;\n> > > + return -EINVAL;\n> > > + };\n> > > +\n> > > + return 0;\n> > > +}\n> > > +\n> > > +int SDLSink::start()\n> > > +{\n> > > + int ret = SDL_Init(SDL_INIT_VIDEO);\n> > > + if (ret) {\n> > > + std::cerr << \"Failed to initialize SDL: \" << SDL_GetError()\n> > > + << std::endl;\n> > > + return ret;\n> > > + }\n> > > +\n> > > + sdlInit_ = true;\n> > > + sdlWindow_ = SDL_CreateWindow(\"\", SDL_WINDOWPOS_UNDEFINED,\n> > > + SDL_WINDOWPOS_UNDEFINED, sdlRect_.w,\n> > > + sdlRect_.h,\n> > > + SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE);\n> > > + if (!sdlWindow_) {\n> > > + std::cerr << \"Failed to create SDL window: \" << SDL_GetError()\n> > > + << std::endl;\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + sdlRenderer_ = SDL_CreateRenderer(sdlWindow_, -1, 0);\n> > > + if (!sdlRenderer_) {\n> > > + std::cerr << \"Failed to create SDL renderer: \" << SDL_GetError()\n> > > + << std::endl;\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + ret = SDL_RenderSetLogicalSize(sdlRenderer_, sdlRect_.w, sdlRect_.h);\n> > > + if (ret) { /* Not critical to set, don't return in this case, set for scaling purposes */\n> > > + std::cerr << \"Failed to set SDL render logical size: \"\n> > > + << SDL_GetError() << std::endl;\n> > > + }\n> \n> /*\n> * Not critical to set, don't return in this case, set for scaling\n> * purposes.\n> */\n> if (ret)\n> std::cerr << \"Failed to set SDL render logical size: \"\n> << SDL_GetError() << std::endl;\n> \n> > > +\n> > > + ret = sdlTexture_->create(sdlRenderer_);\n> > > + if (ret) {\n> > > + return ret;\n> > > + }\n> > > +\n> > > + EventLoop::instance()->addTimerEvent(\n> > > + 10ms, std::bind(&SDLSink::processSDLEvents, this));\n> \n> I think I mentioned before that this won't work nicely if we stop and\n> restart. cam doesn't do so at the moment, so it doesn't have to be fixed\n> yet, but a todo comment would be good:\n> \n> /* \\todo Make the event cancellable to support stop/start cycles. */\n> EventLoop::instance()->addTimerEvent(\n> 10ms, std::bind(&SDLSink::processSDLEvents, this));\n> \n> > > +\n> > > + return 0;\n> > > +}\n> > > +\n> > > +int SDLSink::stop()\n> > > +{\n> > > + if (sdlTexture_) {\n> > > + sdlTexture_->destroy();\n> \n> Can you turn SDLTexture::destroy() into a destructor ? Then you could\n> simply write\n> \n> sdlTexture_ = nullptr;\n> \n> or\n> \n> sdlTexture_.reset();\n> \n> > > + sdlTexture_ = nullptr;\n> > > + }\n> > > +\n> > > + if (sdlRenderer_) {\n> > > + SDL_DestroyRenderer(sdlRenderer_);\n> > > + sdlRenderer_ = nullptr;\n> > > + }\n> > > +\n> > > + if (sdlWindow_) {\n> > > + SDL_DestroyWindow(sdlWindow_);\n> > > + sdlWindow_ = nullptr;\n> > > + }\n> > > +\n> > > + if (sdlInit_) {\n> > > + SDL_Quit();\n> > > + sdlInit_ = false;\n> > > + }\n> > > +\n> > > + return FrameSink::stop();\n> > > +}\n> > > +\n> > > +void SDLSink::mapBuffer(FrameBuffer *buffer)\n> > > +{\n> > > + std::unique_ptr image =\n> > > + Image::fromFrameBuffer(buffer, Image::MapMode::ReadOnly);\n> > > + assert(image != nullptr);\n> > > +\n> > > + mappedBuffers_[buffer] = std::move(image);\n> > > +}\n> > > +\n> > > +bool SDLSink::processRequest(Request *request)\n> > > +{\n> > > + for (auto [stream, buffer] : request->buffers()) {\n> > > + renderBuffer(buffer);\n> > > + break; /* to be expanded to launch SDL window per buffer */\n> > > + }\n> > > +\n> > > + return true;\n> > > +}\n> > > +\n> > > +/*\n> > > + * Process SDL events, required for things like window resize and quit button\n> > > + */\n> > > +void SDLSink::processSDLEvents()\n> > > +{\n> > > + for (SDL_Event e; SDL_PollEvent(&e);) {\n> > > + if (e.type == SDL_QUIT) {\n> > > + /* Click close icon then quit */\n> > > + EventLoop::instance()->exit(0);\n> > > + }\n> > > + }\n> > > +}\n> > > +\n> > > +void SDLSink::renderBuffer(FrameBuffer *buffer)\n> > > +{\n> > > + Image *image = mappedBuffers_[buffer].get();\n> > > +\n> > > + for (unsigned int i = 0; i < buffer->planes().size(); ++i) {\n> > > + const FrameMetadata::Plane &meta =\n> > > + buffer->metadata().planes()[i];\n> > > +\n> > > + Span data = image->data(i);\n> > > + if (meta.bytesused > data.size())\n> > > + std::cerr << \"payload size \" << meta.bytesused\n> > > + << \" larger than plane size \" << data.size()\n> > > + << std::endl;\n> > > +\n> > > + sdlTexture_->update(data);\n> \n> I really don't think this will do what you expect for multi-planar\n> formats. As multi-planar formats are not supported yet, I'd replace the\n> loop by a single pass over plane 0, and add a\n> \n> /* \\todo Implement support for multi-planar formats. */\n> \n> > > + }\n> > > +\n> > > + SDL_RenderClear(sdlRenderer_);\n> > > + SDL_RenderCopy(sdlRenderer_, sdlTexture_->ptr_, nullptr, nullptr);\n> > > + SDL_RenderPresent(sdlRenderer_);\n> > > +}\n> > > diff --git a/src/cam/sdl_sink.h b/src/cam/sdl_sink.h\n> > > new file mode 100644\n> > > index 00000000..37dd9f7e\n> > > --- /dev/null\n> > > +++ b/src/cam/sdl_sink.h\n> > > @@ -0,0 +1,48 @@\n> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > +/*\n> > > + * sdl_sink.h - SDL Sink\n> > > + */\n> > > +\n> > > +#pragma once\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> \n> I think you can drop the string header.\n> \n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \"frame_sink.h\"\n> > > +#include \"sdl_texture.h\"\n> \n> You could also forward-declare the SDLTexture class and drop this\n> header.\n> \n> > > +\n> > > +class Image;\n> > > +\n> > > +class SDLSink : public FrameSink\n> > > +{\n> > > +public:\n> > > + SDLSink();\n> > > + ~SDLSink();\n> > > +\n> > > + int configure(const libcamera::CameraConfiguration &config) override;\n> > > + int start() override;\n> > > + int stop() override;\n> > > + void mapBuffer(libcamera::FrameBuffer *buffer) override;\n> > > +\n> > > + bool processRequest(libcamera::Request *request) override;\n> > > +\n> > > +private:\n> > > + void renderBuffer(libcamera::FrameBuffer *buffer);\n> > > + void processSDLEvents();\n> > > +\n> > > + std::map>\n> > > + mappedBuffers_;\n> > > +\n> > > + std::unique_ptr sdlTexture_;\n> > > +\n> > > + SDL_Window *sdlWindow_;\n> > > + SDL_Renderer *sdlRenderer_;\n> > > + SDL_Rect sdlRect_;\n> \n> I'm tempted to drop the sdl prefix from variable names, up to you.\n> \n> > > + SDL_PixelFormatEnum pixelFormat_;\n> > > + bool sdlInit_;\n> > > +};\n> > > diff --git a/src/cam/sdl_texture.cpp b/src/cam/sdl_texture.cpp\n> > > new file mode 100644\n> > > index 00000000..b39080c9\n> > > --- /dev/null\n> > > +++ b/src/cam/sdl_texture.cpp\n> > > @@ -0,0 +1,32 @@\n> \n> Missing SPDX tag and copyright header. Same below.\n> \n> > > +#include \"sdl_texture.h\"\n> > > +\n> > > +#include \n> > > +\n> > > +SDLTexture::SDLTexture(const SDL_Rect &sdlRect, SDL_PixelFormatEnum pixelFormat,\n> > > + const int pitch)\n> > > + : sdlRect_(sdlRect), pixelFormat_(pixelFormat), pitch_(pitch)\n> > > +{\n> > > +}\n> > > +\n> > > +SDLTexture::~SDLTexture()\n> > > +{\n> > > +}\n> > > +\n> > > +int SDLTexture::create(SDL_Renderer *sdlRenderer_)\n> > > +{\n> > > + ptr_ = SDL_CreateTexture(sdlRenderer_, pixelFormat_,\n> > > + SDL_TEXTUREACCESS_STREAMING, sdlRect_.w,\n> > > + sdlRect_.h);\n> > > + if (!ptr_) {\n> > > + std::cerr << \"Failed to create SDL texture: \" << SDL_GetError()\n> > > + << std::endl;\n> > > + return -ENOMEM;\n> > > + }\n> > > +\n> > > + return 0;\n> > > +}\n> > > +\n> > > +void SDLTexture::destroy()\n> > > +{\n> > > + SDL_DestroyTexture(ptr_);\n> > > +}\n> > > diff --git a/src/cam/sdl_texture.h b/src/cam/sdl_texture.h\n> > > new file mode 100644\n> > > index 00000000..f9525f7f\n> > > --- /dev/null\n> > > +++ b/src/cam/sdl_texture.h\n> > > @@ -0,0 +1,23 @@\n> > > +#pragma once\n> > > +\n> > > +#include \n> > > +\n> > > +#include \"image.h\"\n> > > +\n> > > +class SDLTexture\n> > > +{\n> > > +public:\n> > > + SDLTexture(const SDL_Rect &sdlRect, SDL_PixelFormatEnum pixelFormat,\n> > > + const int pitch);\n> > > + virtual ~SDLTexture();\n> > > + int create(SDL_Renderer *sdlRenderer_);\n> \n> s/sdlRenderer_/sdlRenderer/\n> \n> And same here (and with sdlRect in the previous function) about the sdl\n> prefix.\n> \n> > > + void destroy();\n> > > + virtual void update(const libcamera::Span &data) = 0;\n> > > +\n> > > + SDL_Texture *ptr_;\n> \n> Is it worth making this protected and adding an accessor function ?\n> \n> > > +\n> > > +protected:\n> > > + const SDL_Rect &sdlRect_;\n> \n> Same here, rect_.\n> \n> > > + SDL_PixelFormatEnum pixelFormat_;\n> > > + const int pitch_;\n> > > +};\n> > > diff --git a/src/cam/sdl_texture_yuyv.cpp b/src/cam/sdl_texture_yuyv.cpp\n> > > new file mode 100644\n> > > index 00000000..cf519a5d\n> > > --- /dev/null\n> > > +++ b/src/cam/sdl_texture_yuyv.cpp\n> > > @@ -0,0 +1,13 @@\n> > > +#include \"sdl_texture_yuyv.h\"\n> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +SDLTextureYUYV::SDLTextureYUYV(const SDL_Rect &sdlRect)\n> > > + : SDLTexture(sdlRect, SDL_PIXELFORMAT_YUY2, 4 * ((sdlRect.w + 1) / 2))\n> > > +{\n> > > +}\n> > > +\n> > > +void SDLTextureYUYV::update(const Span &data)\n> > > +{\n> > > + SDL_UpdateTexture(ptr_, &sdlRect_, data.data(), pitch_);\n> > > +}\n> > > diff --git a/src/cam/sdl_texture_yuyv.h b/src/cam/sdl_texture_yuyv.h\n> > > new file mode 100644\n> > > index 00000000..ad70f14f\n> > > --- /dev/null\n> > > +++ b/src/cam/sdl_texture_yuyv.h\n> > > @@ -0,0 +1,10 @@\n> > > +#pragma once\n> > > +\n> > > +#include \"sdl_texture.h\"\n> > > +\n> > > +class SDLTextureYUYV : public SDLTexture\n> > > +{\n> > > +public:\n> > > + SDLTextureYUYV(const SDL_Rect &sdlRect);\n> > > + void update(const libcamera::Span &data) override;\n> > > +};\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0AC55C0F2A\n\tfor ;\n\tThu, 19 May 2022 16:22:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3B10D65665;\n\tThu, 19 May 2022 18:22:46 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 442A260420\n\tfor ;\n\tThu, 19 May 2022 18:22:44 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B73AC6D4;\n\tThu, 19 May 2022 18:22:43 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1652977366;\n\tbh=+YfsKdSPJraOs3WmOuKWN+BlU9pEuRhmSFipmArnXO4=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=mmIIUE7KHT/5gFsUy2TGZD6V6aZbR7exPoAx6D6NQQfn5oOwRBK1fnC5Xw1dfw/1b\n\tTzkmGUIGl4eSBQ7wTC4u56Rpz0xKrR3kPgw0k7qGWAAceZ/nAUyHZQJ6XFCaTtZPG1\n\tjNa5sB+VukwPSv/EAoCy0w3rZbfFGdRZH8h9S/4c6LqQkvl9V4zYe1pxKzB+PNX9M9\n\tvrfzaquK6G/r3WQV7eY+dCu/LdapFHzZo/cpFhEuuj5241ZLueoHa93xxDfOrLTswB\n\t24Z1DtOPoLtoEoCa5bZwRMrdTodVUiwDiuZITmR8FEkFL24fqudl0812gzTIatbNTC\n\tAsl32FQJ/sHcQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1652977363;\n\tbh=+YfsKdSPJraOs3WmOuKWN+BlU9pEuRhmSFipmArnXO4=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=R7s6PUO+orvqhTrTqD8hHPtxG1qUtNIWPHGUtzqf3Ts1/lgH9c1v4UDLqUDNnbGTf\n\twvuk1HJSkM71+23m/iBTlsxdMfw6I27cpN4rSu35sCKQnQTYZrOeZvKFXLKx6neu0q\n\tEr51o5kzNxxt0PoGEzp0dWyTzzEYYmPhC93wBdqE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"R7s6PUO+\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20220505151851.2421-1-ecurtin@redhat.com>\n\t<20220505151851.2421-3-ecurtin@redhat.com>\n\t<165283070083.2416244.15906036641040599419@Monstersaurus>\n\t","To":"Eric Curtin ,\n\tLaurent Pinchart ","Date":"Thu, 19 May 2022 17:22:41 +0100","Message-ID":"<165297736139.368702.12158192629293893246@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v8 3/4] cam: sdl_sink: Add SDL sink\n\twith initial YUYV support","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23114,"web_url":"https://patchwork.libcamera.org/comment/23114/","msgid":"","date":"2022-05-20T14:58:20","subject":"Re: [libcamera-devel] [PATCH v8 3/4] cam: sdl_sink: Add SDL sink\n\twith initial YUYV support","submitter":{"id":101,"url":"https://patchwork.libcamera.org/api/people/101/","name":"Eric Curtin","email":"ecurtin@redhat.com"},"content":"On Thu, 19 May 2022 at 17:22, Kieran Bingham\n wrote:\n>\n> Quoting Laurent Pinchart (2022-05-18 21:06:10)\n> > Hi Eric,\n> >\n> > Thank you for the patch.\n> >\n> > On Wed, May 18, 2022 at 12:38:20AM +0100, Kieran Bingham via libcamera-devel wrote:\n> > > Quoting Eric Curtin via libcamera-devel (2022-05-05 16:18:50)\n> > > > This adds more portability to existing cam sinks. You can pass a\n> > > > YUYV camera buffer and SDL will handle the pixel buffer conversion\n> > > > to the display. This allows cam reference implementation to display\n> > > > images on VMs, Mac M1, Raspberry Pi, etc. This also enables cam\n> > > > reference implementation, to run as a desktop application in wayland or\n> >\n> > s/wayland/Wayland/\n> >\n> > > > x11. SDL also has support for Android and ChromeOS which has not been\n> >\n> > s/x11/X11/\n> >\n> > > > tested. Also tested on simpledrm raspberry pi 4 framebuffer\n> >\n> > s/raspberry pi/Raspberry Pi/\n> >\n> > > > successfully where existing kms sink did not work. Can also be used as\n> > > > kmsdrm sink. Only supports one camera stream at present.\n> > > >\n> > > > Signed-off-by: Eric Curtin \n> > > > ---\n> > > > src/cam/camera_session.cpp | 8 ++\n> > > > src/cam/main.cpp | 4 +\n> > > > src/cam/main.h | 1 +\n> > > > src/cam/meson.build | 12 +++\n> > > > src/cam/sdl_sink.cpp | 192 +++++++++++++++++++++++++++++++++++\n> > > > src/cam/sdl_sink.h | 48 +++++++++\n> > > > src/cam/sdl_texture.cpp | 32 ++++++\n> > > > src/cam/sdl_texture.h | 23 +++++\n> > > > src/cam/sdl_texture_yuyv.cpp | 13 +++\n> > > > src/cam/sdl_texture_yuyv.h | 10 ++\n> > >\n> > > Given the number of sdl_ files here I wonder if they should be under\n> > > src/cam/sdl/....\n> > >\n> > > But not crucial to this patch.\n> > >\n> > > I can't test this right now - but I'm looking forward to seeing a\n> > > preview on cam ... so I'll hopefully test this tomorrow.\n> > >\n> > > > 10 files changed, 343 insertions(+)\n> > > > create mode 100644 src/cam/sdl_sink.cpp\n> > > > create mode 100644 src/cam/sdl_sink.h\n> > > > create mode 100644 src/cam/sdl_texture.cpp\n> > > > create mode 100644 src/cam/sdl_texture.h\n> > > > create mode 100644 src/cam/sdl_texture_yuyv.cpp\n> > > > create mode 100644 src/cam/sdl_texture_yuyv.h\n> > > >\n> > > > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp\n> > > > index efffafbf..71e6bd60 100644\n> > > > --- a/src/cam/camera_session.cpp\n> > > > +++ b/src/cam/camera_session.cpp\n> > > > @@ -19,6 +19,9 @@\n> > > > #ifdef HAVE_KMS\n> > > > #include \"kms_sink.h\"\n> > > > #endif\n> > > > +#ifdef HAVE_SDL\n> > > > +#include \"sdl_sink.h\"\n> > > > +#endif\n> >\n> > Please move this below in alphabetical order.\n> >\n> > > > #include \"main.h\"\n> > > > #include \"stream_options.h\"\n> > > >\n> > > > @@ -186,6 +189,11 @@ int CameraSession::start()\n> > > > sink_ = std::make_unique(options_[OptDisplay].toString());\n> > > > #endif\n> > > >\n> > > > +#ifdef HAVE_SDL\n> > > > + if (options_.isSet(OptSDL))\n> > > > + sink_ = std::make_unique();\n> > > > +#endif\n> > > > +\n> > >\n> > > It looks like the precedence of which sink really gets used if multiples\n> > > are set is simply the order in this file that they get set. But I think\n> > > that's ok for the moment. Not sure we could do much else right now\n> > > without extending to support multiple sinks which is out of scope here.\n> > >\n> > > Reviewed-by: Kieran Bingham \n> > >\n> > >\n> > > > if (options_.isSet(OptFile)) {\n> > > > if (!options_[OptFile].toString().empty())\n> > > > sink_ = std::make_unique(streamNames_,\n> > > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> > > > index c7f664b9..fd3108b0 100644\n> > > > --- a/src/cam/main.cpp\n> > > > +++ b/src/cam/main.cpp\n> > > > @@ -137,6 +137,10 @@ int CamApp::parseOptions(int argc, char *argv[])\n> > > > \"Display viewfinder through DRM/KMS on specified connector\",\n> > > > \"display\", ArgumentOptional, \"connector\", false,\n> > > > OptCamera);\n> > > > +#endif\n> > > > +#ifdef HAVE_SDL\n> > > > + parser.addOption(OptSDL, OptionNone, \"Display viewfinder through SDL\",\n> > > > + \"sdl\", ArgumentNone, \"\", false, OptCamera);\n> > > > #endif\n> >\n> > I wonder if we should merge all sink arguments into one, but that can be\n> > done on top (the cam arguments syntax isn't stable yet).\n> >\n> > > > parser.addOption(OptFile, OptionString,\n> > > > \"Write captured frames to disk\\n\"\n> > > > diff --git a/src/cam/main.h b/src/cam/main.h\n> > > > index 62f7bbc9..2b285808 100644\n> > > > --- a/src/cam/main.h\n> > > > +++ b/src/cam/main.h\n> > > > @@ -18,6 +18,7 @@ enum {\n> > > > OptListProperties = 'p',\n> > > > OptMonitor = 'm',\n> > > > OptStream = 's',\n> > > > + OptSDL = 'S',\n> > > > OptListControls = 256,\n> > > > OptStrictFormats = 257,\n> > > > OptMetadata = 258,\n> > > > diff --git a/src/cam/meson.build b/src/cam/meson.build\n> > > > index 5bab8c9e..3032730b 100644\n> > > > --- a/src/cam/meson.build\n> > > > +++ b/src/cam/meson.build\n> > > > @@ -32,11 +32,23 @@ if libdrm.found()\n> > > > ])\n> > > > endif\n> > > >\n> > > > +libsdl2 = dependency('SDL2', required : false)\n>\n> I installed libsdl2-dev but got failures around SDL2/SDL_image.h\n>\n> On ubuntu I installed libsdl2-image-dev, and then rebuilt. That found\n> the header, but then caused a link error...\n\nWhat version of Ubuntu? Fedora tends to be my primary build machine.\nIt was intended that patch 3 was only the SDL core library dependant\ncode and patch 4 was the SDL image dependant code for MJPG.\n\n>\n> Does this also need to have some 'SDL2-Image' dependency check?\n>\n>\n> > > > +\n> > > > +if libsdl2.found()\n> > > > + cam_cpp_args += ['-DHAVE_SDL']\n> > > > + cam_sources += files([\n> > > > + 'sdl_sink.cpp',\n> > > > + 'sdl_texture.cpp',\n> > > > + 'sdl_texture_yuyv.cpp'\n> > > > + ])\n> > > > +endif\n> > > > +\n> > > > cam = executable('cam', cam_sources,\n> > > > dependencies : [\n> > > > libatomic,\n> > > > libcamera_public,\n> > > > libdrm,\n> > > > + libsdl2,\n> >\n> > Please keep the list alphabetically ordered.\n> >\n> > > > libevent,\n> > > > ],\n> > > > cpp_args : cam_cpp_args,\n> > > > diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp\n> > > > new file mode 100644\n> > > > index 00000000..6fbeaf56\n> > > > --- /dev/null\n> > > > +++ b/src/cam/sdl_sink.cpp\n> > > > @@ -0,0 +1,192 @@\n> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > +/*\n> > > > + * sdl_sink.cpp - SDL Sink\n> > > > + */\n> > > > +\n> > > > +#include \"sdl_sink.h\"\n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \"sdl_texture_yuyv.h\"\n> > > > +\n> > > > +#include \"event_loop.h\"\n> > > > +#include \"image.h\"\n> >\n> > I don't think there's a need to split sdl_texture_yuyv.h and the next\n> > two headers in two groups.\n> >\n> > > > +\n> > > > +using namespace libcamera;\n> > > > +\n> > > > +SDLSink::SDLSink()\n> > > > + : sdlWindow_(nullptr), sdlRenderer_(nullptr), sdlRect_({}),\n> > > > + sdlInit_(false)\n> > > > +{\n> > > > +}\n> > > > +\n> > > > +SDLSink::~SDLSink()\n> > > > +{\n> > > > + stop();\n> > > > +}\n> > > > +\n> > > > +int SDLSink::configure(const libcamera::CameraConfiguration &config)\n> > > > +{\n> > > > + int ret = FrameSink::configure(config);\n> > > > + if (ret < 0)\n> > > > + return ret;\n> > > > +\n> > > > + if (config.size() > 1) {\n> > > > + std::cerr\n> > > > + << \"SDL sink only supports one camera stream at present, streaming first camera stream\"\n> > > > + << std::endl;\n> > > > + } else if (config.empty()) {\n> > > > + std::cerr << \"Require at least one camera stream to process\"\n> > > > + << std::endl;\n> > > > + return -EINVAL;\n> > > > + }\n> > > > +\n> > > > + const libcamera::StreamConfiguration &cfg = config.at(0);\n> > > > + sdlRect_.w = cfg.size.width;\n> > > > + sdlRect_.h = cfg.size.height;\n> > > > +\n> > > > + switch (cfg.pixelFormat) {\n> > > > + case libcamera::formats::YUYV:\n> > > > + sdlTexture_ = std::make_unique(sdlRect_);\n> > > > + break;\n> > > > + default:\n> > > > + std::cerr << \"Unsupported pixel format \"\n> > > > + << cfg.pixelFormat.toString() << std::endl;\n> > > > + return -EINVAL;\n> > > > + };\n> > > > +\n> > > > + return 0;\n> > > > +}\n> > > > +\n> > > > +int SDLSink::start()\n> > > > +{\n> > > > + int ret = SDL_Init(SDL_INIT_VIDEO);\n> > > > + if (ret) {\n> > > > + std::cerr << \"Failed to initialize SDL: \" << SDL_GetError()\n> > > > + << std::endl;\n> > > > + return ret;\n> > > > + }\n> > > > +\n> > > > + sdlInit_ = true;\n> > > > + sdlWindow_ = SDL_CreateWindow(\"\", SDL_WINDOWPOS_UNDEFINED,\n> > > > + SDL_WINDOWPOS_UNDEFINED, sdlRect_.w,\n> > > > + sdlRect_.h,\n> > > > + SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE);\n> > > > + if (!sdlWindow_) {\n> > > > + std::cerr << \"Failed to create SDL window: \" << SDL_GetError()\n> > > > + << std::endl;\n> > > > + return -EINVAL;\n> > > > + }\n> > > > +\n> > > > + sdlRenderer_ = SDL_CreateRenderer(sdlWindow_, -1, 0);\n> > > > + if (!sdlRenderer_) {\n> > > > + std::cerr << \"Failed to create SDL renderer: \" << SDL_GetError()\n> > > > + << std::endl;\n> > > > + return -EINVAL;\n> > > > + }\n> > > > +\n> > > > + ret = SDL_RenderSetLogicalSize(sdlRenderer_, sdlRect_.w, sdlRect_.h);\n> > > > + if (ret) { /* Not critical to set, don't return in this case, set for scaling purposes */\n> > > > + std::cerr << \"Failed to set SDL render logical size: \"\n> > > > + << SDL_GetError() << std::endl;\n> > > > + }\n> >\n> > /*\n> > * Not critical to set, don't return in this case, set for scaling\n> > * purposes.\n> > */\n> > if (ret)\n> > std::cerr << \"Failed to set SDL render logical size: \"\n> > << SDL_GetError() << std::endl;\n> >\n> > > > +\n> > > > + ret = sdlTexture_->create(sdlRenderer_);\n> > > > + if (ret) {\n> > > > + return ret;\n> > > > + }\n> > > > +\n> > > > + EventLoop::instance()->addTimerEvent(\n> > > > + 10ms, std::bind(&SDLSink::processSDLEvents, this));\n> >\n> > I think I mentioned before that this won't work nicely if we stop and\n> > restart. cam doesn't do so at the moment, so it doesn't have to be fixed\n> > yet, but a todo comment would be good:\n> >\n> > /* \\todo Make the event cancellable to support stop/start cycles. */\n> > EventLoop::instance()->addTimerEvent(\n> > 10ms, std::bind(&SDLSink::processSDLEvents, this));\n> >\n> > > > +\n> > > > + return 0;\n> > > > +}\n> > > > +\n> > > > +int SDLSink::stop()\n> > > > +{\n> > > > + if (sdlTexture_) {\n> > > > + sdlTexture_->destroy();\n> >\n> > Can you turn SDLTexture::destroy() into a destructor ? Then you could\n> > simply write\n> >\n> > sdlTexture_ = nullptr;\n> >\n> > or\n> >\n> > sdlTexture_.reset();\n> >\n> > > > + sdlTexture_ = nullptr;\n> > > > + }\n> > > > +\n> > > > + if (sdlRenderer_) {\n> > > > + SDL_DestroyRenderer(sdlRenderer_);\n> > > > + sdlRenderer_ = nullptr;\n> > > > + }\n> > > > +\n> > > > + if (sdlWindow_) {\n> > > > + SDL_DestroyWindow(sdlWindow_);\n> > > > + sdlWindow_ = nullptr;\n> > > > + }\n> > > > +\n> > > > + if (sdlInit_) {\n> > > > + SDL_Quit();\n> > > > + sdlInit_ = false;\n> > > > + }\n> > > > +\n> > > > + return FrameSink::stop();\n> > > > +}\n> > > > +\n> > > > +void SDLSink::mapBuffer(FrameBuffer *buffer)\n> > > > +{\n> > > > + std::unique_ptr image =\n> > > > + Image::fromFrameBuffer(buffer, Image::MapMode::ReadOnly);\n> > > > + assert(image != nullptr);\n> > > > +\n> > > > + mappedBuffers_[buffer] = std::move(image);\n> > > > +}\n> > > > +\n> > > > +bool SDLSink::processRequest(Request *request)\n> > > > +{\n> > > > + for (auto [stream, buffer] : request->buffers()) {\n> > > > + renderBuffer(buffer);\n> > > > + break; /* to be expanded to launch SDL window per buffer */\n> > > > + }\n> > > > +\n> > > > + return true;\n> > > > +}\n> > > > +\n> > > > +/*\n> > > > + * Process SDL events, required for things like window resize and quit button\n> > > > + */\n> > > > +void SDLSink::processSDLEvents()\n> > > > +{\n> > > > + for (SDL_Event e; SDL_PollEvent(&e);) {\n> > > > + if (e.type == SDL_QUIT) {\n> > > > + /* Click close icon then quit */\n> > > > + EventLoop::instance()->exit(0);\n> > > > + }\n> > > > + }\n> > > > +}\n> > > > +\n> > > > +void SDLSink::renderBuffer(FrameBuffer *buffer)\n> > > > +{\n> > > > + Image *image = mappedBuffers_[buffer].get();\n> > > > +\n> > > > + for (unsigned int i = 0; i < buffer->planes().size(); ++i) {\n> > > > + const FrameMetadata::Plane &meta =\n> > > > + buffer->metadata().planes()[i];\n> > > > +\n> > > > + Span data = image->data(i);\n> > > > + if (meta.bytesused > data.size())\n> > > > + std::cerr << \"payload size \" << meta.bytesused\n> > > > + << \" larger than plane size \" << data.size()\n> > > > + << std::endl;\n> > > > +\n> > > > + sdlTexture_->update(data);\n> >\n> > I really don't think this will do what you expect for multi-planar\n> > formats. As multi-planar formats are not supported yet, I'd replace the\n> > loop by a single pass over plane 0, and add a\n> >\n> > /* \\todo Implement support for multi-planar formats. */\n> >\n> > > > + }\n> > > > +\n> > > > + SDL_RenderClear(sdlRenderer_);\n> > > > + SDL_RenderCopy(sdlRenderer_, sdlTexture_->ptr_, nullptr, nullptr);\n> > > > + SDL_RenderPresent(sdlRenderer_);\n> > > > +}\n> > > > diff --git a/src/cam/sdl_sink.h b/src/cam/sdl_sink.h\n> > > > new file mode 100644\n> > > > index 00000000..37dd9f7e\n> > > > --- /dev/null\n> > > > +++ b/src/cam/sdl_sink.h\n> > > > @@ -0,0 +1,48 @@\n> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > +/*\n> > > > + * sdl_sink.h - SDL Sink\n> > > > + */\n> > > > +\n> > > > +#pragma once\n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +#include \n> >\n> > I think you can drop the string header.\n> >\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \"frame_sink.h\"\n> > > > +#include \"sdl_texture.h\"\n> >\n> > You could also forward-declare the SDLTexture class and drop this\n> > header.\n> >\n> > > > +\n> > > > +class Image;\n> > > > +\n> > > > +class SDLSink : public FrameSink\n> > > > +{\n> > > > +public:\n> > > > + SDLSink();\n> > > > + ~SDLSink();\n> > > > +\n> > > > + int configure(const libcamera::CameraConfiguration &config) override;\n> > > > + int start() override;\n> > > > + int stop() override;\n> > > > + void mapBuffer(libcamera::FrameBuffer *buffer) override;\n> > > > +\n> > > > + bool processRequest(libcamera::Request *request) override;\n> > > > +\n> > > > +private:\n> > > > + void renderBuffer(libcamera::FrameBuffer *buffer);\n> > > > + void processSDLEvents();\n> > > > +\n> > > > + std::map>\n> > > > + mappedBuffers_;\n> > > > +\n> > > > + std::unique_ptr sdlTexture_;\n> > > > +\n> > > > + SDL_Window *sdlWindow_;\n> > > > + SDL_Renderer *sdlRenderer_;\n> > > > + SDL_Rect sdlRect_;\n> >\n> > I'm tempted to drop the sdl prefix from variable names, up to you.\n> >\n> > > > + SDL_PixelFormatEnum pixelFormat_;\n> > > > + bool sdlInit_;\n> > > > +};\n> > > > diff --git a/src/cam/sdl_texture.cpp b/src/cam/sdl_texture.cpp\n> > > > new file mode 100644\n> > > > index 00000000..b39080c9\n> > > > --- /dev/null\n> > > > +++ b/src/cam/sdl_texture.cpp\n> > > > @@ -0,0 +1,32 @@\n> >\n> > Missing SPDX tag and copyright header. Same below.\n> >\n> > > > +#include \"sdl_texture.h\"\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +SDLTexture::SDLTexture(const SDL_Rect &sdlRect, SDL_PixelFormatEnum pixelFormat,\n> > > > + const int pitch)\n> > > > + : sdlRect_(sdlRect), pixelFormat_(pixelFormat), pitch_(pitch)\n> > > > +{\n> > > > +}\n> > > > +\n> > > > +SDLTexture::~SDLTexture()\n> > > > +{\n> > > > +}\n> > > > +\n> > > > +int SDLTexture::create(SDL_Renderer *sdlRenderer_)\n> > > > +{\n> > > > + ptr_ = SDL_CreateTexture(sdlRenderer_, pixelFormat_,\n> > > > + SDL_TEXTUREACCESS_STREAMING, sdlRect_.w,\n> > > > + sdlRect_.h);\n> > > > + if (!ptr_) {\n> > > > + std::cerr << \"Failed to create SDL texture: \" << SDL_GetError()\n> > > > + << std::endl;\n> > > > + return -ENOMEM;\n> > > > + }\n> > > > +\n> > > > + return 0;\n> > > > +}\n> > > > +\n> > > > +void SDLTexture::destroy()\n> > > > +{\n> > > > + SDL_DestroyTexture(ptr_);\n> > > > +}\n> > > > diff --git a/src/cam/sdl_texture.h b/src/cam/sdl_texture.h\n> > > > new file mode 100644\n> > > > index 00000000..f9525f7f\n> > > > --- /dev/null\n> > > > +++ b/src/cam/sdl_texture.h\n> > > > @@ -0,0 +1,23 @@\n> > > > +#pragma once\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \"image.h\"\n> > > > +\n> > > > +class SDLTexture\n> > > > +{\n> > > > +public:\n> > > > + SDLTexture(const SDL_Rect &sdlRect, SDL_PixelFormatEnum pixelFormat,\n> > > > + const int pitch);\n> > > > + virtual ~SDLTexture();\n> > > > + int create(SDL_Renderer *sdlRenderer_);\n> >\n> > s/sdlRenderer_/sdlRenderer/\n> >\n> > And same here (and with sdlRect in the previous function) about the sdl\n> > prefix.\n> >\n> > > > + void destroy();\n> > > > + virtual void update(const libcamera::Span &data) = 0;\n> > > > +\n> > > > + SDL_Texture *ptr_;\n> >\n> > Is it worth making this protected and adding an accessor function ?\n> >\n> > > > +\n> > > > +protected:\n> > > > + const SDL_Rect &sdlRect_;\n> >\n> > Same here, rect_.\n> >\n> > > > + SDL_PixelFormatEnum pixelFormat_;\n> > > > + const int pitch_;\n> > > > +};\n> > > > diff --git a/src/cam/sdl_texture_yuyv.cpp b/src/cam/sdl_texture_yuyv.cpp\n> > > > new file mode 100644\n> > > > index 00000000..cf519a5d\n> > > > --- /dev/null\n> > > > +++ b/src/cam/sdl_texture_yuyv.cpp\n> > > > @@ -0,0 +1,13 @@\n> > > > +#include \"sdl_texture_yuyv.h\"\n> > > > +\n> > > > +using namespace libcamera;\n> > > > +\n> > > > +SDLTextureYUYV::SDLTextureYUYV(const SDL_Rect &sdlRect)\n> > > > + : SDLTexture(sdlRect, SDL_PIXELFORMAT_YUY2, 4 * ((sdlRect.w + 1) / 2))\n> > > > +{\n> > > > +}\n> > > > +\n> > > > +void SDLTextureYUYV::update(const Span &data)\n> > > > +{\n> > > > + SDL_UpdateTexture(ptr_, &sdlRect_, data.data(), pitch_);\n> > > > +}\n> > > > diff --git a/src/cam/sdl_texture_yuyv.h b/src/cam/sdl_texture_yuyv.h\n> > > > new file mode 100644\n> > > > index 00000000..ad70f14f\n> > > > --- /dev/null\n> > > > +++ b/src/cam/sdl_texture_yuyv.h\n> > > > @@ -0,0 +1,10 @@\n> > > > +#pragma once\n> > > > +\n> > > > +#include \"sdl_texture.h\"\n> > > > +\n> > > > +class SDLTextureYUYV : public SDLTexture\n> > > > +{\n> > > > +public:\n> > > > + SDLTextureYUYV(const SDL_Rect &sdlRect);\n> > > > + void update(const libcamera::Span &data) override;\n> > > > +};\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 37AB9C3256\n\tfor ;\n\tFri, 20 May 2022 14:58:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9AF5560440;\n\tFri, 20 May 2022 16:58:42 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 52B4A60440\n\tfor ;\n\tFri, 20 May 2022 16:58:40 +0200 (CEST)","from mail-qt1-f199.google.com (mail-qt1-f199.google.com\n\t[209.85.160.199]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id\n\tus-mta-83-IgTOYFTGO8WVhEFJLgkKuQ-1; Fri, 20 May 2022 10:58:37 -0400","by mail-qt1-f199.google.com with SMTP id\n\tg1-20020ac85801000000b002f3b281f745so6810563qtg.22\n\tfor ;\n\tFri, 20 May 2022 07:58:37 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1653058722;\n\tbh=OLjg0ymXevi/VztZA0ipJUzikEYXGvIenb3Yi/9OF8A=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=VIF7E8xQ+1yMZ/SeEpkRKE1ly01Dv/b19GciOWWji5bwKHR/xjrD/sCwN6rxVI5tW\n\t0MAJJYsbulivIjPEjtG9RrarsBknkmBJAObQChtu4/87KUy8jyzvLEf0SHmugj40iB\n\tNfCVFDEaqgY6VYyDHp5CRjqQBa7d8BuY9xI8iSXY2tBy6cO+KbJiAcbmgy3c9b094g\n\tJdK+Fr1m9yg7BWPfbn/7qPrXJmzF8fuAkXWC1j/5CklWKVCs5c2NVi3ENOZvO81xGE\n\tKyZ453/dI7oXs0jozAhThEXqteBYZScUBPw0JPZa7W/47rAr1Zluv5yFiiKmEMDIyz\n\tg9LjbjlyQbb9A==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1653058719;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=Ev8E4b8VgwYUf35HC0pGoPLjlyVaYhBnsI2FRnr5NQY=;\n\tb=XlIiFMYdI20JXpmDayBy8Nl+ORQobXZ52A0hU1TJ10nyAG0ZeFC5TRF3kV8fNXTpy3eurl\n\tyzzIKK0wvw+nXSvl/XNgjwCBf9RhK31YUVe3LeCVgrP86/Ha/nuJYSzUcD4QsPScM9kbVF\n\tgUVCKp8BGMy2o/Mst250dWr23r75is0="],"Authentication-Results":["lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=redhat.com\n\theader.i=@redhat.com header.b=\"XlIiFMYd\"; \n\tdkim-atps=neutral","relay.mimecast.com;\n\tauth=pass smtp.auth=CUSA124A263 smtp.mailfrom=ecurtin@redhat.com"],"X-MC-Unique":"IgTOYFTGO8WVhEFJLgkKuQ-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=Ev8E4b8VgwYUf35HC0pGoPLjlyVaYhBnsI2FRnr5NQY=;\n\tb=gSF/Kwnxz4vcwIqZsaDMNhp8DuD3xJm2Pt76cQJ9wDkzHWhURzFqVA1xQUIwyZUNGh\n\tCGC1wwTGLnpO6K+oj/JpNYPKLcjrB+aPasPrVEH9KOJz5lJgSqJzSxNL6E0tYqk6HgTc\n\tV4tg6fw0jAe15cvyhV8bJ6fGETpzsorMmMPszZ+apoIR9xuA7bCmQc46CVspRsnnVuzQ\n\tLMunzuMNU2ML11yI0zrkJIw4b97seFvWDQy4HJxpIVhheat4T+0C7kXP5miekCTVsjyF\n\tJRXlfKGxw6LlF1qwu9EZq0a3Hpzoc1+zeZpdIqg1BIl5ctp11wu5SUwwVzLjApa2x5CU\n\trmsQ==","X-Gm-Message-State":"AOAM532jR6aaU+XtF1fIdFNdAAg2bIbBNlbfupwbZhrZhMQU8W67KS1K\n\tHU+jQbcBnW27xz6HY8AUSKe7C0oIjRZ2KwMun7CWzMrK4p/3zt7JMfEZSobVIgvf89jtsKsTR7J\n\tM6lmuyAzZgA97c0qjWepiet/KZfysFOn0uAhb9GdVJABDHtPjOA==","X-Received":["by 2002:a37:848:0:b0:6a3:25ea:3cd1 with SMTP id\n\t69-20020a370848000000b006a325ea3cd1mr6595703qki.686.1653058716984; \n\tFri, 20 May 2022 07:58:36 -0700 (PDT)","by 2002:a37:848:0:b0:6a3:25ea:3cd1 with SMTP id\n\t69-20020a370848000000b006a325ea3cd1mr6595683qki.686.1653058716644;\n\tFri, 20 May 2022 07:58:36 -0700 (PDT)"],"X-Google-Smtp-Source":"ABdhPJzTHMGjhvTRpWsGASwbnBBKOY/XH9b0wzOaQ7XhOk5Lw9HLoya/tQyjm0k+PKWH45fcazfyvCBTOmnjCQIkkIY=","MIME-Version":"1.0","References":"<20220505151851.2421-1-ecurtin@redhat.com>\n\t<20220505151851.2421-3-ecurtin@redhat.com>\n\t<165283070083.2416244.15906036641040599419@Monstersaurus>\n\t\n\t<165297736139.368702.12158192629293893246@Monstersaurus>","In-Reply-To":"<165297736139.368702.12158192629293893246@Monstersaurus>","Date":"Fri, 20 May 2022 15:58:20 +0100","Message-ID":"","To":"Kieran Bingham ","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v8 3/4] cam: sdl_sink: Add SDL sink\n\twith initial YUYV support","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Eric Curtin via libcamera-devel ","Reply-To":"Eric Curtin ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]