[libcamera-devel] [PATCH v4] cam: sdl_sink: Use libjpeg over SDL2_image
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jul 20 13:55:22 CEST 2022
Hi Eric,
Thank you for the patch.
On Tue, Jul 19, 2022 at 10:17:46AM +0100, Eric Curtin wrote:
> We were using the libjpeg functionality of SDL2_image only, instead just
> use libjpeg directly to reduce our dependancy count, it is a more
> commonly available library.
>
> Signed-off-by: Eric Curtin <ecurtin at redhat.com>
> ---
> Changes in v4:
> - decompress function takes a Span<const uint8_t>&
>
> Changes in v3:
> - create JpegErrorManager struct
> - change c cast to reinterpret_cast
>
> Changes in v2:
> - alphabetically sorted various orderings
> - pitch_ is const again
> - added setjmp logic for error handling in libjpeg
> - rgbdata_ to unique_ptr and renamed to rgb_
> - removed a copy from buffer to rgb_
> - removed a destructor
> ---
> README.rst | 2 +-
> src/cam/jpeg_error_manager.cpp | 26 ++++++++++++++++++++
> src/cam/jpeg_error_manager.h | 21 ++++++++++++++++
> src/cam/meson.build | 9 +++----
> src/cam/sdl_sink.cpp | 4 ++--
> src/cam/sdl_texture_mjpg.cpp | 44 +++++++++++++++++++++++++++++-----
> src/cam/sdl_texture_mjpg.h | 6 +++++
> 7 files changed, 99 insertions(+), 13 deletions(-)
> create mode 100644 src/cam/jpeg_error_manager.cpp
> create mode 100644 src/cam/jpeg_error_manager.h
>
> diff --git a/README.rst b/README.rst
> index b9e72d81..47b914f0 100644
> --- a/README.rst
> +++ b/README.rst
> @@ -92,8 +92,8 @@ for cam: [optional]
> tool:
>
> - libdrm-dev: Enables the KMS sink
> + - libjpeg-dev: Enables MJPEG on the SDL sink
> - libsdl2-dev: Enables the SDL sink
> - - libsdl2-image-dev: Supports MJPEG on the SDL sink
>
> for qcam: [optional]
> qtbase5-dev libqt5core5a libqt5gui5 libqt5widgets5 qttools5-dev-tools libtiff-dev
> diff --git a/src/cam/jpeg_error_manager.cpp b/src/cam/jpeg_error_manager.cpp
> new file mode 100644
> index 00000000..7e45e577
> --- /dev/null
> +++ b/src/cam/jpeg_error_manager.cpp
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2022, Ideas on Board Oy
> + *
> + * jpeg_error_manager.cpp - JPEG Error Manager
> + */
> +
> +#include "jpeg_error_manager.h"
> +
> +static void errorExit(j_common_ptr cinfo)
> +{
> + JpegErrorManager *self =
> + reinterpret_cast<JpegErrorManager *>(cinfo->err);
> + longjmp(self->escape_, 1);
> +}
> +
> +static void outputMessage([[maybe_unused]] j_common_ptr cinfo)
> +{
> +}
> +
> +JpegErrorManager::JpegErrorManager(struct jpeg_decompress_struct &cinfo)
I wouldn't mix the error manager and decompressor. You can keep the
jpeg_std_error() call below, and move the assignment of cinfo.err to the
code that creates the decompressor.
> +{
> + cinfo.err = jpeg_std_error(&errmgr_);
> + errmgr_.error_exit = errorExit;
> + errmgr_.output_message = outputMessage;
> +}
This can live in sdl_texture_mjpg.cpp as it's internal to the
implementation of the SDL texture for MJPEG, there's no need to have one
file per class.
> diff --git a/src/cam/jpeg_error_manager.h b/src/cam/jpeg_error_manager.h
> new file mode 100644
> index 00000000..0af28da1
> --- /dev/null
> +++ b/src/cam/jpeg_error_manager.h
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2022, Ideas on Board Oy
> + *
> + * jpeg_error_manager.h - JPEG Error Manager
> + */
> +
> +#pragma once
> +
> +#include <setjmp.h>
> +#include <stdio.h>
> +
> +#include <jpeglib.h>
> +
> +struct JpegErrorManager {
> + JpegErrorManager(struct jpeg_decompress_struct &cinfo);
> +
> + /* Order very important for reinterpret_cast */
> + struct jpeg_error_mgr errmgr_;
> + jmp_buf escape_;
> +};
> diff --git a/src/cam/meson.build b/src/cam/meson.build
> index 5957ce14..86cef683 100644
> --- a/src/cam/meson.build
> +++ b/src/cam/meson.build
> @@ -24,8 +24,8 @@ cam_sources = files([
> cam_cpp_args = []
>
> libdrm = dependency('libdrm', required : false)
> +libjpeg = dependency('libjpeg', required : false)
> libsdl2 = dependency('SDL2', required : false)
> -libsdl2_image = dependency('SDL2_image', required : false)
>
> if libdrm.found()
> cam_cpp_args += [ '-DHAVE_KMS' ]
> @@ -43,9 +43,10 @@ if libsdl2.found()
> 'sdl_texture_yuyv.cpp'
> ])
>
> - if libsdl2_image.found()
> - cam_cpp_args += ['-DHAVE_SDL_IMAGE']
> + if libjpeg.found()
> + cam_cpp_args += ['-DHAVE_LIBJPEG']
> cam_sources += files([
> + 'jpeg_error_manager.cpp',
> 'sdl_texture_mjpg.cpp'
> ])
> endif
> @@ -57,8 +58,8 @@ cam = executable('cam', cam_sources,
> libcamera_public,
> libdrm,
> libevent,
> + libjpeg,
> libsdl2,
> - libsdl2_image,
> libyaml,
> ],
> cpp_args : cam_cpp_args,
> diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp
> index f8e3e95d..19fdfd6d 100644
> --- a/src/cam/sdl_sink.cpp
> +++ b/src/cam/sdl_sink.cpp
> @@ -21,7 +21,7 @@
>
> #include "event_loop.h"
> #include "image.h"
> -#ifdef HAVE_SDL_IMAGE
> +#ifdef HAVE_LIBJPEG
> #include "sdl_texture_mjpg.h"
> #endif
> #include "sdl_texture_yuyv.h"
> @@ -62,7 +62,7 @@ int SDLSink::configure(const libcamera::CameraConfiguration &config)
> rect_.h = cfg.size.height;
>
> switch (cfg.pixelFormat) {
> -#ifdef HAVE_SDL_IMAGE
> +#ifdef HAVE_LIBJPEG
> case libcamera::formats::MJPEG:
> texture_ = std::make_unique<SDLTextureMJPG>(rect_);
> break;
> diff --git a/src/cam/sdl_texture_mjpg.cpp b/src/cam/sdl_texture_mjpg.cpp
> index 69e99ad3..22f570c6 100644
> --- a/src/cam/sdl_texture_mjpg.cpp
> +++ b/src/cam/sdl_texture_mjpg.cpp
> @@ -7,19 +7,51 @@
>
> #include "sdl_texture_mjpg.h"
>
> -#include <SDL2/SDL_image.h>
> +#include "jpeg_error_manager.h"
> +
> +#include <iostream>
>
> using namespace libcamera;
>
> SDLTextureMJPG::SDLTextureMJPG(const SDL_Rect &rect)
> - : SDLTexture(rect, SDL_PIXELFORMAT_RGB24, 0)
> + : SDLTexture(rect, SDL_PIXELFORMAT_RGB24, rect.w * 3),
> + rgb_(std::make_unique<unsigned char[]>(pitch_ * rect.h))
> +{
> +}
> +
> +int SDLTextureMJPG::decompress(const Span<uint8_t> &data)
int SDLTextureMJPG::decompress(Span<const uint8_t> data)
for two reasons:
- a Span is a pointer + size, which will fit in registers when calling
this function, so that should be more efficient than passing a pointer
to the Span
- Span<const uint8_t> to indicate that the function doesn't modify the
data. A const Span<uint8_t> prevents changing the Span itself, but
allows modifying the data that the Span points to.
This would however require changing the update() function accordingly,
so I'll submit a patch on top.
I've made those modifications locally to test and make sure I wasn't
saying something stupid, so I'll send a v5 shortly as it's ready.
> {
> + struct jpeg_decompress_struct cinfo;
> + JpegErrorManager jpegErrorManager(cinfo);
> + if (setjmp(jpegErrorManager.escape_)) {
> + /* libjpeg found an error */
> + jpeg_destroy_decompress(&cinfo);
> + std::cerr << "JPEG decompression error" << std::endl;
> + return -EINVAL;
> + }
> +
> + jpeg_create_decompress(&cinfo);
> +
> + jpeg_mem_src(&cinfo, data.data(), data.size());
> +
> + jpeg_read_header(&cinfo, TRUE);
> +
> + jpeg_start_decompress(&cinfo);
> +
> + for (int i = 0; cinfo.output_scanline < cinfo.output_height; ++i) {
> + JSAMPROW rowptr = rgb_.get() + i * pitch_;
> + jpeg_read_scanlines(&cinfo, &rowptr, 1);
> + }
> +
> + jpeg_finish_decompress(&cinfo);
> +
> + jpeg_destroy_decompress(&cinfo);
> +
> + return 0;
> }
>
> void SDLTextureMJPG::update(const Span<uint8_t> &data)
> {
> - SDL_RWops *bufferStream = SDL_RWFromMem(data.data(), data.size());
> - SDL_Surface *frame = IMG_Load_RW(bufferStream, 0);
> - SDL_UpdateTexture(ptr_, nullptr, frame->pixels, frame->pitch);
> - SDL_FreeSurface(frame);
> + decompress(data);
> + SDL_UpdateTexture(ptr_, nullptr, rgb_.get(), pitch_);
> }
> diff --git a/src/cam/sdl_texture_mjpg.h b/src/cam/sdl_texture_mjpg.h
> index b103f801..328c45a9 100644
> --- a/src/cam/sdl_texture_mjpg.h
> +++ b/src/cam/sdl_texture_mjpg.h
> @@ -13,5 +13,11 @@ class SDLTextureMJPG : public SDLTexture
> {
> public:
> SDLTextureMJPG(const SDL_Rect &rect);
> +
> void update(const libcamera::Span<uint8_t> &data) override;
> +
> +private:
> + int decompress(const libcamera::Span<uint8_t> &data);
> +
> + std::unique_ptr<unsigned char[]> rgb_;
> };
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list