[{"id":24004,"web_url":"https://patchwork.libcamera.org/comment/24004/","msgid":"","date":"2022-07-20T11:55:22","subject":"Re: [libcamera-devel] [PATCH v4] cam: sdl_sink: Use libjpeg over\n\tSDL2_image","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Eric,\n\nThank you for the patch.\n\nOn Tue, Jul 19, 2022 at 10:17:46AM +0100, Eric Curtin wrote:\n> We were using the libjpeg functionality of SDL2_image only, instead just\n> use libjpeg directly to reduce our dependancy count, it is a more\n> commonly available library.\n> \n> Signed-off-by: Eric Curtin \n> ---\n> Changes in v4:\n> - decompress function takes a Span&\n> \n> Changes in v3:\n> - create JpegErrorManager struct\n> - change c cast to reinterpret_cast\n> \n> Changes in v2:\n> - alphabetically sorted various orderings\n> - pitch_ is const again\n> - added setjmp logic for error handling in libjpeg\n> - rgbdata_ to unique_ptr and renamed to rgb_\n> - removed a copy from buffer to rgb_\n> - removed a destructor\n> ---\n> README.rst | 2 +-\n> src/cam/jpeg_error_manager.cpp | 26 ++++++++++++++++++++\n> src/cam/jpeg_error_manager.h | 21 ++++++++++++++++\n> src/cam/meson.build | 9 +++----\n> src/cam/sdl_sink.cpp | 4 ++--\n> src/cam/sdl_texture_mjpg.cpp | 44 +++++++++++++++++++++++++++++-----\n> src/cam/sdl_texture_mjpg.h | 6 +++++\n> 7 files changed, 99 insertions(+), 13 deletions(-)\n> create mode 100644 src/cam/jpeg_error_manager.cpp\n> create mode 100644 src/cam/jpeg_error_manager.h\n> \n> diff --git a/README.rst b/README.rst\n> index b9e72d81..47b914f0 100644\n> --- a/README.rst\n> +++ b/README.rst\n> @@ -92,8 +92,8 @@ for cam: [optional]\n> tool:\n> \n> - libdrm-dev: Enables the KMS sink\n> + - libjpeg-dev: Enables MJPEG on the SDL sink\n> - libsdl2-dev: Enables the SDL sink\n> - - libsdl2-image-dev: Supports MJPEG on the SDL sink\n> \n> for qcam: [optional]\n> qtbase5-dev libqt5core5a libqt5gui5 libqt5widgets5 qttools5-dev-tools libtiff-dev\n> diff --git a/src/cam/jpeg_error_manager.cpp b/src/cam/jpeg_error_manager.cpp\n> new file mode 100644\n> index 00000000..7e45e577\n> --- /dev/null\n> +++ b/src/cam/jpeg_error_manager.cpp\n> @@ -0,0 +1,26 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2022, Ideas on Board Oy\n> + *\n> + * jpeg_error_manager.cpp - JPEG Error Manager\n> + */\n> +\n> +#include \"jpeg_error_manager.h\"\n> +\n> +static void errorExit(j_common_ptr cinfo)\n> +{\n> +\tJpegErrorManager *self =\n> +\t\treinterpret_cast(cinfo->err);\n> +\tlongjmp(self->escape_, 1);\n> +}\n> +\n> +static void outputMessage([[maybe_unused]] j_common_ptr cinfo)\n> +{\n> +}\n> +\n> +JpegErrorManager::JpegErrorManager(struct jpeg_decompress_struct &cinfo)\n\nI wouldn't mix the error manager and decompressor. You can keep the\njpeg_std_error() call below, and move the assignment of cinfo.err to the\ncode that creates the decompressor.\n\n> +{\n> +\tcinfo.err = jpeg_std_error(&errmgr_);\n> +\terrmgr_.error_exit = errorExit;\n> +\terrmgr_.output_message = outputMessage;\n> +}\n\nThis can live in sdl_texture_mjpg.cpp as it's internal to the\nimplementation of the SDL texture for MJPEG, there's no need to have one\nfile per class.\n\n> diff --git a/src/cam/jpeg_error_manager.h b/src/cam/jpeg_error_manager.h\n> new file mode 100644\n> index 00000000..0af28da1\n> --- /dev/null\n> +++ b/src/cam/jpeg_error_manager.h\n> @@ -0,0 +1,21 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2022, Ideas on Board Oy\n> + *\n> + * jpeg_error_manager.h - JPEG Error Manager\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +struct JpegErrorManager {\n> +\tJpegErrorManager(struct jpeg_decompress_struct &cinfo);\n> +\n> +\t/* Order very important for reinterpret_cast */\n> +\tstruct jpeg_error_mgr errmgr_;\n> +\tjmp_buf escape_;\n> +};\n> diff --git a/src/cam/meson.build b/src/cam/meson.build\n> index 5957ce14..86cef683 100644\n> --- a/src/cam/meson.build\n> +++ b/src/cam/meson.build\n> @@ -24,8 +24,8 @@ cam_sources = files([\n> cam_cpp_args = []\n> \n> libdrm = dependency('libdrm', required : false)\n> +libjpeg = dependency('libjpeg', required : false)\n> libsdl2 = dependency('SDL2', required : false)\n> -libsdl2_image = dependency('SDL2_image', required : false)\n> \n> if libdrm.found()\n> cam_cpp_args += [ '-DHAVE_KMS' ]\n> @@ -43,9 +43,10 @@ if libsdl2.found()\n> 'sdl_texture_yuyv.cpp'\n> ])\n> \n> - if libsdl2_image.found()\n> - cam_cpp_args += ['-DHAVE_SDL_IMAGE']\n> + if libjpeg.found()\n> + cam_cpp_args += ['-DHAVE_LIBJPEG']\n> cam_sources += files([\n> + 'jpeg_error_manager.cpp',\n> 'sdl_texture_mjpg.cpp'\n> ])\n> endif\n> @@ -57,8 +58,8 @@ cam = executable('cam', cam_sources,\n> libcamera_public,\n> libdrm,\n> libevent,\n> + libjpeg,\n> libsdl2,\n> - libsdl2_image,\n> libyaml,\n> ],\n> cpp_args : cam_cpp_args,\n> diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp\n> index f8e3e95d..19fdfd6d 100644\n> --- a/src/cam/sdl_sink.cpp\n> +++ b/src/cam/sdl_sink.cpp\n> @@ -21,7 +21,7 @@\n> \n> #include \"event_loop.h\"\n> #include \"image.h\"\n> -#ifdef HAVE_SDL_IMAGE\n> +#ifdef HAVE_LIBJPEG\n> #include \"sdl_texture_mjpg.h\"\n> #endif\n> #include \"sdl_texture_yuyv.h\"\n> @@ -62,7 +62,7 @@ int SDLSink::configure(const libcamera::CameraConfiguration &config)\n> \trect_.h = cfg.size.height;\n> \n> \tswitch (cfg.pixelFormat) {\n> -#ifdef HAVE_SDL_IMAGE\n> +#ifdef HAVE_LIBJPEG\n> \tcase libcamera::formats::MJPEG:\n> \t\ttexture_ = std::make_unique(rect_);\n> \t\tbreak;\n> diff --git a/src/cam/sdl_texture_mjpg.cpp b/src/cam/sdl_texture_mjpg.cpp\n> index 69e99ad3..22f570c6 100644\n> --- a/src/cam/sdl_texture_mjpg.cpp\n> +++ b/src/cam/sdl_texture_mjpg.cpp\n> @@ -7,19 +7,51 @@\n> \n> #include \"sdl_texture_mjpg.h\"\n> \n> -#include \n> +#include \"jpeg_error_manager.h\"\n> +\n> +#include \n> \n> using namespace libcamera;\n> \n> SDLTextureMJPG::SDLTextureMJPG(const SDL_Rect &rect)\n> -\t: SDLTexture(rect, SDL_PIXELFORMAT_RGB24, 0)\n> +\t: SDLTexture(rect, SDL_PIXELFORMAT_RGB24, rect.w * 3),\n> +\t rgb_(std::make_unique(pitch_ * rect.h))\n> +{\n> +}\n> +\n> +int SDLTextureMJPG::decompress(const Span &data)\n\nint SDLTextureMJPG::decompress(Span data)\n\nfor two reasons:\n\n- a Span is a pointer + size, which will fit in registers when calling\n this function, so that should be more efficient than passing a pointer\n to the Span\n\n- Span to indicate that the function doesn't modify the\n data. A const Span prevents changing the Span itself, but\n allows modifying the data that the Span points to.\n\nThis would however require changing the update() function accordingly,\nso I'll submit a patch on top.\n\nI've made those modifications locally to test and make sure I wasn't\nsaying something stupid, so I'll send a v5 shortly as it's ready.\n\n> {\n> +\tstruct jpeg_decompress_struct cinfo;\n> +\tJpegErrorManager jpegErrorManager(cinfo);\n> +\tif (setjmp(jpegErrorManager.escape_)) {\n> +\t\t/* libjpeg found an error */\n> +\t\tjpeg_destroy_decompress(&cinfo);\n> +\t\tstd::cerr << \"JPEG decompression error\" << std::endl;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tjpeg_create_decompress(&cinfo);\n> +\n> +\tjpeg_mem_src(&cinfo, data.data(), data.size());\n> +\n> +\tjpeg_read_header(&cinfo, TRUE);\n> +\n> +\tjpeg_start_decompress(&cinfo);\n> +\n> +\tfor (int i = 0; cinfo.output_scanline < cinfo.output_height; ++i) {\n> +\t\tJSAMPROW rowptr = rgb_.get() + i * pitch_;\n> +\t\tjpeg_read_scanlines(&cinfo, &rowptr, 1);\n> +\t}\n> +\n> +\tjpeg_finish_decompress(&cinfo);\n> +\n> +\tjpeg_destroy_decompress(&cinfo);\n> +\n> +\treturn 0;\n> }\n> \n> void SDLTextureMJPG::update(const Span &data)\n> {\n> -\tSDL_RWops *bufferStream = SDL_RWFromMem(data.data(), data.size());\n> -\tSDL_Surface *frame = IMG_Load_RW(bufferStream, 0);\n> -\tSDL_UpdateTexture(ptr_, nullptr, frame->pixels, frame->pitch);\n> -\tSDL_FreeSurface(frame);\n> +\tdecompress(data);\n> +\tSDL_UpdateTexture(ptr_, nullptr, rgb_.get(), pitch_);\n> }\n> diff --git a/src/cam/sdl_texture_mjpg.h b/src/cam/sdl_texture_mjpg.h\n> index b103f801..328c45a9 100644\n> --- a/src/cam/sdl_texture_mjpg.h\n> +++ b/src/cam/sdl_texture_mjpg.h\n> @@ -13,5 +13,11 @@ class SDLTextureMJPG : public SDLTexture\n> {\n> public:\n> \tSDLTextureMJPG(const SDL_Rect &rect);\n> +\n> \tvoid update(const libcamera::Span &data) override;\n> +\n> +private:\n> +\tint decompress(const libcamera::Span &data);\n> +\n> +\tstd::unique_ptr rgb_;\n> };","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 2AD12BE173\n\tfor ;\n\tWed, 20 Jul 2022 11:56:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7FB4E60489;\n\tWed, 20 Jul 2022 13:55:59 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5798B601B8\n\tfor ;\n\tWed, 20 Jul 2022 13:55:57 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BC5FF6DB;\n\tWed, 20 Jul 2022 13:55:56 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658318159;\n\tbh=mwmICpYxJnezIuKbiF7RV9Rv/CUlUtjEvpICYLInJEs=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=X3+W5iuEvcoGoFLqZNwIDE34THgtC2Fq5xTpxfxtHQOB+8B/r0lxO6WdS3MjzYcAM\n\tolBkKFC8DwE3Hsi3DAQVkF2cSmwdefpBXLpJ0MO3BBA405If+I7D5Yns45iEqqZqwJ\n\txGLUGlmdWKWwSuFxEnB8drYDrmb9KPMWRAqMuM9GkFX/otNYkMv+XOaWfLNdBmzEsk\n\thj8CdmO7viHjmd5nHUK9s4EG3EURyzZi69YmwLY+GQeR0JTSoKAAGX5R9+0J4agGIE\n\tXG+6HxLi4O5JpnTgx587z/zzZiDx3qNnmH1CqL3q8vM2o6/QvMtwrPj+89q+ES5WUE\n\tcxStPjzA3Ph8w==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1658318156;\n\tbh=mwmICpYxJnezIuKbiF7RV9Rv/CUlUtjEvpICYLInJEs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ST7U9Jn9qfiFzysqb2WNfwRlgUcABQUyQDYssK0CqUVqgCKh2Ulsayv2SX2RXzHKZ\n\t3qpQ6ZzgDxI9gJ0Ij0vHBus9OC69k1/aM/Dqzfu05/9htuI42bVVJap5IZLNd1/tca\n\tlX8hOwvwjoyDDo/cRGRDIm6ANNE6h0gJo680puic="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ST7U9Jn9\"; dkim-atps=neutral","Date":"Wed, 20 Jul 2022 14:55:22 +0300","To":"Eric Curtin ","Message-ID":"","References":"<20220719091745.8040-1-ecurtin@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220719091745.8040-1-ecurtin@redhat.com>","Subject":"Re: [libcamera-devel] [PATCH v4] cam: sdl_sink: Use libjpeg over\n\tSDL2_image","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"Ian Mullins , libcamera-devel@lists.libcamera.org, \n\tjozef@mlich.cz","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24006,"web_url":"https://patchwork.libcamera.org/comment/24006/","msgid":"","date":"2022-07-20T13:54:29","subject":"Re: [libcamera-devel] [PATCH v4] cam: sdl_sink: Use libjpeg over\n\tSDL2_image","submitter":{"id":101,"url":"https://patchwork.libcamera.org/api/people/101/","name":"Eric Curtin","email":"ecurtin@redhat.com"},"content":"On Wed, 20 Jul 2022 at 12:56, Laurent Pinchart\n wrote:\n>\n> Hi Eric,\n>\n> Thank you for the patch.\n>\n> On Tue, Jul 19, 2022 at 10:17:46AM +0100, Eric Curtin wrote:\n> > We were using the libjpeg functionality of SDL2_image only, instead just\n> > use libjpeg directly to reduce our dependancy count, it is a more\n> > commonly available library.\n> >\n> > Signed-off-by: Eric Curtin \n> > ---\n> > Changes in v4:\n> > - decompress function takes a Span&\n> >\n> > Changes in v3:\n> > - create JpegErrorManager struct\n> > - change c cast to reinterpret_cast\n> >\n> > Changes in v2:\n> > - alphabetically sorted various orderings\n> > - pitch_ is const again\n> > - added setjmp logic for error handling in libjpeg\n> > - rgbdata_ to unique_ptr and renamed to rgb_\n> > - removed a copy from buffer to rgb_\n> > - removed a destructor\n> > ---\n> > README.rst | 2 +-\n> > src/cam/jpeg_error_manager.cpp | 26 ++++++++++++++++++++\n> > src/cam/jpeg_error_manager.h | 21 ++++++++++++++++\n> > src/cam/meson.build | 9 +++----\n> > src/cam/sdl_sink.cpp | 4 ++--\n> > src/cam/sdl_texture_mjpg.cpp | 44 +++++++++++++++++++++++++++++-----\n> > src/cam/sdl_texture_mjpg.h | 6 +++++\n> > 7 files changed, 99 insertions(+), 13 deletions(-)\n> > create mode 100644 src/cam/jpeg_error_manager.cpp\n> > create mode 100644 src/cam/jpeg_error_manager.h\n> >\n> > diff --git a/README.rst b/README.rst\n> > index b9e72d81..47b914f0 100644\n> > --- a/README.rst\n> > +++ b/README.rst\n> > @@ -92,8 +92,8 @@ for cam: [optional]\n> > tool:\n> >\n> > - libdrm-dev: Enables the KMS sink\n> > + - libjpeg-dev: Enables MJPEG on the SDL sink\n> > - libsdl2-dev: Enables the SDL sink\n> > - - libsdl2-image-dev: Supports MJPEG on the SDL sink\n> >\n> > for qcam: [optional]\n> > qtbase5-dev libqt5core5a libqt5gui5 libqt5widgets5 qttools5-dev-tools libtiff-dev\n> > diff --git a/src/cam/jpeg_error_manager.cpp b/src/cam/jpeg_error_manager.cpp\n> > new file mode 100644\n> > index 00000000..7e45e577\n> > --- /dev/null\n> > +++ b/src/cam/jpeg_error_manager.cpp\n> > @@ -0,0 +1,26 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2022, Ideas on Board Oy\n> > + *\n> > + * jpeg_error_manager.cpp - JPEG Error Manager\n> > + */\n> > +\n> > +#include \"jpeg_error_manager.h\"\n> > +\n> > +static void errorExit(j_common_ptr cinfo)\n> > +{\n> > + JpegErrorManager *self =\n> > + reinterpret_cast(cinfo->err);\n> > + longjmp(self->escape_, 1);\n> > +}\n> > +\n> > +static void outputMessage([[maybe_unused]] j_common_ptr cinfo)\n> > +{\n> > +}\n> > +\n> > +JpegErrorManager::JpegErrorManager(struct jpeg_decompress_struct &cinfo)\n>\n> I wouldn't mix the error manager and decompressor. You can keep the\n> jpeg_std_error() call below, and move the assignment of cinfo.err to the\n> code that creates the decompressor.\n>\n> > +{\n> > + cinfo.err = jpeg_std_error(&errmgr_);\n> > + errmgr_.error_exit = errorExit;\n> > + errmgr_.output_message = outputMessage;\n> > +}\n>\n> This can live in sdl_texture_mjpg.cpp as it's internal to the\n> implementation of the SDL texture for MJPEG, there's no need to have one\n> file per class.\n>\n> > diff --git a/src/cam/jpeg_error_manager.h b/src/cam/jpeg_error_manager.h\n> > new file mode 100644\n> > index 00000000..0af28da1\n> > --- /dev/null\n> > +++ b/src/cam/jpeg_error_manager.h\n> > @@ -0,0 +1,21 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2022, Ideas on Board Oy\n> > + *\n> > + * jpeg_error_manager.h - JPEG Error Manager\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +struct JpegErrorManager {\n> > + JpegErrorManager(struct jpeg_decompress_struct &cinfo);\n> > +\n> > + /* Order very important for reinterpret_cast */\n> > + struct jpeg_error_mgr errmgr_;\n> > + jmp_buf escape_;\n> > +};\n> > diff --git a/src/cam/meson.build b/src/cam/meson.build\n> > index 5957ce14..86cef683 100644\n> > --- a/src/cam/meson.build\n> > +++ b/src/cam/meson.build\n> > @@ -24,8 +24,8 @@ cam_sources = files([\n> > cam_cpp_args = []\n> >\n> > libdrm = dependency('libdrm', required : false)\n> > +libjpeg = dependency('libjpeg', required : false)\n> > libsdl2 = dependency('SDL2', required : false)\n> > -libsdl2_image = dependency('SDL2_image', required : false)\n> >\n> > if libdrm.found()\n> > cam_cpp_args += [ '-DHAVE_KMS' ]\n> > @@ -43,9 +43,10 @@ if libsdl2.found()\n> > 'sdl_texture_yuyv.cpp'\n> > ])\n> >\n> > - if libsdl2_image.found()\n> > - cam_cpp_args += ['-DHAVE_SDL_IMAGE']\n> > + if libjpeg.found()\n> > + cam_cpp_args += ['-DHAVE_LIBJPEG']\n> > cam_sources += files([\n> > + 'jpeg_error_manager.cpp',\n> > 'sdl_texture_mjpg.cpp'\n> > ])\n> > endif\n> > @@ -57,8 +58,8 @@ cam = executable('cam', cam_sources,\n> > libcamera_public,\n> > libdrm,\n> > libevent,\n> > + libjpeg,\n> > libsdl2,\n> > - libsdl2_image,\n> > libyaml,\n> > ],\n> > cpp_args : cam_cpp_args,\n> > diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp\n> > index f8e3e95d..19fdfd6d 100644\n> > --- a/src/cam/sdl_sink.cpp\n> > +++ b/src/cam/sdl_sink.cpp\n> > @@ -21,7 +21,7 @@\n> >\n> > #include \"event_loop.h\"\n> > #include \"image.h\"\n> > -#ifdef HAVE_SDL_IMAGE\n> > +#ifdef HAVE_LIBJPEG\n> > #include \"sdl_texture_mjpg.h\"\n> > #endif\n> > #include \"sdl_texture_yuyv.h\"\n> > @@ -62,7 +62,7 @@ int SDLSink::configure(const libcamera::CameraConfiguration &config)\n> > rect_.h = cfg.size.height;\n> >\n> > switch (cfg.pixelFormat) {\n> > -#ifdef HAVE_SDL_IMAGE\n> > +#ifdef HAVE_LIBJPEG\n> > case libcamera::formats::MJPEG:\n> > texture_ = std::make_unique(rect_);\n> > break;\n> > diff --git a/src/cam/sdl_texture_mjpg.cpp b/src/cam/sdl_texture_mjpg.cpp\n> > index 69e99ad3..22f570c6 100644\n> > --- a/src/cam/sdl_texture_mjpg.cpp\n> > +++ b/src/cam/sdl_texture_mjpg.cpp\n> > @@ -7,19 +7,51 @@\n> >\n> > #include \"sdl_texture_mjpg.h\"\n> >\n> > -#include \n> > +#include \"jpeg_error_manager.h\"\n> > +\n> > +#include \n> >\n> > using namespace libcamera;\n> >\n> > SDLTextureMJPG::SDLTextureMJPG(const SDL_Rect &rect)\n> > - : SDLTexture(rect, SDL_PIXELFORMAT_RGB24, 0)\n> > + : SDLTexture(rect, SDL_PIXELFORMAT_RGB24, rect.w * 3),\n> > + rgb_(std::make_unique(pitch_ * rect.h))\n> > +{\n> > +}\n> > +\n> > +int SDLTextureMJPG::decompress(const Span &data)\n>\n> int SDLTextureMJPG::decompress(Span data)\n>\n> for two reasons:\n>\n> - a Span is a pointer + size, which will fit in registers when calling\n> this function, so that should be more efficient than passing a pointer\n> to the Span\n\nI'm not sure about that:\n\n#include \n#include \n\nint main() {\n std::cout << sizeof(const libcamera::Span) << std::endl;\n std::cout << sizeof(const libcamera::Span*) << std::endl;\n std::cout << sizeof(const libcamera::Span&) << std::endl;\n}\n\nOutput:\n\n16\n8\n16\n\nEven though pass by value and pass by reference are the same amount of\nbytes (both 16 bytes and the pointer is 8 on x86), I would expect pass\nby reference in the worst case to be of equal performance because it\nensures no constructor of any kind is called (where another copy or\ntwo could occur in pass by value, I don't even need to read the\nconstructor's source code if it's a reference, that's why class/struct\nas reference parameter is a great rule of thumb). But I'm not sure\nsomething like this is worth a v6 either, so it's fine the way it is I\nguess.\n\n>\n> - Span to indicate that the function doesn't modify the\n> data. A const Span prevents changing the Span itself, but\n> allows modifying the data that the Span points to.\n>\n> This would however require changing the update() function accordingly,\n> so I'll submit a patch on top.\n>\n> I've made those modifications locally to test and make sure I wasn't\n> saying something stupid, so I'll send a v5 shortly as it's ready.\n\nGonna run a test.\n\n>\n> > {\n> > + struct jpeg_decompress_struct cinfo;\n> > + JpegErrorManager jpegErrorManager(cinfo);\n> > + if (setjmp(jpegErrorManager.escape_)) {\n> > + /* libjpeg found an error */\n> > + jpeg_destroy_decompress(&cinfo);\n> > + std::cerr << \"JPEG decompression error\" << std::endl;\n> > + return -EINVAL;\n> > + }\n> > +\n> > + jpeg_create_decompress(&cinfo);\n> > +\n> > + jpeg_mem_src(&cinfo, data.data(), data.size());\n> > +\n> > + jpeg_read_header(&cinfo, TRUE);\n> > +\n> > + jpeg_start_decompress(&cinfo);\n> > +\n> > + for (int i = 0; cinfo.output_scanline < cinfo.output_height; ++i) {\n> > + JSAMPROW rowptr = rgb_.get() + i * pitch_;\n> > + jpeg_read_scanlines(&cinfo, &rowptr, 1);\n> > + }\n> > +\n> > + jpeg_finish_decompress(&cinfo);\n> > +\n> > + jpeg_destroy_decompress(&cinfo);\n> > +\n> > + return 0;\n> > }\n> >\n> > void SDLTextureMJPG::update(const Span &data)\n> > {\n> > - SDL_RWops *bufferStream = SDL_RWFromMem(data.data(), data.size());\n> > - SDL_Surface *frame = IMG_Load_RW(bufferStream, 0);\n> > - SDL_UpdateTexture(ptr_, nullptr, frame->pixels, frame->pitch);\n> > - SDL_FreeSurface(frame);\n> > + decompress(data);\n> > + SDL_UpdateTexture(ptr_, nullptr, rgb_.get(), pitch_);\n> > }\n> > diff --git a/src/cam/sdl_texture_mjpg.h b/src/cam/sdl_texture_mjpg.h\n> > index b103f801..328c45a9 100644\n> > --- a/src/cam/sdl_texture_mjpg.h\n> > +++ b/src/cam/sdl_texture_mjpg.h\n> > @@ -13,5 +13,11 @@ class SDLTextureMJPG : public SDLTexture\n> > {\n> > public:\n> > SDLTextureMJPG(const SDL_Rect &rect);\n> > +\n> > void update(const libcamera::Span &data) override;\n> > +\n> > +private:\n> > + int decompress(const libcamera::Span &data);\n> > +\n> > + std::unique_ptr rgb_;\n> > };\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id F2FD1BD1F1\n\tfor ;\n\tWed, 20 Jul 2022 13:54:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 497BD63310;\n\tWed, 20 Jul 2022 15:54:51 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2F261601B8\n\tfor ;\n\tWed, 20 Jul 2022 15:54:49 +0200 (CEST)","from mail-qv1-f71.google.com (mail-qv1-f71.google.com\n\t[209.85.219.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id\n\tus-mta-208-esz0MA6IPGCyIY_2GEnPpg-1; Wed, 20 Jul 2022 09:54:46 -0400","by mail-qv1-f71.google.com with SMTP id\n\tu15-20020a0ced2f000000b004732a5e7a99so9443875qvq.5\n\tfor ;\n\tWed, 20 Jul 2022 06:54:46 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658325291;\n\tbh=OyRnsL+drq1gtQloCcadK4Qn489fbgTPgtdyTyqUnhg=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=iOCO/8rwtEC3JZG4mhcsE02kKqejxCzkgjC3M9AJxKBQjdKijjWwCa4Z4O83vtCB4\n\t5dR6h9qrbLH8HZahsUAJNJazRkKQuVKfgCs90x431EfeU/zmKbgpPcEBAobUxQx0Rr\n\tM+bJuUvrhB3QxYrKUJ7DPTtoiv8F08xwRKQtqF8ODiitSeZQYhlSb1Ud/MYEnXc5oc\n\tZv98iOAAxvQaOsRSTaXKssIgmRGbidUiP7nV67/NmcabdEsVz12Tq5OhVpjtc6KdxI\n\tn3E7CnnHICG3pViDzdz3FYORSs25bXEzw6N7+5WTfy8mcsIvWX/gUUh9npeshvIGXJ\n\tzQGABk1Ay2XEw==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1658325288;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=e+TgbOZmRFlzP01Z0C6bQ6xE/dRGOxuwfKDbaW4ZHgs=;\n\tb=FFLiq4mhgfpZpMDe/2PYn5zB7r7ncu8E4acdRkn2mPt5KZI2W+V9pW/6P8Pcqt0zAx5D9D\n\teGIDujwi2xUX0jilfh2h6tyfXeBZVfW9ueWLFT5T02VVKk04QPNym7Qt5JinB8HvijJ3Mz\n\t8pyPUceuKIJKSszdxSra2hEWiupY0ek="],"Authentication-Results":["lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=redhat.com\n\theader.i=@redhat.com header.b=\"FFLiq4mh\"; \n\tdkim-atps=neutral","relay.mimecast.com;\n\tauth=pass smtp.auth=CUSA124A263 smtp.mailfrom=ecurtin@redhat.com"],"X-MC-Unique":"esz0MA6IPGCyIY_2GEnPpg-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=e+TgbOZmRFlzP01Z0C6bQ6xE/dRGOxuwfKDbaW4ZHgs=;\n\tb=O2KbJx9m/I3LDEVV83OxJo2hFrbgiurlvolWpX/jwyjQXbehkIZzKiyMgAV+dPpVS4\n\tJPtiVZKb+kY1nzxnOIJYrLl73w5M/SSnJ/UwffcSgbtxJMLOpxMB/u4zYy9U7I1sIQiM\n\tmsIqDRJN8UAc2VoUnQbsefgneuxiuTNT/viQygRLi7uPcrErtEJXMPGQTzpBKwjDAMlS\n\tnJiyqS9/MkslvCg7QycAxpEpPHBCEik3VFEBGFWHrti0RRH+geFLWis6j0ONc/VcPG+Y\n\tHlvd4CyVNxQUuSupMtPY8KhVl20C/rf/TJqcktKP8pz+VljsJSfAyQERVQIbi180IJev\n\tQIIA==","X-Gm-Message-State":"AJIora/vCTmQ74nNKJ5RchkTHc9nWK2iQHLmNqUdZduvtAaGl2redTQx\n\t61aTwn1A9hTuXzqUFXEaoe9JEXJwNazvO2H+WjvU+nyuu6brQfg+k5MUxTl0LjLulBOyrdnHD8W\n\tJVW48DZDHXUg1S+ODTVXcjQZl/VmhPdjio/eru+tH1kG3USX+Cw==","X-Received":["by 2002:a0c:b2d0:0:b0:473:2c19:f1ee with SMTP id\n\td16-20020a0cb2d0000000b004732c19f1eemr29549867qvf.130.1658325285990; \n\tWed, 20 Jul 2022 06:54:45 -0700 (PDT)","by 2002:a0c:b2d0:0:b0:473:2c19:f1ee with SMTP id\n\td16-20020a0cb2d0000000b004732c19f1eemr29549842qvf.130.1658325285679;\n\tWed, 20 Jul 2022 06:54:45 -0700 (PDT)"],"X-Google-Smtp-Source":"AGRyM1seP9n8/ancCpzRIXGQ8neulkLzuFlJ0ZzrXe7yylg2WNfy5qeDiARYEdihmBjrhVoQlSDtAIwz2dLvrM+Rl0A=","MIME-Version":"1.0","References":"<20220719091745.8040-1-ecurtin@redhat.com>\n\t","In-Reply-To":"","Date":"Wed, 20 Jul 2022 14:54:29 +0100","Message-ID":"","To":"Laurent Pinchart ","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v4] cam: sdl_sink: Use libjpeg over\n\tSDL2_image","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Eric Curtin via libcamera-devel ","Reply-To":"Eric Curtin ","Cc":"Ian Mullins ,\n\tlibcamera devel , jozef@mlich.cz","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24040,"web_url":"https://patchwork.libcamera.org/comment/24040/","msgid":"","date":"2022-07-21T20:48:59","subject":"Re: [libcamera-devel] [PATCH v4] cam: sdl_sink: Use libjpeg over\n\tSDL2_image","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Eric,\n\nOn Wed, Jul 20, 2022 at 02:54:29PM +0100, Eric Curtin wrote:\n> On Wed, 20 Jul 2022 at 12:56, Laurent Pinchart wrote:\n> > On Tue, Jul 19, 2022 at 10:17:46AM +0100, Eric Curtin wrote:\n> > > We were using the libjpeg functionality of SDL2_image only, instead just\n> > > use libjpeg directly to reduce our dependancy count, it is a more\n> > > commonly available library.\n> > >\n> > > Signed-off-by: Eric Curtin \n> > > ---\n> > > Changes in v4:\n> > > - decompress function takes a Span&\n> > >\n> > > Changes in v3:\n> > > - create JpegErrorManager struct\n> > > - change c cast to reinterpret_cast\n> > >\n> > > Changes in v2:\n> > > - alphabetically sorted various orderings\n> > > - pitch_ is const again\n> > > - added setjmp logic for error handling in libjpeg\n> > > - rgbdata_ to unique_ptr and renamed to rgb_\n> > > - removed a copy from buffer to rgb_\n> > > - removed a destructor\n> > > ---\n> > > README.rst | 2 +-\n> > > src/cam/jpeg_error_manager.cpp | 26 ++++++++++++++++++++\n> > > src/cam/jpeg_error_manager.h | 21 ++++++++++++++++\n> > > src/cam/meson.build | 9 +++----\n> > > src/cam/sdl_sink.cpp | 4 ++--\n> > > src/cam/sdl_texture_mjpg.cpp | 44 +++++++++++++++++++++++++++++-----\n> > > src/cam/sdl_texture_mjpg.h | 6 +++++\n> > > 7 files changed, 99 insertions(+), 13 deletions(-)\n> > > create mode 100644 src/cam/jpeg_error_manager.cpp\n> > > create mode 100644 src/cam/jpeg_error_manager.h\n> > >\n> > > diff --git a/README.rst b/README.rst\n> > > index b9e72d81..47b914f0 100644\n> > > --- a/README.rst\n> > > +++ b/README.rst\n> > > @@ -92,8 +92,8 @@ for cam: [optional]\n> > > tool:\n> > >\n> > > - libdrm-dev: Enables the KMS sink\n> > > + - libjpeg-dev: Enables MJPEG on the SDL sink\n> > > - libsdl2-dev: Enables the SDL sink\n> > > - - libsdl2-image-dev: Supports MJPEG on the SDL sink\n> > >\n> > > for qcam: [optional]\n> > > qtbase5-dev libqt5core5a libqt5gui5 libqt5widgets5 qttools5-dev-tools libtiff-dev\n> > > diff --git a/src/cam/jpeg_error_manager.cpp b/src/cam/jpeg_error_manager.cpp\n> > > new file mode 100644\n> > > index 00000000..7e45e577\n> > > --- /dev/null\n> > > +++ b/src/cam/jpeg_error_manager.cpp\n> > > @@ -0,0 +1,26 @@\n> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > +/*\n> > > + * Copyright (C) 2022, Ideas on Board Oy\n> > > + *\n> > > + * jpeg_error_manager.cpp - JPEG Error Manager\n> > > + */\n> > > +\n> > > +#include \"jpeg_error_manager.h\"\n> > > +\n> > > +static void errorExit(j_common_ptr cinfo)\n> > > +{\n> > > + JpegErrorManager *self =\n> > > + reinterpret_cast(cinfo->err);\n> > > + longjmp(self->escape_, 1);\n> > > +}\n> > > +\n> > > +static void outputMessage([[maybe_unused]] j_common_ptr cinfo)\n> > > +{\n> > > +}\n> > > +\n> > > +JpegErrorManager::JpegErrorManager(struct jpeg_decompress_struct &cinfo)\n> >\n> > I wouldn't mix the error manager and decompressor. You can keep the\n> > jpeg_std_error() call below, and move the assignment of cinfo.err to the\n> > code that creates the decompressor.\n> >\n> > > +{\n> > > + cinfo.err = jpeg_std_error(&errmgr_);\n> > > + errmgr_.error_exit = errorExit;\n> > > + errmgr_.output_message = outputMessage;\n> > > +}\n> >\n> > This can live in sdl_texture_mjpg.cpp as it's internal to the\n> > implementation of the SDL texture for MJPEG, there's no need to have one\n> > file per class.\n> >\n> > > diff --git a/src/cam/jpeg_error_manager.h b/src/cam/jpeg_error_manager.h\n> > > new file mode 100644\n> > > index 00000000..0af28da1\n> > > --- /dev/null\n> > > +++ b/src/cam/jpeg_error_manager.h\n> > > @@ -0,0 +1,21 @@\n> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > +/*\n> > > + * Copyright (C) 2022, Ideas on Board Oy\n> > > + *\n> > > + * jpeg_error_manager.h - JPEG Error Manager\n> > > + */\n> > > +\n> > > +#pragma once\n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +struct JpegErrorManager {\n> > > + JpegErrorManager(struct jpeg_decompress_struct &cinfo);\n> > > +\n> > > + /* Order very important for reinterpret_cast */\n> > > + struct jpeg_error_mgr errmgr_;\n> > > + jmp_buf escape_;\n> > > +};\n> > > diff --git a/src/cam/meson.build b/src/cam/meson.build\n> > > index 5957ce14..86cef683 100644\n> > > --- a/src/cam/meson.build\n> > > +++ b/src/cam/meson.build\n> > > @@ -24,8 +24,8 @@ cam_sources = files([\n> > > cam_cpp_args = []\n> > >\n> > > libdrm = dependency('libdrm', required : false)\n> > > +libjpeg = dependency('libjpeg', required : false)\n> > > libsdl2 = dependency('SDL2', required : false)\n> > > -libsdl2_image = dependency('SDL2_image', required : false)\n> > >\n> > > if libdrm.found()\n> > > cam_cpp_args += [ '-DHAVE_KMS' ]\n> > > @@ -43,9 +43,10 @@ if libsdl2.found()\n> > > 'sdl_texture_yuyv.cpp'\n> > > ])\n> > >\n> > > - if libsdl2_image.found()\n> > > - cam_cpp_args += ['-DHAVE_SDL_IMAGE']\n> > > + if libjpeg.found()\n> > > + cam_cpp_args += ['-DHAVE_LIBJPEG']\n> > > cam_sources += files([\n> > > + 'jpeg_error_manager.cpp',\n> > > 'sdl_texture_mjpg.cpp'\n> > > ])\n> > > endif\n> > > @@ -57,8 +58,8 @@ cam = executable('cam', cam_sources,\n> > > libcamera_public,\n> > > libdrm,\n> > > libevent,\n> > > + libjpeg,\n> > > libsdl2,\n> > > - libsdl2_image,\n> > > libyaml,\n> > > ],\n> > > cpp_args : cam_cpp_args,\n> > > diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp\n> > > index f8e3e95d..19fdfd6d 100644\n> > > --- a/src/cam/sdl_sink.cpp\n> > > +++ b/src/cam/sdl_sink.cpp\n> > > @@ -21,7 +21,7 @@\n> > >\n> > > #include \"event_loop.h\"\n> > > #include \"image.h\"\n> > > -#ifdef HAVE_SDL_IMAGE\n> > > +#ifdef HAVE_LIBJPEG\n> > > #include \"sdl_texture_mjpg.h\"\n> > > #endif\n> > > #include \"sdl_texture_yuyv.h\"\n> > > @@ -62,7 +62,7 @@ int SDLSink::configure(const libcamera::CameraConfiguration &config)\n> > > rect_.h = cfg.size.height;\n> > >\n> > > switch (cfg.pixelFormat) {\n> > > -#ifdef HAVE_SDL_IMAGE\n> > > +#ifdef HAVE_LIBJPEG\n> > > case libcamera::formats::MJPEG:\n> > > texture_ = std::make_unique(rect_);\n> > > break;\n> > > diff --git a/src/cam/sdl_texture_mjpg.cpp b/src/cam/sdl_texture_mjpg.cpp\n> > > index 69e99ad3..22f570c6 100644\n> > > --- a/src/cam/sdl_texture_mjpg.cpp\n> > > +++ b/src/cam/sdl_texture_mjpg.cpp\n> > > @@ -7,19 +7,51 @@\n> > >\n> > > #include \"sdl_texture_mjpg.h\"\n> > >\n> > > -#include \n> > > +#include \"jpeg_error_manager.h\"\n> > > +\n> > > +#include \n> > >\n> > > using namespace libcamera;\n> > >\n> > > SDLTextureMJPG::SDLTextureMJPG(const SDL_Rect &rect)\n> > > - : SDLTexture(rect, SDL_PIXELFORMAT_RGB24, 0)\n> > > + : SDLTexture(rect, SDL_PIXELFORMAT_RGB24, rect.w * 3),\n> > > + rgb_(std::make_unique(pitch_ * rect.h))\n> > > +{\n> > > +}\n> > > +\n> > > +int SDLTextureMJPG::decompress(const Span &data)\n> >\n> > int SDLTextureMJPG::decompress(Span data)\n> >\n> > for two reasons:\n> >\n> > - a Span is a pointer + size, which will fit in registers when calling\n> > this function, so that should be more efficient than passing a pointer\n> > to the Span\n> \n> I'm not sure about that:\n> \n> #include \n> #include \n> \n> int main() {\n> std::cout << sizeof(const libcamera::Span) << std::endl;\n> std::cout << sizeof(const libcamera::Span*) << std::endl;\n> std::cout << sizeof(const libcamera::Span&) << std::endl;\n\nThis one is a bit misleading, it will indeed print 16, but passing by\nreference will pass the address of the variable, so 8 bytes only.\n\n--------\n#include \n\nnamespace libcamera {\n\nvoid by_pointer(Span *data);\nvoid by_reference(Span &data);\nvoid by_value(Span data);\n\nvoid test_function()\n{\n\tSpan data{ reinterpret_cast(0x01234567), 0x89abcdef };\n\n\tby_pointer(&data);\n\tby_reference(data);\n\tby_value(data);\n}\n\n}\n--------\n\nCompiled with -O0 and disassembled with\n\n$ objdump -d -r --demangle src/libcamera/libcamera.so.0.0.0.p/reference.cpp.o \n\nproduces\n\n--------\n0000000000000000 :\n 0: 55 push %rbp\n 1: 48 89 e5 mov %rsp,%rbp\n 4: 48 83 ec 20 sub $0x20,%rsp\n 8: 64 48 8b 04 25 28 00 mov %fs:0x28,%rax\n f: 00 00 \n 11: 48 89 45 f8 mov %rax,-0x8(%rbp)\n 15: 31 c0 xor %eax,%eax\n 17: 48 8d 45 e0 lea -0x20(%rbp),%rax\n 1b: ba ef cd ab 89 mov $0x89abcdef,%edx\n 20: be 67 45 23 01 mov $0x1234567,%esi\n 25: 48 89 c7 mov %rax,%rdi\n 28: e8 00 00 00 00 call 2d \n 29: R_X86_64_PLT32 libcamera::Span::Span(unsigned char*, unsigned long)-0x4\n 2d: 48 8d 45 e0 lea -0x20(%rbp),%rax\n 31: 48 89 c7 mov %rax,%rdi\n 34: e8 00 00 00 00 call 39 \n 35: R_X86_64_PLT32 libcamera::by_pointer(libcamera::Span*)-0x4\n 39: 48 8d 45 e0 lea -0x20(%rbp),%rax\n 3d: 48 89 c7 mov %rax,%rdi\n 40: e8 00 00 00 00 call 45 \n 41: R_X86_64_PLT32 libcamera::by_reference(libcamera::Span&)-0x4\n 45: 48 8b 55 e0 mov -0x20(%rbp),%rdx\n 49: 48 8b 45 e8 mov -0x18(%rbp),%rax\n 4d: 48 89 d7 mov %rdx,%rdi\n 50: 48 89 c6 mov %rax,%rsi\n 53: e8 00 00 00 00 call 58 \n 54: R_X86_64_PLT32 libcamera::by_value(libcamera::Span)-0x4\n 58: 90 nop\n 59: 48 8b 45 f8 mov -0x8(%rbp),%rax\n 5d: 64 48 2b 04 25 28 00 sub %fs:0x28,%rax\n 64: 00 00 \n 66: 74 05 je 6d \n 68: e8 00 00 00 00 call 6d \n 69: R_X86_64_PLT32 __stack_chk_fail-0x4\n 6d: c9 leave \n 6e: c3 ret\n--------\n\nAs you can see, both by_pointer() and by_reference() get passed the\naddress of the data variable in rdi, while by_value() get the contents\nof the Span in rdi and rsi.\n\nObviously this only works if the number of parameters to the function is\nlow enough to fit everything in registers, and it only helps if the\ncaller has the Span data and size in registers already, as if it has to\nload them from memory, the callee could do so as well.\n\nI any case, I agree it's not a big optimization (if at all). The second\npoint I mentioned (Span) is more important.\n\n> }\n> \n> Output:\n> \n> 16\n> 8\n> 16\n> \n> Even though pass by value and pass by reference are the same amount of\n> bytes (both 16 bytes and the pointer is 8 on x86), I would expect pass\n> by reference in the worst case to be of equal performance because it\n> ensures no constructor of any kind is called (where another copy or\n> two could occur in pass by value, I don't even need to read the\n> constructor's source code if it's a reference, that's why class/struct\n> as reference parameter is a great rule of thumb). But I'm not sure\n> something like this is worth a v6 either, so it's fine the way it is I\n> guess.\n> \n> >\n> > - Span to indicate that the function doesn't modify the\n> > data. A const Span prevents changing the Span itself, but\n> > allows modifying the data that the Span points to.\n> >\n> > This would however require changing the update() function accordingly,\n> > so I'll submit a patch on top.\n> >\n> > I've made those modifications locally to test and make sure I wasn't\n> > saying something stupid, so I'll send a v5 shortly as it's ready.\n> \n> Gonna run a test.\n> \n> > > {\n> > > + struct jpeg_decompress_struct cinfo;\n> > > + JpegErrorManager jpegErrorManager(cinfo);\n> > > + if (setjmp(jpegErrorManager.escape_)) {\n> > > + /* libjpeg found an error */\n> > > + jpeg_destroy_decompress(&cinfo);\n> > > + std::cerr << \"JPEG decompression error\" << std::endl;\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + jpeg_create_decompress(&cinfo);\n> > > +\n> > > + jpeg_mem_src(&cinfo, data.data(), data.size());\n> > > +\n> > > + jpeg_read_header(&cinfo, TRUE);\n> > > +\n> > > + jpeg_start_decompress(&cinfo);\n> > > +\n> > > + for (int i = 0; cinfo.output_scanline < cinfo.output_height; ++i) {\n> > > + JSAMPROW rowptr = rgb_.get() + i * pitch_;\n> > > + jpeg_read_scanlines(&cinfo, &rowptr, 1);\n> > > + }\n> > > +\n> > > + jpeg_finish_decompress(&cinfo);\n> > > +\n> > > + jpeg_destroy_decompress(&cinfo);\n> > > +\n> > > + return 0;\n> > > }\n> > >\n> > > void SDLTextureMJPG::update(const Span &data)\n> > > {\n> > > - SDL_RWops *bufferStream = SDL_RWFromMem(data.data(), data.size());\n> > > - SDL_Surface *frame = IMG_Load_RW(bufferStream, 0);\n> > > - SDL_UpdateTexture(ptr_, nullptr, frame->pixels, frame->pitch);\n> > > - SDL_FreeSurface(frame);\n> > > + decompress(data);\n> > > + SDL_UpdateTexture(ptr_, nullptr, rgb_.get(), pitch_);\n> > > }\n> > > diff --git a/src/cam/sdl_texture_mjpg.h b/src/cam/sdl_texture_mjpg.h\n> > > index b103f801..328c45a9 100644\n> > > --- a/src/cam/sdl_texture_mjpg.h\n> > > +++ b/src/cam/sdl_texture_mjpg.h\n> > > @@ -13,5 +13,11 @@ class SDLTextureMJPG : public SDLTexture\n> > > {\n> > > public:\n> > > SDLTextureMJPG(const SDL_Rect &rect);\n> > > +\n> > > void update(const libcamera::Span &data) override;\n> > > +\n> > > +private:\n> > > + int decompress(const libcamera::Span &data);\n> > > +\n> > > + std::unique_ptr rgb_;\n> > > };","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 346DCBE173\n\tfor ;\n\tThu, 21 Jul 2022 20:49:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 66CB26330F;\n\tThu, 21 Jul 2022 22:49:03 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C1A76601B8\n\tfor ;\n\tThu, 21 Jul 2022 22:49:01 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 303D1496;\n\tThu, 21 Jul 2022 22:49:01 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658436543;\n\tbh=eBRXdqvXggPit4VXRTHAHAURD1cA6NHR4t8d18ciH+c=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=UM/BKe8K9FrjK2FrvtyyUYl7RFjXTAv4EreGlIj5Ca3GZY5XvlFxRPvb3rZyVF05E\n\ty2nz3Z/vznh3pUJIa+Svjp7IVED2DPo8fH8WEp9kZ9h3rRrFyoBxQaXjb4bWU/INBT\n\tVWt0a6ordBhNaiOI+PChBKYQkxU63KUNqDhiMICEvVqcsv16hBvl0a2/jm9vW+fTHm\n\t8Hj9GdYiEnbrthjJGA2QZXuYpin0wV27mIjEqmd+5K6yWsBwxZSPUbWNmEgqmx0wmP\n\th4gripMj8rUoT0Me0z2lGDo0de9+al8ErjlQAqtqfqXxDZwk9ZpqbGcy0MSkoihCL4\n\t1v6j6iURLXCnQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1658436541;\n\tbh=eBRXdqvXggPit4VXRTHAHAURD1cA6NHR4t8d18ciH+c=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=JSVGxythr/4Yr8fKXTIrquFw6vhBMi6EZWmFLud3GIBRIXjIy10GboXyYJd7J/E7S\n\tKlGzR6/Bxq14rGdZsfZba3mRSDZNET+KQgG0YgljtUk+CLyZEzhkTZYB6JpiZRBI7N\n\tkbaxzUGkHI1z3ikPNb9tG0+DZzP/9Ja1d2rI3BgI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"JSVGxyth\"; dkim-atps=neutral","Date":"Thu, 21 Jul 2022 23:48:59 +0300","To":"Eric Curtin ","Message-ID":"","References":"<20220719091745.8040-1-ecurtin@redhat.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4] cam: sdl_sink: Use libjpeg over\n\tSDL2_image","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"Ian Mullins ,\n\tlibcamera devel , jozef@mlich.cz","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24053,"web_url":"https://patchwork.libcamera.org/comment/24053/","msgid":"","date":"2022-07-22T09:32:46","subject":"Re: [libcamera-devel] [PATCH v4] cam: sdl_sink: Use libjpeg over\n\tSDL2_image","submitter":{"id":101,"url":"https://patchwork.libcamera.org/api/people/101/","name":"Eric Curtin","email":"ecurtin@redhat.com"},"content":"On Thu, 21 Jul 2022 at 21:49, Laurent Pinchart\n wrote:\n>\n> Hi Eric,\n>\n> On Wed, Jul 20, 2022 at 02:54:29PM +0100, Eric Curtin wrote:\n> > On Wed, 20 Jul 2022 at 12:56, Laurent Pinchart wrote:\n> > > On Tue, Jul 19, 2022 at 10:17:46AM +0100, Eric Curtin wrote:\n> > > > We were using the libjpeg functionality of SDL2_image only, instead just\n> > > > use libjpeg directly to reduce our dependancy count, it is a more\n> > > > commonly available library.\n> > > >\n> > > > Signed-off-by: Eric Curtin \n> > > > ---\n> > > > Changes in v4:\n> > > > - decompress function takes a Span&\n> > > >\n> > > > Changes in v3:\n> > > > - create JpegErrorManager struct\n> > > > - change c cast to reinterpret_cast\n> > > >\n> > > > Changes in v2:\n> > > > - alphabetically sorted various orderings\n> > > > - pitch_ is const again\n> > > > - added setjmp logic for error handling in libjpeg\n> > > > - rgbdata_ to unique_ptr and renamed to rgb_\n> > > > - removed a copy from buffer to rgb_\n> > > > - removed a destructor\n> > > > ---\n> > > > README.rst | 2 +-\n> > > > src/cam/jpeg_error_manager.cpp | 26 ++++++++++++++++++++\n> > > > src/cam/jpeg_error_manager.h | 21 ++++++++++++++++\n> > > > src/cam/meson.build | 9 +++----\n> > > > src/cam/sdl_sink.cpp | 4 ++--\n> > > > src/cam/sdl_texture_mjpg.cpp | 44 +++++++++++++++++++++++++++++-----\n> > > > src/cam/sdl_texture_mjpg.h | 6 +++++\n> > > > 7 files changed, 99 insertions(+), 13 deletions(-)\n> > > > create mode 100644 src/cam/jpeg_error_manager.cpp\n> > > > create mode 100644 src/cam/jpeg_error_manager.h\n> > > >\n> > > > diff --git a/README.rst b/README.rst\n> > > > index b9e72d81..47b914f0 100644\n> > > > --- a/README.rst\n> > > > +++ b/README.rst\n> > > > @@ -92,8 +92,8 @@ for cam: [optional]\n> > > > tool:\n> > > >\n> > > > - libdrm-dev: Enables the KMS sink\n> > > > + - libjpeg-dev: Enables MJPEG on the SDL sink\n> > > > - libsdl2-dev: Enables the SDL sink\n> > > > - - libsdl2-image-dev: Supports MJPEG on the SDL sink\n> > > >\n> > > > for qcam: [optional]\n> > > > qtbase5-dev libqt5core5a libqt5gui5 libqt5widgets5 qttools5-dev-tools libtiff-dev\n> > > > diff --git a/src/cam/jpeg_error_manager.cpp b/src/cam/jpeg_error_manager.cpp\n> > > > new file mode 100644\n> > > > index 00000000..7e45e577\n> > > > --- /dev/null\n> > > > +++ b/src/cam/jpeg_error_manager.cpp\n> > > > @@ -0,0 +1,26 @@\n> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2022, Ideas on Board Oy\n> > > > + *\n> > > > + * jpeg_error_manager.cpp - JPEG Error Manager\n> > > > + */\n> > > > +\n> > > > +#include \"jpeg_error_manager.h\"\n> > > > +\n> > > > +static void errorExit(j_common_ptr cinfo)\n> > > > +{\n> > > > + JpegErrorManager *self =\n> > > > + reinterpret_cast(cinfo->err);\n> > > > + longjmp(self->escape_, 1);\n> > > > +}\n> > > > +\n> > > > +static void outputMessage([[maybe_unused]] j_common_ptr cinfo)\n> > > > +{\n> > > > +}\n> > > > +\n> > > > +JpegErrorManager::JpegErrorManager(struct jpeg_decompress_struct &cinfo)\n> > >\n> > > I wouldn't mix the error manager and decompressor. You can keep the\n> > > jpeg_std_error() call below, and move the assignment of cinfo.err to the\n> > > code that creates the decompressor.\n> > >\n> > > > +{\n> > > > + cinfo.err = jpeg_std_error(&errmgr_);\n> > > > + errmgr_.error_exit = errorExit;\n> > > > + errmgr_.output_message = outputMessage;\n> > > > +}\n> > >\n> > > This can live in sdl_texture_mjpg.cpp as it's internal to the\n> > > implementation of the SDL texture for MJPEG, there's no need to have one\n> > > file per class.\n> > >\n> > > > diff --git a/src/cam/jpeg_error_manager.h b/src/cam/jpeg_error_manager.h\n> > > > new file mode 100644\n> > > > index 00000000..0af28da1\n> > > > --- /dev/null\n> > > > +++ b/src/cam/jpeg_error_manager.h\n> > > > @@ -0,0 +1,21 @@\n> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2022, Ideas on Board Oy\n> > > > + *\n> > > > + * jpeg_error_manager.h - JPEG Error Manager\n> > > > + */\n> > > > +\n> > > > +#pragma once\n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +\n> > > > +struct JpegErrorManager {\n> > > > + JpegErrorManager(struct jpeg_decompress_struct &cinfo);\n> > > > +\n> > > > + /* Order very important for reinterpret_cast */\n> > > > + struct jpeg_error_mgr errmgr_;\n> > > > + jmp_buf escape_;\n> > > > +};\n> > > > diff --git a/src/cam/meson.build b/src/cam/meson.build\n> > > > index 5957ce14..86cef683 100644\n> > > > --- a/src/cam/meson.build\n> > > > +++ b/src/cam/meson.build\n> > > > @@ -24,8 +24,8 @@ cam_sources = files([\n> > > > cam_cpp_args = []\n> > > >\n> > > > libdrm = dependency('libdrm', required : false)\n> > > > +libjpeg = dependency('libjpeg', required : false)\n> > > > libsdl2 = dependency('SDL2', required : false)\n> > > > -libsdl2_image = dependency('SDL2_image', required : false)\n> > > >\n> > > > if libdrm.found()\n> > > > cam_cpp_args += [ '-DHAVE_KMS' ]\n> > > > @@ -43,9 +43,10 @@ if libsdl2.found()\n> > > > 'sdl_texture_yuyv.cpp'\n> > > > ])\n> > > >\n> > > > - if libsdl2_image.found()\n> > > > - cam_cpp_args += ['-DHAVE_SDL_IMAGE']\n> > > > + if libjpeg.found()\n> > > > + cam_cpp_args += ['-DHAVE_LIBJPEG']\n> > > > cam_sources += files([\n> > > > + 'jpeg_error_manager.cpp',\n> > > > 'sdl_texture_mjpg.cpp'\n> > > > ])\n> > > > endif\n> > > > @@ -57,8 +58,8 @@ cam = executable('cam', cam_sources,\n> > > > libcamera_public,\n> > > > libdrm,\n> > > > libevent,\n> > > > + libjpeg,\n> > > > libsdl2,\n> > > > - libsdl2_image,\n> > > > libyaml,\n> > > > ],\n> > > > cpp_args : cam_cpp_args,\n> > > > diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp\n> > > > index f8e3e95d..19fdfd6d 100644\n> > > > --- a/src/cam/sdl_sink.cpp\n> > > > +++ b/src/cam/sdl_sink.cpp\n> > > > @@ -21,7 +21,7 @@\n> > > >\n> > > > #include \"event_loop.h\"\n> > > > #include \"image.h\"\n> > > > -#ifdef HAVE_SDL_IMAGE\n> > > > +#ifdef HAVE_LIBJPEG\n> > > > #include \"sdl_texture_mjpg.h\"\n> > > > #endif\n> > > > #include \"sdl_texture_yuyv.h\"\n> > > > @@ -62,7 +62,7 @@ int SDLSink::configure(const libcamera::CameraConfiguration &config)\n> > > > rect_.h = cfg.size.height;\n> > > >\n> > > > switch (cfg.pixelFormat) {\n> > > > -#ifdef HAVE_SDL_IMAGE\n> > > > +#ifdef HAVE_LIBJPEG\n> > > > case libcamera::formats::MJPEG:\n> > > > texture_ = std::make_unique(rect_);\n> > > > break;\n> > > > diff --git a/src/cam/sdl_texture_mjpg.cpp b/src/cam/sdl_texture_mjpg.cpp\n> > > > index 69e99ad3..22f570c6 100644\n> > > > --- a/src/cam/sdl_texture_mjpg.cpp\n> > > > +++ b/src/cam/sdl_texture_mjpg.cpp\n> > > > @@ -7,19 +7,51 @@\n> > > >\n> > > > #include \"sdl_texture_mjpg.h\"\n> > > >\n> > > > -#include \n> > > > +#include \"jpeg_error_manager.h\"\n> > > > +\n> > > > +#include \n> > > >\n> > > > using namespace libcamera;\n> > > >\n> > > > SDLTextureMJPG::SDLTextureMJPG(const SDL_Rect &rect)\n> > > > - : SDLTexture(rect, SDL_PIXELFORMAT_RGB24, 0)\n> > > > + : SDLTexture(rect, SDL_PIXELFORMAT_RGB24, rect.w * 3),\n> > > > + rgb_(std::make_unique(pitch_ * rect.h))\n> > > > +{\n> > > > +}\n> > > > +\n> > > > +int SDLTextureMJPG::decompress(const Span &data)\n> > >\n> > > int SDLTextureMJPG::decompress(Span data)\n> > >\n> > > for two reasons:\n> > >\n> > > - a Span is a pointer + size, which will fit in registers when calling\n> > > this function, so that should be more efficient than passing a pointer\n> > > to the Span\n> >\n> > I'm not sure about that:\n> >\n> > #include \n> > #include \n> >\n> > int main() {\n> > std::cout << sizeof(const libcamera::Span) << std::endl;\n> > std::cout << sizeof(const libcamera::Span*) << std::endl;\n> > std::cout << sizeof(const libcamera::Span&) << std::endl;\n>\n> This one is a bit misleading, it will indeed print 16, but passing by\n> reference will pass the address of the variable, so 8 bytes only.\n\nI was confused by that I'll admit, I realized afterwards by\ninvestigating more. I'm good with v5.\n\n>\n> --------\n> #include \n>\n> namespace libcamera {\n>\n> void by_pointer(Span *data);\n> void by_reference(Span &data);\n> void by_value(Span data);\n>\n> void test_function()\n> {\n> Span data{ reinterpret_cast(0x01234567), 0x89abcdef };\n>\n> by_pointer(&data);\n> by_reference(data);\n> by_value(data);\n> }\n>\n> }\n> --------\n>\n> Compiled with -O0 and disassembled with\n>\n> $ objdump -d -r --demangle src/libcamera/libcamera.so.0.0.0.p/reference.cpp.o\n>\n> produces\n>\n> --------\n> 0000000000000000 :\n> 0: 55 push %rbp\n> 1: 48 89 e5 mov %rsp,%rbp\n> 4: 48 83 ec 20 sub $0x20,%rsp\n> 8: 64 48 8b 04 25 28 00 mov %fs:0x28,%rax\n> f: 00 00\n> 11: 48 89 45 f8 mov %rax,-0x8(%rbp)\n> 15: 31 c0 xor %eax,%eax\n> 17: 48 8d 45 e0 lea -0x20(%rbp),%rax\n> 1b: ba ef cd ab 89 mov $0x89abcdef,%edx\n> 20: be 67 45 23 01 mov $0x1234567,%esi\n> 25: 48 89 c7 mov %rax,%rdi\n> 28: e8 00 00 00 00 call 2d \n> 29: R_X86_64_PLT32 libcamera::Span::Span(unsigned char*, unsigned long)-0x4\n> 2d: 48 8d 45 e0 lea -0x20(%rbp),%rax\n> 31: 48 89 c7 mov %rax,%rdi\n> 34: e8 00 00 00 00 call 39 \n> 35: R_X86_64_PLT32 libcamera::by_pointer(libcamera::Span*)-0x4\n> 39: 48 8d 45 e0 lea -0x20(%rbp),%rax\n> 3d: 48 89 c7 mov %rax,%rdi\n> 40: e8 00 00 00 00 call 45 \n> 41: R_X86_64_PLT32 libcamera::by_reference(libcamera::Span&)-0x4\n> 45: 48 8b 55 e0 mov -0x20(%rbp),%rdx\n> 49: 48 8b 45 e8 mov -0x18(%rbp),%rax\n> 4d: 48 89 d7 mov %rdx,%rdi\n> 50: 48 89 c6 mov %rax,%rsi\n> 53: e8 00 00 00 00 call 58 \n> 54: R_X86_64_PLT32 libcamera::by_value(libcamera::Span)-0x4\n> 58: 90 nop\n> 59: 48 8b 45 f8 mov -0x8(%rbp),%rax\n> 5d: 64 48 2b 04 25 28 00 sub %fs:0x28,%rax\n> 64: 00 00\n> 66: 74 05 je 6d \n> 68: e8 00 00 00 00 call 6d \n> 69: R_X86_64_PLT32 __stack_chk_fail-0x4\n> 6d: c9 leave\n> 6e: c3 ret\n> --------\n>\n> As you can see, both by_pointer() and by_reference() get passed the\n> address of the data variable in rdi, while by_value() get the contents\n> of the Span in rdi and rsi.\n>\n> Obviously this only works if the number of parameters to the function is\n> low enough to fit everything in registers, and it only helps if the\n> caller has the Span data and size in registers already, as if it has to\n> load them from memory, the callee could do so as well.\n>\n> I any case, I agree it's not a big optimization (if at all). The second\n> point I mentioned (Span) is more important.\n>\n> > }\n> >\n> > Output:\n> >\n> > 16\n> > 8\n> > 16\n> >\n> > Even though pass by value and pass by reference are the same amount of\n> > bytes (both 16 bytes and the pointer is 8 on x86), I would expect pass\n> > by reference in the worst case to be of equal performance because it\n> > ensures no constructor of any kind is called (where another copy or\n> > two could occur in pass by value, I don't even need to read the\n> > constructor's source code if it's a reference, that's why class/struct\n> > as reference parameter is a great rule of thumb). But I'm not sure\n> > something like this is worth a v6 either, so it's fine the way it is I\n> > guess.\n> >\n> > >\n> > > - Span to indicate that the function doesn't modify the\n> > > data. A const Span prevents changing the Span itself, but\n> > > allows modifying the data that the Span points to.\n> > >\n> > > This would however require changing the update() function accordingly,\n> > > so I'll submit a patch on top.\n> > >\n> > > I've made those modifications locally to test and make sure I wasn't\n> > > saying something stupid, so I'll send a v5 shortly as it's ready.\n> >\n> > Gonna run a test.\n> >\n> > > > {\n> > > > + struct jpeg_decompress_struct cinfo;\n> > > > + JpegErrorManager jpegErrorManager(cinfo);\n> > > > + if (setjmp(jpegErrorManager.escape_)) {\n> > > > + /* libjpeg found an error */\n> > > > + jpeg_destroy_decompress(&cinfo);\n> > > > + std::cerr << \"JPEG decompression error\" << std::endl;\n> > > > + return -EINVAL;\n> > > > + }\n> > > > +\n> > > > + jpeg_create_decompress(&cinfo);\n> > > > +\n> > > > + jpeg_mem_src(&cinfo, data.data(), data.size());\n> > > > +\n> > > > + jpeg_read_header(&cinfo, TRUE);\n> > > > +\n> > > > + jpeg_start_decompress(&cinfo);\n> > > > +\n> > > > + for (int i = 0; cinfo.output_scanline < cinfo.output_height; ++i) {\n> > > > + JSAMPROW rowptr = rgb_.get() + i * pitch_;\n> > > > + jpeg_read_scanlines(&cinfo, &rowptr, 1);\n> > > > + }\n> > > > +\n> > > > + jpeg_finish_decompress(&cinfo);\n> > > > +\n> > > > + jpeg_destroy_decompress(&cinfo);\n> > > > +\n> > > > + return 0;\n> > > > }\n> > > >\n> > > > void SDLTextureMJPG::update(const Span &data)\n> > > > {\n> > > > - SDL_RWops *bufferStream = SDL_RWFromMem(data.data(), data.size());\n> > > > - SDL_Surface *frame = IMG_Load_RW(bufferStream, 0);\n> > > > - SDL_UpdateTexture(ptr_, nullptr, frame->pixels, frame->pitch);\n> > > > - SDL_FreeSurface(frame);\n> > > > + decompress(data);\n> > > > + SDL_UpdateTexture(ptr_, nullptr, rgb_.get(), pitch_);\n> > > > }\n> > > > diff --git a/src/cam/sdl_texture_mjpg.h b/src/cam/sdl_texture_mjpg.h\n> > > > index b103f801..328c45a9 100644\n> > > > --- a/src/cam/sdl_texture_mjpg.h\n> > > > +++ b/src/cam/sdl_texture_mjpg.h\n> > > > @@ -13,5 +13,11 @@ class SDLTextureMJPG : public SDLTexture\n> > > > {\n> > > > public:\n> > > > SDLTextureMJPG(const SDL_Rect &rect);\n> > > > +\n> > > > void update(const libcamera::Span &data) override;\n> > > > +\n> > > > +private:\n> > > > + int decompress(const libcamera::Span &data);\n> > > > +\n> > > > + std::unique_ptr rgb_;\n> > > > };\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A7F34BD1F1\n\tfor ;\n\tFri, 22 Jul 2022 09:33:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E50F163311;\n\tFri, 22 Jul 2022 11:33:37 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D6AD96330B\n\tfor ;\n\tFri, 22 Jul 2022 11:33:36 +0200 (CEST)","from mail-qt1-f200.google.com (mail-qt1-f200.google.com\n\t[209.85.160.200]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id\n\tus-mta-144-DP3SRnSxPtq7kPTl5NqgGw-1; Fri, 22 Jul 2022 05:33:32 -0400","by mail-qt1-f200.google.com with SMTP id\n\ti8-20020ac871c8000000b0031ed35facf3so2583494qtp.0\n\tfor ;\n\tFri, 22 Jul 2022 02:33:32 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658482417;\n\tbh=i1OOAlZrypYDCMF6mnIhyIcY0eRagaPLx58UKoCTTWI=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=SkYoMExMHXFs2mnaAXFdX0yjwHw61NwuczdNavVbFfUPV9tNOrncyQA35qjlFn3rx\n\tgqTnl+sZKtVeVkh/XqGfjc5r7GyMl1LpU05x6V2Ih3gRVGT2ZZUXPB0rxcvt82H9VE\n\ta3TkYbx1szJIRai7El37d7inxOywCupD1F0byXCyoFG6jfaf9m+a1R5D5zmCbZ8fVI\n\t1cmkYxmVZDL7828vGmWVVhvqXTpoiBXXobnQPIBn4ULMrWaT0Jy5Hy6Hij8bEBKA2I\n\t1VfjJpwyPJnI51ToLmYtykehJfqIyq8q01G8u29Ok5P4wLr9lRPI5TfCb7F8iOfeYe\n\t3g7+73NsEoJMg==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1658482415;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=dttLA/GcPUFeZRCTEvVITiV/sWxKgkidDhJ3QHnDTYU=;\n\tb=QoGm0u2XQtUcDFghFoR8w7AzZCOoNpqNfaxQuSqSAnwqZsrzrg4s/A9ejPx47ziuBXW6KO\n\taMtR4lTqrzOxR3DaRJtVIToxbPv2gsaloMKqBC+MPhKS+1PATEZpAR7PSxsft5xUs/1Bnp\n\trVGlAxi7C9MZagF9V9cIFcAtidUA2x0="],"Authentication-Results":["lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=redhat.com\n\theader.i=@redhat.com header.b=\"QoGm0u2X\"; \n\tdkim-atps=neutral","relay.mimecast.com;\n\tauth=pass smtp.auth=CUSA124A263 smtp.mailfrom=ecurtin@redhat.com"],"X-MC-Unique":"DP3SRnSxPtq7kPTl5NqgGw-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=dttLA/GcPUFeZRCTEvVITiV/sWxKgkidDhJ3QHnDTYU=;\n\tb=7OooHOFCq1YFJGqLf8/Ca64zFQ9sBG89kYfGtLyUS38Zwe2U99pjZw1OFID6TMLcbp\n\tkGbAWV7EKsoUYVMuGfnlWBNfNX1tbjvOYy2XCE4UvLQztIIReWJEf3NgAhJw7FrNkgqi\n\t5UCZN7WjHQgDyPmgX3qLx2lSrYb01Hpv+gb5OdDiQRHAXqzXecSLDxQ6b7wP6woYE3tL\n\tsDc0oMjBichi304POb5CxoeoN5ZmAXGhV1AGbhgEO5dRTuyHyESYVKv7GgohX+onVHEJ\n\t0Bkt/GmAgijP7P/te3M6sK8t9gnwubANJ2s/gpQuC1QiNcbE9s9VzOg8Wy3Eim2O5n6o\n\tgnTA==","X-Gm-Message-State":"AJIora/Ygk6vICIbIAHOPNkVsnMCPpAcEEAjUnjpw+wKPzHCzXyShtgL\n\tb4DBASHJWEb3EYwsQJT2cnTbfrciD0nSAmfAGWohOwk1zGQZgVHZxUB+LxDEECLPedK0TJL6pwy\n\tXc82lwJM0r72xiY9OxE3o7GI+gl0q5yOAgj1qi2XgY835wUOleA==","X-Received":["by 2002:ae9:de84:0:b0:6af:a5a:ccd6 with SMTP id\n\ts126-20020ae9de84000000b006af0a5accd6mr1734701qkf.686.1658482411806; \n\tFri, 22 Jul 2022 02:33:31 -0700 (PDT)","by 2002:ae9:de84:0:b0:6af:a5a:ccd6 with SMTP id\n\ts126-20020ae9de84000000b006af0a5accd6mr1734690qkf.686.1658482411407;\n\tFri, 22 Jul 2022 02:33:31 -0700 (PDT)"],"X-Google-Smtp-Source":"AGRyM1tKxgOZ4yst00wrFp+2l9FigNTRsHcoib1/yyUWDQ8FJs1k3nNMRT54Tgeegyc16GyamTbj8ba+qQMWxYjY6sk=","MIME-Version":"1.0","References":"<20220719091745.8040-1-ecurtin@redhat.com>\n\t\n\t\n\t","In-Reply-To":"","Date":"Fri, 22 Jul 2022 10:32:46 +0100","Message-ID":"","To":"Laurent Pinchart ","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v4] cam: sdl_sink: Use libjpeg over\n\tSDL2_image","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Eric Curtin via libcamera-devel ","Reply-To":"Eric Curtin ","Cc":"Ian Mullins ,\n\tlibcamera devel , jozef@mlich.cz","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]