[{"id":23797,"web_url":"https://patchwork.libcamera.org/comment/23797/","msgid":"","date":"2022-07-07T20:13:08","subject":"Re: [libcamera-devel] [PATCH] 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 Thu, Jul 07, 2022 at 04:53:13PM +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> README.rst | 2 +-\n> src/cam/meson.build | 8 +++----\n> src/cam/sdl_sink.cpp | 4 ++--\n> src/cam/sdl_texture.h | 2 +-\n> src/cam/sdl_texture_mjpg.cpp | 43 +++++++++++++++++++++++++++++++-----\n> src/cam/sdl_texture_mjpg.h | 5 +++++\n> 6 files changed, 51 insertions(+), 13 deletions(-)\n> \n> diff --git a/README.rst b/README.rst\n> index b9e72d81..5e8bc1cc 100644\n> --- a/README.rst\n> +++ b/README.rst\n> @@ -93,7 +93,7 @@ for cam: [optional]\n> \n> - libdrm-dev: Enables the KMS sink\n> - libsdl2-dev: Enables the SDL sink\n> - - libsdl2-image-dev: Supports MJPEG on the SDL sink\n> + - libjpeg-dev: Supports MJPEG on the SDL sink\n\nCould you please keep this list sorted ?\n\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..27cbd0de 100644\n> --- a/src/cam/meson.build\n> +++ b/src/cam/meson.build\n> @@ -25,7 +25,7 @@ cam_cpp_args = []\n> \n> libdrm = dependency('libdrm', required : false)\n> libsdl2 = dependency('SDL2', required : false)\n> -libsdl2_image = dependency('SDL2_image', required : false)\n> +libjpeg = dependency('libjpeg', required : false)\n\nSame here.\n\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> @@ -58,7 +58,7 @@ cam = executable('cam', cam_sources,\n> libdrm,\n> libevent,\n> libsdl2,\n> - libsdl2_image,\n> + libjpeg,\n\nAnd here.\n\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.h b/src/cam/sdl_texture.h\n> index 90974798..42c906f2 100644\n> --- a/src/cam/sdl_texture.h\n> +++ b/src/cam/sdl_texture.h\n> @@ -25,5 +25,5 @@ protected:\n> \tSDL_Texture *ptr_;\n> \tconst SDL_Rect rect_;\n> \tconst SDL_PixelFormatEnum pixelFormat_;\n> -\tconst int pitch_;\n> +\tint pitch_;\n> };\n> diff --git a/src/cam/sdl_texture_mjpg.cpp b/src/cam/sdl_texture_mjpg.cpp\n> index 69e99ad3..232af673 100644\n> --- a/src/cam/sdl_texture_mjpg.cpp\n> +++ b/src/cam/sdl_texture_mjpg.cpp\n> @@ -7,19 +7,52 @@\n> \n> #include \"sdl_texture_mjpg.h\"\n> \n> -#include \n> +#include \n> \n> using namespace libcamera;\n> \n> SDLTextureMJPG::SDLTextureMJPG(const SDL_Rect &rect)\n> \t: SDLTexture(rect, SDL_PIXELFORMAT_RGB24, 0)\n\nYou can write\n\n \t: SDLTexture(rect, SDL_PIXELFORMAT_RGB24, rect.w * 3)\n\n> {\n> +\tpitch_ = rect_.w * 3;\n\nand drop this line, and keep pitch_ const.\n\n> +\trgbdata_ = (unsigned char *)malloc(pitch_ * rect_.h);\n\nPlease use C++-style memory allocation\n\n\trgbdata_ = new unsigned char[pitch_ * rect_.h];\n\nThe destructor should then use\n\n\tdelete[] rgbdata_;\n\nEven better, I think you can turn rgbdata_ into a\n\n\tstd::unique_ptr rgbdata_;\n\nand drop the delete.\n\nAnother option would have been to turn rgbdata_ into an\nstd::vector to also remove the need for a custom\ndestructor. The drawback is that, unlike new that performs\ndefault-initialization, std::vector::resize() performs\ndefault-insertion, which uses value-initialization, effectively\nmemsetting the vector to 0. There's a way around that by providing a\ncustom allocator to the vector, but I think we're getting into the \"not\nworth it\" territory (at least as long as libcamera-base doesn't provide\na helper for this). Let's thus use a unique_ptr.\n\n> +}\n> +\n> +SDLTextureMJPG::~SDLTextureMJPG()\n> +{\n> +\tfree(rgbdata_);\n> +}\n> +\n> +void SDLTextureMJPG::decompress(const unsigned char *jpeg,\n> +\t\t\t\tunsigned long jpeg_size)\n> +{\n> +\tstruct jpeg_error_mgr jerr;\n> +\tstruct jpeg_decompress_struct cinfo;\n> +\tcinfo.err = jpeg_std_error(&jerr);\n\nThere's a risk the JPEG data could be corrupted (that's quite common\nwith UVC cameras). Error handling should be a bit more robust, you\nshould create your own error handler, store that an error occurred, and\nreturn an error from this function and skip the SDL_UpdateTexture() in\nthat case.\n\nFurthermore, jpg_std_error() will by default call exit() when a fatal\nerror occurs. That's not good. The recommended way to deal with this is\nto use setjmp() and longjmp() to exit cleanly instead.\n\n> +\n> +\tjpeg_create_decompress(&cinfo);\n> +\n> +\tjpeg_mem_src(&cinfo, jpeg, jpeg_size);\n> +\n> +\tjpeg_read_header(&cinfo, TRUE);\n> +\n> +\tjpeg_start_decompress(&cinfo);\n> +\n> +\tint row_stride = cinfo.output_width * cinfo.output_components;\n\nrowStride (libcamera coding style)\n\n> +\tunsigned char *buffer = (unsigned char *)malloc(row_stride);\n\nHere too, std::unique_ptr and new[], or just\n\n\tunsigned char buffer[row_stride);\n\n> +\tfor (int i = 0; cinfo.output_scanline < cinfo.output_height; ++i) {\n> +\t\tjpeg_read_scanlines(&cinfo, &buffer, 1);\n> +\t\tmemcpy(rgbdata_ + i * pitch_, buffer, row_stride);\n> +\t}\n> +\n> +\tfree(buffer);\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> +\tSDL_UpdateTexture(ptr_, nullptr, rgbdata_, pitch_);\n> }\n> diff --git a/src/cam/sdl_texture_mjpg.h b/src/cam/sdl_texture_mjpg.h\n> index b103f801..73c7fb68 100644\n> --- a/src/cam/sdl_texture_mjpg.h\n> +++ b/src/cam/sdl_texture_mjpg.h\n> @@ -13,5 +13,10 @@ class SDLTextureMJPG : public SDLTexture\n> {\n> public:\n> \tSDLTextureMJPG(const SDL_Rect &rect);\n> +\tvirtual ~SDLTextureMJPG();\n\nNo need for virtual here, and while at it, you can insert a blank line.\n\n> \tvoid update(const libcamera::Span &data) override;\n> +\n> +private:\n> +\tunsigned char *rgbdata_ = 0;\n\nMember data after member functions. I'd name the member rgbData_ or just\nrgb_, up to you.\n\n> +\tvoid decompress(const unsigned char *jpeg, unsigned long jpeg_size);\n\n\tvoid decompress(const unsigned char *jpeg, std::size size);\n\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 2049BBD1F1\n\tfor ;\n\tThu, 7 Jul 2022 20:13:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 488F76330D;\n\tThu, 7 Jul 2022 22:13:36 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EC0C960400\n\tfor ;\n\tThu, 7 Jul 2022 22:13:34 +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 4DABAD00;\n\tThu, 7 Jul 2022 22:13:34 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1657224816;\n\tbh=OHVkjeJ8krggakq+pqMPPDgJkOMx9soAZfmgSVf8JOg=;\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=isgOmMlq5xY00z3oqNKsa2rlwb7wZkHwsvjE/6f5HEqy7YmWzf273DSd+pmrW85hA\n\t/4Cjc1sqjXly3YNdZ/kz4b4JQs2zOCbyKg2blQ6qRSJJLdsOfvfnbPbOFLkGQP7seD\n\tO0+UVxAsjQ0+OFbkE8U8Wzt2630VmDVaCu15Tz5232eEcY2ak24iZT+1tKWqlIj7Ty\n\tLTS0z6qApOP7gpFRXb1mzkCYvM7J4TRgTVIpOxC6hKhNAGk6fSmWueQ2/5kMWZK/c8\n\thMtQGwYs/KJHNHNoFyYDGPpRkMd/419nNCmdBuIvsIx/uHbZwgnXiDnPfaM9xOqL2k\n\tX2FdomdBC1K7Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1657224814;\n\tbh=OHVkjeJ8krggakq+pqMPPDgJkOMx9soAZfmgSVf8JOg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=E++p0pOGUejibVnsxoSBKN2O3ZbVh3fPcP5TERwT/MzMlYER/7iuRKv4oF3fMFn79\n\tmRaiRmN1rsnUnv33e2TVYdgjSBeZtyFlWqMK5KA2px1y7uT82h9q+gJSeBWq50wAx5\n\txeTQSEXuOG9jG3TSOroWIMjfTetKJQS5JcHQ0i9g="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"E++p0pOG\"; dkim-atps=neutral","Date":"Thu, 7 Jul 2022 23:13:08 +0300","To":"Eric Curtin ","Message-ID":"","References":"<20220707155312.45807-1-ecurtin@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220707155312.45807-1-ecurtin@redhat.com>","Subject":"Re: [libcamera-devel] [PATCH] 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":"libcamera-devel@lists.libcamera.org, jozef@mlich.cz","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23803,"web_url":"https://patchwork.libcamera.org/comment/23803/","msgid":"","date":"2022-07-07T22:25:58","subject":"Re: [libcamera-devel] [PATCH] 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, 7 Jul 2022 at 21:13, Laurent Pinchart\n wrote:\n>\n> Hi Eric,\n>\n> Thank you for the patch.\n>\n> On Thu, Jul 07, 2022 at 04:53:13PM +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> > README.rst | 2 +-\n> > src/cam/meson.build | 8 +++----\n> > src/cam/sdl_sink.cpp | 4 ++--\n> > src/cam/sdl_texture.h | 2 +-\n> > src/cam/sdl_texture_mjpg.cpp | 43 +++++++++++++++++++++++++++++++-----\n> > src/cam/sdl_texture_mjpg.h | 5 +++++\n> > 6 files changed, 51 insertions(+), 13 deletions(-)\n> >\n> > diff --git a/README.rst b/README.rst\n> > index b9e72d81..5e8bc1cc 100644\n> > --- a/README.rst\n> > +++ b/README.rst\n> > @@ -93,7 +93,7 @@ for cam: [optional]\n> >\n> > - libdrm-dev: Enables the KMS sink\n> > - libsdl2-dev: Enables the SDL sink\n> > - - libsdl2-image-dev: Supports MJPEG on the SDL sink\n> > + - libjpeg-dev: Supports MJPEG on the SDL sink\n>\n> Could you please keep this list sorted ?\n>\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..27cbd0de 100644\n> > --- a/src/cam/meson.build\n> > +++ b/src/cam/meson.build\n> > @@ -25,7 +25,7 @@ cam_cpp_args = []\n> >\n> > libdrm = dependency('libdrm', required : false)\n> > libsdl2 = dependency('SDL2', required : false)\n> > -libsdl2_image = dependency('SDL2_image', required : false)\n> > +libjpeg = dependency('libjpeg', required : false)\n>\n> Same here.\n>\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> > @@ -58,7 +58,7 @@ cam = executable('cam', cam_sources,\n> > libdrm,\n> > libevent,\n> > libsdl2,\n> > - libsdl2_image,\n> > + libjpeg,\n>\n> And here.\n>\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.h b/src/cam/sdl_texture.h\n> > index 90974798..42c906f2 100644\n> > --- a/src/cam/sdl_texture.h\n> > +++ b/src/cam/sdl_texture.h\n> > @@ -25,5 +25,5 @@ protected:\n> > SDL_Texture *ptr_;\n> > const SDL_Rect rect_;\n> > const SDL_PixelFormatEnum pixelFormat_;\n> > - const int pitch_;\n> > + int pitch_;\n> > };\n> > diff --git a/src/cam/sdl_texture_mjpg.cpp b/src/cam/sdl_texture_mjpg.cpp\n> > index 69e99ad3..232af673 100644\n> > --- a/src/cam/sdl_texture_mjpg.cpp\n> > +++ b/src/cam/sdl_texture_mjpg.cpp\n> > @@ -7,19 +7,52 @@\n> >\n> > #include \"sdl_texture_mjpg.h\"\n> >\n> > -#include \n> > +#include \n> >\n> > using namespace libcamera;\n> >\n> > SDLTextureMJPG::SDLTextureMJPG(const SDL_Rect &rect)\n> > : SDLTexture(rect, SDL_PIXELFORMAT_RGB24, 0)\n>\n> You can write\n>\n> : SDLTexture(rect, SDL_PIXELFORMAT_RGB24, rect.w * 3)\n>\n> > {\n> > + pitch_ = rect_.w * 3;\n>\n> and drop this line, and keep pitch_ const.\n>\n> > + rgbdata_ = (unsigned char *)malloc(pitch_ * rect_.h);\n>\n> Please use C++-style memory allocation\n>\n> rgbdata_ = new unsigned char[pitch_ * rect_.h];\n>\n> The destructor should then use\n>\n> delete[] rgbdata_;\n>\n> Even better, I think you can turn rgbdata_ into a\n>\n> std::unique_ptr rgbdata_;\n>\n> and drop the delete.\n>\n> Another option would have been to turn rgbdata_ into an\n> std::vector to also remove the need for a custom\n> destructor. The drawback is that, unlike new that performs\n> default-initialization, std::vector::resize() performs\n> default-insertion, which uses value-initialization, effectively\n> memsetting the vector to 0. There's a way around that by providing a\n> custom allocator to the vector, but I think we're getting into the \"not\n> worth it\" territory (at least as long as libcamera-base doesn't provide\n> a helper for this). Let's thus use a unique_ptr.\n>\n> > +}\n> > +\n> > +SDLTextureMJPG::~SDLTextureMJPG()\n> > +{\n> > + free(rgbdata_);\n> > +}\n> > +\n> > +void SDLTextureMJPG::decompress(const unsigned char *jpeg,\n> > + unsigned long jpeg_size)\n> > +{\n> > + struct jpeg_error_mgr jerr;\n> > + struct jpeg_decompress_struct cinfo;\n> > + cinfo.err = jpeg_std_error(&jerr);\n>\n> There's a risk the JPEG data could be corrupted (that's quite common\n> with UVC cameras). Error handling should be a bit more robust, you\n> should create your own error handler, store that an error occurred, and\n> return an error from this function and skip the SDL_UpdateTexture() in\n> that case.\n>\n> Furthermore, jpg_std_error() will by default call exit() when a fatal\n> error occurs. That's not good. The recommended way to deal with this is\n> to use setjmp() and longjmp() to exit cleanly instead.\n>\n> > +\n> > + jpeg_create_decompress(&cinfo);\n> > +\n> > + jpeg_mem_src(&cinfo, jpeg, jpeg_size);\n> > +\n> > + jpeg_read_header(&cinfo, TRUE);\n> > +\n> > + jpeg_start_decompress(&cinfo);\n> > +\n> > + int row_stride = cinfo.output_width * cinfo.output_components;\n>\n> rowStride (libcamera coding style)\n>\n> > + unsigned char *buffer = (unsigned char *)malloc(row_stride);\n>\n> Here too, std::unique_ptr and new[], or just\n>\n> unsigned char buffer[row_stride);\n\nFor buffer, I need to pass the address of the variable storing the\npointer to jpeg_read_scanlines, not sure unique_ptr is possible in\nthis very specific case? (although I can easily change rgbdata_ to\nunique_ptr, thanks for spotting that).\n\n\"unsigned char buffer[row_stride];\" won't work for the same reason,\nneeds to be a raw C pointer to pass &buffer to jpeg_read_scanlines.\n\nAll the other feedback makes sense and will be addressed.\n\n>\n> > + for (int i = 0; cinfo.output_scanline < cinfo.output_height; ++i) {\n> > + jpeg_read_scanlines(&cinfo, &buffer, 1);\n> > + memcpy(rgbdata_ + i * pitch_, buffer, row_stride);\n> > + }\n> > +\n> > + free(buffer);\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> > + SDL_UpdateTexture(ptr_, nullptr, rgbdata_, pitch_);\n> > }\n> > diff --git a/src/cam/sdl_texture_mjpg.h b/src/cam/sdl_texture_mjpg.h\n> > index b103f801..73c7fb68 100644\n> > --- a/src/cam/sdl_texture_mjpg.h\n> > +++ b/src/cam/sdl_texture_mjpg.h\n> > @@ -13,5 +13,10 @@ class SDLTextureMJPG : public SDLTexture\n> > {\n> > public:\n> > SDLTextureMJPG(const SDL_Rect &rect);\n> > + virtual ~SDLTextureMJPG();\n>\n> No need for virtual here, and while at it, you can insert a blank line.\n>\n> > void update(const libcamera::Span &data) override;\n> > +\n> > +private:\n> > + unsigned char *rgbdata_ = 0;\n>\n> Member data after member functions. I'd name the member rgbData_ or just\n> rgb_, up to you.\n>\n> > + void decompress(const unsigned char *jpeg, unsigned long jpeg_size);\n>\n> void decompress(const unsigned char *jpeg, std::size size);\n>\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 DDBF7BD1F1\n\tfor ;\n\tThu, 7 Jul 2022 22:26:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D42A26330D;\n\tFri, 8 Jul 2022 00:26:19 +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 C3B0D60403\n\tfor ;\n\tFri, 8 Jul 2022 00:26:17 +0200 (CEST)","from mail-qt1-f198.google.com (mail-qt1-f198.google.com\n\t[209.85.160.198]) 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-395-wkCQtwGvPdq6qa9p3kNpYA-1; Thu, 07 Jul 2022 18:26:15 -0400","by mail-qt1-f198.google.com with SMTP id\n\tp13-20020a05622a048d00b0031e94b7c106so4077864qtx.2\n\tfor ;\n\tThu, 07 Jul 2022 15:26:15 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1657232779;\n\tbh=LrsF4lNFL4+6AyGxEvyCwaX3jwLXLgEgedaDE/Qbt2g=;\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=uOlBje+nGkj0nCTRE5TgfXnMLhehFJc7BB6LfxbF0v73E3iHCBep/vxE/OQMChpfI\n\tOEXf1hLtbzxkjqXF5tmRVxAaychIL2ytZt1XAbWJNlQTq3o9fSF/jIa+abyGufdHg1\n\tuCK/TlHsteTr8IlGfNICCeRpBZTMBdeqy5+18BX9vhvv/0EKJtfSscP6XVZ4ae50ik\n\toZXpSn3CBqpIDSzKufQKqR9vIbdy8cFKK1UZMKpAW2a/1f/FJUZO85rsFgl8UoDRFl\n\tFA1mUZz5ZFmEZY7AQ26mDacA5ZnWOF19Hki+0bdbEL83edmqGW24wAQ/mXxgjtu3lT\n\ttO2zy7EQABvDg==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1657232776;\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=aMKFir1H3fPY6DNCu19jDXk58KFSmn1TgWFVITcGghA=;\n\tb=dg8DE7Yu2yfy+CxnQfB1Yl/c5XeQcjdHhuvcYtuCu7JVamPYLejQBH1B72mQttzPESB5/D\n\tmKG/zZdeEY0FG7Si/s5TICqJsq5QYYXckFor0F3O5cVYt8f+iT/4xmgMsU1GaJcfle2fzz\n\tftuCYyTo9PiXdC9fW/NgtY7eA9KdCTI="],"Authentication-Results":["lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=redhat.com\n\theader.i=@redhat.com header.b=\"dg8DE7Yu\"; \n\tdkim-atps=neutral","relay.mimecast.com;\n\tauth=pass smtp.auth=CUSA124A263 smtp.mailfrom=ecurtin@redhat.com"],"X-MC-Unique":"wkCQtwGvPdq6qa9p3kNpYA-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=aMKFir1H3fPY6DNCu19jDXk58KFSmn1TgWFVITcGghA=;\n\tb=J4stuMf5RCVEPLDIejBH6Y0jKpArLritUdoXTTxolR/+gcNBR6XngIY7OWx549+Vuf\n\tAm+PCRYYGVik13UI5ziZZXFVa6QEdZ+T3JRWMYhuyQexucnXkTTY2vybagkTEfSEMGDx\n\tzxkxJLlU0ckhKW9HUboMHE5s3VC8SGnh3v4eYI/0arFQH1DyEdy0m4ycZUher39CK0o+\n\tJK4RKcpYnT2AeAtQyy7kmKaPxsTivqHm9UrmjaBU10uyMztYCWfhhLCHrqu1DmZcoe+C\n\tITHOymkvxNjJsm/zKL3y7Fcpk9Ert4R17vCRiRFEAA2RTas76P6sWysEK8Pn2nwOKkxt\n\tFS5Q==","X-Gm-Message-State":"AJIora/sl9RVd8ptCxGBmOpSymJFJlhw8PGWtLUfE8H7AlIwPgT1q20o\n\tgMnpTkF9fiA7WClc2lnOcLT58jg3w3EKAxJg8MsMHRNASO0YpkRoGFqT+yTjZEStYEbpi/nV2Qk\n\tvm9yv6e2hN4/uHL5+PL4NJ6wZd1W/kHuQykSV9hCnGNpN5gQ7Jg==","X-Received":["by 2002:a05:620a:f10:b0:6aa:318e:55a9 with SMTP id\n\tv16-20020a05620a0f1000b006aa318e55a9mr175339qkl.559.1657232774510; \n\tThu, 07 Jul 2022 15:26:14 -0700 (PDT)","by 2002:a05:620a:f10:b0:6aa:318e:55a9 with SMTP id\n\tv16-20020a05620a0f1000b006aa318e55a9mr175317qkl.559.1657232774140;\n\tThu, 07 Jul 2022 15:26:14 -0700 (PDT)"],"X-Google-Smtp-Source":"AGRyM1tnz64LRd9w0DZOumpuqD6WV8ulqIi3UZBecpSJbHjZnTZNYvirCXRirRvSK2404mlkX3O7+OM4iQKhbi1V0QM=","MIME-Version":"1.0","References":"<20220707155312.45807-1-ecurtin@redhat.com>\n\t","In-Reply-To":"","Date":"Thu, 7 Jul 2022 23:25:58 +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] 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":"libcamera devel , jozef@mlich.cz","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23805,"web_url":"https://patchwork.libcamera.org/comment/23805/","msgid":"","date":"2022-07-08T13:37:50","subject":"Re: [libcamera-devel] [PATCH] 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 Thu, Jul 07, 2022 at 11:25:58PM +0100, Eric Curtin wrote:\n> On Thu, 7 Jul 2022 at 21:13, Laurent Pinchart wrote:\n> > On Thu, Jul 07, 2022 at 04:53:13PM +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> > > README.rst | 2 +-\n> > > src/cam/meson.build | 8 +++----\n> > > src/cam/sdl_sink.cpp | 4 ++--\n> > > src/cam/sdl_texture.h | 2 +-\n> > > src/cam/sdl_texture_mjpg.cpp | 43 +++++++++++++++++++++++++++++++-----\n> > > src/cam/sdl_texture_mjpg.h | 5 +++++\n> > > 6 files changed, 51 insertions(+), 13 deletions(-)\n> > >\n> > > diff --git a/README.rst b/README.rst\n> > > index b9e72d81..5e8bc1cc 100644\n> > > --- a/README.rst\n> > > +++ b/README.rst\n> > > @@ -93,7 +93,7 @@ for cam: [optional]\n> > >\n> > > - libdrm-dev: Enables the KMS sink\n> > > - libsdl2-dev: Enables the SDL sink\n> > > - - libsdl2-image-dev: Supports MJPEG on the SDL sink\n> > > + - libjpeg-dev: Supports MJPEG on the SDL sink\n> >\n> > Could you please keep this list sorted ?\n> >\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..27cbd0de 100644\n> > > --- a/src/cam/meson.build\n> > > +++ b/src/cam/meson.build\n> > > @@ -25,7 +25,7 @@ cam_cpp_args = []\n> > >\n> > > libdrm = dependency('libdrm', required : false)\n> > > libsdl2 = dependency('SDL2', required : false)\n> > > -libsdl2_image = dependency('SDL2_image', required : false)\n> > > +libjpeg = dependency('libjpeg', required : false)\n> >\n> > Same here.\n> >\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> > > @@ -58,7 +58,7 @@ cam = executable('cam', cam_sources,\n> > > libdrm,\n> > > libevent,\n> > > libsdl2,\n> > > - libsdl2_image,\n> > > + libjpeg,\n> >\n> > And here.\n> >\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.h b/src/cam/sdl_texture.h\n> > > index 90974798..42c906f2 100644\n> > > --- a/src/cam/sdl_texture.h\n> > > +++ b/src/cam/sdl_texture.h\n> > > @@ -25,5 +25,5 @@ protected:\n> > > SDL_Texture *ptr_;\n> > > const SDL_Rect rect_;\n> > > const SDL_PixelFormatEnum pixelFormat_;\n> > > - const int pitch_;\n> > > + int pitch_;\n> > > };\n> > > diff --git a/src/cam/sdl_texture_mjpg.cpp b/src/cam/sdl_texture_mjpg.cpp\n> > > index 69e99ad3..232af673 100644\n> > > --- a/src/cam/sdl_texture_mjpg.cpp\n> > > +++ b/src/cam/sdl_texture_mjpg.cpp\n> > > @@ -7,19 +7,52 @@\n> > >\n> > > #include \"sdl_texture_mjpg.h\"\n> > >\n> > > -#include \n> > > +#include \n> > >\n> > > using namespace libcamera;\n> > >\n> > > SDLTextureMJPG::SDLTextureMJPG(const SDL_Rect &rect)\n> > > : SDLTexture(rect, SDL_PIXELFORMAT_RGB24, 0)\n> >\n> > You can write\n> >\n> > : SDLTexture(rect, SDL_PIXELFORMAT_RGB24, rect.w * 3)\n> >\n> > > {\n> > > + pitch_ = rect_.w * 3;\n> >\n> > and drop this line, and keep pitch_ const.\n> >\n> > > + rgbdata_ = (unsigned char *)malloc(pitch_ * rect_.h);\n> >\n> > Please use C++-style memory allocation\n> >\n> > rgbdata_ = new unsigned char[pitch_ * rect_.h];\n> >\n> > The destructor should then use\n> >\n> > delete[] rgbdata_;\n> >\n> > Even better, I think you can turn rgbdata_ into a\n> >\n> > std::unique_ptr rgbdata_;\n> >\n> > and drop the delete.\n> >\n> > Another option would have been to turn rgbdata_ into an\n> > std::vector to also remove the need for a custom\n> > destructor. The drawback is that, unlike new that performs\n> > default-initialization, std::vector::resize() performs\n> > default-insertion, which uses value-initialization, effectively\n> > memsetting the vector to 0. There's a way around that by providing a\n> > custom allocator to the vector, but I think we're getting into the \"not\n> > worth it\" territory (at least as long as libcamera-base doesn't provide\n> > a helper for this). Let's thus use a unique_ptr.\n> >\n> > > +}\n> > > +\n> > > +SDLTextureMJPG::~SDLTextureMJPG()\n> > > +{\n> > > + free(rgbdata_);\n> > > +}\n> > > +\n> > > +void SDLTextureMJPG::decompress(const unsigned char *jpeg,\n> > > + unsigned long jpeg_size)\n> > > +{\n> > > + struct jpeg_error_mgr jerr;\n> > > + struct jpeg_decompress_struct cinfo;\n> > > + cinfo.err = jpeg_std_error(&jerr);\n> >\n> > There's a risk the JPEG data could be corrupted (that's quite common\n> > with UVC cameras). Error handling should be a bit more robust, you\n> > should create your own error handler, store that an error occurred, and\n> > return an error from this function and skip the SDL_UpdateTexture() in\n> > that case.\n> >\n> > Furthermore, jpg_std_error() will by default call exit() when a fatal\n> > error occurs. That's not good. The recommended way to deal with this is\n> > to use setjmp() and longjmp() to exit cleanly instead.\n> >\n> > > +\n> > > + jpeg_create_decompress(&cinfo);\n> > > +\n> > > + jpeg_mem_src(&cinfo, jpeg, jpeg_size);\n> > > +\n> > > + jpeg_read_header(&cinfo, TRUE);\n> > > +\n> > > + jpeg_start_decompress(&cinfo);\n> > > +\n> > > + int row_stride = cinfo.output_width * cinfo.output_components;\n> >\n> > rowStride (libcamera coding style)\n> >\n> > > + unsigned char *buffer = (unsigned char *)malloc(row_stride);\n> >\n> > Here too, std::unique_ptr and new[], or just\n> >\n> > unsigned char buffer[row_stride);\n> \n> For buffer, I need to pass the address of the variable storing the\n> pointer to jpeg_read_scanlines, not sure unique_ptr is possible in\n> this very specific case? (although I can easily change rgbdata_ to\n> unique_ptr, thanks for spotting that).\n> \n> \"unsigned char buffer[row_stride];\" won't work for the same reason,\n> needs to be a raw C pointer to pass &buffer to jpeg_read_scanlines.\n\nGood point. You could have a separate variable:\n\n\tunsigned char row[rowStride];\n\tunsigned char *rows[1] = { &row };\n\nNot sure if it's worth it. This makes me wonder, is there a performance\nbenefit from reading multiple scanlines in one go ?\n\n> All the other feedback makes sense and will be addressed.\n> \n> > > + for (int i = 0; cinfo.output_scanline < cinfo.output_height; ++i) {\n> > > + jpeg_read_scanlines(&cinfo, &buffer, 1);\n> > > + memcpy(rgbdata_ + i * pitch_, buffer, row_stride);\n> > > + }\n> > > +\n> > > + free(buffer);\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> > > + SDL_UpdateTexture(ptr_, nullptr, rgbdata_, pitch_);\n> > > }\n> > > diff --git a/src/cam/sdl_texture_mjpg.h b/src/cam/sdl_texture_mjpg.h\n> > > index b103f801..73c7fb68 100644\n> > > --- a/src/cam/sdl_texture_mjpg.h\n> > > +++ b/src/cam/sdl_texture_mjpg.h\n> > > @@ -13,5 +13,10 @@ class SDLTextureMJPG : public SDLTexture\n> > > {\n> > > public:\n> > > SDLTextureMJPG(const SDL_Rect &rect);\n> > > + virtual ~SDLTextureMJPG();\n> >\n> > No need for virtual here, and while at it, you can insert a blank line.\n> >\n> > > void update(const libcamera::Span &data) override;\n> > > +\n> > > +private:\n> > > + unsigned char *rgbdata_ = 0;\n> >\n> > Member data after member functions. I'd name the member rgbData_ or just\n> > rgb_, up to you.\n> >\n> > > + void decompress(const unsigned char *jpeg, unsigned long jpeg_size);\n> >\n> > void decompress(const unsigned char *jpeg, std::size size);\n> >\n> > > };\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 6FC8EBD1F1\n\tfor ;\n\tFri, 8 Jul 2022 13:38:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A460B63312;\n\tFri, 8 Jul 2022 15:38:19 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8C2EB6330D\n\tfor ;\n\tFri, 8 Jul 2022 15:38:17 +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 E940656D;\n\tFri, 8 Jul 2022 15:38:16 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1657287499;\n\tbh=nkZT6dSMb2hcEvToUjwwTTNY5KQOt9gfBevCUFRz7Lg=;\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=oJCHleGUBk4duLALfcNqREbwsA6coGMlo+m80vndQYUF7HuD1EGEjG1W9NWL1LR1Y\n\trXZj/CBl0ClhdStEnF+dwayKYiUKgM3V8ZBEARVMlYdNX7KRruV8tXwEdviE3rk2ew\n\tgY6ovQfnSzLbJYQFFX6Z8cz1uvrqLtgBxzktcQ2obrd6kuDfdm30l+ZcdQPSTIIJmj\n\t5mljygMvu++BlyfSdb/x2JIOVbhVUjY7HpwohHFXrQNaCb7Zm98+gdBrLqCzkPc63s\n\to/efkBrWPRlMZLiNUYKNTdDlDeTRf2R6I1Soem7DhiTZQNNLFD5jA3U1aUOQNM8bZw\n\tQCLSCsYbLpCXQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1657287497;\n\tbh=nkZT6dSMb2hcEvToUjwwTTNY5KQOt9gfBevCUFRz7Lg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=vTdgd5k1OCrwUsjKuMYSZNDSYpHjPzXixf3ZvC2L7NWrVAAZYNMeo1wYsxZYpRF0i\n\tuNdVqyGXW7Ar3D6HaOhfevKwG0IhVupZy6WTMreW8E/xYoemEYtkPwUQt6Um6fnWJw\n\tBP4/jACnrbxjCbUFf6m4bSXsq5eIflAUuAdjBVDM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"vTdgd5k1\"; dkim-atps=neutral","Date":"Fri, 8 Jul 2022 16:37:50 +0300","To":"Eric Curtin ","Message-ID":"","References":"<20220707155312.45807-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] 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":"libcamera devel , jozef@mlich.cz","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23871,"web_url":"https://patchwork.libcamera.org/comment/23871/","msgid":"","date":"2022-07-14T11:24:09","subject":"Re: [libcamera-devel] [PATCH] 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\nI was wondering if you planned to submit a new version of this patch.\nThere's no big urgency, but if you expect to be too busy for the\nforeseable future, I can send a v2 that addresses my review comments.\n\nOn Fri, Jul 08, 2022 at 04:37:50PM +0300, Laurent Pinchart via libcamera-devel wrote:\n> On Thu, Jul 07, 2022 at 11:25:58PM +0100, Eric Curtin wrote:\n> > On Thu, 7 Jul 2022 at 21:13, Laurent Pinchart wrote:\n> > > On Thu, Jul 07, 2022 at 04:53:13PM +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> > > > README.rst | 2 +-\n> > > > src/cam/meson.build | 8 +++----\n> > > > src/cam/sdl_sink.cpp | 4 ++--\n> > > > src/cam/sdl_texture.h | 2 +-\n> > > > src/cam/sdl_texture_mjpg.cpp | 43 +++++++++++++++++++++++++++++++-----\n> > > > src/cam/sdl_texture_mjpg.h | 5 +++++\n> > > > 6 files changed, 51 insertions(+), 13 deletions(-)\n> > > >\n> > > > diff --git a/README.rst b/README.rst\n> > > > index b9e72d81..5e8bc1cc 100644\n> > > > --- a/README.rst\n> > > > +++ b/README.rst\n> > > > @@ -93,7 +93,7 @@ for cam: [optional]\n> > > >\n> > > > - libdrm-dev: Enables the KMS sink\n> > > > - libsdl2-dev: Enables the SDL sink\n> > > > - - libsdl2-image-dev: Supports MJPEG on the SDL sink\n> > > > + - libjpeg-dev: Supports MJPEG on the SDL sink\n> > >\n> > > Could you please keep this list sorted ?\n> > >\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..27cbd0de 100644\n> > > > --- a/src/cam/meson.build\n> > > > +++ b/src/cam/meson.build\n> > > > @@ -25,7 +25,7 @@ cam_cpp_args = []\n> > > >\n> > > > libdrm = dependency('libdrm', required : false)\n> > > > libsdl2 = dependency('SDL2', required : false)\n> > > > -libsdl2_image = dependency('SDL2_image', required : false)\n> > > > +libjpeg = dependency('libjpeg', required : false)\n> > >\n> > > Same here.\n> > >\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> > > > @@ -58,7 +58,7 @@ cam = executable('cam', cam_sources,\n> > > > libdrm,\n> > > > libevent,\n> > > > libsdl2,\n> > > > - libsdl2_image,\n> > > > + libjpeg,\n> > >\n> > > And here.\n> > >\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.h b/src/cam/sdl_texture.h\n> > > > index 90974798..42c906f2 100644\n> > > > --- a/src/cam/sdl_texture.h\n> > > > +++ b/src/cam/sdl_texture.h\n> > > > @@ -25,5 +25,5 @@ protected:\n> > > > SDL_Texture *ptr_;\n> > > > const SDL_Rect rect_;\n> > > > const SDL_PixelFormatEnum pixelFormat_;\n> > > > - const int pitch_;\n> > > > + int pitch_;\n> > > > };\n> > > > diff --git a/src/cam/sdl_texture_mjpg.cpp b/src/cam/sdl_texture_mjpg.cpp\n> > > > index 69e99ad3..232af673 100644\n> > > > --- a/src/cam/sdl_texture_mjpg.cpp\n> > > > +++ b/src/cam/sdl_texture_mjpg.cpp\n> > > > @@ -7,19 +7,52 @@\n> > > >\n> > > > #include \"sdl_texture_mjpg.h\"\n> > > >\n> > > > -#include \n> > > > +#include \n> > > >\n> > > > using namespace libcamera;\n> > > >\n> > > > SDLTextureMJPG::SDLTextureMJPG(const SDL_Rect &rect)\n> > > > : SDLTexture(rect, SDL_PIXELFORMAT_RGB24, 0)\n> > >\n> > > You can write\n> > >\n> > > : SDLTexture(rect, SDL_PIXELFORMAT_RGB24, rect.w * 3)\n> > >\n> > > > {\n> > > > + pitch_ = rect_.w * 3;\n> > >\n> > > and drop this line, and keep pitch_ const.\n> > >\n> > > > + rgbdata_ = (unsigned char *)malloc(pitch_ * rect_.h);\n> > >\n> > > Please use C++-style memory allocation\n> > >\n> > > rgbdata_ = new unsigned char[pitch_ * rect_.h];\n> > >\n> > > The destructor should then use\n> > >\n> > > delete[] rgbdata_;\n> > >\n> > > Even better, I think you can turn rgbdata_ into a\n> > >\n> > > std::unique_ptr rgbdata_;\n> > >\n> > > and drop the delete.\n> > >\n> > > Another option would have been to turn rgbdata_ into an\n> > > std::vector to also remove the need for a custom\n> > > destructor. The drawback is that, unlike new that performs\n> > > default-initialization, std::vector::resize() performs\n> > > default-insertion, which uses value-initialization, effectively\n> > > memsetting the vector to 0. There's a way around that by providing a\n> > > custom allocator to the vector, but I think we're getting into the \"not\n> > > worth it\" territory (at least as long as libcamera-base doesn't provide\n> > > a helper for this). Let's thus use a unique_ptr.\n> > >\n> > > > +}\n> > > > +\n> > > > +SDLTextureMJPG::~SDLTextureMJPG()\n> > > > +{\n> > > > + free(rgbdata_);\n> > > > +}\n> > > > +\n> > > > +void SDLTextureMJPG::decompress(const unsigned char *jpeg,\n> > > > + unsigned long jpeg_size)\n> > > > +{\n> > > > + struct jpeg_error_mgr jerr;\n> > > > + struct jpeg_decompress_struct cinfo;\n> > > > + cinfo.err = jpeg_std_error(&jerr);\n> > >\n> > > There's a risk the JPEG data could be corrupted (that's quite common\n> > > with UVC cameras). Error handling should be a bit more robust, you\n> > > should create your own error handler, store that an error occurred, and\n> > > return an error from this function and skip the SDL_UpdateTexture() in\n> > > that case.\n> > >\n> > > Furthermore, jpg_std_error() will by default call exit() when a fatal\n> > > error occurs. That's not good. The recommended way to deal with this is\n> > > to use setjmp() and longjmp() to exit cleanly instead.\n> > >\n> > > > +\n> > > > + jpeg_create_decompress(&cinfo);\n> > > > +\n> > > > + jpeg_mem_src(&cinfo, jpeg, jpeg_size);\n> > > > +\n> > > > + jpeg_read_header(&cinfo, TRUE);\n> > > > +\n> > > > + jpeg_start_decompress(&cinfo);\n> > > > +\n> > > > + int row_stride = cinfo.output_width * cinfo.output_components;\n> > >\n> > > rowStride (libcamera coding style)\n> > >\n> > > > + unsigned char *buffer = (unsigned char *)malloc(row_stride);\n> > >\n> > > Here too, std::unique_ptr and new[], or just\n> > >\n> > > unsigned char buffer[row_stride);\n> > \n> > For buffer, I need to pass the address of the variable storing the\n> > pointer to jpeg_read_scanlines, not sure unique_ptr is possible in\n> > this very specific case? (although I can easily change rgbdata_ to\n> > unique_ptr, thanks for spotting that).\n> > \n> > \"unsigned char buffer[row_stride];\" won't work for the same reason,\n> > needs to be a raw C pointer to pass &buffer to jpeg_read_scanlines.\n> \n> Good point. You could have a separate variable:\n> \n> \tunsigned char row[rowStride];\n> \tunsigned char *rows[1] = { &row };\n> \n> Not sure if it's worth it. This makes me wonder, is there a performance\n> benefit from reading multiple scanlines in one go ?\n> \n> > All the other feedback makes sense and will be addressed.\n> > \n> > > > + for (int i = 0; cinfo.output_scanline < cinfo.output_height; ++i) {\n> > > > + jpeg_read_scanlines(&cinfo, &buffer, 1);\n> > > > + memcpy(rgbdata_ + i * pitch_, buffer, row_stride);\n> > > > + }\n> > > > +\n> > > > + free(buffer);\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> > > > + SDL_UpdateTexture(ptr_, nullptr, rgbdata_, pitch_);\n> > > > }\n> > > > diff --git a/src/cam/sdl_texture_mjpg.h b/src/cam/sdl_texture_mjpg.h\n> > > > index b103f801..73c7fb68 100644\n> > > > --- a/src/cam/sdl_texture_mjpg.h\n> > > > +++ b/src/cam/sdl_texture_mjpg.h\n> > > > @@ -13,5 +13,10 @@ class SDLTextureMJPG : public SDLTexture\n> > > > {\n> > > > public:\n> > > > SDLTextureMJPG(const SDL_Rect &rect);\n> > > > + virtual ~SDLTextureMJPG();\n> > >\n> > > No need for virtual here, and while at it, you can insert a blank line.\n> > >\n> > > > void update(const libcamera::Span &data) override;\n> > > > +\n> > > > +private:\n> > > > + unsigned char *rgbdata_ = 0;\n> > >\n> > > Member data after member functions. I'd name the member rgbData_ or just\n> > > rgb_, up to you.\n> > >\n> > > > + void decompress(const unsigned char *jpeg, unsigned long jpeg_size);\n> > >\n> > > void decompress(const unsigned char *jpeg, std::size size);\n> > >\n> > > > };\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 29575BD1F1\n\tfor ;\n\tThu, 14 Jul 2022 11:24:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 92B8B63312;\n\tThu, 14 Jul 2022 13:24:41 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 209B56330D\n\tfor ;\n\tThu, 14 Jul 2022 13:24:40 +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 8174B383;\n\tThu, 14 Jul 2022 13:24:39 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1657797881;\n\tbh=WbDMKlSaOHuZyplzcmhyz4bsDOr6jo/jXzz35w7jXZw=;\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:\n\tFrom;\n\tb=mE1WH0R0VPG2hVgKcCBDGnUhQ0+0xy1j/bbZw+Wz/hB+RJgPpQRNAiDql4WFGSKMv\n\ta57rwnq7syIDD7o+SWfh5aT1wgN1x0A9jbAhgDNtx9YPzwFMZACnLb7hKKzxBs5JRB\n\tjBymAYf3x9WsCcYdywAn9QGDrs2yGjfyLDYH3elqnCsKFc6k13yjTb8iSipJXd/JBf\n\tRAu44gFrnEXq6t7NciTW7WiSjRBTAchUgvtMMZEe0UITPg0pwdTNc5Rn4fLRfJp5Py\n\tKPWBBzpPatuaOXljIMjH0JK1eRcnSdfiRfjz33maFVisEWtd7By9RXLLs4Rzqw20X+\n\tj3m89QOUww9+g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1657797879;\n\tbh=WbDMKlSaOHuZyplzcmhyz4bsDOr6jo/jXzz35w7jXZw=;\n\th=Date:From:To:Subject:References:In-Reply-To:From;\n\tb=Dm2aenJexdZC9bOQd5N1Q3gyC3wumg5GNJeLApyO7EaaRgzP2UCG6JT0iHduyyzXf\n\tOgtTyq+Y+m5ozthjiAMu8HsDwRpX0SAKsKCocDb7/rUVJaofmn7IdreCPHBWijdXY2\n\taiqueXVH8luzT8FYTK7CzCtwj7ykAwDPnCHD/wnE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Dm2aenJe\"; dkim-atps=neutral","Date":"Thu, 14 Jul 2022 14:24:09 +0300","To":"Eric Curtin ,\n\tlibcamera devel , jozef@mlich.cz","Message-ID":"","References":"<20220707155312.45807-1-ecurtin@redhat.com>\n\t\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] 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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23872,"web_url":"https://patchwork.libcamera.org/comment/23872/","msgid":"","date":"2022-07-14T12:05:39","subject":"Re: [libcamera-devel] [PATCH] 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":"I did plan on, I had a few days PTO and I'm stuck in training at the\nmoment, might not get around to until next week.\n\nFeel free to submit a v2 if you'd like.\n\nIs mise le meas/Regards,\n\nEric Curtin\n\n\nOn Thu, 14 Jul 2022 at 12:24, Laurent Pinchart\n wrote:\n>\n> Hi Eric,\n>\n> I was wondering if you planned to submit a new version of this patch.\n> There's no big urgency, but if you expect to be too busy for the\n> foreseable future, I can send a v2 that addresses my review comments.\n>\n> On Fri, Jul 08, 2022 at 04:37:50PM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > On Thu, Jul 07, 2022 at 11:25:58PM +0100, Eric Curtin wrote:\n> > > On Thu, 7 Jul 2022 at 21:13, Laurent Pinchart wrote:\n> > > > On Thu, Jul 07, 2022 at 04:53:13PM +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> > > > > README.rst | 2 +-\n> > > > > src/cam/meson.build | 8 +++----\n> > > > > src/cam/sdl_sink.cpp | 4 ++--\n> > > > > src/cam/sdl_texture.h | 2 +-\n> > > > > src/cam/sdl_texture_mjpg.cpp | 43 +++++++++++++++++++++++++++++++-----\n> > > > > src/cam/sdl_texture_mjpg.h | 5 +++++\n> > > > > 6 files changed, 51 insertions(+), 13 deletions(-)\n> > > > >\n> > > > > diff --git a/README.rst b/README.rst\n> > > > > index b9e72d81..5e8bc1cc 100644\n> > > > > --- a/README.rst\n> > > > > +++ b/README.rst\n> > > > > @@ -93,7 +93,7 @@ for cam: [optional]\n> > > > >\n> > > > > - libdrm-dev: Enables the KMS sink\n> > > > > - libsdl2-dev: Enables the SDL sink\n> > > > > - - libsdl2-image-dev: Supports MJPEG on the SDL sink\n> > > > > + - libjpeg-dev: Supports MJPEG on the SDL sink\n> > > >\n> > > > Could you please keep this list sorted ?\n> > > >\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..27cbd0de 100644\n> > > > > --- a/src/cam/meson.build\n> > > > > +++ b/src/cam/meson.build\n> > > > > @@ -25,7 +25,7 @@ cam_cpp_args = []\n> > > > >\n> > > > > libdrm = dependency('libdrm', required : false)\n> > > > > libsdl2 = dependency('SDL2', required : false)\n> > > > > -libsdl2_image = dependency('SDL2_image', required : false)\n> > > > > +libjpeg = dependency('libjpeg', required : false)\n> > > >\n> > > > Same here.\n> > > >\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> > > > > @@ -58,7 +58,7 @@ cam = executable('cam', cam_sources,\n> > > > > libdrm,\n> > > > > libevent,\n> > > > > libsdl2,\n> > > > > - libsdl2_image,\n> > > > > + libjpeg,\n> > > >\n> > > > And here.\n> > > >\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.h b/src/cam/sdl_texture.h\n> > > > > index 90974798..42c906f2 100644\n> > > > > --- a/src/cam/sdl_texture.h\n> > > > > +++ b/src/cam/sdl_texture.h\n> > > > > @@ -25,5 +25,5 @@ protected:\n> > > > > SDL_Texture *ptr_;\n> > > > > const SDL_Rect rect_;\n> > > > > const SDL_PixelFormatEnum pixelFormat_;\n> > > > > - const int pitch_;\n> > > > > + int pitch_;\n> > > > > };\n> > > > > diff --git a/src/cam/sdl_texture_mjpg.cpp b/src/cam/sdl_texture_mjpg.cpp\n> > > > > index 69e99ad3..232af673 100644\n> > > > > --- a/src/cam/sdl_texture_mjpg.cpp\n> > > > > +++ b/src/cam/sdl_texture_mjpg.cpp\n> > > > > @@ -7,19 +7,52 @@\n> > > > >\n> > > > > #include \"sdl_texture_mjpg.h\"\n> > > > >\n> > > > > -#include \n> > > > > +#include \n> > > > >\n> > > > > using namespace libcamera;\n> > > > >\n> > > > > SDLTextureMJPG::SDLTextureMJPG(const SDL_Rect &rect)\n> > > > > : SDLTexture(rect, SDL_PIXELFORMAT_RGB24, 0)\n> > > >\n> > > > You can write\n> > > >\n> > > > : SDLTexture(rect, SDL_PIXELFORMAT_RGB24, rect.w * 3)\n> > > >\n> > > > > {\n> > > > > + pitch_ = rect_.w * 3;\n> > > >\n> > > > and drop this line, and keep pitch_ const.\n> > > >\n> > > > > + rgbdata_ = (unsigned char *)malloc(pitch_ * rect_.h);\n> > > >\n> > > > Please use C++-style memory allocation\n> > > >\n> > > > rgbdata_ = new unsigned char[pitch_ * rect_.h];\n> > > >\n> > > > The destructor should then use\n> > > >\n> > > > delete[] rgbdata_;\n> > > >\n> > > > Even better, I think you can turn rgbdata_ into a\n> > > >\n> > > > std::unique_ptr rgbdata_;\n> > > >\n> > > > and drop the delete.\n> > > >\n> > > > Another option would have been to turn rgbdata_ into an\n> > > > std::vector to also remove the need for a custom\n> > > > destructor. The drawback is that, unlike new that performs\n> > > > default-initialization, std::vector::resize() performs\n> > > > default-insertion, which uses value-initialization, effectively\n> > > > memsetting the vector to 0. There's a way around that by providing a\n> > > > custom allocator to the vector, but I think we're getting into the \"not\n> > > > worth it\" territory (at least as long as libcamera-base doesn't provide\n> > > > a helper for this). Let's thus use a unique_ptr.\n> > > >\n> > > > > +}\n> > > > > +\n> > > > > +SDLTextureMJPG::~SDLTextureMJPG()\n> > > > > +{\n> > > > > + free(rgbdata_);\n> > > > > +}\n> > > > > +\n> > > > > +void SDLTextureMJPG::decompress(const unsigned char *jpeg,\n> > > > > + unsigned long jpeg_size)\n> > > > > +{\n> > > > > + struct jpeg_error_mgr jerr;\n> > > > > + struct jpeg_decompress_struct cinfo;\n> > > > > + cinfo.err = jpeg_std_error(&jerr);\n> > > >\n> > > > There's a risk the JPEG data could be corrupted (that's quite common\n> > > > with UVC cameras). Error handling should be a bit more robust, you\n> > > > should create your own error handler, store that an error occurred, and\n> > > > return an error from this function and skip the SDL_UpdateTexture() in\n> > > > that case.\n> > > >\n> > > > Furthermore, jpg_std_error() will by default call exit() when a fatal\n> > > > error occurs. That's not good. The recommended way to deal with this is\n> > > > to use setjmp() and longjmp() to exit cleanly instead.\n> > > >\n> > > > > +\n> > > > > + jpeg_create_decompress(&cinfo);\n> > > > > +\n> > > > > + jpeg_mem_src(&cinfo, jpeg, jpeg_size);\n> > > > > +\n> > > > > + jpeg_read_header(&cinfo, TRUE);\n> > > > > +\n> > > > > + jpeg_start_decompress(&cinfo);\n> > > > > +\n> > > > > + int row_stride = cinfo.output_width * cinfo.output_components;\n> > > >\n> > > > rowStride (libcamera coding style)\n> > > >\n> > > > > + unsigned char *buffer = (unsigned char *)malloc(row_stride);\n> > > >\n> > > > Here too, std::unique_ptr and new[], or just\n> > > >\n> > > > unsigned char buffer[row_stride);\n> > >\n> > > For buffer, I need to pass the address of the variable storing the\n> > > pointer to jpeg_read_scanlines, not sure unique_ptr is possible in\n> > > this very specific case? (although I can easily change rgbdata_ to\n> > > unique_ptr, thanks for spotting that).\n> > >\n> > > \"unsigned char buffer[row_stride];\" won't work for the same reason,\n> > > needs to be a raw C pointer to pass &buffer to jpeg_read_scanlines.\n> >\n> > Good point. You could have a separate variable:\n> >\n> > unsigned char row[rowStride];\n> > unsigned char *rows[1] = { &row };\n> >\n> > Not sure if it's worth it. This makes me wonder, is there a performance\n> > benefit from reading multiple scanlines in one go ?\n> >\n> > > All the other feedback makes sense and will be addressed.\n> > >\n> > > > > + for (int i = 0; cinfo.output_scanline < cinfo.output_height; ++i) {\n> > > > > + jpeg_read_scanlines(&cinfo, &buffer, 1);\n> > > > > + memcpy(rgbdata_ + i * pitch_, buffer, row_stride);\n> > > > > + }\n> > > > > +\n> > > > > + free(buffer);\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> > > > > + SDL_UpdateTexture(ptr_, nullptr, rgbdata_, pitch_);\n> > > > > }\n> > > > > diff --git a/src/cam/sdl_texture_mjpg.h b/src/cam/sdl_texture_mjpg.h\n> > > > > index b103f801..73c7fb68 100644\n> > > > > --- a/src/cam/sdl_texture_mjpg.h\n> > > > > +++ b/src/cam/sdl_texture_mjpg.h\n> > > > > @@ -13,5 +13,10 @@ class SDLTextureMJPG : public SDLTexture\n> > > > > {\n> > > > > public:\n> > > > > SDLTextureMJPG(const SDL_Rect &rect);\n> > > > > + virtual ~SDLTextureMJPG();\n> > > >\n> > > > No need for virtual here, and while at it, you can insert a blank line.\n> > > >\n> > > > > void update(const libcamera::Span &data) override;\n> > > > > +\n> > > > > +private:\n> > > > > + unsigned char *rgbdata_ = 0;\n> > > >\n> > > > Member data after member functions. I'd name the member rgbData_ or just\n> > > > rgb_, up to you.\n> > > >\n> > > > > + void decompress(const unsigned char *jpeg, unsigned long jpeg_size);\n> > > >\n> > > > void decompress(const unsigned char *jpeg, std::size size);\n> > > >\n> > > > > };\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 266AFBE173\n\tfor ;\n\tThu, 14 Jul 2022 12:06:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 60EB163312;\n\tThu, 14 Jul 2022 14:06:05 +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 D8AAB6330D\n\tfor ;\n\tThu, 14 Jul 2022 14:06:03 +0200 (CEST)","from mail-qk1-f198.google.com (mail-qk1-f198.google.com\n\t[209.85.222.198]) 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-468-cZCZepPaMFeRw2mf7dFgoA-1; Thu, 14 Jul 2022 08:05:55 -0400","by mail-qk1-f198.google.com with SMTP id\n\te128-20020a376986000000b006af6adf035cso1011823qkc.8\n\tfor ;\n\tThu, 14 Jul 2022 05:05:55 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1657800365;\n\tbh=DNkggJNxHWSLNSKmyziOHmg6e+gzB0Fj67grhgRy814=;\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=zsvCXJXOXSIsmuapp8B8994CJQTDr4LkAZQrPUYz0HHT25hinoYDgULDU1yyIlts+\n\tuUUt6hUV4jtQgRR2v+h0QRN7nZTQUQ/nNFp7+s6yFev4b5juhiHMRZs5GI43zOSOmm\n\tLaihQX12eZUuBkX/agO5VQzFqDkPBWv1q2rvrUIEjSDCjIcZe14xyYAm+JxPEa47s5\n\t6aVg0oFqOmdj7WlaUw1+GzG7JM2Z2Qw76SvAo2hR6NVj2VHcq/mVxS4kW5tzCcMxdV\n\tmqzuqssEv75AGNivZ50QDfeuwqDWDtTAOe21P0qbInFrKe5B/nlMTsSQB3AUY5HvvM\n\tu2Nwi6l/5ZUlQ==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1657800362;\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=CEuAGp8JOKdJNDSr3tPSj1s7+4cmgCQOUCxqK6YCeSo=;\n\tb=h5s3cJcbDUkhnKaPZ3ZN4OAzpMzI0/xnGAUgkwx8KN4OceUarMP8I/apFbhHDnCCA33TGg\n\tp9Uyd648WyDpS8srPceZgXMbl0ffcJs6IYFVb47ugnDlNcvPBlJwsqZRHwFKuG0gs5kZYu\n\t2LN6LH6bVlV1xACedKQc5eAFSBl/AO0="],"Authentication-Results":["lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=redhat.com\n\theader.i=@redhat.com header.b=\"h5s3cJcb\"; \n\tdkim-atps=neutral","relay.mimecast.com;\n\tauth=pass smtp.auth=CUSA124A263 smtp.mailfrom=ecurtin@redhat.com"],"X-MC-Unique":"cZCZepPaMFeRw2mf7dFgoA-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=CEuAGp8JOKdJNDSr3tPSj1s7+4cmgCQOUCxqK6YCeSo=;\n\tb=Zfc8zWprbduwoeVU1jSYuEj9OR5Rn5eaXLD8H1W1ye1B4IGRbcjO1FxxgJIR98OMaV\n\tCkcpnGBc3qr+OkUDwcHXqeBM76GzgWnbsvPODt/mkON2eOqZCl+LXm6+PJUM2NPxFRfF\n\tnvPqVsUBzemBjSvKa6cVJ10OzLIr9Ri/wmXWIzPvgaN+Gk5jpYvxV3ftAF1jX6k1t9uj\n\tfb7uwcEIZSlZSKlxbBM0mNdxr9Q1knHu3KqDjVGBG+M1zs2/TE6zDwdUmefI/hnfuQYP\n\tsMqGKRAJR31Klee09JUBxvf7Hb7Kx3FthI0AiX0AFWdvtbiaWs2C8pNjal3rawhuqpqk\n\tZKCQ==","X-Gm-Message-State":"AJIora/H3DNuadPDQu/9D1TPwTk5hQqkoIz634eF3CAUR9H5IiXlVSr7\n\ta0xMMijd/SkWxJE1/HTNFFWu5j9GmucB3TyIFEjjMrIlrAMIrk8VJs/vWwXsxiqzwDRgXR3rjSm\n\ttM6s4wgC0YaUzYQC4/VCOBAoFr8+STlwQRGH5sUyD7W4fG1qL0Q==","X-Received":["by 2002:ac8:7d94:0:b0:31e:ccbc:eed with SMTP id\n\tc20-20020ac87d94000000b0031eccbc0eedmr6392638qtd.106.1657800355154; \n\tThu, 14 Jul 2022 05:05:55 -0700 (PDT)","by 2002:ac8:7d94:0:b0:31e:ccbc:eed with SMTP id\n\tc20-20020ac87d94000000b0031eccbc0eedmr6392611qtd.106.1657800354831;\n\tThu, 14 Jul 2022 05:05:54 -0700 (PDT)"],"X-Google-Smtp-Source":"AGRyM1tNuEWe6BQ7ERZTgGktgQ5/H5vUSIA+nAGoXfh/3PAENvLcda5yHsfu+HhiROvWxiR+bfdFVHvW36Ieuak862Q=","MIME-Version":"1.0","References":"<20220707155312.45807-1-ecurtin@redhat.com>\n\t\n\t\n\t\n\t","In-Reply-To":"","Date":"Thu, 14 Jul 2022 13:05:39 +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] 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":"libcamera devel , jozef@mlich.cz","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23900,"web_url":"https://patchwork.libcamera.org/comment/23900/","msgid":"","date":"2022-07-15T00:06:22","subject":"Re: [libcamera-devel] [PATCH] 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 Thu, Jul 14, 2022 at 01:05:39PM +0100, Eric Curtin wrote:\n> I did plan on, I had a few days PTO and I'm stuck in training at the\n> moment, might not get around to until next week.\n> \n> Feel free to submit a v2 if you'd like.\n\nThere's no urgency, so I'm happy to let you get back to it when you'll\nhave time. Please make sure to rebase first then, as we had to make a\nsmall change to the SDL sink to fix a compilation failure with SDL 2.0.9\nor older.\n\n> On Thu, 14 Jul 2022 at 12:24, Laurent Pinchart wrote:\n> >\n> > Hi Eric,\n> >\n> > I was wondering if you planned to submit a new version of this patch.\n> > There's no big urgency, but if you expect to be too busy for the\n> > foreseable future, I can send a v2 that addresses my review comments.\n> >\n> > On Fri, Jul 08, 2022 at 04:37:50PM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > > On Thu, Jul 07, 2022 at 11:25:58PM +0100, Eric Curtin wrote:\n> > > > On Thu, 7 Jul 2022 at 21:13, Laurent Pinchart wrote:\n> > > > > On Thu, Jul 07, 2022 at 04:53:13PM +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> > > > > > README.rst | 2 +-\n> > > > > > src/cam/meson.build | 8 +++----\n> > > > > > src/cam/sdl_sink.cpp | 4 ++--\n> > > > > > src/cam/sdl_texture.h | 2 +-\n> > > > > > src/cam/sdl_texture_mjpg.cpp | 43 +++++++++++++++++++++++++++++++-----\n> > > > > > src/cam/sdl_texture_mjpg.h | 5 +++++\n> > > > > > 6 files changed, 51 insertions(+), 13 deletions(-)\n> > > > > >\n> > > > > > diff --git a/README.rst b/README.rst\n> > > > > > index b9e72d81..5e8bc1cc 100644\n> > > > > > --- a/README.rst\n> > > > > > +++ b/README.rst\n> > > > > > @@ -93,7 +93,7 @@ for cam: [optional]\n> > > > > >\n> > > > > > - libdrm-dev: Enables the KMS sink\n> > > > > > - libsdl2-dev: Enables the SDL sink\n> > > > > > - - libsdl2-image-dev: Supports MJPEG on the SDL sink\n> > > > > > + - libjpeg-dev: Supports MJPEG on the SDL sink\n> > > > >\n> > > > > Could you please keep this list sorted ?\n> > > > >\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..27cbd0de 100644\n> > > > > > --- a/src/cam/meson.build\n> > > > > > +++ b/src/cam/meson.build\n> > > > > > @@ -25,7 +25,7 @@ cam_cpp_args = []\n> > > > > >\n> > > > > > libdrm = dependency('libdrm', required : false)\n> > > > > > libsdl2 = dependency('SDL2', required : false)\n> > > > > > -libsdl2_image = dependency('SDL2_image', required : false)\n> > > > > > +libjpeg = dependency('libjpeg', required : false)\n> > > > >\n> > > > > Same here.\n> > > > >\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> > > > > > @@ -58,7 +58,7 @@ cam = executable('cam', cam_sources,\n> > > > > > libdrm,\n> > > > > > libevent,\n> > > > > > libsdl2,\n> > > > > > - libsdl2_image,\n> > > > > > + libjpeg,\n> > > > >\n> > > > > And here.\n> > > > >\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.h b/src/cam/sdl_texture.h\n> > > > > > index 90974798..42c906f2 100644\n> > > > > > --- a/src/cam/sdl_texture.h\n> > > > > > +++ b/src/cam/sdl_texture.h\n> > > > > > @@ -25,5 +25,5 @@ protected:\n> > > > > > SDL_Texture *ptr_;\n> > > > > > const SDL_Rect rect_;\n> > > > > > const SDL_PixelFormatEnum pixelFormat_;\n> > > > > > - const int pitch_;\n> > > > > > + int pitch_;\n> > > > > > };\n> > > > > > diff --git a/src/cam/sdl_texture_mjpg.cpp b/src/cam/sdl_texture_mjpg.cpp\n> > > > > > index 69e99ad3..232af673 100644\n> > > > > > --- a/src/cam/sdl_texture_mjpg.cpp\n> > > > > > +++ b/src/cam/sdl_texture_mjpg.cpp\n> > > > > > @@ -7,19 +7,52 @@\n> > > > > >\n> > > > > > #include \"sdl_texture_mjpg.h\"\n> > > > > >\n> > > > > > -#include \n> > > > > > +#include \n> > > > > >\n> > > > > > using namespace libcamera;\n> > > > > >\n> > > > > > SDLTextureMJPG::SDLTextureMJPG(const SDL_Rect &rect)\n> > > > > > : SDLTexture(rect, SDL_PIXELFORMAT_RGB24, 0)\n> > > > >\n> > > > > You can write\n> > > > >\n> > > > > : SDLTexture(rect, SDL_PIXELFORMAT_RGB24, rect.w * 3)\n> > > > >\n> > > > > > {\n> > > > > > + pitch_ = rect_.w * 3;\n> > > > >\n> > > > > and drop this line, and keep pitch_ const.\n> > > > >\n> > > > > > + rgbdata_ = (unsigned char *)malloc(pitch_ * rect_.h);\n> > > > >\n> > > > > Please use C++-style memory allocation\n> > > > >\n> > > > > rgbdata_ = new unsigned char[pitch_ * rect_.h];\n> > > > >\n> > > > > The destructor should then use\n> > > > >\n> > > > > delete[] rgbdata_;\n> > > > >\n> > > > > Even better, I think you can turn rgbdata_ into a\n> > > > >\n> > > > > std::unique_ptr rgbdata_;\n> > > > >\n> > > > > and drop the delete.\n> > > > >\n> > > > > Another option would have been to turn rgbdata_ into an\n> > > > > std::vector to also remove the need for a custom\n> > > > > destructor. The drawback is that, unlike new that performs\n> > > > > default-initialization, std::vector::resize() performs\n> > > > > default-insertion, which uses value-initialization, effectively\n> > > > > memsetting the vector to 0. There's a way around that by providing a\n> > > > > custom allocator to the vector, but I think we're getting into the \"not\n> > > > > worth it\" territory (at least as long as libcamera-base doesn't provide\n> > > > > a helper for this). Let's thus use a unique_ptr.\n> > > > >\n> > > > > > +}\n> > > > > > +\n> > > > > > +SDLTextureMJPG::~SDLTextureMJPG()\n> > > > > > +{\n> > > > > > + free(rgbdata_);\n> > > > > > +}\n> > > > > > +\n> > > > > > +void SDLTextureMJPG::decompress(const unsigned char *jpeg,\n> > > > > > + unsigned long jpeg_size)\n> > > > > > +{\n> > > > > > + struct jpeg_error_mgr jerr;\n> > > > > > + struct jpeg_decompress_struct cinfo;\n> > > > > > + cinfo.err = jpeg_std_error(&jerr);\n> > > > >\n> > > > > There's a risk the JPEG data could be corrupted (that's quite common\n> > > > > with UVC cameras). Error handling should be a bit more robust, you\n> > > > > should create your own error handler, store that an error occurred, and\n> > > > > return an error from this function and skip the SDL_UpdateTexture() in\n> > > > > that case.\n> > > > >\n> > > > > Furthermore, jpg_std_error() will by default call exit() when a fatal\n> > > > > error occurs. That's not good. The recommended way to deal with this is\n> > > > > to use setjmp() and longjmp() to exit cleanly instead.\n> > > > >\n> > > > > > +\n> > > > > > + jpeg_create_decompress(&cinfo);\n> > > > > > +\n> > > > > > + jpeg_mem_src(&cinfo, jpeg, jpeg_size);\n> > > > > > +\n> > > > > > + jpeg_read_header(&cinfo, TRUE);\n> > > > > > +\n> > > > > > + jpeg_start_decompress(&cinfo);\n> > > > > > +\n> > > > > > + int row_stride = cinfo.output_width * cinfo.output_components;\n> > > > >\n> > > > > rowStride (libcamera coding style)\n> > > > >\n> > > > > > + unsigned char *buffer = (unsigned char *)malloc(row_stride);\n> > > > >\n> > > > > Here too, std::unique_ptr and new[], or just\n> > > > >\n> > > > > unsigned char buffer[row_stride);\n> > > >\n> > > > For buffer, I need to pass the address of the variable storing the\n> > > > pointer to jpeg_read_scanlines, not sure unique_ptr is possible in\n> > > > this very specific case? (although I can easily change rgbdata_ to\n> > > > unique_ptr, thanks for spotting that).\n> > > >\n> > > > \"unsigned char buffer[row_stride];\" won't work for the same reason,\n> > > > needs to be a raw C pointer to pass &buffer to jpeg_read_scanlines.\n> > >\n> > > Good point. You could have a separate variable:\n> > >\n> > > unsigned char row[rowStride];\n> > > unsigned char *rows[1] = { &row };\n> > >\n> > > Not sure if it's worth it. This makes me wonder, is there a performance\n> > > benefit from reading multiple scanlines in one go ?\n> > >\n> > > > All the other feedback makes sense and will be addressed.\n> > > >\n> > > > > > + for (int i = 0; cinfo.output_scanline < cinfo.output_height; ++i) {\n> > > > > > + jpeg_read_scanlines(&cinfo, &buffer, 1);\n> > > > > > + memcpy(rgbdata_ + i * pitch_, buffer, row_stride);\n> > > > > > + }\n> > > > > > +\n> > > > > > + free(buffer);\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> > > > > > + SDL_UpdateTexture(ptr_, nullptr, rgbdata_, pitch_);\n> > > > > > }\n> > > > > > diff --git a/src/cam/sdl_texture_mjpg.h b/src/cam/sdl_texture_mjpg.h\n> > > > > > index b103f801..73c7fb68 100644\n> > > > > > --- a/src/cam/sdl_texture_mjpg.h\n> > > > > > +++ b/src/cam/sdl_texture_mjpg.h\n> > > > > > @@ -13,5 +13,10 @@ class SDLTextureMJPG : public SDLTexture\n> > > > > > {\n> > > > > > public:\n> > > > > > SDLTextureMJPG(const SDL_Rect &rect);\n> > > > > > + virtual ~SDLTextureMJPG();\n> > > > >\n> > > > > No need for virtual here, and while at it, you can insert a blank line.\n> > > > >\n> > > > > > void update(const libcamera::Span &data) override;\n> > > > > > +\n> > > > > > +private:\n> > > > > > + unsigned char *rgbdata_ = 0;\n> > > > >\n> > > > > Member data after member functions. I'd name the member rgbData_ or just\n> > > > > rgb_, up to you.\n> > > > >\n> > > > > > + void decompress(const unsigned char *jpeg, unsigned long jpeg_size);\n> > > > >\n> > > > > void decompress(const unsigned char *jpeg, std::size size);\n> > > > >\n> > > > > > };\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 28364BD1F1\n\tfor ;\n\tFri, 15 Jul 2022 00:06:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 93DBA63312;\n\tFri, 15 Jul 2022 02:06:55 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E503360489\n\tfor ;\n\tFri, 15 Jul 2022 02:06:53 +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 6C0679DA;\n\tFri, 15 Jul 2022 02:06:53 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1657843615;\n\tbh=MHHA3Ftu2zabARWznC15+VKbHohLWjBrtw3GZco2Ue8=;\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=JgC5g93YZjPMbImFtRw1C/iF5c45er7feN6Qs3BHuuUQ9p523lZFH3ER+uPY07OJg\n\tbeBrczidLERQhWZzBBrMrnzGkh0IyBJp031qxSxDC1AaUu7sgeEXi2vi2N/TpDFYvR\n\t458Ge4b0KewU6Db+6QpFlAP/CAC68PT4s2jt5S2msyHe6aQoAGVmSBBeqaGrY2EoPw\n\tDOOGDns4KcFNhG3JGhqnESnP/BOAtSdKTRMFXEvGgmLbBFun1pj4bU0T5hg1ld8Y4M\n\tjmV2HvxOuPcMbLKyOHkbVQA8bEwi7u5J1zxChJJDgy/F451qNlNcNBGvUlmKhVD6Eh\n\t6lhVDohILBOUA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1657843613;\n\tbh=MHHA3Ftu2zabARWznC15+VKbHohLWjBrtw3GZco2Ue8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WbaIBycsG5QWKHGAE01rI+tBdH5CSDORL4+DcVKq2KWL04D8U6+2RRBWFrqcKNfDm\n\tbSxa8yr+gHemNroYAe6XCqBsqkL5ISX7WYuFLyeyaB8RVxt8mKZ/nNle4MtIJkAqaM\n\tniNEuIjpIaLmExDMnKv2toX5fvS2JBgv132YAFq0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"WbaIBycs\"; dkim-atps=neutral","Date":"Fri, 15 Jul 2022 03:06:22 +0300","To":"Eric Curtin ","Message-ID":"","References":"<20220707155312.45807-1-ecurtin@redhat.com>\n\t\n\t\n\t\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] 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":"libcamera devel , jozef@mlich.cz","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23938,"web_url":"https://patchwork.libcamera.org/comment/23938/","msgid":"","date":"2022-07-15T16:42:36","subject":"Re: [libcamera-devel] [PATCH] 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 Fri, 8 Jul 2022 at 14:38, Laurent Pinchart\n wrote:\n>\n> Hi Eric,\n>\n> On Thu, Jul 07, 2022 at 11:25:58PM +0100, Eric Curtin wrote:\n> > On Thu, 7 Jul 2022 at 21:13, Laurent Pinchart wrote:\n> > > On Thu, Jul 07, 2022 at 04:53:13PM +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> > > > README.rst | 2 +-\n> > > > src/cam/meson.build | 8 +++----\n> > > > src/cam/sdl_sink.cpp | 4 ++--\n> > > > src/cam/sdl_texture.h | 2 +-\n> > > > src/cam/sdl_texture_mjpg.cpp | 43 +++++++++++++++++++++++++++++++-----\n> > > > src/cam/sdl_texture_mjpg.h | 5 +++++\n> > > > 6 files changed, 51 insertions(+), 13 deletions(-)\n> > > >\n> > > > diff --git a/README.rst b/README.rst\n> > > > index b9e72d81..5e8bc1cc 100644\n> > > > --- a/README.rst\n> > > > +++ b/README.rst\n> > > > @@ -93,7 +93,7 @@ for cam: [optional]\n> > > >\n> > > > - libdrm-dev: Enables the KMS sink\n> > > > - libsdl2-dev: Enables the SDL sink\n> > > > - - libsdl2-image-dev: Supports MJPEG on the SDL sink\n> > > > + - libjpeg-dev: Supports MJPEG on the SDL sink\n> > >\n> > > Could you please keep this list sorted ?\n> > >\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..27cbd0de 100644\n> > > > --- a/src/cam/meson.build\n> > > > +++ b/src/cam/meson.build\n> > > > @@ -25,7 +25,7 @@ cam_cpp_args = []\n> > > >\n> > > > libdrm = dependency('libdrm', required : false)\n> > > > libsdl2 = dependency('SDL2', required : false)\n> > > > -libsdl2_image = dependency('SDL2_image', required : false)\n> > > > +libjpeg = dependency('libjpeg', required : false)\n> > >\n> > > Same here.\n> > >\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> > > > @@ -58,7 +58,7 @@ cam = executable('cam', cam_sources,\n> > > > libdrm,\n> > > > libevent,\n> > > > libsdl2,\n> > > > - libsdl2_image,\n> > > > + libjpeg,\n> > >\n> > > And here.\n> > >\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.h b/src/cam/sdl_texture.h\n> > > > index 90974798..42c906f2 100644\n> > > > --- a/src/cam/sdl_texture.h\n> > > > +++ b/src/cam/sdl_texture.h\n> > > > @@ -25,5 +25,5 @@ protected:\n> > > > SDL_Texture *ptr_;\n> > > > const SDL_Rect rect_;\n> > > > const SDL_PixelFormatEnum pixelFormat_;\n> > > > - const int pitch_;\n> > > > + int pitch_;\n> > > > };\n> > > > diff --git a/src/cam/sdl_texture_mjpg.cpp b/src/cam/sdl_texture_mjpg.cpp\n> > > > index 69e99ad3..232af673 100644\n> > > > --- a/src/cam/sdl_texture_mjpg.cpp\n> > > > +++ b/src/cam/sdl_texture_mjpg.cpp\n> > > > @@ -7,19 +7,52 @@\n> > > >\n> > > > #include \"sdl_texture_mjpg.h\"\n> > > >\n> > > > -#include \n> > > > +#include \n> > > >\n> > > > using namespace libcamera;\n> > > >\n> > > > SDLTextureMJPG::SDLTextureMJPG(const SDL_Rect &rect)\n> > > > : SDLTexture(rect, SDL_PIXELFORMAT_RGB24, 0)\n> > >\n> > > You can write\n> > >\n> > > : SDLTexture(rect, SDL_PIXELFORMAT_RGB24, rect.w * 3)\n> > >\n> > > > {\n> > > > + pitch_ = rect_.w * 3;\n> > >\n> > > and drop this line, and keep pitch_ const.\n> > >\n> > > > + rgbdata_ = (unsigned char *)malloc(pitch_ * rect_.h);\n> > >\n> > > Please use C++-style memory allocation\n> > >\n> > > rgbdata_ = new unsigned char[pitch_ * rect_.h];\n> > >\n> > > The destructor should then use\n> > >\n> > > delete[] rgbdata_;\n> > >\n> > > Even better, I think you can turn rgbdata_ into a\n> > >\n> > > std::unique_ptr rgbdata_;\n> > >\n> > > and drop the delete.\n> > >\n> > > Another option would have been to turn rgbdata_ into an\n> > > std::vector to also remove the need for a custom\n> > > destructor. The drawback is that, unlike new that performs\n> > > default-initialization, std::vector::resize() performs\n> > > default-insertion, which uses value-initialization, effectively\n> > > memsetting the vector to 0. There's a way around that by providing a\n> > > custom allocator to the vector, but I think we're getting into the \"not\n> > > worth it\" territory (at least as long as libcamera-base doesn't provide\n> > > a helper for this). Let's thus use a unique_ptr.\n> > >\n> > > > +}\n> > > > +\n> > > > +SDLTextureMJPG::~SDLTextureMJPG()\n> > > > +{\n> > > > + free(rgbdata_);\n> > > > +}\n> > > > +\n> > > > +void SDLTextureMJPG::decompress(const unsigned char *jpeg,\n> > > > + unsigned long jpeg_size)\n> > > > +{\n> > > > + struct jpeg_error_mgr jerr;\n> > > > + struct jpeg_decompress_struct cinfo;\n> > > > + cinfo.err = jpeg_std_error(&jerr);\n> > >\n> > > There's a risk the JPEG data could be corrupted (that's quite common\n> > > with UVC cameras). Error handling should be a bit more robust, you\n> > > should create your own error handler, store that an error occurred, and\n> > > return an error from this function and skip the SDL_UpdateTexture() in\n> > > that case.\n> > >\n> > > Furthermore, jpg_std_error() will by default call exit() when a fatal\n> > > error occurs. That's not good. The recommended way to deal with this is\n> > > to use setjmp() and longjmp() to exit cleanly instead.\n> > >\n> > > > +\n> > > > + jpeg_create_decompress(&cinfo);\n> > > > +\n> > > > + jpeg_mem_src(&cinfo, jpeg, jpeg_size);\n> > > > +\n> > > > + jpeg_read_header(&cinfo, TRUE);\n> > > > +\n> > > > + jpeg_start_decompress(&cinfo);\n> > > > +\n> > > > + int row_stride = cinfo.output_width * cinfo.output_components;\n> > >\n> > > rowStride (libcamera coding style)\n> > >\n> > > > + unsigned char *buffer = (unsigned char *)malloc(row_stride);\n> > >\n> > > Here too, std::unique_ptr and new[], or just\n> > >\n> > > unsigned char buffer[row_stride);\n> >\n> > For buffer, I need to pass the address of the variable storing the\n> > pointer to jpeg_read_scanlines, not sure unique_ptr is possible in\n> > this very specific case? (although I can easily change rgbdata_ to\n> > unique_ptr, thanks for spotting that).\n> >\n> > \"unsigned char buffer[row_stride];\" won't work for the same reason,\n> > needs to be a raw C pointer to pass &buffer to jpeg_read_scanlines.\n>\n> Good point. You could have a separate variable:\n>\n> unsigned char row[rowStride];\n> unsigned char *rows[1] = { &row };\n>\n> Not sure if it's worth it. This makes me wonder, is there a performance\n> benefit from reading multiple scanlines in one go ?\n\nFor the record I tried to read every scanline in one go and the\ndecompression failed, then I left it as is, one scanline at a time\nseems to be the most common approach, it's simple and seems to have\ndecent performance.\n\n>\n> > All the other feedback makes sense and will be addressed.\n> >\n> > > > + for (int i = 0; cinfo.output_scanline < cinfo.output_height; ++i) {\n> > > > + jpeg_read_scanlines(&cinfo, &buffer, 1);\n> > > > + memcpy(rgbdata_ + i * pitch_, buffer, row_stride);\n> > > > + }\n> > > > +\n> > > > + free(buffer);\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> > > > + SDL_UpdateTexture(ptr_, nullptr, rgbdata_, pitch_);\n> > > > }\n> > > > diff --git a/src/cam/sdl_texture_mjpg.h b/src/cam/sdl_texture_mjpg.h\n> > > > index b103f801..73c7fb68 100644\n> > > > --- a/src/cam/sdl_texture_mjpg.h\n> > > > +++ b/src/cam/sdl_texture_mjpg.h\n> > > > @@ -13,5 +13,10 @@ class SDLTextureMJPG : public SDLTexture\n> > > > {\n> > > > public:\n> > > > SDLTextureMJPG(const SDL_Rect &rect);\n> > > > + virtual ~SDLTextureMJPG();\n> > >\n> > > No need for virtual here, and while at it, you can insert a blank line.\n> > >\n> > > > void update(const libcamera::Span &data) override;\n> > > > +\n> > > > +private:\n> > > > + unsigned char *rgbdata_ = 0;\n> > >\n> > > Member data after member functions. I'd name the member rgbData_ or just\n> > > rgb_, up to you.\n> > >\n> > > > + void decompress(const unsigned char *jpeg, unsigned long jpeg_size);\n> > >\n> > > void decompress(const unsigned char *jpeg, std::size size);\n> > >\n> > > > };\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 46460BD1F1\n\tfor ;\n\tFri, 15 Jul 2022 16:43:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4110063312;\n\tFri, 15 Jul 2022 18:43:08 +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 50ABE6330A\n\tfor ;\n\tFri, 15 Jul 2022 18:43:06 +0200 (CEST)","from mail-qk1-f199.google.com (mail-qk1-f199.google.com\n\t[209.85.222.199]) 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-85-23YX5xpPPSyU3NmWOXjPGA-1; Fri, 15 Jul 2022 12:43:03 -0400","by mail-qk1-f199.google.com with SMTP id\n\tbp14-20020a05620a458e00b006b59195c391so3817684qkb.19\n\tfor ;\n\tFri, 15 Jul 2022 09:43:03 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1657903388;\n\tbh=u+IHJSkdqFBIOr7klPb9mVFiQCnUVTdp9ZrPLcACWhc=;\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=tiKDXlAg6dkZgxjsFbedpFyF5B2jdEX11mzmouhQLQG/RGVanlY1D1IkUXb5a42Ii\n\tpYf0zKJihYF2yufUTNbEkRphbZsziMnfwySwSou3pxCTgGN5wL6jaDZdH5lppzcS2B\n\t4atfmIo4zWzc4QZbGqns+/S2nh/wDFDbNBnwejBR89UN2cvMalBXFP4bNSnINsio1g\n\tWO10XZ+3hshLWSQYuuMmFG4atdV6N+ONtcnfY/naKP29ENm9vlFRZHqIKu5LXtJupw\n\tECEMtUx9yWZ09hQLkAV5U/oe4UqP0Gzq7kZFLhIHRzspRPdrnWLyLnzU+zMCmI643e\n\tqgGqG0Hjvrvxg==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1657903385;\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=UeZVm+6GBLYSya1Ko9VP3rCeJV+27BKTJvPQFOigotE=;\n\tb=N9Tqxr4GG5T+TlTD27TJE8PIbFF5jwFNXLr46kCJF7W4moXi/ZQZMRWJmBHVuGwhVB0+67\n\tHqE6bA2nox8jF9ELVMEM05qTdKBeSzZCvGLPWzASsDnN6Ltj5btgTUjRJr+rWvRI3rG7Oq\n\t4SCVG+rnjS7OxF/5c3GpPhxZ2smaCUc="],"Authentication-Results":["lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=redhat.com\n\theader.i=@redhat.com header.b=\"N9Tqxr4G\"; \n\tdkim-atps=neutral","relay.mimecast.com;\n\tauth=pass smtp.auth=CUSA124A263 smtp.mailfrom=ecurtin@redhat.com"],"X-MC-Unique":"23YX5xpPPSyU3NmWOXjPGA-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=UeZVm+6GBLYSya1Ko9VP3rCeJV+27BKTJvPQFOigotE=;\n\tb=xPu1yhrQkV6t2FyOltKQaWzKux1Iwgm3/mkDvLn4VVWWJOjlQUzXubagylEvjhvSQm\n\tyXEHcRopDTyLP10dED0BpkXDsjcoxcQC9PTA9BquWu/aYy5HdrEHKWL9EmxVyGauEanD\n\tFpFCZBEaItMAihqt0Zgkq2ADoLFd19FQt+MU/fTMokMoZ2e/hGLQR4Yh1pcXIK1auNvW\n\tV5KjNFAQGFCQU6aIA2KRnpk5BvfCJaBY4hBT2lnFewldV55ffIkQ6qe/fKoNBD3VK98p\n\tvKfrogbwCvw4Jv54C0c4ZUq9pTl4u4hptZqNaeYJqWfeuFCYzMFd9OUe4DHTpYEC6n5e\n\ti2LQ==","X-Gm-Message-State":"AJIora9XTTWNbsgWhPuWiqq2MZnfuXXFmz352MHTBDzKfqF5Wxki9Y++\n\tvbPwe006Kwv3/rUBJBxLTjPILtusLgvX/Oi5a6hFiSSeos6R9AesEioV5fkK8ZmcCwmg28pbu9x\n\t2gIYcEXpU+JXTyhH1jqKACMFLTcKXW0pu3zhMnsUN9/7217pT+g==","X-Received":["by 2002:a0c:b2d0:0:b0:473:2c19:f1ee with SMTP id\n\td16-20020a0cb2d0000000b004732c19f1eemr12687094qvf.130.1657903382930; \n\tFri, 15 Jul 2022 09:43:02 -0700 (PDT)","by 2002:a0c:b2d0:0:b0:473:2c19:f1ee with SMTP id\n\td16-20020a0cb2d0000000b004732c19f1eemr12687070qvf.130.1657903382559;\n\tFri, 15 Jul 2022 09:43:02 -0700 (PDT)"],"X-Google-Smtp-Source":"AGRyM1vCxGFcz++tQLj4iRLOJsdWs61xA+x+uo4Jl73TalNuUz2Sjnb3oIJTxp9hYOdKQ0cBuTrGZiA/4GP1e/W0xEs=","MIME-Version":"1.0","References":"<20220707155312.45807-1-ecurtin@redhat.com>\n\t\n\t\n\t","In-Reply-To":"","Date":"Fri, 15 Jul 2022 17:42:36 +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] 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":"libcamera devel , jozef@mlich.cz","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]