[{"id":23944,"web_url":"https://patchwork.libcamera.org/comment/23944/","msgid":"","date":"2022-07-17T20:41:25","subject":"Re: [libcamera-devel] [PATCH v2] 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 Fri, Jul 15, 2022 at 12:36:05PM +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 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/meson.build | 8 ++---\n> src/cam/sdl_sink.cpp | 4 +--\n> src/cam/sdl_texture_mjpg.cpp | 63 ++++++++++++++++++++++++++++++++----\n> src/cam/sdl_texture_mjpg.h | 6 ++++\n> 5 files changed, 70 insertions(+), 13 deletions(-)\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/meson.build b/src/cam/meson.build\n> index 5957ce14..4dfa7b22 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,8 +43,8 @@ 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> 'sdl_texture_mjpg.cpp'\n> ])\n> @@ -57,8 +57,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..a064e6e5 100644\n> --- a/src/cam/sdl_texture_mjpg.cpp\n> +++ b/src/cam/sdl_texture_mjpg.cpp\n> @@ -7,19 +7,70 @@\n> \n> #include \"sdl_texture_mjpg.h\"\n> \n> -#include \n> +#include \n> +#include \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> +struct error_mgr {\n> +\tstruct jpeg_error_mgr errmgr;\n> +\tjmp_buf escape;\n> +};\n> +\n> +static void error_exit(j_common_ptr cinfo)\n\nThis function should be marked with [[noreturn]].\n\n> +{\n> +\tstruct error_mgr *err = (struct error_mgr *)cinfo->err;\n\n\tstruct error_mgr *err = static_cast(cinfo->err);\n\n> +\tlongjmp(err->escape, 1);\n> +}\n> +\n> +static void output_no_message(j_common_ptr cinfo)\n\nYou can write\n\nstatic void output_no_message([[maybed_unused]] j_common_ptr cinfo)\n\n> +{\n> +\t(void)cinfo;\n\nand drop this.\n\n> +}\n\nHow about grouping the structure and functions together, as we use C++ ?\n\nstruct JpegErrorManager {\n\tJpegErrorManager()\n\t{\n\t\tjpeg_std_error(&errmgr);\n\t\terrmgr.error_exit = errorExit;\n\t\terrmgr.output_message = outputMessage;\n\t}\n\n\t[[noreturn]] static void errorExit(j_common_ptr cinfo)\n\t{\n\t\tJpegErrorManager *self = static_cast(cinfo->err);\n\t\tlongjmp(self->escape, 1);\n\t}\n\n\tstatic void outputMessage([[maybed_unused]] j_common_ptr cinfo)\n\t{\n\t}\n\n\tstruct jpeg_error_mgr errmgr;\n\tjmp_buf escape;\n}\n\nI think that's longjmp-safe as there's nothing to do at destruction\ntime.\n\n> +\n> +void SDLTextureMJPG::decompress(const unsigned char *jpeg,\n> +\t\t\t\tunsigned long jpeg_size)\n\nThe function should take a Span instead of separate data\npointer and size.\n\n> {\n> +\tstruct error_mgr jerr;\n> +\tstruct jpeg_decompress_struct cinfo;\n> +\tcinfo.err = jpeg_std_error(&jerr.errmgr);\n> +\tjerr.errmgr.error_exit = error_exit;\n> +\tjerr.errmgr.output_message = output_no_message;\n\nThis would become\n\n\tstruct jpeg_decompress_struct cinfo;\n\tJpegErrorManager jerr;\n\n\tcinfo.err = &jerr.errmgr;\n\nAnd I'd add a blank line here for clarity.\n\nIf you'd rather avoid the JpegErrorManager, I would write\n\n\tstruct jpeg_decompress_struct cinfo;\n\n\tstruct error_mgr jerr;\n\tjpeg_std_error(&jerr.errmgr);\n\tjerr.errmgr.error_exit = error_exit;\n\tjerr.errmgr.output_message = output_no_message;\n\n\tif (setjmp(jerr.escape)) {\n\t\t...\n\t}\n\n\tcinfo.err = &jerr.errmgr;\n\n> +\tif (setjmp(jerr.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;\n\nLet's return an error from this function.\n\n> +\t}\n> +\n> +\tjpeg_create_decompress(&cinfo);\n\nThe decompressor and the error manager don't need to be recreated for\neach image. Optimizing this would require substantial changes though, as\nwe would need to create the JPEG encoder separately from the texture, so\nthis can be done later.\n\n> +\n> +\tjpeg_mem_src(&cinfo, jpeg, jpeg_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> \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.data(), data.size());\n\nWhat should we do here if an error occurred during decompression ? We\ncould proceed nonetheless (which will result in some garbage being\ndisplayed), or return an error from this function and skip the\nSDL_Render* calls in the caller. What do you think would be best ?\n\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..c68b03b0 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> +\tvoid decompress(const unsigned char *jpeg, unsigned long jpeg_size);\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 C0AF7BE173\n\tfor ;\n\tSun, 17 Jul 2022 20:42:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 10DBA63312;\n\tSun, 17 Jul 2022 22:42:00 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3B08A61FAF\n\tfor ;\n\tSun, 17 Jul 2022 22:41:58 +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 86EE674C;\n\tSun, 17 Jul 2022 22:41:57 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658090520;\n\tbh=NUkCVgGvCZMdYlNZ1pdJHErG4Pg/vj+AU4wn+yXS+p8=;\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=RD4jw9C3HTlrl+n7ZcT3aPjY+ODdwCz2sDWLw/st4OJDoagfLAhmcDMNSrLQpVR4H\n\tp8/xaGjxMzd4EdqqdafNsqDHe2immyRENRNey2BUwBnoUifXcKjUhMHCkO7vDFwAHX\n\tUG5Fzgi3/tuafVor458PG8yI8sgGsJrE738NYUXxXpyzWzUP8crsal+nxVf2lVPlmJ\n\tQudBGw8IcQWksShWITFQLyVv38Jtd6v1b4FYg7PG6f2tgiZtZwYbtVRpQOtLVPbE48\n\tMJySeQoavfy5EnIPXbaA/4HRCrwV0sTDJJwpg8SuggzcS/V5+jiJYSXiyL4+Btc8fo\n\tRSP71cl3D1J8A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1658090517;\n\tbh=NUkCVgGvCZMdYlNZ1pdJHErG4Pg/vj+AU4wn+yXS+p8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=RU3DMOrarTnNnKv2r85vXf/Cih1HfvKV0YokAMbJuXfid0ACdDsiqjzMA1qD1FYm6\n\tppOfn5p/Zq4Gz7lcLLFcGG+yjaCOBNheHpV8k3S4mmhd54jTGGW2em8+YcinbdjMYr\n\tXsXWoXjnBeCYLvdWAxhN3N/TYECn0lz9cyuEPv2s="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"RU3DMOra\"; dkim-atps=neutral","Date":"Sun, 17 Jul 2022 23:41:25 +0300","To":"Eric Curtin ","Message-ID":"","References":"<20220715113604.10131-1-ecurtin@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220715113604.10131-1-ecurtin@redhat.com>","Subject":"Re: [libcamera-devel] [PATCH v2] 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":23948,"web_url":"https://patchwork.libcamera.org/comment/23948/","msgid":"","date":"2022-07-18T10:13:31","subject":"Re: [libcamera-devel] [PATCH v2] 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 Sun, 17 Jul 2022 at 21:42, Laurent Pinchart\n wrote:\n>\n> Hi Eric,\n>\n> Thank you for the patch.\n>\n> On Fri, Jul 15, 2022 at 12:36:05PM +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 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/meson.build | 8 ++---\n> > src/cam/sdl_sink.cpp | 4 +--\n> > src/cam/sdl_texture_mjpg.cpp | 63 ++++++++++++++++++++++++++++++++----\n> > src/cam/sdl_texture_mjpg.h | 6 ++++\n> > 5 files changed, 70 insertions(+), 13 deletions(-)\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/meson.build b/src/cam/meson.build\n> > index 5957ce14..4dfa7b22 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,8 +43,8 @@ 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> > 'sdl_texture_mjpg.cpp'\n> > ])\n> > @@ -57,8 +57,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..a064e6e5 100644\n> > --- a/src/cam/sdl_texture_mjpg.cpp\n> > +++ b/src/cam/sdl_texture_mjpg.cpp\n> > @@ -7,19 +7,70 @@\n> >\n> > #include \"sdl_texture_mjpg.h\"\n> >\n> > -#include \n> > +#include \n> > +#include \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> > +struct error_mgr {\n> > + struct jpeg_error_mgr errmgr;\n> > + jmp_buf escape;\n> > +};\n> > +\n> > +static void error_exit(j_common_ptr cinfo)\n>\n> This function should be marked with [[noreturn]].\n>\n> > +{\n> > + struct error_mgr *err = (struct error_mgr *)cinfo->err;\n>\n> struct error_mgr *err = static_cast(cinfo->err);\n\nThis has to be reinterpret_cast, that seem dangerous but it's really\nnot, it's basically used to add jmp_buf to jpeg_error_mgr struct. It's\nkinda being used to fake inheritance in C, without the api supporting\nit.\n\n>\n> > + longjmp(err->escape, 1);\n> > +}\n> > +\n> > +static void output_no_message(j_common_ptr cinfo)\n>\n> You can write\n>\n> static void output_no_message([[maybed_unused]] j_common_ptr cinfo)\n>\n> > +{\n> > + (void)cinfo;\n>\n> and drop this.\n>\n> > +}\n>\n> How about grouping the structure and functions together, as we use C++ ?\n\nI guess we can.\n\n>\n> struct JpegErrorManager {\n> JpegErrorManager()\n> {\n> jpeg_std_error(&errmgr);\n> errmgr.error_exit = errorExit;\n> errmgr.output_message = outputMessage;\n> }\n>\n> [[noreturn]] static void errorExit(j_common_ptr cinfo)\n> {\n> JpegErrorManager *self = static_cast(cinfo->err);\n> longjmp(self->escape, 1);\n> }\n>\n> static void outputMessage([[maybed_unused]] j_common_ptr cinfo)\n> {\n> }\n>\n> struct jpeg_error_mgr errmgr;\n> jmp_buf escape;\n> }\n>\n> I think that's longjmp-safe as there's nothing to do at destruction\n> time.\n>\n> > +\n> > +void SDLTextureMJPG::decompress(const unsigned char *jpeg,\n> > + unsigned long jpeg_size)\n>\n> The function should take a Span instead of separate data\n> pointer and size.\n>\n> > {\n> > + struct error_mgr jerr;\n> > + struct jpeg_decompress_struct cinfo;\n> > + cinfo.err = jpeg_std_error(&jerr.errmgr);\n> > + jerr.errmgr.error_exit = error_exit;\n> > + jerr.errmgr.output_message = output_no_message;\n>\n> This would become\n>\n> struct jpeg_decompress_struct cinfo;\n> JpegErrorManager jerr;\n>\n> cinfo.err = &jerr.errmgr;\n>\n> And I'd add a blank line here for clarity.\n>\n> If you'd rather avoid the JpegErrorManager, I would write\n>\n> struct jpeg_decompress_struct cinfo;\n>\n> struct error_mgr jerr;\n> jpeg_std_error(&jerr.errmgr);\n> jerr.errmgr.error_exit = error_exit;\n> jerr.errmgr.output_message = output_no_message;\n>\n> if (setjmp(jerr.escape)) {\n> ...\n> }\n>\n> cinfo.err = &jerr.errmgr;\n>\n> > + if (setjmp(jerr.escape)) {\n> > + /* libjpeg found an error */\n> > + jpeg_destroy_decompress(&cinfo);\n> > + std::cerr << \"JPEG decompression error\" << std::endl;\n> > + return;\n>\n> Let's return an error from this function.\n>\n> > + }\n> > +\n> > + jpeg_create_decompress(&cinfo);\n>\n> The decompressor and the error manager don't need to be recreated for\n> each image. Optimizing this would require substantial changes though, as\n> we would need to create the JPEG encoder separately from the texture, so\n> this can be done later.\n>\n> > +\n> > + jpeg_mem_src(&cinfo, jpeg, jpeg_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> >\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.data(), data.size());\n>\n> What should we do here if an error occurred during decompression ? We\n> could proceed nonetheless (which will result in some garbage being\n> displayed), or return an error from this function and skip the\n> SDL_Render* calls in the caller. What do you think would be best ?\n\nI'm unsure, if say the last two scanlines were garbage, maybe you\nshould still display the frame (but maybe this sort of case is\nunlikely when the data is corrupt), for simplicity was gonna display\nanyway, No harm in returning non-zero I guess in this case though, if\nsomeone decides that's useful in future.\n\n\n>\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..c68b03b0 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> > + void decompress(const unsigned char *jpeg, unsigned long jpeg_size);\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 10D13BD1F1\n\tfor ;\n\tMon, 18 Jul 2022 10:13:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 66AD56048B;\n\tMon, 18 Jul 2022 12:13:52 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A6C156048B\n\tfor ;\n\tMon, 18 Jul 2022 12:13:50 +0200 (CEST)","from mail-qk1-f200.google.com (mail-qk1-f200.google.com\n\t[209.85.222.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-197-0rtbi4aJNCC26V_4QE4Nig-1; Mon, 18 Jul 2022 06:13:48 -0400","by mail-qk1-f200.google.com with SMTP id\n\tt203-20020a3746d4000000b006af1d3e8068so9055116qka.0\n\tfor ;\n\tMon, 18 Jul 2022 03:13:48 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658139232;\n\tbh=6PuYr9b8XPpm67u7oZfS0L4Q4nQf/pjnHFq9J4rlN68=;\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=INttCbPJ7rbvWSCol6vxB9zppIekEV3sRxxQk14F9RvCa004d3X0Ao1CJRi993gpQ\n\t8cpyAnpMeZNnCnFr0pWi0RtV2rdfHCcMflFlHFkKrVLsEX2coAcje83l/qjRhtw0mv\n\tEQlZOsLHHY6kNo6fKnGmhII0G+ayiTQWaZZTpqyLBdp8sbJvqHPV3xes4O5pLrH+2a\n\tTKhKL6xdOP3SOdXwxUeNlC2a2m3CpTziuCiLEDOytKx7Ny5y/YUmEHOSThp/X7QeFD\n\tVeIQOa5NW9U3j1MdbCs1BJhaQ36yWcayrtQNAysGsfkSUIXwYUBa5v4j1oJJveH6H+\n\tbPbzdMV61nY5g==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1658139229;\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=/wJUEmgADCVqqB+ydjmYdVHZ3XmnGTjYVbRSVHWWW0s=;\n\tb=dT2AO1kV7B1Klc1ss1TAl6JS5qwKpR1GL+NN3tihDYKEz6u5rXuY9JX3t5bAz3FrynJrUG\n\t2ISsZqgie/zukqt5Jr0dYMAS1IGtJpVCucYO4vNzWjbqTYmlxe1gyFBW22356RyBuXhLxn\n\tVGTvTHsXmQ2sH1gJ4cvINzWb4bZs0AU="],"Authentication-Results":["lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=redhat.com\n\theader.i=@redhat.com header.b=\"dT2AO1kV\"; \n\tdkim-atps=neutral","relay.mimecast.com;\n\tauth=pass smtp.auth=CUSA124A263 smtp.mailfrom=ecurtin@redhat.com"],"X-MC-Unique":"0rtbi4aJNCC26V_4QE4Nig-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=/wJUEmgADCVqqB+ydjmYdVHZ3XmnGTjYVbRSVHWWW0s=;\n\tb=uKUb6ueEqp9Veo0E5ZdOLBgpfNLgh4p1aiQ54YyvfnzPO5AnASVBtKdyR7Fx2/M0XG\n\t4OM0ox5aRcSy0teOSsE2USUvNVaw5ycw8nCwoRV69tLAY2lnygMNYKJz6DYvEt4UiPh4\n\tgVAYqlGXbnPM9S7p1EeZVPB4Uh3PH3CubnuiMjIE2Mq4mldF8NonEf5u1nhshjDHntmt\n\t0wXhGKyWG6neJomrcCa4BS5HbRCsKds9WxVW9GFJZyfqYj0RkBA7pRu3JkmZ3hOaU7Xo\n\tEv1xDK1Usu9hTfR/+NURGCfYbscq3Aeddkf9gYj/aP+83IGr5RLDb7MxbJ+PeHiakFce\n\tsiGA==","X-Gm-Message-State":"AJIora8vptITtVqgLHhlXk5weQge7LODv59elWO6uF2hVl/lb9m6gtQF\n\timLEbL2imMiWTQksajpxf6rtjbApjGJnN6Z8WFx9HVbEVpO781NhQqWlwHlEz+tj57XEROUgRs0\n\tu+XaZRBzI5Q1q5UzAAw3TlmjQUgx6l8MYoqPGQFA+39DY4eWI6g==","X-Received":["by 2002:ac8:7d07:0:b0:31e:e094:ef12 with SMTP id\n\tg7-20020ac87d07000000b0031ee094ef12mr9268965qtb.301.1658139227610; \n\tMon, 18 Jul 2022 03:13:47 -0700 (PDT)","by 2002:ac8:7d07:0:b0:31e:e094:ef12 with SMTP id\n\tg7-20020ac87d07000000b0031ee094ef12mr9268951qtb.301.1658139227269;\n\tMon, 18 Jul 2022 03:13:47 -0700 (PDT)"],"X-Google-Smtp-Source":"AGRyM1u7iZBaAe1OdvQ8tOcFcr1+54Fx7IXGJbtp5pIHFwnUDdqiDLh7GDXFFZ5xDIGsRcJPlACzUS8jL9wxwnF1Jhc=","MIME-Version":"1.0","References":"<20220715113604.10131-1-ecurtin@redhat.com>\n\t","In-Reply-To":"","Date":"Mon, 18 Jul 2022 11:13:31 +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 v2] 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\" "}}]