[libcamera-devel] [PATCH v7 2/2] cam: sdl_sink: Add SDL sink
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Apr 15 19:13:11 CEST 2022
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 at 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_;
> +};
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list