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