[libcamera-devel] [PATCH v8 3/4] cam: sdl_sink: Add SDL sink with initial YUYV support
Eric Curtin
ecurtin at redhat.com
Fri May 20 16:58:20 CEST 2022
On Thu, 19 May 2022 at 17:22, Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> Quoting Laurent Pinchart (2022-05-18 21:06:10)
> > Hi Eric,
> >
> > Thank you for the patch.
> >
> > On Wed, May 18, 2022 at 12:38:20AM +0100, Kieran Bingham via libcamera-devel wrote:
> > > Quoting Eric Curtin via libcamera-devel (2022-05-05 16:18:50)
> > > > This adds more portability to existing cam sinks. You can pass a
> > > > YUYV camera buffer and SDL will handle the pixel buffer conversion
> > > > to the display. This allows cam reference implementation to display
> > > > images on VMs, Mac M1, Raspberry Pi, etc. This also enables cam
> > > > reference implementation, to run as a desktop application in wayland or
> >
> > s/wayland/Wayland/
> >
> > > > x11. SDL also has support for Android and ChromeOS which has not been
> >
> > s/x11/X11/
> >
> > > > tested. Also tested on simpledrm raspberry pi 4 framebuffer
> >
> > s/raspberry pi/Raspberry Pi/
> >
> > > > 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 | 4 +
> > > > src/cam/main.h | 1 +
> > > > src/cam/meson.build | 12 +++
> > > > src/cam/sdl_sink.cpp | 192 +++++++++++++++++++++++++++++++++++
> > > > src/cam/sdl_sink.h | 48 +++++++++
> > > > src/cam/sdl_texture.cpp | 32 ++++++
> > > > src/cam/sdl_texture.h | 23 +++++
> > > > src/cam/sdl_texture_yuyv.cpp | 13 +++
> > > > src/cam/sdl_texture_yuyv.h | 10 ++
> > >
> > > Given the number of sdl_ files here I wonder if they should be under
> > > src/cam/sdl/....
> > >
> > > But not crucial to this patch.
> > >
> > > I can't test this right now - but I'm looking forward to seeing a
> > > preview on cam ... so I'll hopefully test this tomorrow.
> > >
> > > > 10 files changed, 343 insertions(+)
> > > > create mode 100644 src/cam/sdl_sink.cpp
> > > > create mode 100644 src/cam/sdl_sink.h
> > > > create mode 100644 src/cam/sdl_texture.cpp
> > > > create mode 100644 src/cam/sdl_texture.h
> > > > create mode 100644 src/cam/sdl_texture_yuyv.cpp
> > > > create mode 100644 src/cam/sdl_texture_yuyv.h
> > > >
> > > > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp
> > > > index efffafbf..71e6bd60 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
> >
> > Please move this below in alphabetical order.
> >
> > > > #include "main.h"
> > > > #include "stream_options.h"
> > > >
> > > > @@ -186,6 +189,11 @@ int CameraSession::start()
> > > > sink_ = std::make_unique<KMSSink>(options_[OptDisplay].toString());
> > > > #endif
> > > >
> > > > +#ifdef HAVE_SDL
> > > > + if (options_.isSet(OptSDL))
> > > > + sink_ = std::make_unique<SDLSink>();
> > > > +#endif
> > > > +
> > >
> > > It looks like the precedence of which sink really gets used if multiples
> > > are set is simply the order in this file that they get set. But I think
> > > that's ok for the moment. Not sure we could do much else right now
> > > without extending to support multiple sinks which is out of scope here.
> > >
> > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > >
> > >
> > > > 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..fd3108b0 100644
> > > > --- a/src/cam/main.cpp
> > > > +++ b/src/cam/main.cpp
> > > > @@ -137,6 +137,10 @@ 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
> >
> > I wonder if we should merge all sink arguments into one, but that can be
> > done on top (the cam arguments syntax isn't stable yet).
> >
> > > > 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..3032730b 100644
> > > > --- a/src/cam/meson.build
> > > > +++ b/src/cam/meson.build
> > > > @@ -32,11 +32,23 @@ if libdrm.found()
> > > > ])
> > > > endif
> > > >
> > > > +libsdl2 = dependency('SDL2', required : false)
>
> I installed libsdl2-dev but got failures around SDL2/SDL_image.h
>
> On ubuntu I installed libsdl2-image-dev, and then rebuilt. That found
> the header, but then caused a link error...
What version of Ubuntu? Fedora tends to be my primary build machine.
It was intended that patch 3 was only the SDL core library dependant
code and patch 4 was the SDL image dependant code for MJPG.
>
> Does this also need to have some 'SDL2-Image' dependency check?
>
>
> > > > +
> > > > +if libsdl2.found()
> > > > + cam_cpp_args += ['-DHAVE_SDL']
> > > > + cam_sources += files([
> > > > + 'sdl_sink.cpp',
> > > > + 'sdl_texture.cpp',
> > > > + 'sdl_texture_yuyv.cpp'
> > > > + ])
> > > > +endif
> > > > +
> > > > cam = executable('cam', cam_sources,
> > > > dependencies : [
> > > > libatomic,
> > > > libcamera_public,
> > > > libdrm,
> > > > + libsdl2,
> >
> > Please keep the list alphabetically ordered.
> >
> > > > 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..6fbeaf56
> > > > --- /dev/null
> > > > +++ b/src/cam/sdl_sink.cpp
> > > > @@ -0,0 +1,192 @@
> > > > +/* 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 "sdl_texture_yuyv.h"
> > > > +
> > > > +#include "event_loop.h"
> > > > +#include "image.h"
> >
> > I don't think there's a need to split sdl_texture_yuyv.h and the next
> > two headers in two groups.
> >
> > > > +
> > > > +using namespace libcamera;
> > > > +
> > > > +SDLSink::SDLSink()
> > > > + : sdlWindow_(nullptr), sdlRenderer_(nullptr), sdlRect_({}),
> > > > + sdlInit_(false)
> > > > +{
> > > > +}
> > > > +
> > > > +SDLSink::~SDLSink()
> > > > +{
> > > > + stop();
> > > > +}
> > > > +
> > > > +int SDLSink::configure(const libcamera::CameraConfiguration &config)
> > > > +{
> > > > + int ret = FrameSink::configure(config);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > +
> > > > + if (config.size() > 1) {
> > > > + std::cerr
> > > > + << "SDL sink only supports one camera stream at present, streaming first camera stream"
> > > > + << std::endl;
> > > > + } else if (config.empty()) {
> > > > + std::cerr << "Require at least one camera stream to process"
> > > > + << std::endl;
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + const libcamera::StreamConfiguration &cfg = config.at(0);
> > > > + sdlRect_.w = cfg.size.width;
> > > > + sdlRect_.h = cfg.size.height;
> > > > +
> > > > + switch (cfg.pixelFormat) {
> > > > + case libcamera::formats::YUYV:
> > > > + sdlTexture_ = std::make_unique<SDLTextureYUYV>(sdlRect_);
> > > > + break;
> > > > + default:
> > > > + std::cerr << "Unsupported pixel format "
> > > > + << cfg.pixelFormat.toString() << std::endl;
> > > > + return -EINVAL;
> > > > + };
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +int SDLSink::start()
> > > > +{
> > > > + int ret = SDL_Init(SDL_INIT_VIDEO);
> > > > + if (ret) {
> > > > + std::cerr << "Failed to initialize SDL: " << SDL_GetError()
> > > > + << std::endl;
> > > > + return ret;
> > > > + }
> > > > +
> > > > + 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;
> > > > + }
> >
> > /*
> > * Not critical to set, don't return in this case, set for scaling
> > * purposes.
> > */
> > if (ret)
> > std::cerr << "Failed to set SDL render logical size: "
> > << SDL_GetError() << std::endl;
> >
> > > > +
> > > > + ret = sdlTexture_->create(sdlRenderer_);
> > > > + if (ret) {
> > > > + return ret;
> > > > + }
> > > > +
> > > > + EventLoop::instance()->addTimerEvent(
> > > > + 10ms, std::bind(&SDLSink::processSDLEvents, this));
> >
> > I think I mentioned before that this won't work nicely if we stop and
> > restart. cam doesn't do so at the moment, so it doesn't have to be fixed
> > yet, but a todo comment would be good:
> >
> > /* \todo Make the event cancellable to support stop/start cycles. */
> > EventLoop::instance()->addTimerEvent(
> > 10ms, std::bind(&SDLSink::processSDLEvents, this));
> >
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +int SDLSink::stop()
> > > > +{
> > > > + if (sdlTexture_) {
> > > > + sdlTexture_->destroy();
> >
> > Can you turn SDLTexture::destroy() into a destructor ? Then you could
> > simply write
> >
> > sdlTexture_ = nullptr;
> >
> > or
> >
> > sdlTexture_.reset();
> >
> > > > + 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;
> > > > +
> > > > + sdlTexture_->update(data);
> >
> > I really don't think this will do what you expect for multi-planar
> > formats. As multi-planar formats are not supported yet, I'd replace the
> > loop by a single pass over plane 0, and add a
> >
> > /* \todo Implement support for multi-planar formats. */
> >
> > > > + }
> > > > +
> > > > + SDL_RenderClear(sdlRenderer_);
> > > > + SDL_RenderCopy(sdlRenderer_, sdlTexture_->ptr_, 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..37dd9f7e
> > > > --- /dev/null
> > > > +++ b/src/cam/sdl_sink.h
> > > > @@ -0,0 +1,48 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > > +/*
> > > > + * sdl_sink.h - SDL Sink
> > > > + */
> > > > +
> > > > +#pragma once
> > > > +
> > > > +#include <map>
> > > > +#include <memory>
> > > > +#include <string>
> >
> > I think you can drop the string header.
> >
> > > > +
> > > > +#include <libcamera/stream.h>
> > > > +
> > > > +#include <SDL2/SDL.h>
> > > > +
> > > > +#include "frame_sink.h"
> > > > +#include "sdl_texture.h"
> >
> > You could also forward-declare the SDLTexture class and drop this
> > header.
> >
> > > > +
> > > > +class Image;
> > > > +
> > > > +class SDLSink : public FrameSink
> > > > +{
> > > > +public:
> > > > + SDLSink();
> > > > + ~SDLSink();
> > > > +
> > > > + int configure(const libcamera::CameraConfiguration &config) override;
> > > > + int start() override;
> > > > + int stop() override;
> > > > + void mapBuffer(libcamera::FrameBuffer *buffer) override;
> > > > +
> > > > + bool processRequest(libcamera::Request *request) override;
> > > > +
> > > > +private:
> > > > + void renderBuffer(libcamera::FrameBuffer *buffer);
> > > > + void processSDLEvents();
> > > > +
> > > > + std::map<libcamera::FrameBuffer *, std::unique_ptr<Image>>
> > > > + mappedBuffers_;
> > > > +
> > > > + std::unique_ptr<SDLTexture> sdlTexture_;
> > > > +
> > > > + SDL_Window *sdlWindow_;
> > > > + SDL_Renderer *sdlRenderer_;
> > > > + SDL_Rect sdlRect_;
> >
> > I'm tempted to drop the sdl prefix from variable names, up to you.
> >
> > > > + SDL_PixelFormatEnum pixelFormat_;
> > > > + bool sdlInit_;
> > > > +};
> > > > diff --git a/src/cam/sdl_texture.cpp b/src/cam/sdl_texture.cpp
> > > > new file mode 100644
> > > > index 00000000..b39080c9
> > > > --- /dev/null
> > > > +++ b/src/cam/sdl_texture.cpp
> > > > @@ -0,0 +1,32 @@
> >
> > Missing SPDX tag and copyright header. Same below.
> >
> > > > +#include "sdl_texture.h"
> > > > +
> > > > +#include <iostream>
> > > > +
> > > > +SDLTexture::SDLTexture(const SDL_Rect &sdlRect, SDL_PixelFormatEnum pixelFormat,
> > > > + const int pitch)
> > > > + : sdlRect_(sdlRect), pixelFormat_(pixelFormat), pitch_(pitch)
> > > > +{
> > > > +}
> > > > +
> > > > +SDLTexture::~SDLTexture()
> > > > +{
> > > > +}
> > > > +
> > > > +int SDLTexture::create(SDL_Renderer *sdlRenderer_)
> > > > +{
> > > > + ptr_ = SDL_CreateTexture(sdlRenderer_, pixelFormat_,
> > > > + SDL_TEXTUREACCESS_STREAMING, sdlRect_.w,
> > > > + sdlRect_.h);
> > > > + if (!ptr_) {
> > > > + std::cerr << "Failed to create SDL texture: " << SDL_GetError()
> > > > + << std::endl;
> > > > + return -ENOMEM;
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +void SDLTexture::destroy()
> > > > +{
> > > > + SDL_DestroyTexture(ptr_);
> > > > +}
> > > > diff --git a/src/cam/sdl_texture.h b/src/cam/sdl_texture.h
> > > > new file mode 100644
> > > > index 00000000..f9525f7f
> > > > --- /dev/null
> > > > +++ b/src/cam/sdl_texture.h
> > > > @@ -0,0 +1,23 @@
> > > > +#pragma once
> > > > +
> > > > +#include <SDL2/SDL.h>
> > > > +
> > > > +#include "image.h"
> > > > +
> > > > +class SDLTexture
> > > > +{
> > > > +public:
> > > > + SDLTexture(const SDL_Rect &sdlRect, SDL_PixelFormatEnum pixelFormat,
> > > > + const int pitch);
> > > > + virtual ~SDLTexture();
> > > > + int create(SDL_Renderer *sdlRenderer_);
> >
> > s/sdlRenderer_/sdlRenderer/
> >
> > And same here (and with sdlRect in the previous function) about the sdl
> > prefix.
> >
> > > > + void destroy();
> > > > + virtual void update(const libcamera::Span<uint8_t> &data) = 0;
> > > > +
> > > > + SDL_Texture *ptr_;
> >
> > Is it worth making this protected and adding an accessor function ?
> >
> > > > +
> > > > +protected:
> > > > + const SDL_Rect &sdlRect_;
> >
> > Same here, rect_.
> >
> > > > + SDL_PixelFormatEnum pixelFormat_;
> > > > + const int pitch_;
> > > > +};
> > > > diff --git a/src/cam/sdl_texture_yuyv.cpp b/src/cam/sdl_texture_yuyv.cpp
> > > > new file mode 100644
> > > > index 00000000..cf519a5d
> > > > --- /dev/null
> > > > +++ b/src/cam/sdl_texture_yuyv.cpp
> > > > @@ -0,0 +1,13 @@
> > > > +#include "sdl_texture_yuyv.h"
> > > > +
> > > > +using namespace libcamera;
> > > > +
> > > > +SDLTextureYUYV::SDLTextureYUYV(const SDL_Rect &sdlRect)
> > > > + : SDLTexture(sdlRect, SDL_PIXELFORMAT_YUY2, 4 * ((sdlRect.w + 1) / 2))
> > > > +{
> > > > +}
> > > > +
> > > > +void SDLTextureYUYV::update(const Span<uint8_t> &data)
> > > > +{
> > > > + SDL_UpdateTexture(ptr_, &sdlRect_, data.data(), pitch_);
> > > > +}
> > > > diff --git a/src/cam/sdl_texture_yuyv.h b/src/cam/sdl_texture_yuyv.h
> > > > new file mode 100644
> > > > index 00000000..ad70f14f
> > > > --- /dev/null
> > > > +++ b/src/cam/sdl_texture_yuyv.h
> > > > @@ -0,0 +1,10 @@
> > > > +#pragma once
> > > > +
> > > > +#include "sdl_texture.h"
> > > > +
> > > > +class SDLTextureYUYV : public SDLTexture
> > > > +{
> > > > +public:
> > > > + SDLTextureYUYV(const SDL_Rect &sdlRect);
> > > > + void update(const libcamera::Span<uint8_t> &data) override;
> > > > +};
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
>
More information about the libcamera-devel
mailing list