[libcamera-devel] [PATCH v2] cam: sdl_sink: Use libjpeg over SDL2_image
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Jul 17 22:41:25 CEST 2022
Hi Eric,
Thank you for the patch.
On Fri, Jul 15, 2022 at 12:36:05PM +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 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/meson.build | 8 ++---
> src/cam/sdl_sink.cpp | 4 +--
> src/cam/sdl_texture_mjpg.cpp | 63 ++++++++++++++++++++++++++++++++----
> src/cam/sdl_texture_mjpg.h | 6 ++++
> 5 files changed, 70 insertions(+), 13 deletions(-)
>
> 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/meson.build b/src/cam/meson.build
> index 5957ce14..4dfa7b22 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,8 +43,8 @@ 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([
> 'sdl_texture_mjpg.cpp'
> ])
> @@ -57,8 +57,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..a064e6e5 100644
> --- a/src/cam/sdl_texture_mjpg.cpp
> +++ b/src/cam/sdl_texture_mjpg.cpp
> @@ -7,19 +7,70 @@
>
> #include "sdl_texture_mjpg.h"
>
> -#include <SDL2/SDL_image.h>
> +#include <iostream>
> +#include <setjmp.h>
> +
> +#include <jpeglib.h>
>
> 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))
> +{
> +}
> +
> +struct error_mgr {
> + struct jpeg_error_mgr errmgr;
> + jmp_buf escape;
> +};
> +
> +static void error_exit(j_common_ptr cinfo)
This function should be marked with [[noreturn]].
> +{
> + struct error_mgr *err = (struct error_mgr *)cinfo->err;
struct error_mgr *err = static_cast<struct error_mgr *>(cinfo->err);
> + longjmp(err->escape, 1);
> +}
> +
> +static void output_no_message(j_common_ptr cinfo)
You can write
static void output_no_message([[maybed_unused]] j_common_ptr cinfo)
> +{
> + (void)cinfo;
and drop this.
> +}
How about grouping the structure and functions together, as we use C++ ?
struct JpegErrorManager {
JpegErrorManager()
{
jpeg_std_error(&errmgr);
errmgr.error_exit = errorExit;
errmgr.output_message = outputMessage;
}
[[noreturn]] static void errorExit(j_common_ptr cinfo)
{
JpegErrorManager *self = static_cast<JpegErrorManager *>(cinfo->err);
longjmp(self->escape, 1);
}
static void outputMessage([[maybed_unused]] j_common_ptr cinfo)
{
}
struct jpeg_error_mgr errmgr;
jmp_buf escape;
}
I think that's longjmp-safe as there's nothing to do at destruction
time.
> +
> +void SDLTextureMJPG::decompress(const unsigned char *jpeg,
> + unsigned long jpeg_size)
The function should take a Span<const uint8_t> instead of separate data
pointer and size.
> {
> + struct error_mgr jerr;
> + struct jpeg_decompress_struct cinfo;
> + cinfo.err = jpeg_std_error(&jerr.errmgr);
> + jerr.errmgr.error_exit = error_exit;
> + jerr.errmgr.output_message = output_no_message;
This would become
struct jpeg_decompress_struct cinfo;
JpegErrorManager jerr;
cinfo.err = &jerr.errmgr;
And I'd add a blank line here for clarity.
If you'd rather avoid the JpegErrorManager, I would write
struct jpeg_decompress_struct cinfo;
struct error_mgr jerr;
jpeg_std_error(&jerr.errmgr);
jerr.errmgr.error_exit = error_exit;
jerr.errmgr.output_message = output_no_message;
if (setjmp(jerr.escape)) {
...
}
cinfo.err = &jerr.errmgr;
> + if (setjmp(jerr.escape)) {
> + /* libjpeg found an error */
> + jpeg_destroy_decompress(&cinfo);
> + std::cerr << "JPEG decompression error" << std::endl;
> + return;
Let's return an error from this function.
> + }
> +
> + jpeg_create_decompress(&cinfo);
The decompressor and the error manager don't need to be recreated for
each image. Optimizing this would require substantial changes though, as
we would need to create the JPEG encoder separately from the texture, so
this can be done later.
> +
> + jpeg_mem_src(&cinfo, jpeg, jpeg_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);
> }
>
> 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.data(), data.size());
What should we do here if an error occurred during decompression ? We
could proceed nonetheless (which will result in some garbage being
displayed), or return an error from this function and skip the
SDL_Render* calls in the caller. What do you think would be best ?
> + 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..c68b03b0 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:
> + void decompress(const unsigned char *jpeg, unsigned long jpeg_size);
> +
> + std::unique_ptr<unsigned char[]> rgb_;
> };
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list