| Message ID | 20220408160015.33612-2-ecurtin@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Eric, Thank you for the patch. On Fri, Apr 08, 2022 at 05:00:15PM +0100, Eric Curtin via libcamera-devel wrote: > This adds more portability to existing cam sinks. You can pass a > YUYV camera buffer for example and SDL will handle the pixel buffer > conversion, although SDL does not support decompression for pixelformats > like MJPEG. This allows cam reference implementation to display images > on VMs, Mac M1, Raspberry Pi, etc. This also enables cam reference > implementation, to run as a desktop application in wayland or x11. > SDL also has support for Android and ChromeOS which 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> > --- > src/cam/camera_session.cpp | 8 ++ > src/cam/main.cpp | 5 + > src/cam/main.h | 1 + > src/cam/meson.build | 10 ++ > src/cam/sdl_sink.cpp | 198 +++++++++++++++++++++++++++++++++++++ > src/cam/sdl_sink.h | 46 +++++++++ > 6 files changed, 268 insertions(+) > create mode 100644 src/cam/sdl_sink.cpp > create mode 100644 src/cam/sdl_sink.h > > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp > index 0428b538..30162dbd 100644 > --- a/src/cam/camera_session.cpp > +++ b/src/cam/camera_session.cpp > @@ -19,6 +19,9 @@ > #ifdef HAVE_KMS > #include "kms_sink.h" > #endif > +#ifdef HAVE_SDL > +#include "sdl_sink.h" > +#endif > #include "main.h" > #include "stream_options.h" > > @@ -187,6 +190,11 @@ int CameraSession::start() > sink_ = std::make_unique<KMSSink>(options_[OptDisplay].toString()); Space before tab. There are several occurrences. > #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..1d62a64a 100644 > --- a/src/cam/main.cpp > +++ b/src/cam/main.cpp > @@ -137,6 +137,11 @@ int CamApp::parseOptions(int argc, char *argv[]) > "Display viewfinder through DRM/KMS on specified connector", > "display", ArgumentOptional, "connector", false, > OptCamera); > +#endif > +#ifdef HAVE_SDL > + parser.addOption(OptSDL, OptionNone, > + "Display viewfinder through SDL", > + "sdl", ArgumentNone, "", false, OptCamera); I'm tempted to refactor the -D option to specify the backend as an argument, given that the KMS and SDL sinks are mutually exclusive. I think this can be done later though. > #endif > parser.addOption(OptFile, OptionString, > "Write captured frames to disk\n" > diff --git a/src/cam/main.h b/src/cam/main.h > index 62f7bbc9..2b285808 100644 > --- a/src/cam/main.h > +++ b/src/cam/main.h > @@ -18,6 +18,7 @@ enum { > OptListProperties = 'p', > OptMonitor = 'm', > OptStream = 's', > + OptSDL = 'S', > OptListControls = 256, > OptStrictFormats = 257, > OptMetadata = 258, > diff --git a/src/cam/meson.build b/src/cam/meson.build > index 5bab8c9e..59787741 100644 > --- a/src/cam/meson.build > +++ b/src/cam/meson.build > @@ -32,11 +32,21 @@ if libdrm.found() > ]) > endif > > +libsdl2 = dependency('SDL2', required : false) > + > +if libsdl2.found() > + cam_cpp_args += ['-DHAVE_SDL'] > + cam_sources += files([ > + 'sdl_sink.cpp' > + ]) > +endif > + > cam = executable('cam', cam_sources, > dependencies : [ > libatomic, > libcamera_public, > libdrm, > + libsdl2, > libevent, > ], > cpp_args : cam_cpp_args, > diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp > new file mode 100644 > index 00000000..03511974 > --- /dev/null > +++ b/src/cam/sdl_sink.cpp > @@ -0,0 +1,198 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +/* > + * sdl_sink.cpp - SDL Sink > + */ > + > +#include "sdl_sink.h" > + > +#include <assert.h> > +#include <fcntl.h> > +#include <iomanip> > +#include <iostream> > +#include <signal.h> > +#include <sstream> > +#include <string.h> > +#include <unistd.h> > + > +#include <libcamera/camera.h> > +#include <libcamera/formats.h> > + > +#include "event_loop.h" > +#include "image.h" > + > +using namespace libcamera; > + > +SDLSink::SDLSink() > + : sdlWindow_(nullptr), sdlRenderer_(nullptr), sdlTexture_(nullptr), sdlRect_({}), sdlInit_(false) Line wrap please. Same in quite a few locations below. > +{ > +} > + > +SDLSink::~SDLSink() > +{ > + stop(); > +} > + > +int SDLSink::configure(const libcamera::CameraConfiguration &cfg) > +{ > + int ret = FrameSink::configure(cfg); > + if (ret < 0) > + return ret; > + > + if (cfg.size() > 1) { > + std::cerr << "SDL sink only supports one camera stream at present, streaming first camera stream" > + << std::endl; > + } else if (cfg.empty()) { > + std::cerr << "Require at least one camera stream to process" << std::endl; > + return -EINVAL; > + } > + > + const libcamera::StreamConfiguration &sCfg = cfg.at(0); You could rename 'cfg' to 'config' and 'sCfg' to 'cfg' to match KMSSink::configure(). > + sdlRect_.w = sCfg.size.width; > + sdlRect_.h = sCfg.size.height; Please add a blank line here. > + switch (sCfg.pixelFormat) { > + case libcamera::formats::YUYV: > + pixelFormat_ = SDL_PIXELFORMAT_YUY2; > + pitch_ = 4 * ((sdlRect_.w + 1) / 2); > + break; > + > + /* From here down the fourcc values are identical between SDL, drm, libcamera */ They're always identical between DRM and libcamera, that's by design :-) You can write /* From here down the fourcc values are identical between SDL and libcamera */ or simply drop the message, as I don't think it's really relevant. In that case I'd move the YVYU and UYVY formats just fter YUYV as they are related. > + case libcamera::formats::NV21: > + pixelFormat_ = SDL_PIXELFORMAT_NV21; > + pitch_ = sdlRect_.w; > + break; > + case libcamera::formats::NV12: > + pixelFormat_ = SDL_PIXELFORMAT_NV12; > + pitch_ = sdlRect_.w; > + break; > + case libcamera::formats::YVU420: > + pixelFormat_ = SDL_PIXELFORMAT_YV12; > + pitch_ = sdlRect_.w; > + break; > + case libcamera::formats::YVYU: > + pixelFormat_ = SDL_PIXELFORMAT_YVYU; > + pitch_ = 4 * ((sdlRect_.w + 1) / 2); > + break; > + case libcamera::formats::UYVY: > + pixelFormat_ = SDL_PIXELFORMAT_UYVY; > + pitch_ = 4 * ((sdlRect_.w + 1) / 2); > + break; Fhr the same of completeness, does SDL have VYUY support ? NV16 and NV61 would also be nice if they are supported. If not that's fine. > + default: > + std::cerr << sCfg.pixelFormat.toString() << " not present in libcamera <-> SDL map" > + << std::endl; std::cerr << "Unsupported pixel format " << sCfg.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; > + } > + > + sdlInit_ = true; > + sdlWindow_ = SDL_CreateWindow("", SDL_WINDOWPOS_UNDEFINED, SDL_WINDOWPOS_UNDEFINED, sdlRect_.w, sdlRect_.h, SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE); > + if (!sdlWindow_) { > + std::cerr << "Failed to create SDL window: " << SDL_GetError() << std::endl; > + return -EINVAL; > + } > + > + sdlRenderer_ = SDL_CreateRenderer(sdlWindow_, -1, 0); > + if (!sdlRenderer_) { > + std::cerr << "Failed to create SDL renderer: " << SDL_GetError() << std::endl; > + return -EINVAL; > + } > + > + ret = SDL_RenderSetLogicalSize(sdlRenderer_, sdlRect_.w, sdlRect_.h); > + if (ret) { /* Not critical to set, don't return in this case, set for scaling purposes */ Please move the comment to the next line. > + std::cerr << "Failed to set SDL render logical size: " << SDL_GetError() << std::endl; > + } > + > + sdlTexture_ = SDL_CreateTexture(sdlRenderer_, pixelFormat_, SDL_TEXTUREACCESS_STREAMING, sdlRect_.w, sdlRect_.h); > + if (!sdlTexture_) { > + std::cerr << "Failed to create SDL texture: " << SDL_GetError() << std::endl; > + return -EINVAL; > + } > + > + EventLoop::instance()->addTimerEvent(10000, std::bind(&SDLSink::processSDLEvents, this)); Please use std::chrono types for the addTimerEvent duration parameter. Unless I'm mistaken, the event will keep being triggered indefinitely, which means that SDLSink::processSDLEvents() could call SDL_PollEvent() even after stop() calls SDL_Quit(). That seems dangerous to me, I think we need a way to cancel periodic timer events. > + > + return 0; > +} > + > +int SDLSink::stop() > +{ > + if (sdlTexture_) { > + SDL_DestroyTexture(sdlTexture_); > + sdlTexture_ = nullptr; > + } > + > + if (sdlRenderer_) { > + SDL_DestroyRenderer(sdlRenderer_); > + sdlRenderer_ = nullptr; > + } > + > + if (sdlWindow_) { > + SDL_DestroyWindow(sdlWindow_); > + sdlWindow_ = nullptr; > + } > + > + if (sdlInit_) { > + SDL_Quit(); > + sdlInit_ = 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 */ Please move the comment to the next line, and s/click/Click/ Actually, maybe a switch/case would be better, to prepare for adding support for more events ? Up to you. > + EventLoop::instance()->exit(0); > + } > + } > +} > + > +void SDLSink::renderBuffer(FrameBuffer *buffer) > +{ > + Image *image = mappedBuffers_[buffer].get(); > + > + for (unsigned int i = 0; i < buffer->planes().size(); ++i) { > + const FrameMetadata::Plane &meta = buffer->metadata().planes()[i]; > + > + Span<uint8_t> data = image->data(i); > + if (meta.bytesused > data.size()) > + std::cerr << "payload size " << meta.bytesused > + << " larger than plane size " << data.size() > + << std::endl; > + > + SDL_UpdateTexture(sdlTexture_, &sdlRect_, data.data(), pitch_); According to https://wiki.libsdl.org/SDL_UpdateTexture, "This is a fairly slow function, intended for use with static textures that do not change often. If the texture is intended to be updated often, it is preferred to create the texture as streaming and use the locking functions referenced below. While this function will work with streaming textures, for optimization reasons you may not get the pixels back if you lock the texture afterward." Have you considered this ? I don't know what difference it makes when updating the full texture though, maybe the added complexity isn't worth it. > + SDL_RenderClear(sdlRenderer_); > + SDL_RenderCopy(sdlRenderer_, sdlTexture_, nullptr, nullptr); > + SDL_RenderPresent(sdlRenderer_); Does this really have to be repeated for each plane ? That sounds strange. > + } > +} > diff --git a/src/cam/sdl_sink.h b/src/cam/sdl_sink.h > new file mode 100644 > index 00000000..c9b0ab8e > --- /dev/null > +++ b/src/cam/sdl_sink.h > @@ -0,0 +1,46 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +/* > + * sdl_sink.h - SDL Sink > + */ > + > +#pragma once > + > +#include <map> > +#include <memory> > +#include <string> > + > +#include <libcamera/stream.h> > + > +#include <SDL2/SDL.h> > + > +#include "frame_sink.h" > + > +class Image; > + > +class SDLSink : public FrameSink > +{ > +public: > + SDLSink(); > + ~SDLSink(); > + > + int configure(const libcamera::CameraConfiguration &cfg) override; > + int start() override; > + 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_; > + > + SDL_Window *sdlWindow_; > + SDL_Renderer *sdlRenderer_; > + SDL_Texture *sdlTexture_; > + SDL_Rect sdlRect_; > + SDL_PixelFormatEnum pixelFormat_; > + bool sdlInit_; > + int pitch_; > +};
On Fri, 15 Apr 2022 at 18:13, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Eric, > > Thank you for the patch. > > On Fri, Apr 08, 2022 at 05:00:15PM +0100, Eric Curtin via libcamera-devel wrote: > > This adds more portability to existing cam sinks. You can pass a > > YUYV camera buffer for example and SDL will handle the pixel buffer > > conversion, although SDL does not support decompression for pixelformats > > like MJPEG. This allows cam reference implementation to display images > > on VMs, Mac M1, Raspberry Pi, etc. This also enables cam reference > > implementation, to run as a desktop application in wayland or x11. > > SDL also has support for Android and ChromeOS which 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> > > --- > > src/cam/camera_session.cpp | 8 ++ > > src/cam/main.cpp | 5 + > > src/cam/main.h | 1 + > > src/cam/meson.build | 10 ++ > > src/cam/sdl_sink.cpp | 198 +++++++++++++++++++++++++++++++++++++ > > src/cam/sdl_sink.h | 46 +++++++++ > > 6 files changed, 268 insertions(+) > > create mode 100644 src/cam/sdl_sink.cpp > > create mode 100644 src/cam/sdl_sink.h > > > > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp > > index 0428b538..30162dbd 100644 > > --- a/src/cam/camera_session.cpp > > +++ b/src/cam/camera_session.cpp > > @@ -19,6 +19,9 @@ > > #ifdef HAVE_KMS > > #include "kms_sink.h" > > #endif > > +#ifdef HAVE_SDL > > +#include "sdl_sink.h" > > +#endif > > #include "main.h" > > #include "stream_options.h" > > > > @@ -187,6 +190,11 @@ int CameraSession::start() > > sink_ = std::make_unique<KMSSink>(options_[OptDisplay].toString()); > > Space before tab. There are several occurrences. > > > #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..1d62a64a 100644 > > --- a/src/cam/main.cpp > > +++ b/src/cam/main.cpp > > @@ -137,6 +137,11 @@ int CamApp::parseOptions(int argc, char *argv[]) > > "Display viewfinder through DRM/KMS on specified connector", > > "display", ArgumentOptional, "connector", false, > > OptCamera); > > +#endif > > +#ifdef HAVE_SDL > > + parser.addOption(OptSDL, OptionNone, > > + "Display viewfinder through SDL", > > + "sdl", ArgumentNone, "", false, OptCamera); > > I'm tempted to refactor the -D option to specify the backend as an > argument, given that the KMS and SDL sinks are mutually exclusive. I > think this can be done later though. > > > #endif > > parser.addOption(OptFile, OptionString, > > "Write captured frames to disk\n" > > diff --git a/src/cam/main.h b/src/cam/main.h > > index 62f7bbc9..2b285808 100644 > > --- a/src/cam/main.h > > +++ b/src/cam/main.h > > @@ -18,6 +18,7 @@ enum { > > OptListProperties = 'p', > > OptMonitor = 'm', > > OptStream = 's', > > + OptSDL = 'S', > > OptListControls = 256, > > OptStrictFormats = 257, > > OptMetadata = 258, > > diff --git a/src/cam/meson.build b/src/cam/meson.build > > index 5bab8c9e..59787741 100644 > > --- a/src/cam/meson.build > > +++ b/src/cam/meson.build > > @@ -32,11 +32,21 @@ if libdrm.found() > > ]) > > endif > > > > +libsdl2 = dependency('SDL2', required : false) > > + > > +if libsdl2.found() > > + cam_cpp_args += ['-DHAVE_SDL'] > > + cam_sources += files([ > > + 'sdl_sink.cpp' > > + ]) > > +endif > > + > > cam = executable('cam', cam_sources, > > dependencies : [ > > libatomic, > > libcamera_public, > > libdrm, > > + libsdl2, > > libevent, > > ], > > cpp_args : cam_cpp_args, > > diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp > > new file mode 100644 > > index 00000000..03511974 > > --- /dev/null > > +++ b/src/cam/sdl_sink.cpp > > @@ -0,0 +1,198 @@ > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > +/* > > + * sdl_sink.cpp - SDL Sink > > + */ > > + > > +#include "sdl_sink.h" > > + > > +#include <assert.h> > > +#include <fcntl.h> > > +#include <iomanip> > > +#include <iostream> > > +#include <signal.h> > > +#include <sstream> > > +#include <string.h> > > +#include <unistd.h> > > + > > +#include <libcamera/camera.h> > > +#include <libcamera/formats.h> > > + > > +#include "event_loop.h" > > +#include "image.h" > > + > > +using namespace libcamera; > > + > > +SDLSink::SDLSink() > > + : sdlWindow_(nullptr), sdlRenderer_(nullptr), sdlTexture_(nullptr), sdlRect_({}), sdlInit_(false) > > Line wrap please. Same in quite a few locations below. Ok. > > > +{ > > +} > > + > > +SDLSink::~SDLSink() > > +{ > > + stop(); > > +} > > + > > +int SDLSink::configure(const libcamera::CameraConfiguration &cfg) > > +{ > > + int ret = FrameSink::configure(cfg); > > + if (ret < 0) > > + return ret; > > + > > + if (cfg.size() > 1) { > > + std::cerr << "SDL sink only supports one camera stream at present, streaming first camera stream" > > + << std::endl; > > + } else if (cfg.empty()) { > > + std::cerr << "Require at least one camera stream to process" << std::endl; > > + return -EINVAL; > > + } > > + > > + const libcamera::StreamConfiguration &sCfg = cfg.at(0); > > You could rename 'cfg' to 'config' and 'sCfg' to 'cfg' to match > KMSSink::configure(). Ok. > > > + sdlRect_.w = sCfg.size.width; > > + sdlRect_.h = sCfg.size.height; > > Please add a blank line here. Ok. > > > + switch (sCfg.pixelFormat) { > > + case libcamera::formats::YUYV: > > + pixelFormat_ = SDL_PIXELFORMAT_YUY2; > > + pitch_ = 4 * ((sdlRect_.w + 1) / 2); > > + break; > > + > > + /* From here down the fourcc values are identical between SDL, drm, libcamera */ > > They're always identical between DRM and libcamera, that's by design :-) > You can write > > /* From here down the fourcc values are identical between SDL and libcamera */ > > or simply drop the message, as I don't think it's really relevant. In > that case I'd move the YVYU and UYVY formats just fter YUYV as they are > related. Ok. > > > + case libcamera::formats::NV21: > > + pixelFormat_ = SDL_PIXELFORMAT_NV21; > > + pitch_ = sdlRect_.w; > > + break; > > + case libcamera::formats::NV12: > > + pixelFormat_ = SDL_PIXELFORMAT_NV12; > > + pitch_ = sdlRect_.w; > > + break; > > + case libcamera::formats::YVU420: > > + pixelFormat_ = SDL_PIXELFORMAT_YV12; > > + pitch_ = sdlRect_.w; > > + break; > > + case libcamera::formats::YVYU: > > + pixelFormat_ = SDL_PIXELFORMAT_YVYU; > > + pitch_ = 4 * ((sdlRect_.w + 1) / 2); > > + break; > > + case libcamera::formats::UYVY: > > + pixelFormat_ = SDL_PIXELFORMAT_UYVY; > > + pitch_ = 4 * ((sdlRect_.w + 1) / 2); > > + break; > > Fhr the same of completeness, does SDL have VYUY support ? NV16 and > NV61 would also be nice if they are supported. If not that's fine. Ok, I'll add if they are supported. > > > + default: > > + std::cerr << sCfg.pixelFormat.toString() << " not present in libcamera <-> SDL map" > > + << std::endl; > > std::cerr << "Unsupported pixel format " > << sCfg.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; > > + } > > + > > + sdlInit_ = true; > > + sdlWindow_ = SDL_CreateWindow("", SDL_WINDOWPOS_UNDEFINED, SDL_WINDOWPOS_UNDEFINED, sdlRect_.w, sdlRect_.h, SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE); > > + if (!sdlWindow_) { > > + std::cerr << "Failed to create SDL window: " << SDL_GetError() << std::endl; > > + return -EINVAL; > > + } > > + > > + sdlRenderer_ = SDL_CreateRenderer(sdlWindow_, -1, 0); > > + if (!sdlRenderer_) { > > + std::cerr << "Failed to create SDL renderer: " << SDL_GetError() << std::endl; > > + return -EINVAL; > > + } > > + > > + ret = SDL_RenderSetLogicalSize(sdlRenderer_, sdlRect_.w, sdlRect_.h); > > + if (ret) { /* Not critical to set, don't return in this case, set for scaling purposes */ > > Please move the comment to the next line. Ok. > > > + std::cerr << "Failed to set SDL render logical size: " << SDL_GetError() << std::endl; > > + } > > + > > + sdlTexture_ = SDL_CreateTexture(sdlRenderer_, pixelFormat_, SDL_TEXTUREACCESS_STREAMING, sdlRect_.w, sdlRect_.h); > > + if (!sdlTexture_) { > > + std::cerr << "Failed to create SDL texture: " << SDL_GetError() << std::endl; > > + return -EINVAL; > > + } > > + > > + EventLoop::instance()->addTimerEvent(10000, std::bind(&SDLSink::processSDLEvents, this)); > > Please use std::chrono types for the addTimerEvent duration parameter. Are we sure we want to do this, just for the sake of using the C++ version? You end up having to cast because it's not the actual type libevent requires, libevent requires a C timeval to be passed in. ../src/cam/event_loop.cpp:105:15: error: assigning to '__suseconds_t' (aka 'long') from incompatible type 'const std::chrono::microseconds' (aka 'const duration<long, ratio<1, 1000000>>') tv.tv_usec = period; > > Unless I'm mistaken, the event will keep being triggered indefinitely, which means > that SDLSink::processSDLEvents() could call SDL_PollEvent() even after > stop() calls SDL_Quit(). That seems dangerous to me, I think we need a > way to cancel periodic timer events. Will look into this, this is all ran through valgrind, address santizers, etc. didn't see anything like that pop up. But will check again. > > > + > > + return 0; > > +} > > + > > +int SDLSink::stop() > > +{ > > + if (sdlTexture_) { > > + SDL_DestroyTexture(sdlTexture_); > > + sdlTexture_ = nullptr; > > + } > > + > > + if (sdlRenderer_) { > > + SDL_DestroyRenderer(sdlRenderer_); > > + sdlRenderer_ = nullptr; > > + } > > + > > + if (sdlWindow_) { > > + SDL_DestroyWindow(sdlWindow_); > > + sdlWindow_ = nullptr; > > + } > > + > > + if (sdlInit_) { > > + SDL_Quit(); > > + sdlInit_ = 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 */ > > Please move the comment to the next line, and s/click/Click/ > > Actually, maybe a switch/case would be better, to prepare for adding > support for more events ? Up to you. I'll leave it as is, if more events prove useful and someone wants to change to switch in future, I really don't mind. > > > + EventLoop::instance()->exit(0); > > + } > > + } > > +} > > + > > +void SDLSink::renderBuffer(FrameBuffer *buffer) > > +{ > > + Image *image = mappedBuffers_[buffer].get(); > > + > > + for (unsigned int i = 0; i < buffer->planes().size(); ++i) { > > + const FrameMetadata::Plane &meta = buffer->metadata().planes()[i]; > > + > > + Span<uint8_t> data = image->data(i); > > + if (meta.bytesused > data.size()) > > + std::cerr << "payload size " << meta.bytesused > > + << " larger than plane size " << data.size() > > + << std::endl; > > + > > + SDL_UpdateTexture(sdlTexture_, &sdlRect_, data.data(), pitch_); > > According to https://wiki.libsdl.org/SDL_UpdateTexture, > > "This is a fairly slow function, intended for use with static textures > that do not change often. > > If the texture is intended to be updated often, it is preferred to > create the texture as streaming and use the locking functions referenced > below. While this function will work with streaming textures, for > optimization reasons you may not get the pixels back if you lock the > texture afterward." > > Have you considered this ? I don't know what difference it makes when > updating the full texture though, maybe the added complexity isn't worth > it. Will look into this if it's not overly complex will try other technique suggested in that helpful link you shared. > > > + SDL_RenderClear(sdlRenderer_); > > + SDL_RenderCopy(sdlRenderer_, sdlTexture_, nullptr, nullptr); > > + SDL_RenderPresent(sdlRenderer_); > > Does this really have to be repeated for each plane ? That sounds > strange. This code is commonly written this way, but that doesn't mean it's most optimal like you said :) I'll explore other techniques. > > > + } > > +} > > diff --git a/src/cam/sdl_sink.h b/src/cam/sdl_sink.h > > new file mode 100644 > > index 00000000..c9b0ab8e > > --- /dev/null > > +++ b/src/cam/sdl_sink.h > > @@ -0,0 +1,46 @@ > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > +/* > > + * sdl_sink.h - SDL Sink > > + */ > > + > > +#pragma once > > + > > +#include <map> > > +#include <memory> > > +#include <string> > > + > > +#include <libcamera/stream.h> > > + > > +#include <SDL2/SDL.h> > > + > > +#include "frame_sink.h" > > + > > +class Image; > > + > > +class SDLSink : public FrameSink > > +{ > > +public: > > + SDLSink(); > > + ~SDLSink(); > > + > > + int configure(const libcamera::CameraConfiguration &cfg) override; > > + int start() override; > > + 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_; > > + > > + SDL_Window *sdlWindow_; > > + SDL_Renderer *sdlRenderer_; > > + SDL_Texture *sdlTexture_; > > + SDL_Rect sdlRect_; > > + SDL_PixelFormatEnum pixelFormat_; > > + bool sdlInit_; > > + int pitch_; > > +}; > > -- > Regards, > > Laurent Pinchart >
Quoting Eric Curtin via libcamera-devel (2022-04-19 15:47:17) > On Fri, 15 Apr 2022 at 18:13, Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: > > > > Hi Eric, > > > > Thank you for the patch. > > > > On Fri, Apr 08, 2022 at 05:00:15PM +0100, Eric Curtin via libcamera-devel wrote: > > > This adds more portability to existing cam sinks. You can pass a > > > YUYV camera buffer for example and SDL will handle the pixel buffer > > > conversion, although SDL does not support decompression for pixelformats > > > like MJPEG. This allows cam reference implementation to display images > > > on VMs, Mac M1, Raspberry Pi, etc. This also enables cam reference > > > implementation, to run as a desktop application in wayland or x11. > > > SDL also has support for Android and ChromeOS which 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> > > > --- > > > src/cam/camera_session.cpp | 8 ++ > > > src/cam/main.cpp | 5 + > > > src/cam/main.h | 1 + > > > src/cam/meson.build | 10 ++ > > > src/cam/sdl_sink.cpp | 198 +++++++++++++++++++++++++++++++++++++ > > > src/cam/sdl_sink.h | 46 +++++++++ > > > 6 files changed, 268 insertions(+) > > > create mode 100644 src/cam/sdl_sink.cpp > > > create mode 100644 src/cam/sdl_sink.h > > > > > > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp > > > index 0428b538..30162dbd 100644 > > > --- a/src/cam/camera_session.cpp > > > +++ b/src/cam/camera_session.cpp > > > @@ -19,6 +19,9 @@ > > > #ifdef HAVE_KMS > > > #include "kms_sink.h" > > > #endif > > > +#ifdef HAVE_SDL > > > +#include "sdl_sink.h" > > > +#endif > > > #include "main.h" > > > #include "stream_options.h" > > > > > > @@ -187,6 +190,11 @@ int CameraSession::start() > > > sink_ = std::make_unique<KMSSink>(options_[OptDisplay].toString()); > > > > Space before tab. There are several occurrences. > > > > > #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..1d62a64a 100644 > > > --- a/src/cam/main.cpp > > > +++ b/src/cam/main.cpp > > > @@ -137,6 +137,11 @@ int CamApp::parseOptions(int argc, char *argv[]) > > > "Display viewfinder through DRM/KMS on specified connector", > > > "display", ArgumentOptional, "connector", false, > > > OptCamera); > > > +#endif > > > +#ifdef HAVE_SDL > > > + parser.addOption(OptSDL, OptionNone, > > > + "Display viewfinder through SDL", > > > + "sdl", ArgumentNone, "", false, OptCamera); > > > > I'm tempted to refactor the -D option to specify the backend as an > > argument, given that the KMS and SDL sinks are mutually exclusive. I > > think this can be done later though. > > > > > #endif > > > parser.addOption(OptFile, OptionString, > > > "Write captured frames to disk\n" > > > diff --git a/src/cam/main.h b/src/cam/main.h > > > index 62f7bbc9..2b285808 100644 > > > --- a/src/cam/main.h > > > +++ b/src/cam/main.h > > > @@ -18,6 +18,7 @@ enum { > > > OptListProperties = 'p', > > > OptMonitor = 'm', > > > OptStream = 's', > > > + OptSDL = 'S', > > > OptListControls = 256, > > > OptStrictFormats = 257, > > > OptMetadata = 258, > > > diff --git a/src/cam/meson.build b/src/cam/meson.build > > > index 5bab8c9e..59787741 100644 > > > --- a/src/cam/meson.build > > > +++ b/src/cam/meson.build > > > @@ -32,11 +32,21 @@ if libdrm.found() > > > ]) > > > endif > > > > > > +libsdl2 = dependency('SDL2', required : false) > > > + > > > +if libsdl2.found() > > > + cam_cpp_args += ['-DHAVE_SDL'] > > > + cam_sources += files([ > > > + 'sdl_sink.cpp' > > > + ]) > > > +endif > > > + > > > cam = executable('cam', cam_sources, > > > dependencies : [ > > > libatomic, > > > libcamera_public, > > > libdrm, > > > + libsdl2, > > > libevent, > > > ], > > > cpp_args : cam_cpp_args, > > > diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp > > > new file mode 100644 > > > index 00000000..03511974 > > > --- /dev/null > > > +++ b/src/cam/sdl_sink.cpp > > > @@ -0,0 +1,198 @@ > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > > +/* > > > + * sdl_sink.cpp - SDL Sink > > > + */ > > > + > > > +#include "sdl_sink.h" > > > + > > > +#include <assert.h> > > > +#include <fcntl.h> > > > +#include <iomanip> > > > +#include <iostream> > > > +#include <signal.h> > > > +#include <sstream> > > > +#include <string.h> > > > +#include <unistd.h> > > > + > > > +#include <libcamera/camera.h> > > > +#include <libcamera/formats.h> > > > + > > > +#include "event_loop.h" > > > +#include "image.h" > > > + > > > +using namespace libcamera; > > > + > > > +SDLSink::SDLSink() > > > + : sdlWindow_(nullptr), sdlRenderer_(nullptr), sdlTexture_(nullptr), sdlRect_({}), sdlInit_(false) > > > > Line wrap please. Same in quite a few locations below. > > Ok. > > > > > > +{ > > > +} > > > + > > > +SDLSink::~SDLSink() > > > +{ > > > + stop(); > > > +} > > > + > > > +int SDLSink::configure(const libcamera::CameraConfiguration &cfg) > > > +{ > > > + int ret = FrameSink::configure(cfg); > > > + if (ret < 0) > > > + return ret; > > > + > > > + if (cfg.size() > 1) { > > > + std::cerr << "SDL sink only supports one camera stream at present, streaming first camera stream" > > > + << std::endl; > > > + } else if (cfg.empty()) { > > > + std::cerr << "Require at least one camera stream to process" << std::endl; > > > + return -EINVAL; > > > + } > > > + > > > + const libcamera::StreamConfiguration &sCfg = cfg.at(0); > > > > You could rename 'cfg' to 'config' and 'sCfg' to 'cfg' to match > > KMSSink::configure(). > > Ok. > > > > > > + sdlRect_.w = sCfg.size.width; > > > + sdlRect_.h = sCfg.size.height; > > > > Please add a blank line here. > > Ok. > > > > > > + switch (sCfg.pixelFormat) { > > > + case libcamera::formats::YUYV: > > > + pixelFormat_ = SDL_PIXELFORMAT_YUY2; > > > + pitch_ = 4 * ((sdlRect_.w + 1) / 2); > > > + break; > > > + > > > + /* From here down the fourcc values are identical between SDL, drm, libcamera */ > > > > They're always identical between DRM and libcamera, that's by design :-) > > You can write > > > > /* From here down the fourcc values are identical between SDL and libcamera */ > > > > or simply drop the message, as I don't think it's really relevant. In > > that case I'd move the YVYU and UYVY formats just fter YUYV as they are > > related. > > Ok. > > > > > > + case libcamera::formats::NV21: > > > + pixelFormat_ = SDL_PIXELFORMAT_NV21; > > > + pitch_ = sdlRect_.w; > > > + break; > > > + case libcamera::formats::NV12: > > > + pixelFormat_ = SDL_PIXELFORMAT_NV12; > > > + pitch_ = sdlRect_.w; > > > + break; > > > + case libcamera::formats::YVU420: > > > + pixelFormat_ = SDL_PIXELFORMAT_YV12; > > > + pitch_ = sdlRect_.w; > > > + break; > > > + case libcamera::formats::YVYU: > > > + pixelFormat_ = SDL_PIXELFORMAT_YVYU; > > > + pitch_ = 4 * ((sdlRect_.w + 1) / 2); > > > + break; > > > + case libcamera::formats::UYVY: > > > + pixelFormat_ = SDL_PIXELFORMAT_UYVY; > > > + pitch_ = 4 * ((sdlRect_.w + 1) / 2); > > > + break; > > > > Fhr the same of completeness, does SDL have VYUY support ? NV16 and > > NV61 would also be nice if they are supported. If not that's fine. > > Ok, I'll add if they are supported. > > > > > > + default: > > > + std::cerr << sCfg.pixelFormat.toString() << " not present in libcamera <-> SDL map" > > > + << std::endl; > > > > std::cerr << "Unsupported pixel format " > > << sCfg.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; > > > + } > > > + > > > + sdlInit_ = true; > > > + sdlWindow_ = SDL_CreateWindow("", SDL_WINDOWPOS_UNDEFINED, SDL_WINDOWPOS_UNDEFINED, sdlRect_.w, sdlRect_.h, SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE); > > > + if (!sdlWindow_) { > > > + std::cerr << "Failed to create SDL window: " << SDL_GetError() << std::endl; > > > + return -EINVAL; > > > + } > > > + > > > + sdlRenderer_ = SDL_CreateRenderer(sdlWindow_, -1, 0); > > > + if (!sdlRenderer_) { > > > + std::cerr << "Failed to create SDL renderer: " << SDL_GetError() << std::endl; > > > + return -EINVAL; > > > + } > > > + > > > + ret = SDL_RenderSetLogicalSize(sdlRenderer_, sdlRect_.w, sdlRect_.h); > > > + if (ret) { /* Not critical to set, don't return in this case, set for scaling purposes */ > > > > Please move the comment to the next line. > > Ok. > > > > > > + std::cerr << "Failed to set SDL render logical size: " << SDL_GetError() << std::endl; > > > + } > > > + > > > + sdlTexture_ = SDL_CreateTexture(sdlRenderer_, pixelFormat_, SDL_TEXTUREACCESS_STREAMING, sdlRect_.w, sdlRect_.h); > > > + if (!sdlTexture_) { > > > + std::cerr << "Failed to create SDL texture: " << SDL_GetError() << std::endl; > > > + return -EINVAL; > > > + } > > > + > > > + EventLoop::instance()->addTimerEvent(10000, std::bind(&SDLSink::processSDLEvents, this)); > > > > Please use std::chrono types for the addTimerEvent duration parameter. > > Are we sure we want to do this, just for the sake of using the C++ > version? You end up having to cast because it's not the actual type > libevent requires, libevent requires a C timeval to be passed in. > > ../src/cam/event_loop.cpp:105:15: error: assigning to '__suseconds_t' > (aka 'long') from incompatible type 'const std::chrono::microseconds' > (aka 'const duration<long, ratio<1, 1000000>>') > tv.tv_usec = period; > I'd say 'yes' - that's the point of Chrono. It means you should be able to call EventLoop::instance()->addTimerEvent(1s, std::bind(&SDLSink::processSDLEvents, this)); or EventLoop::instance()->addTimerEvent(100ms, std::bind(&SDLSink::processSDLEvents, this)); or EventLoop::instance()->addTimerEvent(10000us, std::bind(&SDLSink::processSDLEvents, this)); (not expected to be equivalent durations above) And the event loop would get added correctly. > > > > Unless I'm mistaken, the event will keep being triggered indefinitely, which means > > that SDLSink::processSDLEvents() could call SDL_PollEvent() even after > > stop() calls SDL_Quit(). That seems dangerous to me, I think we need a > > way to cancel periodic timer events. > > Will look into this, this is all ran through valgrind, address > santizers, etc. didn't see anything like that pop up. But will check > again. > > > > > > + > > > + return 0; > > > +} > > > + > > > +int SDLSink::stop() > > > +{ > > > + if (sdlTexture_) { > > > + SDL_DestroyTexture(sdlTexture_); > > > + sdlTexture_ = nullptr; > > > + } > > > + > > > + if (sdlRenderer_) { > > > + SDL_DestroyRenderer(sdlRenderer_); > > > + sdlRenderer_ = nullptr; > > > + } > > > + > > > + if (sdlWindow_) { > > > + SDL_DestroyWindow(sdlWindow_); > > > + sdlWindow_ = nullptr; > > > + } > > > + > > > + if (sdlInit_) { > > > + SDL_Quit(); > > > + sdlInit_ = 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 */ > > > > Please move the comment to the next line, and s/click/Click/ > > > > Actually, maybe a switch/case would be better, to prepare for adding > > support for more events ? Up to you. > > I'll leave it as is, if more events prove useful and someone wants to > change to switch in future, I really don't mind. A switch would already let unhandled events be reported via the default case. But maybe that would be too noisy anyway or unwanted. So I guess it doesn't matter too much. -- Kieran > > > > > > + EventLoop::instance()->exit(0); > > > + } > > > + } > > > +} > > > + > > > +void SDLSink::renderBuffer(FrameBuffer *buffer) > > > +{ > > > + Image *image = mappedBuffers_[buffer].get(); > > > + > > > + for (unsigned int i = 0; i < buffer->planes().size(); ++i) { > > > + const FrameMetadata::Plane &meta = buffer->metadata().planes()[i]; > > > + > > > + Span<uint8_t> data = image->data(i); > > > + if (meta.bytesused > data.size()) > > > + std::cerr << "payload size " << meta.bytesused > > > + << " larger than plane size " << data.size() > > > + << std::endl; > > > + > > > + SDL_UpdateTexture(sdlTexture_, &sdlRect_, data.data(), pitch_); > > > > According to https://wiki.libsdl.org/SDL_UpdateTexture, > > > > "This is a fairly slow function, intended for use with static textures > > that do not change often. > > > > If the texture is intended to be updated often, it is preferred to > > create the texture as streaming and use the locking functions referenced > > below. While this function will work with streaming textures, for > > optimization reasons you may not get the pixels back if you lock the > > texture afterward." > > > > Have you considered this ? I don't know what difference it makes when > > updating the full texture though, maybe the added complexity isn't worth > > it. > > Will look into this if it's not overly complex will try other > technique suggested in that helpful link you shared. > > > > > > + SDL_RenderClear(sdlRenderer_); > > > + SDL_RenderCopy(sdlRenderer_, sdlTexture_, nullptr, nullptr); > > > + SDL_RenderPresent(sdlRenderer_); > > > > Does this really have to be repeated for each plane ? That sounds > > strange. > > This code is commonly written this way, but that doesn't mean it's > most optimal like you said :) > > I'll explore other techniques. > > > > > > + } > > > +} > > > diff --git a/src/cam/sdl_sink.h b/src/cam/sdl_sink.h > > > new file mode 100644 > > > index 00000000..c9b0ab8e > > > --- /dev/null > > > +++ b/src/cam/sdl_sink.h > > > @@ -0,0 +1,46 @@ > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > > +/* > > > + * sdl_sink.h - SDL Sink > > > + */ > > > + > > > +#pragma once > > > + > > > +#include <map> > > > +#include <memory> > > > +#include <string> > > > + > > > +#include <libcamera/stream.h> > > > + > > > +#include <SDL2/SDL.h> > > > + > > > +#include "frame_sink.h" > > > + > > > +class Image; > > > + > > > +class SDLSink : public FrameSink > > > +{ > > > +public: > > > + SDLSink(); > > > + ~SDLSink(); > > > + > > > + int configure(const libcamera::CameraConfiguration &cfg) override; > > > + int start() override; > > > + 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_; > > > + > > > + SDL_Window *sdlWindow_; > > > + SDL_Renderer *sdlRenderer_; > > > + SDL_Texture *sdlTexture_; > > > + SDL_Rect sdlRect_; > > > + SDL_PixelFormatEnum pixelFormat_; > > > + bool sdlInit_; > > > + int pitch_; > > > +}; > > > > -- > > Regards, > > > > Laurent Pinchart > > >
Hi Eric, On Tue, Apr 19, 2022 at 03:47:17PM +0100, Eric Curtin wrote: > On Fri, 15 Apr 2022 at 18:13, Laurent Pinchart wrote: > > On Fri, Apr 08, 2022 at 05:00:15PM +0100, Eric Curtin via libcamera-devel wrote: > > > This adds more portability to existing cam sinks. You can pass a > > > YUYV camera buffer for example and SDL will handle the pixel buffer > > > conversion, although SDL does not support decompression for pixelformats > > > like MJPEG. This allows cam reference implementation to display images > > > on VMs, Mac M1, Raspberry Pi, etc. This also enables cam reference > > > implementation, to run as a desktop application in wayland or x11. > > > SDL also has support for Android and ChromeOS which 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> > > > --- > > > src/cam/camera_session.cpp | 8 ++ > > > src/cam/main.cpp | 5 + > > > src/cam/main.h | 1 + > > > src/cam/meson.build | 10 ++ > > > src/cam/sdl_sink.cpp | 198 +++++++++++++++++++++++++++++++++++++ > > > src/cam/sdl_sink.h | 46 +++++++++ > > > 6 files changed, 268 insertions(+) > > > create mode 100644 src/cam/sdl_sink.cpp > > > create mode 100644 src/cam/sdl_sink.h > > > > > > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp > > > index 0428b538..30162dbd 100644 > > > --- a/src/cam/camera_session.cpp > > > +++ b/src/cam/camera_session.cpp > > > @@ -19,6 +19,9 @@ > > > #ifdef HAVE_KMS > > > #include "kms_sink.h" > > > #endif > > > +#ifdef HAVE_SDL > > > +#include "sdl_sink.h" > > > +#endif > > > #include "main.h" > > > #include "stream_options.h" > > > > > > @@ -187,6 +190,11 @@ int CameraSession::start() > > > sink_ = std::make_unique<KMSSink>(options_[OptDisplay].toString()); > > > > Space before tab. There are several occurrences. > > > > > #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..1d62a64a 100644 > > > --- a/src/cam/main.cpp > > > +++ b/src/cam/main.cpp > > > @@ -137,6 +137,11 @@ int CamApp::parseOptions(int argc, char *argv[]) > > > "Display viewfinder through DRM/KMS on specified connector", > > > "display", ArgumentOptional, "connector", false, > > > OptCamera); > > > +#endif > > > +#ifdef HAVE_SDL > > > + parser.addOption(OptSDL, OptionNone, > > > + "Display viewfinder through SDL", > > > + "sdl", ArgumentNone, "", false, OptCamera); > > > > I'm tempted to refactor the -D option to specify the backend as an > > argument, given that the KMS and SDL sinks are mutually exclusive. I > > think this can be done later though. > > > > > #endif > > > parser.addOption(OptFile, OptionString, > > > "Write captured frames to disk\n" > > > diff --git a/src/cam/main.h b/src/cam/main.h > > > index 62f7bbc9..2b285808 100644 > > > --- a/src/cam/main.h > > > +++ b/src/cam/main.h > > > @@ -18,6 +18,7 @@ enum { > > > OptListProperties = 'p', > > > OptMonitor = 'm', > > > OptStream = 's', > > > + OptSDL = 'S', > > > OptListControls = 256, > > > OptStrictFormats = 257, > > > OptMetadata = 258, > > > diff --git a/src/cam/meson.build b/src/cam/meson.build > > > index 5bab8c9e..59787741 100644 > > > --- a/src/cam/meson.build > > > +++ b/src/cam/meson.build > > > @@ -32,11 +32,21 @@ if libdrm.found() > > > ]) > > > endif > > > > > > +libsdl2 = dependency('SDL2', required : false) > > > + > > > +if libsdl2.found() > > > + cam_cpp_args += ['-DHAVE_SDL'] > > > + cam_sources += files([ > > > + 'sdl_sink.cpp' > > > + ]) > > > +endif > > > + > > > cam = executable('cam', cam_sources, > > > dependencies : [ > > > libatomic, > > > libcamera_public, > > > libdrm, > > > + libsdl2, > > > libevent, > > > ], > > > cpp_args : cam_cpp_args, > > > diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp > > > new file mode 100644 > > > index 00000000..03511974 > > > --- /dev/null > > > +++ b/src/cam/sdl_sink.cpp > > > @@ -0,0 +1,198 @@ > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > > +/* > > > + * sdl_sink.cpp - SDL Sink > > > + */ > > > + > > > +#include "sdl_sink.h" > > > + > > > +#include <assert.h> > > > +#include <fcntl.h> > > > +#include <iomanip> > > > +#include <iostream> > > > +#include <signal.h> > > > +#include <sstream> > > > +#include <string.h> > > > +#include <unistd.h> > > > + > > > +#include <libcamera/camera.h> > > > +#include <libcamera/formats.h> > > > + > > > +#include "event_loop.h" > > > +#include "image.h" > > > + > > > +using namespace libcamera; > > > + > > > +SDLSink::SDLSink() > > > + : sdlWindow_(nullptr), sdlRenderer_(nullptr), sdlTexture_(nullptr), sdlRect_({}), sdlInit_(false) > > > > Line wrap please. Same in quite a few locations below. > > Ok. > > > > > > +{ > > > +} > > > + > > > +SDLSink::~SDLSink() > > > +{ > > > + stop(); > > > +} > > > + > > > +int SDLSink::configure(const libcamera::CameraConfiguration &cfg) > > > +{ > > > + int ret = FrameSink::configure(cfg); > > > + if (ret < 0) > > > + return ret; > > > + > > > + if (cfg.size() > 1) { > > > + std::cerr << "SDL sink only supports one camera stream at present, streaming first camera stream" > > > + << std::endl; > > > + } else if (cfg.empty()) { > > > + std::cerr << "Require at least one camera stream to process" << std::endl; > > > + return -EINVAL; > > > + } > > > + > > > + const libcamera::StreamConfiguration &sCfg = cfg.at(0); > > > > You could rename 'cfg' to 'config' and 'sCfg' to 'cfg' to match > > KMSSink::configure(). > > Ok. > > > > > > + sdlRect_.w = sCfg.size.width; > > > + sdlRect_.h = sCfg.size.height; > > > > Please add a blank line here. > > Ok. > > > > > > + switch (sCfg.pixelFormat) { > > > + case libcamera::formats::YUYV: > > > + pixelFormat_ = SDL_PIXELFORMAT_YUY2; > > > + pitch_ = 4 * ((sdlRect_.w + 1) / 2); > > > + break; > > > + > > > + /* From here down the fourcc values are identical between SDL, drm, libcamera */ > > > > They're always identical between DRM and libcamera, that's by design :-) > > You can write > > > > /* From here down the fourcc values are identical between SDL and libcamera */ > > > > or simply drop the message, as I don't think it's really relevant. In > > that case I'd move the YVYU and UYVY formats just fter YUYV as they are > > related. > > Ok. > > > > > > + case libcamera::formats::NV21: > > > + pixelFormat_ = SDL_PIXELFORMAT_NV21; > > > + pitch_ = sdlRect_.w; > > > + break; > > > + case libcamera::formats::NV12: > > > + pixelFormat_ = SDL_PIXELFORMAT_NV12; > > > + pitch_ = sdlRect_.w; > > > + break; > > > + case libcamera::formats::YVU420: > > > + pixelFormat_ = SDL_PIXELFORMAT_YV12; > > > + pitch_ = sdlRect_.w; > > > + break; > > > + case libcamera::formats::YVYU: > > > + pixelFormat_ = SDL_PIXELFORMAT_YVYU; > > > + pitch_ = 4 * ((sdlRect_.w + 1) / 2); > > > + break; > > > + case libcamera::formats::UYVY: > > > + pixelFormat_ = SDL_PIXELFORMAT_UYVY; > > > + pitch_ = 4 * ((sdlRect_.w + 1) / 2); > > > + break; > > > > Fhr the same of completeness, does SDL have VYUY support ? NV16 and > > NV61 would also be nice if they are supported. If not that's fine. > > Ok, I'll add if they are supported. > > > > > > + default: > > > + std::cerr << sCfg.pixelFormat.toString() << " not present in libcamera <-> SDL map" > > > + << std::endl; > > > > std::cerr << "Unsupported pixel format " > > << sCfg.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; > > > + } > > > + > > > + sdlInit_ = true; > > > + sdlWindow_ = SDL_CreateWindow("", SDL_WINDOWPOS_UNDEFINED, SDL_WINDOWPOS_UNDEFINED, sdlRect_.w, sdlRect_.h, SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE); > > > + if (!sdlWindow_) { > > > + std::cerr << "Failed to create SDL window: " << SDL_GetError() << std::endl; > > > + return -EINVAL; > > > + } > > > + > > > + sdlRenderer_ = SDL_CreateRenderer(sdlWindow_, -1, 0); > > > + if (!sdlRenderer_) { > > > + std::cerr << "Failed to create SDL renderer: " << SDL_GetError() << std::endl; > > > + return -EINVAL; > > > + } > > > + > > > + ret = SDL_RenderSetLogicalSize(sdlRenderer_, sdlRect_.w, sdlRect_.h); > > > + if (ret) { /* Not critical to set, don't return in this case, set for scaling purposes */ > > > > Please move the comment to the next line. > > Ok. > > > > > > + std::cerr << "Failed to set SDL render logical size: " << SDL_GetError() << std::endl; > > > + } > > > + > > > + sdlTexture_ = SDL_CreateTexture(sdlRenderer_, pixelFormat_, SDL_TEXTUREACCESS_STREAMING, sdlRect_.w, sdlRect_.h); > > > + if (!sdlTexture_) { > > > + std::cerr << "Failed to create SDL texture: " << SDL_GetError() << std::endl; > > > + return -EINVAL; > > > + } > > > + > > > + EventLoop::instance()->addTimerEvent(10000, std::bind(&SDLSink::processSDLEvents, this)); > > > > Please use std::chrono types for the addTimerEvent duration parameter. > > Are we sure we want to do this, just for the sake of using the C++ > version? You end up having to cast because it's not the actual type > libevent requires, libevent requires a C timeval to be passed in. > > ../src/cam/event_loop.cpp:105:15: error: assigning to '__suseconds_t' > (aka 'long') from incompatible type 'const std::chrono::microseconds' > (aka 'const duration<long, ratio<1, 1000000>>') > tv.tv_usec = period; That's the whole point of std::chrono. With the existing code it's too easy to write EventLoop::instance()->addTimerEvent(10 /* ms */, std::bind(&SDLSink::processSDLEvents, this)); when you would actually mean EventLoop::instance()->addTimerEvent(10000 /* µs */, std::bind(&SDLSink::processSDLEvents, this)); With std::chrono types, the compiler will accept both EventLoop::instance()->addTimerEvent(10ms, std::bind(&SDLSink::processSDLEvents, this)); and EventLoop::instance()->addTimerEvent(10000µs, std::bind(&SDLSink::processSDLEvents, this)); and do the right thing, and will reject the ambiguous calls EventLoop::instance()->addTimerEvent(10 /* ms */, std::bind(&SDLSink::processSDLEvents, this)); EventLoop::instance()->addTimerEvent(10000 /* µs */, std::bind(&SDLSink::processSDLEvents, this)); > > Unless I'm mistaken, the event will keep being triggered indefinitely, which means > > that SDLSink::processSDLEvents() could call SDL_PollEvent() even after > > stop() calls SDL_Quit(). That seems dangerous to me, I think we need a > > way to cancel periodic timer events. > > Will look into this, this is all ran through valgrind, address > santizers, etc. didn't see anything like that pop up. But will check > again. It could be that the issue is subject to race conditions, or that the event loops is stopped after the sink is stopped without a chance to process more events. This could change in the future, so I'd like to fix this issue already, even if it can't be triggered today. > > > + > > > + return 0; > > > +} > > > + > > > +int SDLSink::stop() > > > +{ > > > + if (sdlTexture_) { > > > + SDL_DestroyTexture(sdlTexture_); > > > + sdlTexture_ = nullptr; > > > + } > > > + > > > + if (sdlRenderer_) { > > > + SDL_DestroyRenderer(sdlRenderer_); > > > + sdlRenderer_ = nullptr; > > > + } > > > + > > > + if (sdlWindow_) { > > > + SDL_DestroyWindow(sdlWindow_); > > > + sdlWindow_ = nullptr; > > > + } > > > + > > > + if (sdlInit_) { > > > + SDL_Quit(); > > > + sdlInit_ = 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 */ > > > > Please move the comment to the next line, and s/click/Click/ > > > > Actually, maybe a switch/case would be better, to prepare for adding > > support for more events ? Up to you. > > I'll leave it as is, if more events prove useful and someone wants to > change to switch in future, I really don't mind. > > > > + EventLoop::instance()->exit(0); > > > + } > > > + } > > > +} > > > + > > > +void SDLSink::renderBuffer(FrameBuffer *buffer) > > > +{ > > > + Image *image = mappedBuffers_[buffer].get(); > > > + > > > + for (unsigned int i = 0; i < buffer->planes().size(); ++i) { > > > + const FrameMetadata::Plane &meta = buffer->metadata().planes()[i]; > > > + > > > + Span<uint8_t> data = image->data(i); > > > + if (meta.bytesused > data.size()) > > > + std::cerr << "payload size " << meta.bytesused > > > + << " larger than plane size " << data.size() > > > + << std::endl; > > > + > > > + SDL_UpdateTexture(sdlTexture_, &sdlRect_, data.data(), pitch_); > > > > According to https://wiki.libsdl.org/SDL_UpdateTexture, > > > > "This is a fairly slow function, intended for use with static textures > > that do not change often. > > > > If the texture is intended to be updated often, it is preferred to > > create the texture as streaming and use the locking functions referenced > > below. While this function will work with streaming textures, for > > optimization reasons you may not get the pixels back if you lock the > > texture afterward." > > > > Have you considered this ? I don't know what difference it makes when > > updating the full texture though, maybe the added complexity isn't worth > > it. > > Will look into this if it's not overly complex will try other > technique suggested in that helpful link you shared. > > > > > > + SDL_RenderClear(sdlRenderer_); > > > + SDL_RenderCopy(sdlRenderer_, sdlTexture_, nullptr, nullptr); > > > + SDL_RenderPresent(sdlRenderer_); > > > > Does this really have to be repeated for each plane ? That sounds > > strange. > > This code is commonly written this way, but that doesn't mean it's > most optimal like you said :) If we take NV12 as an example, the loop will execute twice for the same buffer once for the luma plane and once for the chroma plane. I don't see anything in the SDL calls above that tells SDL about which plane is being updated, which makes me believe there's an issue. > I'll explore other techniques. > > > > + } > > > +} > > > diff --git a/src/cam/sdl_sink.h b/src/cam/sdl_sink.h > > > new file mode 100644 > > > index 00000000..c9b0ab8e > > > --- /dev/null > > > +++ b/src/cam/sdl_sink.h > > > @@ -0,0 +1,46 @@ > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > > +/* > > > + * sdl_sink.h - SDL Sink > > > + */ > > > + > > > +#pragma once > > > + > > > +#include <map> > > > +#include <memory> > > > +#include <string> > > > + > > > +#include <libcamera/stream.h> > > > + > > > +#include <SDL2/SDL.h> > > > + > > > +#include "frame_sink.h" > > > + > > > +class Image; > > > + > > > +class SDLSink : public FrameSink > > > +{ > > > +public: > > > + SDLSink(); > > > + ~SDLSink(); > > > + > > > + int configure(const libcamera::CameraConfiguration &cfg) override; > > > + int start() override; > > > + 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_; > > > + > > > + SDL_Window *sdlWindow_; > > > + SDL_Renderer *sdlRenderer_; > > > + SDL_Texture *sdlTexture_; > > > + SDL_Rect sdlRect_; > > > + SDL_PixelFormatEnum pixelFormat_; > > > + bool sdlInit_; > > > + int pitch_; > > > +};
diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp index 0428b538..30162dbd 100644 --- a/src/cam/camera_session.cpp +++ b/src/cam/camera_session.cpp @@ -19,6 +19,9 @@ #ifdef HAVE_KMS #include "kms_sink.h" #endif +#ifdef HAVE_SDL +#include "sdl_sink.h" +#endif #include "main.h" #include "stream_options.h" @@ -187,6 +190,11 @@ int CameraSession::start() sink_ = std::make_unique<KMSSink>(options_[OptDisplay].toString()); #endif +#ifdef HAVE_SDL + if (options_.isSet(OptSDL)) + sink_ = std::make_unique<SDLSink>(); +#endif + if (options_.isSet(OptFile)) { if (!options_[OptFile].toString().empty()) sink_ = std::make_unique<FileSink>(streamNames_, diff --git a/src/cam/main.cpp b/src/cam/main.cpp index c7f664b9..1d62a64a 100644 --- a/src/cam/main.cpp +++ b/src/cam/main.cpp @@ -137,6 +137,11 @@ int CamApp::parseOptions(int argc, char *argv[]) "Display viewfinder through DRM/KMS on specified connector", "display", ArgumentOptional, "connector", false, OptCamera); +#endif +#ifdef HAVE_SDL + parser.addOption(OptSDL, OptionNone, + "Display viewfinder through SDL", + "sdl", ArgumentNone, "", false, OptCamera); #endif parser.addOption(OptFile, OptionString, "Write captured frames to disk\n" diff --git a/src/cam/main.h b/src/cam/main.h index 62f7bbc9..2b285808 100644 --- a/src/cam/main.h +++ b/src/cam/main.h @@ -18,6 +18,7 @@ enum { OptListProperties = 'p', OptMonitor = 'm', OptStream = 's', + OptSDL = 'S', OptListControls = 256, OptStrictFormats = 257, OptMetadata = 258, diff --git a/src/cam/meson.build b/src/cam/meson.build index 5bab8c9e..59787741 100644 --- a/src/cam/meson.build +++ b/src/cam/meson.build @@ -32,11 +32,21 @@ if libdrm.found() ]) endif +libsdl2 = dependency('SDL2', required : false) + +if libsdl2.found() + cam_cpp_args += ['-DHAVE_SDL'] + cam_sources += files([ + 'sdl_sink.cpp' + ]) +endif + cam = executable('cam', cam_sources, dependencies : [ libatomic, libcamera_public, libdrm, + libsdl2, libevent, ], cpp_args : cam_cpp_args, diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp new file mode 100644 index 00000000..03511974 --- /dev/null +++ b/src/cam/sdl_sink.cpp @@ -0,0 +1,198 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * sdl_sink.cpp - SDL Sink + */ + +#include "sdl_sink.h" + +#include <assert.h> +#include <fcntl.h> +#include <iomanip> +#include <iostream> +#include <signal.h> +#include <sstream> +#include <string.h> +#include <unistd.h> + +#include <libcamera/camera.h> +#include <libcamera/formats.h> + +#include "event_loop.h" +#include "image.h" + +using namespace libcamera; + +SDLSink::SDLSink() + : sdlWindow_(nullptr), sdlRenderer_(nullptr), sdlTexture_(nullptr), sdlRect_({}), sdlInit_(false) +{ +} + +SDLSink::~SDLSink() +{ + stop(); +} + +int SDLSink::configure(const libcamera::CameraConfiguration &cfg) +{ + int ret = FrameSink::configure(cfg); + if (ret < 0) + return ret; + + if (cfg.size() > 1) { + std::cerr << "SDL sink only supports one camera stream at present, streaming first camera stream" + << std::endl; + } else if (cfg.empty()) { + std::cerr << "Require at least one camera stream to process" << std::endl; + return -EINVAL; + } + + const libcamera::StreamConfiguration &sCfg = cfg.at(0); + sdlRect_.w = sCfg.size.width; + sdlRect_.h = sCfg.size.height; + switch (sCfg.pixelFormat) { + case libcamera::formats::YUYV: + pixelFormat_ = SDL_PIXELFORMAT_YUY2; + pitch_ = 4 * ((sdlRect_.w + 1) / 2); + break; + + /* From here down the fourcc values are identical between SDL, drm, libcamera */ + case libcamera::formats::NV21: + pixelFormat_ = SDL_PIXELFORMAT_NV21; + pitch_ = sdlRect_.w; + break; + case libcamera::formats::NV12: + pixelFormat_ = SDL_PIXELFORMAT_NV12; + pitch_ = sdlRect_.w; + break; + case libcamera::formats::YVU420: + pixelFormat_ = SDL_PIXELFORMAT_YV12; + pitch_ = sdlRect_.w; + break; + case libcamera::formats::YVYU: + pixelFormat_ = SDL_PIXELFORMAT_YVYU; + pitch_ = 4 * ((sdlRect_.w + 1) / 2); + break; + case libcamera::formats::UYVY: + pixelFormat_ = SDL_PIXELFORMAT_UYVY; + pitch_ = 4 * ((sdlRect_.w + 1) / 2); + break; + default: + std::cerr << sCfg.pixelFormat.toString() << " not present in libcamera <-> SDL map" + << 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; + } + + sdlInit_ = true; + sdlWindow_ = SDL_CreateWindow("", SDL_WINDOWPOS_UNDEFINED, SDL_WINDOWPOS_UNDEFINED, sdlRect_.w, sdlRect_.h, SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE); + if (!sdlWindow_) { + std::cerr << "Failed to create SDL window: " << SDL_GetError() << std::endl; + return -EINVAL; + } + + sdlRenderer_ = SDL_CreateRenderer(sdlWindow_, -1, 0); + if (!sdlRenderer_) { + std::cerr << "Failed to create SDL renderer: " << SDL_GetError() << std::endl; + return -EINVAL; + } + + ret = SDL_RenderSetLogicalSize(sdlRenderer_, sdlRect_.w, sdlRect_.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; + } + + sdlTexture_ = SDL_CreateTexture(sdlRenderer_, pixelFormat_, SDL_TEXTUREACCESS_STREAMING, sdlRect_.w, sdlRect_.h); + if (!sdlTexture_) { + std::cerr << "Failed to create SDL texture: " << SDL_GetError() << std::endl; + return -EINVAL; + } + + EventLoop::instance()->addTimerEvent(10000, std::bind(&SDLSink::processSDLEvents, this)); + + return 0; +} + +int SDLSink::stop() +{ + if (sdlTexture_) { + SDL_DestroyTexture(sdlTexture_); + sdlTexture_ = nullptr; + } + + if (sdlRenderer_) { + SDL_DestroyRenderer(sdlRenderer_); + sdlRenderer_ = nullptr; + } + + if (sdlWindow_) { + SDL_DestroyWindow(sdlWindow_); + sdlWindow_ = nullptr; + } + + if (sdlInit_) { + SDL_Quit(); + sdlInit_ = 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(); + + for (unsigned int i = 0; i < buffer->planes().size(); ++i) { + const FrameMetadata::Plane &meta = buffer->metadata().planes()[i]; + + Span<uint8_t> data = image->data(i); + if (meta.bytesused > data.size()) + std::cerr << "payload size " << meta.bytesused + << " larger than plane size " << data.size() + << std::endl; + + SDL_UpdateTexture(sdlTexture_, &sdlRect_, data.data(), pitch_); + SDL_RenderClear(sdlRenderer_); + SDL_RenderCopy(sdlRenderer_, sdlTexture_, nullptr, nullptr); + SDL_RenderPresent(sdlRenderer_); + } +} diff --git a/src/cam/sdl_sink.h b/src/cam/sdl_sink.h new file mode 100644 index 00000000..c9b0ab8e --- /dev/null +++ b/src/cam/sdl_sink.h @@ -0,0 +1,46 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * sdl_sink.h - SDL Sink + */ + +#pragma once + +#include <map> +#include <memory> +#include <string> + +#include <libcamera/stream.h> + +#include <SDL2/SDL.h> + +#include "frame_sink.h" + +class Image; + +class SDLSink : public FrameSink +{ +public: + SDLSink(); + ~SDLSink(); + + int configure(const libcamera::CameraConfiguration &cfg) override; + int start() override; + 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_; + + SDL_Window *sdlWindow_; + SDL_Renderer *sdlRenderer_; + SDL_Texture *sdlTexture_; + SDL_Rect sdlRect_; + SDL_PixelFormatEnum pixelFormat_; + bool sdlInit_; + int pitch_; +};
This adds more portability to existing cam sinks. You can pass a YUYV camera buffer for example and SDL will handle the pixel buffer conversion, although SDL does not support decompression for pixelformats like MJPEG. This allows cam reference implementation to display images on VMs, Mac M1, Raspberry Pi, etc. This also enables cam reference implementation, to run as a desktop application in wayland or x11. SDL also has support for Android and ChromeOS which 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> --- src/cam/camera_session.cpp | 8 ++ src/cam/main.cpp | 5 + src/cam/main.h | 1 + src/cam/meson.build | 10 ++ src/cam/sdl_sink.cpp | 198 +++++++++++++++++++++++++++++++++++++ src/cam/sdl_sink.h | 46 +++++++++ 6 files changed, 268 insertions(+) create mode 100644 src/cam/sdl_sink.cpp create mode 100644 src/cam/sdl_sink.h