| Message ID | 20220707155312.45807-1-ecurtin@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Eric, Thank you for the patch. On Thu, Jul 07, 2022 at 04:53:13PM +0100, Eric Curtin wrote: > We were using the libjpeg functionality of SDL2_image only, instead just > use libjpeg directly to reduce our dependancy count, it is a more > commonly available library. > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > --- > README.rst | 2 +- > src/cam/meson.build | 8 +++---- > src/cam/sdl_sink.cpp | 4 ++-- > src/cam/sdl_texture.h | 2 +- > src/cam/sdl_texture_mjpg.cpp | 43 +++++++++++++++++++++++++++++++----- > src/cam/sdl_texture_mjpg.h | 5 +++++ > 6 files changed, 51 insertions(+), 13 deletions(-) > > diff --git a/README.rst b/README.rst > index b9e72d81..5e8bc1cc 100644 > --- a/README.rst > +++ b/README.rst > @@ -93,7 +93,7 @@ for cam: [optional] > > - libdrm-dev: Enables the KMS sink > - libsdl2-dev: Enables the SDL sink > - - libsdl2-image-dev: Supports MJPEG on the SDL sink > + - libjpeg-dev: Supports MJPEG on the SDL sink Could you please keep this list sorted ? > > for qcam: [optional] > qtbase5-dev libqt5core5a libqt5gui5 libqt5widgets5 qttools5-dev-tools libtiff-dev > diff --git a/src/cam/meson.build b/src/cam/meson.build > index 5957ce14..27cbd0de 100644 > --- a/src/cam/meson.build > +++ b/src/cam/meson.build > @@ -25,7 +25,7 @@ cam_cpp_args = [] > > libdrm = dependency('libdrm', required : false) > libsdl2 = dependency('SDL2', required : false) > -libsdl2_image = dependency('SDL2_image', required : false) > +libjpeg = dependency('libjpeg', required : false) Same here. > > if libdrm.found() > cam_cpp_args += [ '-DHAVE_KMS' ] > @@ -43,8 +43,8 @@ if libsdl2.found() > 'sdl_texture_yuyv.cpp' > ]) > > - if libsdl2_image.found() > - cam_cpp_args += ['-DHAVE_SDL_IMAGE'] > + if libjpeg.found() > + cam_cpp_args += ['-DHAVE_LIBJPEG'] > cam_sources += files([ > 'sdl_texture_mjpg.cpp' > ]) > @@ -58,7 +58,7 @@ cam = executable('cam', cam_sources, > libdrm, > libevent, > libsdl2, > - libsdl2_image, > + libjpeg, And here. > libyaml, > ], > cpp_args : cam_cpp_args, > diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp > index f8e3e95d..19fdfd6d 100644 > --- a/src/cam/sdl_sink.cpp > +++ b/src/cam/sdl_sink.cpp > @@ -21,7 +21,7 @@ > > #include "event_loop.h" > #include "image.h" > -#ifdef HAVE_SDL_IMAGE > +#ifdef HAVE_LIBJPEG > #include "sdl_texture_mjpg.h" > #endif > #include "sdl_texture_yuyv.h" > @@ -62,7 +62,7 @@ int SDLSink::configure(const libcamera::CameraConfiguration &config) > rect_.h = cfg.size.height; > > switch (cfg.pixelFormat) { > -#ifdef HAVE_SDL_IMAGE > +#ifdef HAVE_LIBJPEG > case libcamera::formats::MJPEG: > texture_ = std::make_unique<SDLTextureMJPG>(rect_); > break; > diff --git a/src/cam/sdl_texture.h b/src/cam/sdl_texture.h > index 90974798..42c906f2 100644 > --- a/src/cam/sdl_texture.h > +++ b/src/cam/sdl_texture.h > @@ -25,5 +25,5 @@ protected: > SDL_Texture *ptr_; > const SDL_Rect rect_; > const SDL_PixelFormatEnum pixelFormat_; > - const int pitch_; > + int pitch_; > }; > diff --git a/src/cam/sdl_texture_mjpg.cpp b/src/cam/sdl_texture_mjpg.cpp > index 69e99ad3..232af673 100644 > --- a/src/cam/sdl_texture_mjpg.cpp > +++ b/src/cam/sdl_texture_mjpg.cpp > @@ -7,19 +7,52 @@ > > #include "sdl_texture_mjpg.h" > > -#include <SDL2/SDL_image.h> > +#include <jpeglib.h> > > using namespace libcamera; > > SDLTextureMJPG::SDLTextureMJPG(const SDL_Rect &rect) > : SDLTexture(rect, SDL_PIXELFORMAT_RGB24, 0) You can write : SDLTexture(rect, SDL_PIXELFORMAT_RGB24, rect.w * 3) > { > + pitch_ = rect_.w * 3; and drop this line, and keep pitch_ const. > + rgbdata_ = (unsigned char *)malloc(pitch_ * rect_.h); Please use C++-style memory allocation rgbdata_ = new unsigned char[pitch_ * rect_.h]; The destructor should then use delete[] rgbdata_; Even better, I think you can turn rgbdata_ into a std::unique_ptr<unsigned char[]> rgbdata_; and drop the delete. Another option would have been to turn rgbdata_ into an std::vector<unsigned char> to also remove the need for a custom destructor. The drawback is that, unlike new that performs default-initialization, std::vector::resize() performs default-insertion, which uses value-initialization, effectively memsetting the vector to 0. There's a way around that by providing a custom allocator to the vector, but I think we're getting into the "not worth it" territory (at least as long as libcamera-base doesn't provide a helper for this). Let's thus use a unique_ptr. > +} > + > +SDLTextureMJPG::~SDLTextureMJPG() > +{ > + free(rgbdata_); > +} > + > +void SDLTextureMJPG::decompress(const unsigned char *jpeg, > + unsigned long jpeg_size) > +{ > + struct jpeg_error_mgr jerr; > + struct jpeg_decompress_struct cinfo; > + cinfo.err = jpeg_std_error(&jerr); There's a risk the JPEG data could be corrupted (that's quite common with UVC cameras). Error handling should be a bit more robust, you should create your own error handler, store that an error occurred, and return an error from this function and skip the SDL_UpdateTexture() in that case. Furthermore, jpg_std_error() will by default call exit() when a fatal error occurs. That's not good. The recommended way to deal with this is to use setjmp() and longjmp() to exit cleanly instead. > + > + jpeg_create_decompress(&cinfo); > + > + jpeg_mem_src(&cinfo, jpeg, jpeg_size); > + > + jpeg_read_header(&cinfo, TRUE); > + > + jpeg_start_decompress(&cinfo); > + > + int row_stride = cinfo.output_width * cinfo.output_components; rowStride (libcamera coding style) > + unsigned char *buffer = (unsigned char *)malloc(row_stride); Here too, std::unique_ptr and new[], or just unsigned char buffer[row_stride); > + for (int i = 0; cinfo.output_scanline < cinfo.output_height; ++i) { > + jpeg_read_scanlines(&cinfo, &buffer, 1); > + memcpy(rgbdata_ + i * pitch_, buffer, row_stride); > + } > + > + free(buffer); > + jpeg_finish_decompress(&cinfo); > + > + jpeg_destroy_decompress(&cinfo); > } > > void SDLTextureMJPG::update(const Span<uint8_t> &data) > { > - SDL_RWops *bufferStream = SDL_RWFromMem(data.data(), data.size()); > - SDL_Surface *frame = IMG_Load_RW(bufferStream, 0); > - SDL_UpdateTexture(ptr_, nullptr, frame->pixels, frame->pitch); > - SDL_FreeSurface(frame); > + decompress(data.data(), data.size()); > + SDL_UpdateTexture(ptr_, nullptr, rgbdata_, pitch_); > } > diff --git a/src/cam/sdl_texture_mjpg.h b/src/cam/sdl_texture_mjpg.h > index b103f801..73c7fb68 100644 > --- a/src/cam/sdl_texture_mjpg.h > +++ b/src/cam/sdl_texture_mjpg.h > @@ -13,5 +13,10 @@ class SDLTextureMJPG : public SDLTexture > { > public: > SDLTextureMJPG(const SDL_Rect &rect); > + virtual ~SDLTextureMJPG(); No need for virtual here, and while at it, you can insert a blank line. > void update(const libcamera::Span<uint8_t> &data) override; > + > +private: > + unsigned char *rgbdata_ = 0; Member data after member functions. I'd name the member rgbData_ or just rgb_, up to you. > + void decompress(const unsigned char *jpeg, unsigned long jpeg_size); void decompress(const unsigned char *jpeg, std::size size); > };
On Thu, 7 Jul 2022 at 21:13, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Eric, > > Thank you for the patch. > > On Thu, Jul 07, 2022 at 04:53:13PM +0100, Eric Curtin wrote: > > We were using the libjpeg functionality of SDL2_image only, instead just > > use libjpeg directly to reduce our dependancy count, it is a more > > commonly available library. > > > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > > --- > > README.rst | 2 +- > > src/cam/meson.build | 8 +++---- > > src/cam/sdl_sink.cpp | 4 ++-- > > src/cam/sdl_texture.h | 2 +- > > src/cam/sdl_texture_mjpg.cpp | 43 +++++++++++++++++++++++++++++++----- > > src/cam/sdl_texture_mjpg.h | 5 +++++ > > 6 files changed, 51 insertions(+), 13 deletions(-) > > > > diff --git a/README.rst b/README.rst > > index b9e72d81..5e8bc1cc 100644 > > --- a/README.rst > > +++ b/README.rst > > @@ -93,7 +93,7 @@ for cam: [optional] > > > > - libdrm-dev: Enables the KMS sink > > - libsdl2-dev: Enables the SDL sink > > - - libsdl2-image-dev: Supports MJPEG on the SDL sink > > + - libjpeg-dev: Supports MJPEG on the SDL sink > > Could you please keep this list sorted ? > > > > > for qcam: [optional] > > qtbase5-dev libqt5core5a libqt5gui5 libqt5widgets5 qttools5-dev-tools libtiff-dev > > diff --git a/src/cam/meson.build b/src/cam/meson.build > > index 5957ce14..27cbd0de 100644 > > --- a/src/cam/meson.build > > +++ b/src/cam/meson.build > > @@ -25,7 +25,7 @@ cam_cpp_args = [] > > > > libdrm = dependency('libdrm', required : false) > > libsdl2 = dependency('SDL2', required : false) > > -libsdl2_image = dependency('SDL2_image', required : false) > > +libjpeg = dependency('libjpeg', required : false) > > Same here. > > > > > if libdrm.found() > > cam_cpp_args += [ '-DHAVE_KMS' ] > > @@ -43,8 +43,8 @@ if libsdl2.found() > > 'sdl_texture_yuyv.cpp' > > ]) > > > > - if libsdl2_image.found() > > - cam_cpp_args += ['-DHAVE_SDL_IMAGE'] > > + if libjpeg.found() > > + cam_cpp_args += ['-DHAVE_LIBJPEG'] > > cam_sources += files([ > > 'sdl_texture_mjpg.cpp' > > ]) > > @@ -58,7 +58,7 @@ cam = executable('cam', cam_sources, > > libdrm, > > libevent, > > libsdl2, > > - libsdl2_image, > > + libjpeg, > > And here. > > > libyaml, > > ], > > cpp_args : cam_cpp_args, > > diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp > > index f8e3e95d..19fdfd6d 100644 > > --- a/src/cam/sdl_sink.cpp > > +++ b/src/cam/sdl_sink.cpp > > @@ -21,7 +21,7 @@ > > > > #include "event_loop.h" > > #include "image.h" > > -#ifdef HAVE_SDL_IMAGE > > +#ifdef HAVE_LIBJPEG > > #include "sdl_texture_mjpg.h" > > #endif > > #include "sdl_texture_yuyv.h" > > @@ -62,7 +62,7 @@ int SDLSink::configure(const libcamera::CameraConfiguration &config) > > rect_.h = cfg.size.height; > > > > switch (cfg.pixelFormat) { > > -#ifdef HAVE_SDL_IMAGE > > +#ifdef HAVE_LIBJPEG > > case libcamera::formats::MJPEG: > > texture_ = std::make_unique<SDLTextureMJPG>(rect_); > > break; > > diff --git a/src/cam/sdl_texture.h b/src/cam/sdl_texture.h > > index 90974798..42c906f2 100644 > > --- a/src/cam/sdl_texture.h > > +++ b/src/cam/sdl_texture.h > > @@ -25,5 +25,5 @@ protected: > > SDL_Texture *ptr_; > > const SDL_Rect rect_; > > const SDL_PixelFormatEnum pixelFormat_; > > - const int pitch_; > > + int pitch_; > > }; > > diff --git a/src/cam/sdl_texture_mjpg.cpp b/src/cam/sdl_texture_mjpg.cpp > > index 69e99ad3..232af673 100644 > > --- a/src/cam/sdl_texture_mjpg.cpp > > +++ b/src/cam/sdl_texture_mjpg.cpp > > @@ -7,19 +7,52 @@ > > > > #include "sdl_texture_mjpg.h" > > > > -#include <SDL2/SDL_image.h> > > +#include <jpeglib.h> > > > > using namespace libcamera; > > > > SDLTextureMJPG::SDLTextureMJPG(const SDL_Rect &rect) > > : SDLTexture(rect, SDL_PIXELFORMAT_RGB24, 0) > > You can write > > : SDLTexture(rect, SDL_PIXELFORMAT_RGB24, rect.w * 3) > > > { > > + pitch_ = rect_.w * 3; > > and drop this line, and keep pitch_ const. > > > + rgbdata_ = (unsigned char *)malloc(pitch_ * rect_.h); > > Please use C++-style memory allocation > > rgbdata_ = new unsigned char[pitch_ * rect_.h]; > > The destructor should then use > > delete[] rgbdata_; > > Even better, I think you can turn rgbdata_ into a > > std::unique_ptr<unsigned char[]> rgbdata_; > > and drop the delete. > > Another option would have been to turn rgbdata_ into an > std::vector<unsigned char> to also remove the need for a custom > destructor. The drawback is that, unlike new that performs > default-initialization, std::vector::resize() performs > default-insertion, which uses value-initialization, effectively > memsetting the vector to 0. There's a way around that by providing a > custom allocator to the vector, but I think we're getting into the "not > worth it" territory (at least as long as libcamera-base doesn't provide > a helper for this). Let's thus use a unique_ptr. > > > +} > > + > > +SDLTextureMJPG::~SDLTextureMJPG() > > +{ > > + free(rgbdata_); > > +} > > + > > +void SDLTextureMJPG::decompress(const unsigned char *jpeg, > > + unsigned long jpeg_size) > > +{ > > + struct jpeg_error_mgr jerr; > > + struct jpeg_decompress_struct cinfo; > > + cinfo.err = jpeg_std_error(&jerr); > > There's a risk the JPEG data could be corrupted (that's quite common > with UVC cameras). Error handling should be a bit more robust, you > should create your own error handler, store that an error occurred, and > return an error from this function and skip the SDL_UpdateTexture() in > that case. > > Furthermore, jpg_std_error() will by default call exit() when a fatal > error occurs. That's not good. The recommended way to deal with this is > to use setjmp() and longjmp() to exit cleanly instead. > > > + > > + jpeg_create_decompress(&cinfo); > > + > > + jpeg_mem_src(&cinfo, jpeg, jpeg_size); > > + > > + jpeg_read_header(&cinfo, TRUE); > > + > > + jpeg_start_decompress(&cinfo); > > + > > + int row_stride = cinfo.output_width * cinfo.output_components; > > rowStride (libcamera coding style) > > > + unsigned char *buffer = (unsigned char *)malloc(row_stride); > > Here too, std::unique_ptr and new[], or just > > unsigned char buffer[row_stride); For buffer, I need to pass the address of the variable storing the pointer to jpeg_read_scanlines, not sure unique_ptr is possible in this very specific case? (although I can easily change rgbdata_ to unique_ptr, thanks for spotting that). "unsigned char buffer[row_stride];" won't work for the same reason, needs to be a raw C pointer to pass &buffer to jpeg_read_scanlines. All the other feedback makes sense and will be addressed. > > > + for (int i = 0; cinfo.output_scanline < cinfo.output_height; ++i) { > > + jpeg_read_scanlines(&cinfo, &buffer, 1); > > + memcpy(rgbdata_ + i * pitch_, buffer, row_stride); > > + } > > + > > + free(buffer); > > + jpeg_finish_decompress(&cinfo); > > + > > + jpeg_destroy_decompress(&cinfo); > > } > > > > void SDLTextureMJPG::update(const Span<uint8_t> &data) > > { > > - SDL_RWops *bufferStream = SDL_RWFromMem(data.data(), data.size()); > > - SDL_Surface *frame = IMG_Load_RW(bufferStream, 0); > > - SDL_UpdateTexture(ptr_, nullptr, frame->pixels, frame->pitch); > > - SDL_FreeSurface(frame); > > + decompress(data.data(), data.size()); > > + SDL_UpdateTexture(ptr_, nullptr, rgbdata_, pitch_); > > } > > diff --git a/src/cam/sdl_texture_mjpg.h b/src/cam/sdl_texture_mjpg.h > > index b103f801..73c7fb68 100644 > > --- a/src/cam/sdl_texture_mjpg.h > > +++ b/src/cam/sdl_texture_mjpg.h > > @@ -13,5 +13,10 @@ class SDLTextureMJPG : public SDLTexture > > { > > public: > > SDLTextureMJPG(const SDL_Rect &rect); > > + virtual ~SDLTextureMJPG(); > > No need for virtual here, and while at it, you can insert a blank line. > > > void update(const libcamera::Span<uint8_t> &data) override; > > + > > +private: > > + unsigned char *rgbdata_ = 0; > > Member data after member functions. I'd name the member rgbData_ or just > rgb_, up to you. > > > + void decompress(const unsigned char *jpeg, unsigned long jpeg_size); > > void decompress(const unsigned char *jpeg, std::size size); > > > }; > > -- > Regards, > > Laurent Pinchart >
Hi Eric, On Thu, Jul 07, 2022 at 11:25:58PM +0100, Eric Curtin wrote: > On Thu, 7 Jul 2022 at 21:13, Laurent Pinchart wrote: > > On Thu, Jul 07, 2022 at 04:53:13PM +0100, Eric Curtin wrote: > > > We were using the libjpeg functionality of SDL2_image only, instead just > > > use libjpeg directly to reduce our dependancy count, it is a more > > > commonly available library. > > > > > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > > > --- > > > README.rst | 2 +- > > > src/cam/meson.build | 8 +++---- > > > src/cam/sdl_sink.cpp | 4 ++-- > > > src/cam/sdl_texture.h | 2 +- > > > src/cam/sdl_texture_mjpg.cpp | 43 +++++++++++++++++++++++++++++++----- > > > src/cam/sdl_texture_mjpg.h | 5 +++++ > > > 6 files changed, 51 insertions(+), 13 deletions(-) > > > > > > diff --git a/README.rst b/README.rst > > > index b9e72d81..5e8bc1cc 100644 > > > --- a/README.rst > > > +++ b/README.rst > > > @@ -93,7 +93,7 @@ for cam: [optional] > > > > > > - libdrm-dev: Enables the KMS sink > > > - libsdl2-dev: Enables the SDL sink > > > - - libsdl2-image-dev: Supports MJPEG on the SDL sink > > > + - libjpeg-dev: Supports MJPEG on the SDL sink > > > > Could you please keep this list sorted ? > > > > > > > > for qcam: [optional] > > > qtbase5-dev libqt5core5a libqt5gui5 libqt5widgets5 qttools5-dev-tools libtiff-dev > > > diff --git a/src/cam/meson.build b/src/cam/meson.build > > > index 5957ce14..27cbd0de 100644 > > > --- a/src/cam/meson.build > > > +++ b/src/cam/meson.build > > > @@ -25,7 +25,7 @@ cam_cpp_args = [] > > > > > > libdrm = dependency('libdrm', required : false) > > > libsdl2 = dependency('SDL2', required : false) > > > -libsdl2_image = dependency('SDL2_image', required : false) > > > +libjpeg = dependency('libjpeg', required : false) > > > > Same here. > > > > > > > > if libdrm.found() > > > cam_cpp_args += [ '-DHAVE_KMS' ] > > > @@ -43,8 +43,8 @@ if libsdl2.found() > > > 'sdl_texture_yuyv.cpp' > > > ]) > > > > > > - if libsdl2_image.found() > > > - cam_cpp_args += ['-DHAVE_SDL_IMAGE'] > > > + if libjpeg.found() > > > + cam_cpp_args += ['-DHAVE_LIBJPEG'] > > > cam_sources += files([ > > > 'sdl_texture_mjpg.cpp' > > > ]) > > > @@ -58,7 +58,7 @@ cam = executable('cam', cam_sources, > > > libdrm, > > > libevent, > > > libsdl2, > > > - libsdl2_image, > > > + libjpeg, > > > > And here. > > > > > libyaml, > > > ], > > > cpp_args : cam_cpp_args, > > > diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp > > > index f8e3e95d..19fdfd6d 100644 > > > --- a/src/cam/sdl_sink.cpp > > > +++ b/src/cam/sdl_sink.cpp > > > @@ -21,7 +21,7 @@ > > > > > > #include "event_loop.h" > > > #include "image.h" > > > -#ifdef HAVE_SDL_IMAGE > > > +#ifdef HAVE_LIBJPEG > > > #include "sdl_texture_mjpg.h" > > > #endif > > > #include "sdl_texture_yuyv.h" > > > @@ -62,7 +62,7 @@ int SDLSink::configure(const libcamera::CameraConfiguration &config) > > > rect_.h = cfg.size.height; > > > > > > switch (cfg.pixelFormat) { > > > -#ifdef HAVE_SDL_IMAGE > > > +#ifdef HAVE_LIBJPEG > > > case libcamera::formats::MJPEG: > > > texture_ = std::make_unique<SDLTextureMJPG>(rect_); > > > break; > > > diff --git a/src/cam/sdl_texture.h b/src/cam/sdl_texture.h > > > index 90974798..42c906f2 100644 > > > --- a/src/cam/sdl_texture.h > > > +++ b/src/cam/sdl_texture.h > > > @@ -25,5 +25,5 @@ protected: > > > SDL_Texture *ptr_; > > > const SDL_Rect rect_; > > > const SDL_PixelFormatEnum pixelFormat_; > > > - const int pitch_; > > > + int pitch_; > > > }; > > > diff --git a/src/cam/sdl_texture_mjpg.cpp b/src/cam/sdl_texture_mjpg.cpp > > > index 69e99ad3..232af673 100644 > > > --- a/src/cam/sdl_texture_mjpg.cpp > > > +++ b/src/cam/sdl_texture_mjpg.cpp > > > @@ -7,19 +7,52 @@ > > > > > > #include "sdl_texture_mjpg.h" > > > > > > -#include <SDL2/SDL_image.h> > > > +#include <jpeglib.h> > > > > > > using namespace libcamera; > > > > > > SDLTextureMJPG::SDLTextureMJPG(const SDL_Rect &rect) > > > : SDLTexture(rect, SDL_PIXELFORMAT_RGB24, 0) > > > > You can write > > > > : SDLTexture(rect, SDL_PIXELFORMAT_RGB24, rect.w * 3) > > > > > { > > > + pitch_ = rect_.w * 3; > > > > and drop this line, and keep pitch_ const. > > > > > + rgbdata_ = (unsigned char *)malloc(pitch_ * rect_.h); > > > > Please use C++-style memory allocation > > > > rgbdata_ = new unsigned char[pitch_ * rect_.h]; > > > > The destructor should then use > > > > delete[] rgbdata_; > > > > Even better, I think you can turn rgbdata_ into a > > > > std::unique_ptr<unsigned char[]> rgbdata_; > > > > and drop the delete. > > > > Another option would have been to turn rgbdata_ into an > > std::vector<unsigned char> to also remove the need for a custom > > destructor. The drawback is that, unlike new that performs > > default-initialization, std::vector::resize() performs > > default-insertion, which uses value-initialization, effectively > > memsetting the vector to 0. There's a way around that by providing a > > custom allocator to the vector, but I think we're getting into the "not > > worth it" territory (at least as long as libcamera-base doesn't provide > > a helper for this). Let's thus use a unique_ptr. > > > > > +} > > > + > > > +SDLTextureMJPG::~SDLTextureMJPG() > > > +{ > > > + free(rgbdata_); > > > +} > > > + > > > +void SDLTextureMJPG::decompress(const unsigned char *jpeg, > > > + unsigned long jpeg_size) > > > +{ > > > + struct jpeg_error_mgr jerr; > > > + struct jpeg_decompress_struct cinfo; > > > + cinfo.err = jpeg_std_error(&jerr); > > > > There's a risk the JPEG data could be corrupted (that's quite common > > with UVC cameras). Error handling should be a bit more robust, you > > should create your own error handler, store that an error occurred, and > > return an error from this function and skip the SDL_UpdateTexture() in > > that case. > > > > Furthermore, jpg_std_error() will by default call exit() when a fatal > > error occurs. That's not good. The recommended way to deal with this is > > to use setjmp() and longjmp() to exit cleanly instead. > > > > > + > > > + jpeg_create_decompress(&cinfo); > > > + > > > + jpeg_mem_src(&cinfo, jpeg, jpeg_size); > > > + > > > + jpeg_read_header(&cinfo, TRUE); > > > + > > > + jpeg_start_decompress(&cinfo); > > > + > > > + int row_stride = cinfo.output_width * cinfo.output_components; > > > > rowStride (libcamera coding style) > > > > > + unsigned char *buffer = (unsigned char *)malloc(row_stride); > > > > Here too, std::unique_ptr and new[], or just > > > > unsigned char buffer[row_stride); > > For buffer, I need to pass the address of the variable storing the > pointer to jpeg_read_scanlines, not sure unique_ptr is possible in > this very specific case? (although I can easily change rgbdata_ to > unique_ptr, thanks for spotting that). > > "unsigned char buffer[row_stride];" won't work for the same reason, > needs to be a raw C pointer to pass &buffer to jpeg_read_scanlines. Good point. You could have a separate variable: unsigned char row[rowStride]; unsigned char *rows[1] = { &row }; Not sure if it's worth it. This makes me wonder, is there a performance benefit from reading multiple scanlines in one go ? > All the other feedback makes sense and will be addressed. > > > > + for (int i = 0; cinfo.output_scanline < cinfo.output_height; ++i) { > > > + jpeg_read_scanlines(&cinfo, &buffer, 1); > > > + memcpy(rgbdata_ + i * pitch_, buffer, row_stride); > > > + } > > > + > > > + free(buffer); > > > + jpeg_finish_decompress(&cinfo); > > > + > > > + jpeg_destroy_decompress(&cinfo); > > > } > > > > > > void SDLTextureMJPG::update(const Span<uint8_t> &data) > > > { > > > - SDL_RWops *bufferStream = SDL_RWFromMem(data.data(), data.size()); > > > - SDL_Surface *frame = IMG_Load_RW(bufferStream, 0); > > > - SDL_UpdateTexture(ptr_, nullptr, frame->pixels, frame->pitch); > > > - SDL_FreeSurface(frame); > > > + decompress(data.data(), data.size()); > > > + SDL_UpdateTexture(ptr_, nullptr, rgbdata_, pitch_); > > > } > > > diff --git a/src/cam/sdl_texture_mjpg.h b/src/cam/sdl_texture_mjpg.h > > > index b103f801..73c7fb68 100644 > > > --- a/src/cam/sdl_texture_mjpg.h > > > +++ b/src/cam/sdl_texture_mjpg.h > > > @@ -13,5 +13,10 @@ class SDLTextureMJPG : public SDLTexture > > > { > > > public: > > > SDLTextureMJPG(const SDL_Rect &rect); > > > + virtual ~SDLTextureMJPG(); > > > > No need for virtual here, and while at it, you can insert a blank line. > > > > > void update(const libcamera::Span<uint8_t> &data) override; > > > + > > > +private: > > > + unsigned char *rgbdata_ = 0; > > > > Member data after member functions. I'd name the member rgbData_ or just > > rgb_, up to you. > > > > > + void decompress(const unsigned char *jpeg, unsigned long jpeg_size); > > > > void decompress(const unsigned char *jpeg, std::size size); > > > > > }; > >
Hi Eric, I was wondering if you planned to submit a new version of this patch. There's no big urgency, but if you expect to be too busy for the foreseable future, I can send a v2 that addresses my review comments. On Fri, Jul 08, 2022 at 04:37:50PM +0300, Laurent Pinchart via libcamera-devel wrote: > On Thu, Jul 07, 2022 at 11:25:58PM +0100, Eric Curtin wrote: > > On Thu, 7 Jul 2022 at 21:13, Laurent Pinchart wrote: > > > On Thu, Jul 07, 2022 at 04:53:13PM +0100, Eric Curtin wrote: > > > > We were using the libjpeg functionality of SDL2_image only, instead just > > > > use libjpeg directly to reduce our dependancy count, it is a more > > > > commonly available library. > > > > > > > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > > > > --- > > > > README.rst | 2 +- > > > > src/cam/meson.build | 8 +++---- > > > > src/cam/sdl_sink.cpp | 4 ++-- > > > > src/cam/sdl_texture.h | 2 +- > > > > src/cam/sdl_texture_mjpg.cpp | 43 +++++++++++++++++++++++++++++++----- > > > > src/cam/sdl_texture_mjpg.h | 5 +++++ > > > > 6 files changed, 51 insertions(+), 13 deletions(-) > > > > > > > > diff --git a/README.rst b/README.rst > > > > index b9e72d81..5e8bc1cc 100644 > > > > --- a/README.rst > > > > +++ b/README.rst > > > > @@ -93,7 +93,7 @@ for cam: [optional] > > > > > > > > - libdrm-dev: Enables the KMS sink > > > > - libsdl2-dev: Enables the SDL sink > > > > - - libsdl2-image-dev: Supports MJPEG on the SDL sink > > > > + - libjpeg-dev: Supports MJPEG on the SDL sink > > > > > > Could you please keep this list sorted ? > > > > > > > > > > > for qcam: [optional] > > > > qtbase5-dev libqt5core5a libqt5gui5 libqt5widgets5 qttools5-dev-tools libtiff-dev > > > > diff --git a/src/cam/meson.build b/src/cam/meson.build > > > > index 5957ce14..27cbd0de 100644 > > > > --- a/src/cam/meson.build > > > > +++ b/src/cam/meson.build > > > > @@ -25,7 +25,7 @@ cam_cpp_args = [] > > > > > > > > libdrm = dependency('libdrm', required : false) > > > > libsdl2 = dependency('SDL2', required : false) > > > > -libsdl2_image = dependency('SDL2_image', required : false) > > > > +libjpeg = dependency('libjpeg', required : false) > > > > > > Same here. > > > > > > > > > > > if libdrm.found() > > > > cam_cpp_args += [ '-DHAVE_KMS' ] > > > > @@ -43,8 +43,8 @@ if libsdl2.found() > > > > 'sdl_texture_yuyv.cpp' > > > > ]) > > > > > > > > - if libsdl2_image.found() > > > > - cam_cpp_args += ['-DHAVE_SDL_IMAGE'] > > > > + if libjpeg.found() > > > > + cam_cpp_args += ['-DHAVE_LIBJPEG'] > > > > cam_sources += files([ > > > > 'sdl_texture_mjpg.cpp' > > > > ]) > > > > @@ -58,7 +58,7 @@ cam = executable('cam', cam_sources, > > > > libdrm, > > > > libevent, > > > > libsdl2, > > > > - libsdl2_image, > > > > + libjpeg, > > > > > > And here. > > > > > > > libyaml, > > > > ], > > > > cpp_args : cam_cpp_args, > > > > diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp > > > > index f8e3e95d..19fdfd6d 100644 > > > > --- a/src/cam/sdl_sink.cpp > > > > +++ b/src/cam/sdl_sink.cpp > > > > @@ -21,7 +21,7 @@ > > > > > > > > #include "event_loop.h" > > > > #include "image.h" > > > > -#ifdef HAVE_SDL_IMAGE > > > > +#ifdef HAVE_LIBJPEG > > > > #include "sdl_texture_mjpg.h" > > > > #endif > > > > #include "sdl_texture_yuyv.h" > > > > @@ -62,7 +62,7 @@ int SDLSink::configure(const libcamera::CameraConfiguration &config) > > > > rect_.h = cfg.size.height; > > > > > > > > switch (cfg.pixelFormat) { > > > > -#ifdef HAVE_SDL_IMAGE > > > > +#ifdef HAVE_LIBJPEG > > > > case libcamera::formats::MJPEG: > > > > texture_ = std::make_unique<SDLTextureMJPG>(rect_); > > > > break; > > > > diff --git a/src/cam/sdl_texture.h b/src/cam/sdl_texture.h > > > > index 90974798..42c906f2 100644 > > > > --- a/src/cam/sdl_texture.h > > > > +++ b/src/cam/sdl_texture.h > > > > @@ -25,5 +25,5 @@ protected: > > > > SDL_Texture *ptr_; > > > > const SDL_Rect rect_; > > > > const SDL_PixelFormatEnum pixelFormat_; > > > > - const int pitch_; > > > > + int pitch_; > > > > }; > > > > diff --git a/src/cam/sdl_texture_mjpg.cpp b/src/cam/sdl_texture_mjpg.cpp > > > > index 69e99ad3..232af673 100644 > > > > --- a/src/cam/sdl_texture_mjpg.cpp > > > > +++ b/src/cam/sdl_texture_mjpg.cpp > > > > @@ -7,19 +7,52 @@ > > > > > > > > #include "sdl_texture_mjpg.h" > > > > > > > > -#include <SDL2/SDL_image.h> > > > > +#include <jpeglib.h> > > > > > > > > using namespace libcamera; > > > > > > > > SDLTextureMJPG::SDLTextureMJPG(const SDL_Rect &rect) > > > > : SDLTexture(rect, SDL_PIXELFORMAT_RGB24, 0) > > > > > > You can write > > > > > > : SDLTexture(rect, SDL_PIXELFORMAT_RGB24, rect.w * 3) > > > > > > > { > > > > + pitch_ = rect_.w * 3; > > > > > > and drop this line, and keep pitch_ const. > > > > > > > + rgbdata_ = (unsigned char *)malloc(pitch_ * rect_.h); > > > > > > Please use C++-style memory allocation > > > > > > rgbdata_ = new unsigned char[pitch_ * rect_.h]; > > > > > > The destructor should then use > > > > > > delete[] rgbdata_; > > > > > > Even better, I think you can turn rgbdata_ into a > > > > > > std::unique_ptr<unsigned char[]> rgbdata_; > > > > > > and drop the delete. > > > > > > Another option would have been to turn rgbdata_ into an > > > std::vector<unsigned char> to also remove the need for a custom > > > destructor. The drawback is that, unlike new that performs > > > default-initialization, std::vector::resize() performs > > > default-insertion, which uses value-initialization, effectively > > > memsetting the vector to 0. There's a way around that by providing a > > > custom allocator to the vector, but I think we're getting into the "not > > > worth it" territory (at least as long as libcamera-base doesn't provide > > > a helper for this). Let's thus use a unique_ptr. > > > > > > > +} > > > > + > > > > +SDLTextureMJPG::~SDLTextureMJPG() > > > > +{ > > > > + free(rgbdata_); > > > > +} > > > > + > > > > +void SDLTextureMJPG::decompress(const unsigned char *jpeg, > > > > + unsigned long jpeg_size) > > > > +{ > > > > + struct jpeg_error_mgr jerr; > > > > + struct jpeg_decompress_struct cinfo; > > > > + cinfo.err = jpeg_std_error(&jerr); > > > > > > There's a risk the JPEG data could be corrupted (that's quite common > > > with UVC cameras). Error handling should be a bit more robust, you > > > should create your own error handler, store that an error occurred, and > > > return an error from this function and skip the SDL_UpdateTexture() in > > > that case. > > > > > > Furthermore, jpg_std_error() will by default call exit() when a fatal > > > error occurs. That's not good. The recommended way to deal with this is > > > to use setjmp() and longjmp() to exit cleanly instead. > > > > > > > + > > > > + jpeg_create_decompress(&cinfo); > > > > + > > > > + jpeg_mem_src(&cinfo, jpeg, jpeg_size); > > > > + > > > > + jpeg_read_header(&cinfo, TRUE); > > > > + > > > > + jpeg_start_decompress(&cinfo); > > > > + > > > > + int row_stride = cinfo.output_width * cinfo.output_components; > > > > > > rowStride (libcamera coding style) > > > > > > > + unsigned char *buffer = (unsigned char *)malloc(row_stride); > > > > > > Here too, std::unique_ptr and new[], or just > > > > > > unsigned char buffer[row_stride); > > > > For buffer, I need to pass the address of the variable storing the > > pointer to jpeg_read_scanlines, not sure unique_ptr is possible in > > this very specific case? (although I can easily change rgbdata_ to > > unique_ptr, thanks for spotting that). > > > > "unsigned char buffer[row_stride];" won't work for the same reason, > > needs to be a raw C pointer to pass &buffer to jpeg_read_scanlines. > > Good point. You could have a separate variable: > > unsigned char row[rowStride]; > unsigned char *rows[1] = { &row }; > > Not sure if it's worth it. This makes me wonder, is there a performance > benefit from reading multiple scanlines in one go ? > > > All the other feedback makes sense and will be addressed. > > > > > > + for (int i = 0; cinfo.output_scanline < cinfo.output_height; ++i) { > > > > + jpeg_read_scanlines(&cinfo, &buffer, 1); > > > > + memcpy(rgbdata_ + i * pitch_, buffer, row_stride); > > > > + } > > > > + > > > > + free(buffer); > > > > + jpeg_finish_decompress(&cinfo); > > > > + > > > > + jpeg_destroy_decompress(&cinfo); > > > > } > > > > > > > > void SDLTextureMJPG::update(const Span<uint8_t> &data) > > > > { > > > > - SDL_RWops *bufferStream = SDL_RWFromMem(data.data(), data.size()); > > > > - SDL_Surface *frame = IMG_Load_RW(bufferStream, 0); > > > > - SDL_UpdateTexture(ptr_, nullptr, frame->pixels, frame->pitch); > > > > - SDL_FreeSurface(frame); > > > > + decompress(data.data(), data.size()); > > > > + SDL_UpdateTexture(ptr_, nullptr, rgbdata_, pitch_); > > > > } > > > > diff --git a/src/cam/sdl_texture_mjpg.h b/src/cam/sdl_texture_mjpg.h > > > > index b103f801..73c7fb68 100644 > > > > --- a/src/cam/sdl_texture_mjpg.h > > > > +++ b/src/cam/sdl_texture_mjpg.h > > > > @@ -13,5 +13,10 @@ class SDLTextureMJPG : public SDLTexture > > > > { > > > > public: > > > > SDLTextureMJPG(const SDL_Rect &rect); > > > > + virtual ~SDLTextureMJPG(); > > > > > > No need for virtual here, and while at it, you can insert a blank line. > > > > > > > void update(const libcamera::Span<uint8_t> &data) override; > > > > + > > > > +private: > > > > + unsigned char *rgbdata_ = 0; > > > > > > Member data after member functions. I'd name the member rgbData_ or just > > > rgb_, up to you. > > > > > > > + void decompress(const unsigned char *jpeg, unsigned long jpeg_size); > > > > > > void decompress(const unsigned char *jpeg, std::size size); > > > > > > > }; > > >
I did plan on, I had a few days PTO and I'm stuck in training at the moment, might not get around to until next week. Feel free to submit a v2 if you'd like. Is mise le meas/Regards, Eric Curtin On Thu, 14 Jul 2022 at 12:24, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Eric, > > I was wondering if you planned to submit a new version of this patch. > There's no big urgency, but if you expect to be too busy for the > foreseable future, I can send a v2 that addresses my review comments. > > On Fri, Jul 08, 2022 at 04:37:50PM +0300, Laurent Pinchart via libcamera-devel wrote: > > On Thu, Jul 07, 2022 at 11:25:58PM +0100, Eric Curtin wrote: > > > On Thu, 7 Jul 2022 at 21:13, Laurent Pinchart wrote: > > > > On Thu, Jul 07, 2022 at 04:53:13PM +0100, Eric Curtin wrote: > > > > > We were using the libjpeg functionality of SDL2_image only, instead just > > > > > use libjpeg directly to reduce our dependancy count, it is a more > > > > > commonly available library. > > > > > > > > > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > > > > > --- > > > > > README.rst | 2 +- > > > > > src/cam/meson.build | 8 +++---- > > > > > src/cam/sdl_sink.cpp | 4 ++-- > > > > > src/cam/sdl_texture.h | 2 +- > > > > > src/cam/sdl_texture_mjpg.cpp | 43 +++++++++++++++++++++++++++++++----- > > > > > src/cam/sdl_texture_mjpg.h | 5 +++++ > > > > > 6 files changed, 51 insertions(+), 13 deletions(-) > > > > > > > > > > diff --git a/README.rst b/README.rst > > > > > index b9e72d81..5e8bc1cc 100644 > > > > > --- a/README.rst > > > > > +++ b/README.rst > > > > > @@ -93,7 +93,7 @@ for cam: [optional] > > > > > > > > > > - libdrm-dev: Enables the KMS sink > > > > > - libsdl2-dev: Enables the SDL sink > > > > > - - libsdl2-image-dev: Supports MJPEG on the SDL sink > > > > > + - libjpeg-dev: Supports MJPEG on the SDL sink > > > > > > > > Could you please keep this list sorted ? > > > > > > > > > > > > > > for qcam: [optional] > > > > > qtbase5-dev libqt5core5a libqt5gui5 libqt5widgets5 qttools5-dev-tools libtiff-dev > > > > > diff --git a/src/cam/meson.build b/src/cam/meson.build > > > > > index 5957ce14..27cbd0de 100644 > > > > > --- a/src/cam/meson.build > > > > > +++ b/src/cam/meson.build > > > > > @@ -25,7 +25,7 @@ cam_cpp_args = [] > > > > > > > > > > libdrm = dependency('libdrm', required : false) > > > > > libsdl2 = dependency('SDL2', required : false) > > > > > -libsdl2_image = dependency('SDL2_image', required : false) > > > > > +libjpeg = dependency('libjpeg', required : false) > > > > > > > > Same here. > > > > > > > > > > > > > > if libdrm.found() > > > > > cam_cpp_args += [ '-DHAVE_KMS' ] > > > > > @@ -43,8 +43,8 @@ if libsdl2.found() > > > > > 'sdl_texture_yuyv.cpp' > > > > > ]) > > > > > > > > > > - if libsdl2_image.found() > > > > > - cam_cpp_args += ['-DHAVE_SDL_IMAGE'] > > > > > + if libjpeg.found() > > > > > + cam_cpp_args += ['-DHAVE_LIBJPEG'] > > > > > cam_sources += files([ > > > > > 'sdl_texture_mjpg.cpp' > > > > > ]) > > > > > @@ -58,7 +58,7 @@ cam = executable('cam', cam_sources, > > > > > libdrm, > > > > > libevent, > > > > > libsdl2, > > > > > - libsdl2_image, > > > > > + libjpeg, > > > > > > > > And here. > > > > > > > > > libyaml, > > > > > ], > > > > > cpp_args : cam_cpp_args, > > > > > diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp > > > > > index f8e3e95d..19fdfd6d 100644 > > > > > --- a/src/cam/sdl_sink.cpp > > > > > +++ b/src/cam/sdl_sink.cpp > > > > > @@ -21,7 +21,7 @@ > > > > > > > > > > #include "event_loop.h" > > > > > #include "image.h" > > > > > -#ifdef HAVE_SDL_IMAGE > > > > > +#ifdef HAVE_LIBJPEG > > > > > #include "sdl_texture_mjpg.h" > > > > > #endif > > > > > #include "sdl_texture_yuyv.h" > > > > > @@ -62,7 +62,7 @@ int SDLSink::configure(const libcamera::CameraConfiguration &config) > > > > > rect_.h = cfg.size.height; > > > > > > > > > > switch (cfg.pixelFormat) { > > > > > -#ifdef HAVE_SDL_IMAGE > > > > > +#ifdef HAVE_LIBJPEG > > > > > case libcamera::formats::MJPEG: > > > > > texture_ = std::make_unique<SDLTextureMJPG>(rect_); > > > > > break; > > > > > diff --git a/src/cam/sdl_texture.h b/src/cam/sdl_texture.h > > > > > index 90974798..42c906f2 100644 > > > > > --- a/src/cam/sdl_texture.h > > > > > +++ b/src/cam/sdl_texture.h > > > > > @@ -25,5 +25,5 @@ protected: > > > > > SDL_Texture *ptr_; > > > > > const SDL_Rect rect_; > > > > > const SDL_PixelFormatEnum pixelFormat_; > > > > > - const int pitch_; > > > > > + int pitch_; > > > > > }; > > > > > diff --git a/src/cam/sdl_texture_mjpg.cpp b/src/cam/sdl_texture_mjpg.cpp > > > > > index 69e99ad3..232af673 100644 > > > > > --- a/src/cam/sdl_texture_mjpg.cpp > > > > > +++ b/src/cam/sdl_texture_mjpg.cpp > > > > > @@ -7,19 +7,52 @@ > > > > > > > > > > #include "sdl_texture_mjpg.h" > > > > > > > > > > -#include <SDL2/SDL_image.h> > > > > > +#include <jpeglib.h> > > > > > > > > > > using namespace libcamera; > > > > > > > > > > SDLTextureMJPG::SDLTextureMJPG(const SDL_Rect &rect) > > > > > : SDLTexture(rect, SDL_PIXELFORMAT_RGB24, 0) > > > > > > > > You can write > > > > > > > > : SDLTexture(rect, SDL_PIXELFORMAT_RGB24, rect.w * 3) > > > > > > > > > { > > > > > + pitch_ = rect_.w * 3; > > > > > > > > and drop this line, and keep pitch_ const. > > > > > > > > > + rgbdata_ = (unsigned char *)malloc(pitch_ * rect_.h); > > > > > > > > Please use C++-style memory allocation > > > > > > > > rgbdata_ = new unsigned char[pitch_ * rect_.h]; > > > > > > > > The destructor should then use > > > > > > > > delete[] rgbdata_; > > > > > > > > Even better, I think you can turn rgbdata_ into a > > > > > > > > std::unique_ptr<unsigned char[]> rgbdata_; > > > > > > > > and drop the delete. > > > > > > > > Another option would have been to turn rgbdata_ into an > > > > std::vector<unsigned char> to also remove the need for a custom > > > > destructor. The drawback is that, unlike new that performs > > > > default-initialization, std::vector::resize() performs > > > > default-insertion, which uses value-initialization, effectively > > > > memsetting the vector to 0. There's a way around that by providing a > > > > custom allocator to the vector, but I think we're getting into the "not > > > > worth it" territory (at least as long as libcamera-base doesn't provide > > > > a helper for this). Let's thus use a unique_ptr. > > > > > > > > > +} > > > > > + > > > > > +SDLTextureMJPG::~SDLTextureMJPG() > > > > > +{ > > > > > + free(rgbdata_); > > > > > +} > > > > > + > > > > > +void SDLTextureMJPG::decompress(const unsigned char *jpeg, > > > > > + unsigned long jpeg_size) > > > > > +{ > > > > > + struct jpeg_error_mgr jerr; > > > > > + struct jpeg_decompress_struct cinfo; > > > > > + cinfo.err = jpeg_std_error(&jerr); > > > > > > > > There's a risk the JPEG data could be corrupted (that's quite common > > > > with UVC cameras). Error handling should be a bit more robust, you > > > > should create your own error handler, store that an error occurred, and > > > > return an error from this function and skip the SDL_UpdateTexture() in > > > > that case. > > > > > > > > Furthermore, jpg_std_error() will by default call exit() when a fatal > > > > error occurs. That's not good. The recommended way to deal with this is > > > > to use setjmp() and longjmp() to exit cleanly instead. > > > > > > > > > + > > > > > + jpeg_create_decompress(&cinfo); > > > > > + > > > > > + jpeg_mem_src(&cinfo, jpeg, jpeg_size); > > > > > + > > > > > + jpeg_read_header(&cinfo, TRUE); > > > > > + > > > > > + jpeg_start_decompress(&cinfo); > > > > > + > > > > > + int row_stride = cinfo.output_width * cinfo.output_components; > > > > > > > > rowStride (libcamera coding style) > > > > > > > > > + unsigned char *buffer = (unsigned char *)malloc(row_stride); > > > > > > > > Here too, std::unique_ptr and new[], or just > > > > > > > > unsigned char buffer[row_stride); > > > > > > For buffer, I need to pass the address of the variable storing the > > > pointer to jpeg_read_scanlines, not sure unique_ptr is possible in > > > this very specific case? (although I can easily change rgbdata_ to > > > unique_ptr, thanks for spotting that). > > > > > > "unsigned char buffer[row_stride];" won't work for the same reason, > > > needs to be a raw C pointer to pass &buffer to jpeg_read_scanlines. > > > > Good point. You could have a separate variable: > > > > unsigned char row[rowStride]; > > unsigned char *rows[1] = { &row }; > > > > Not sure if it's worth it. This makes me wonder, is there a performance > > benefit from reading multiple scanlines in one go ? > > > > > All the other feedback makes sense and will be addressed. > > > > > > > > + for (int i = 0; cinfo.output_scanline < cinfo.output_height; ++i) { > > > > > + jpeg_read_scanlines(&cinfo, &buffer, 1); > > > > > + memcpy(rgbdata_ + i * pitch_, buffer, row_stride); > > > > > + } > > > > > + > > > > > + free(buffer); > > > > > + jpeg_finish_decompress(&cinfo); > > > > > + > > > > > + jpeg_destroy_decompress(&cinfo); > > > > > } > > > > > > > > > > void SDLTextureMJPG::update(const Span<uint8_t> &data) > > > > > { > > > > > - SDL_RWops *bufferStream = SDL_RWFromMem(data.data(), data.size()); > > > > > - SDL_Surface *frame = IMG_Load_RW(bufferStream, 0); > > > > > - SDL_UpdateTexture(ptr_, nullptr, frame->pixels, frame->pitch); > > > > > - SDL_FreeSurface(frame); > > > > > + decompress(data.data(), data.size()); > > > > > + SDL_UpdateTexture(ptr_, nullptr, rgbdata_, pitch_); > > > > > } > > > > > diff --git a/src/cam/sdl_texture_mjpg.h b/src/cam/sdl_texture_mjpg.h > > > > > index b103f801..73c7fb68 100644 > > > > > --- a/src/cam/sdl_texture_mjpg.h > > > > > +++ b/src/cam/sdl_texture_mjpg.h > > > > > @@ -13,5 +13,10 @@ class SDLTextureMJPG : public SDLTexture > > > > > { > > > > > public: > > > > > SDLTextureMJPG(const SDL_Rect &rect); > > > > > + virtual ~SDLTextureMJPG(); > > > > > > > > No need for virtual here, and while at it, you can insert a blank line. > > > > > > > > > void update(const libcamera::Span<uint8_t> &data) override; > > > > > + > > > > > +private: > > > > > + unsigned char *rgbdata_ = 0; > > > > > > > > Member data after member functions. I'd name the member rgbData_ or just > > > > rgb_, up to you. > > > > > > > > > + void decompress(const unsigned char *jpeg, unsigned long jpeg_size); > > > > > > > > void decompress(const unsigned char *jpeg, std::size size); > > > > > > > > > }; > > > > > > -- > Regards, > > Laurent Pinchart >
Hi Eric, On Thu, Jul 14, 2022 at 01:05:39PM +0100, Eric Curtin wrote: > I did plan on, I had a few days PTO and I'm stuck in training at the > moment, might not get around to until next week. > > Feel free to submit a v2 if you'd like. There's no urgency, so I'm happy to let you get back to it when you'll have time. Please make sure to rebase first then, as we had to make a small change to the SDL sink to fix a compilation failure with SDL 2.0.9 or older. > On Thu, 14 Jul 2022 at 12:24, Laurent Pinchart wrote: > > > > Hi Eric, > > > > I was wondering if you planned to submit a new version of this patch. > > There's no big urgency, but if you expect to be too busy for the > > foreseable future, I can send a v2 that addresses my review comments. > > > > On Fri, Jul 08, 2022 at 04:37:50PM +0300, Laurent Pinchart via libcamera-devel wrote: > > > On Thu, Jul 07, 2022 at 11:25:58PM +0100, Eric Curtin wrote: > > > > On Thu, 7 Jul 2022 at 21:13, Laurent Pinchart wrote: > > > > > On Thu, Jul 07, 2022 at 04:53:13PM +0100, Eric Curtin wrote: > > > > > > We were using the libjpeg functionality of SDL2_image only, instead just > > > > > > use libjpeg directly to reduce our dependancy count, it is a more > > > > > > commonly available library. > > > > > > > > > > > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > > > > > > --- > > > > > > README.rst | 2 +- > > > > > > src/cam/meson.build | 8 +++---- > > > > > > src/cam/sdl_sink.cpp | 4 ++-- > > > > > > src/cam/sdl_texture.h | 2 +- > > > > > > src/cam/sdl_texture_mjpg.cpp | 43 +++++++++++++++++++++++++++++++----- > > > > > > src/cam/sdl_texture_mjpg.h | 5 +++++ > > > > > > 6 files changed, 51 insertions(+), 13 deletions(-) > > > > > > > > > > > > diff --git a/README.rst b/README.rst > > > > > > index b9e72d81..5e8bc1cc 100644 > > > > > > --- a/README.rst > > > > > > +++ b/README.rst > > > > > > @@ -93,7 +93,7 @@ for cam: [optional] > > > > > > > > > > > > - libdrm-dev: Enables the KMS sink > > > > > > - libsdl2-dev: Enables the SDL sink > > > > > > - - libsdl2-image-dev: Supports MJPEG on the SDL sink > > > > > > + - libjpeg-dev: Supports MJPEG on the SDL sink > > > > > > > > > > Could you please keep this list sorted ? > > > > > > > > > > > > > > > > > for qcam: [optional] > > > > > > qtbase5-dev libqt5core5a libqt5gui5 libqt5widgets5 qttools5-dev-tools libtiff-dev > > > > > > diff --git a/src/cam/meson.build b/src/cam/meson.build > > > > > > index 5957ce14..27cbd0de 100644 > > > > > > --- a/src/cam/meson.build > > > > > > +++ b/src/cam/meson.build > > > > > > @@ -25,7 +25,7 @@ cam_cpp_args = [] > > > > > > > > > > > > libdrm = dependency('libdrm', required : false) > > > > > > libsdl2 = dependency('SDL2', required : false) > > > > > > -libsdl2_image = dependency('SDL2_image', required : false) > > > > > > +libjpeg = dependency('libjpeg', required : false) > > > > > > > > > > Same here. > > > > > > > > > > > > > > > > > if libdrm.found() > > > > > > cam_cpp_args += [ '-DHAVE_KMS' ] > > > > > > @@ -43,8 +43,8 @@ if libsdl2.found() > > > > > > 'sdl_texture_yuyv.cpp' > > > > > > ]) > > > > > > > > > > > > - if libsdl2_image.found() > > > > > > - cam_cpp_args += ['-DHAVE_SDL_IMAGE'] > > > > > > + if libjpeg.found() > > > > > > + cam_cpp_args += ['-DHAVE_LIBJPEG'] > > > > > > cam_sources += files([ > > > > > > 'sdl_texture_mjpg.cpp' > > > > > > ]) > > > > > > @@ -58,7 +58,7 @@ cam = executable('cam', cam_sources, > > > > > > libdrm, > > > > > > libevent, > > > > > > libsdl2, > > > > > > - libsdl2_image, > > > > > > + libjpeg, > > > > > > > > > > And here. > > > > > > > > > > > libyaml, > > > > > > ], > > > > > > cpp_args : cam_cpp_args, > > > > > > diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp > > > > > > index f8e3e95d..19fdfd6d 100644 > > > > > > --- a/src/cam/sdl_sink.cpp > > > > > > +++ b/src/cam/sdl_sink.cpp > > > > > > @@ -21,7 +21,7 @@ > > > > > > > > > > > > #include "event_loop.h" > > > > > > #include "image.h" > > > > > > -#ifdef HAVE_SDL_IMAGE > > > > > > +#ifdef HAVE_LIBJPEG > > > > > > #include "sdl_texture_mjpg.h" > > > > > > #endif > > > > > > #include "sdl_texture_yuyv.h" > > > > > > @@ -62,7 +62,7 @@ int SDLSink::configure(const libcamera::CameraConfiguration &config) > > > > > > rect_.h = cfg.size.height; > > > > > > > > > > > > switch (cfg.pixelFormat) { > > > > > > -#ifdef HAVE_SDL_IMAGE > > > > > > +#ifdef HAVE_LIBJPEG > > > > > > case libcamera::formats::MJPEG: > > > > > > texture_ = std::make_unique<SDLTextureMJPG>(rect_); > > > > > > break; > > > > > > diff --git a/src/cam/sdl_texture.h b/src/cam/sdl_texture.h > > > > > > index 90974798..42c906f2 100644 > > > > > > --- a/src/cam/sdl_texture.h > > > > > > +++ b/src/cam/sdl_texture.h > > > > > > @@ -25,5 +25,5 @@ protected: > > > > > > SDL_Texture *ptr_; > > > > > > const SDL_Rect rect_; > > > > > > const SDL_PixelFormatEnum pixelFormat_; > > > > > > - const int pitch_; > > > > > > + int pitch_; > > > > > > }; > > > > > > diff --git a/src/cam/sdl_texture_mjpg.cpp b/src/cam/sdl_texture_mjpg.cpp > > > > > > index 69e99ad3..232af673 100644 > > > > > > --- a/src/cam/sdl_texture_mjpg.cpp > > > > > > +++ b/src/cam/sdl_texture_mjpg.cpp > > > > > > @@ -7,19 +7,52 @@ > > > > > > > > > > > > #include "sdl_texture_mjpg.h" > > > > > > > > > > > > -#include <SDL2/SDL_image.h> > > > > > > +#include <jpeglib.h> > > > > > > > > > > > > using namespace libcamera; > > > > > > > > > > > > SDLTextureMJPG::SDLTextureMJPG(const SDL_Rect &rect) > > > > > > : SDLTexture(rect, SDL_PIXELFORMAT_RGB24, 0) > > > > > > > > > > You can write > > > > > > > > > > : SDLTexture(rect, SDL_PIXELFORMAT_RGB24, rect.w * 3) > > > > > > > > > > > { > > > > > > + pitch_ = rect_.w * 3; > > > > > > > > > > and drop this line, and keep pitch_ const. > > > > > > > > > > > + rgbdata_ = (unsigned char *)malloc(pitch_ * rect_.h); > > > > > > > > > > Please use C++-style memory allocation > > > > > > > > > > rgbdata_ = new unsigned char[pitch_ * rect_.h]; > > > > > > > > > > The destructor should then use > > > > > > > > > > delete[] rgbdata_; > > > > > > > > > > Even better, I think you can turn rgbdata_ into a > > > > > > > > > > std::unique_ptr<unsigned char[]> rgbdata_; > > > > > > > > > > and drop the delete. > > > > > > > > > > Another option would have been to turn rgbdata_ into an > > > > > std::vector<unsigned char> to also remove the need for a custom > > > > > destructor. The drawback is that, unlike new that performs > > > > > default-initialization, std::vector::resize() performs > > > > > default-insertion, which uses value-initialization, effectively > > > > > memsetting the vector to 0. There's a way around that by providing a > > > > > custom allocator to the vector, but I think we're getting into the "not > > > > > worth it" territory (at least as long as libcamera-base doesn't provide > > > > > a helper for this). Let's thus use a unique_ptr. > > > > > > > > > > > +} > > > > > > + > > > > > > +SDLTextureMJPG::~SDLTextureMJPG() > > > > > > +{ > > > > > > + free(rgbdata_); > > > > > > +} > > > > > > + > > > > > > +void SDLTextureMJPG::decompress(const unsigned char *jpeg, > > > > > > + unsigned long jpeg_size) > > > > > > +{ > > > > > > + struct jpeg_error_mgr jerr; > > > > > > + struct jpeg_decompress_struct cinfo; > > > > > > + cinfo.err = jpeg_std_error(&jerr); > > > > > > > > > > There's a risk the JPEG data could be corrupted (that's quite common > > > > > with UVC cameras). Error handling should be a bit more robust, you > > > > > should create your own error handler, store that an error occurred, and > > > > > return an error from this function and skip the SDL_UpdateTexture() in > > > > > that case. > > > > > > > > > > Furthermore, jpg_std_error() will by default call exit() when a fatal > > > > > error occurs. That's not good. The recommended way to deal with this is > > > > > to use setjmp() and longjmp() to exit cleanly instead. > > > > > > > > > > > + > > > > > > + jpeg_create_decompress(&cinfo); > > > > > > + > > > > > > + jpeg_mem_src(&cinfo, jpeg, jpeg_size); > > > > > > + > > > > > > + jpeg_read_header(&cinfo, TRUE); > > > > > > + > > > > > > + jpeg_start_decompress(&cinfo); > > > > > > + > > > > > > + int row_stride = cinfo.output_width * cinfo.output_components; > > > > > > > > > > rowStride (libcamera coding style) > > > > > > > > > > > + unsigned char *buffer = (unsigned char *)malloc(row_stride); > > > > > > > > > > Here too, std::unique_ptr and new[], or just > > > > > > > > > > unsigned char buffer[row_stride); > > > > > > > > For buffer, I need to pass the address of the variable storing the > > > > pointer to jpeg_read_scanlines, not sure unique_ptr is possible in > > > > this very specific case? (although I can easily change rgbdata_ to > > > > unique_ptr, thanks for spotting that). > > > > > > > > "unsigned char buffer[row_stride];" won't work for the same reason, > > > > needs to be a raw C pointer to pass &buffer to jpeg_read_scanlines. > > > > > > Good point. You could have a separate variable: > > > > > > unsigned char row[rowStride]; > > > unsigned char *rows[1] = { &row }; > > > > > > Not sure if it's worth it. This makes me wonder, is there a performance > > > benefit from reading multiple scanlines in one go ? > > > > > > > All the other feedback makes sense and will be addressed. > > > > > > > > > > + for (int i = 0; cinfo.output_scanline < cinfo.output_height; ++i) { > > > > > > + jpeg_read_scanlines(&cinfo, &buffer, 1); > > > > > > + memcpy(rgbdata_ + i * pitch_, buffer, row_stride); > > > > > > + } > > > > > > + > > > > > > + free(buffer); > > > > > > + jpeg_finish_decompress(&cinfo); > > > > > > + > > > > > > + jpeg_destroy_decompress(&cinfo); > > > > > > } > > > > > > > > > > > > void SDLTextureMJPG::update(const Span<uint8_t> &data) > > > > > > { > > > > > > - SDL_RWops *bufferStream = SDL_RWFromMem(data.data(), data.size()); > > > > > > - SDL_Surface *frame = IMG_Load_RW(bufferStream, 0); > > > > > > - SDL_UpdateTexture(ptr_, nullptr, frame->pixels, frame->pitch); > > > > > > - SDL_FreeSurface(frame); > > > > > > + decompress(data.data(), data.size()); > > > > > > + SDL_UpdateTexture(ptr_, nullptr, rgbdata_, pitch_); > > > > > > } > > > > > > diff --git a/src/cam/sdl_texture_mjpg.h b/src/cam/sdl_texture_mjpg.h > > > > > > index b103f801..73c7fb68 100644 > > > > > > --- a/src/cam/sdl_texture_mjpg.h > > > > > > +++ b/src/cam/sdl_texture_mjpg.h > > > > > > @@ -13,5 +13,10 @@ class SDLTextureMJPG : public SDLTexture > > > > > > { > > > > > > public: > > > > > > SDLTextureMJPG(const SDL_Rect &rect); > > > > > > + virtual ~SDLTextureMJPG(); > > > > > > > > > > No need for virtual here, and while at it, you can insert a blank line. > > > > > > > > > > > void update(const libcamera::Span<uint8_t> &data) override; > > > > > > + > > > > > > +private: > > > > > > + unsigned char *rgbdata_ = 0; > > > > > > > > > > Member data after member functions. I'd name the member rgbData_ or just > > > > > rgb_, up to you. > > > > > > > > > > > + void decompress(const unsigned char *jpeg, unsigned long jpeg_size); > > > > > > > > > > void decompress(const unsigned char *jpeg, std::size size); > > > > > > > > > > > }; > > > > >
On Fri, 8 Jul 2022 at 14:38, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Eric, > > On Thu, Jul 07, 2022 at 11:25:58PM +0100, Eric Curtin wrote: > > On Thu, 7 Jul 2022 at 21:13, Laurent Pinchart wrote: > > > On Thu, Jul 07, 2022 at 04:53:13PM +0100, Eric Curtin wrote: > > > > We were using the libjpeg functionality of SDL2_image only, instead just > > > > use libjpeg directly to reduce our dependancy count, it is a more > > > > commonly available library. > > > > > > > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > > > > --- > > > > README.rst | 2 +- > > > > src/cam/meson.build | 8 +++---- > > > > src/cam/sdl_sink.cpp | 4 ++-- > > > > src/cam/sdl_texture.h | 2 +- > > > > src/cam/sdl_texture_mjpg.cpp | 43 +++++++++++++++++++++++++++++++----- > > > > src/cam/sdl_texture_mjpg.h | 5 +++++ > > > > 6 files changed, 51 insertions(+), 13 deletions(-) > > > > > > > > diff --git a/README.rst b/README.rst > > > > index b9e72d81..5e8bc1cc 100644 > > > > --- a/README.rst > > > > +++ b/README.rst > > > > @@ -93,7 +93,7 @@ for cam: [optional] > > > > > > > > - libdrm-dev: Enables the KMS sink > > > > - libsdl2-dev: Enables the SDL sink > > > > - - libsdl2-image-dev: Supports MJPEG on the SDL sink > > > > + - libjpeg-dev: Supports MJPEG on the SDL sink > > > > > > Could you please keep this list sorted ? > > > > > > > > > > > for qcam: [optional] > > > > qtbase5-dev libqt5core5a libqt5gui5 libqt5widgets5 qttools5-dev-tools libtiff-dev > > > > diff --git a/src/cam/meson.build b/src/cam/meson.build > > > > index 5957ce14..27cbd0de 100644 > > > > --- a/src/cam/meson.build > > > > +++ b/src/cam/meson.build > > > > @@ -25,7 +25,7 @@ cam_cpp_args = [] > > > > > > > > libdrm = dependency('libdrm', required : false) > > > > libsdl2 = dependency('SDL2', required : false) > > > > -libsdl2_image = dependency('SDL2_image', required : false) > > > > +libjpeg = dependency('libjpeg', required : false) > > > > > > Same here. > > > > > > > > > > > if libdrm.found() > > > > cam_cpp_args += [ '-DHAVE_KMS' ] > > > > @@ -43,8 +43,8 @@ if libsdl2.found() > > > > 'sdl_texture_yuyv.cpp' > > > > ]) > > > > > > > > - if libsdl2_image.found() > > > > - cam_cpp_args += ['-DHAVE_SDL_IMAGE'] > > > > + if libjpeg.found() > > > > + cam_cpp_args += ['-DHAVE_LIBJPEG'] > > > > cam_sources += files([ > > > > 'sdl_texture_mjpg.cpp' > > > > ]) > > > > @@ -58,7 +58,7 @@ cam = executable('cam', cam_sources, > > > > libdrm, > > > > libevent, > > > > libsdl2, > > > > - libsdl2_image, > > > > + libjpeg, > > > > > > And here. > > > > > > > libyaml, > > > > ], > > > > cpp_args : cam_cpp_args, > > > > diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp > > > > index f8e3e95d..19fdfd6d 100644 > > > > --- a/src/cam/sdl_sink.cpp > > > > +++ b/src/cam/sdl_sink.cpp > > > > @@ -21,7 +21,7 @@ > > > > > > > > #include "event_loop.h" > > > > #include "image.h" > > > > -#ifdef HAVE_SDL_IMAGE > > > > +#ifdef HAVE_LIBJPEG > > > > #include "sdl_texture_mjpg.h" > > > > #endif > > > > #include "sdl_texture_yuyv.h" > > > > @@ -62,7 +62,7 @@ int SDLSink::configure(const libcamera::CameraConfiguration &config) > > > > rect_.h = cfg.size.height; > > > > > > > > switch (cfg.pixelFormat) { > > > > -#ifdef HAVE_SDL_IMAGE > > > > +#ifdef HAVE_LIBJPEG > > > > case libcamera::formats::MJPEG: > > > > texture_ = std::make_unique<SDLTextureMJPG>(rect_); > > > > break; > > > > diff --git a/src/cam/sdl_texture.h b/src/cam/sdl_texture.h > > > > index 90974798..42c906f2 100644 > > > > --- a/src/cam/sdl_texture.h > > > > +++ b/src/cam/sdl_texture.h > > > > @@ -25,5 +25,5 @@ protected: > > > > SDL_Texture *ptr_; > > > > const SDL_Rect rect_; > > > > const SDL_PixelFormatEnum pixelFormat_; > > > > - const int pitch_; > > > > + int pitch_; > > > > }; > > > > diff --git a/src/cam/sdl_texture_mjpg.cpp b/src/cam/sdl_texture_mjpg.cpp > > > > index 69e99ad3..232af673 100644 > > > > --- a/src/cam/sdl_texture_mjpg.cpp > > > > +++ b/src/cam/sdl_texture_mjpg.cpp > > > > @@ -7,19 +7,52 @@ > > > > > > > > #include "sdl_texture_mjpg.h" > > > > > > > > -#include <SDL2/SDL_image.h> > > > > +#include <jpeglib.h> > > > > > > > > using namespace libcamera; > > > > > > > > SDLTextureMJPG::SDLTextureMJPG(const SDL_Rect &rect) > > > > : SDLTexture(rect, SDL_PIXELFORMAT_RGB24, 0) > > > > > > You can write > > > > > > : SDLTexture(rect, SDL_PIXELFORMAT_RGB24, rect.w * 3) > > > > > > > { > > > > + pitch_ = rect_.w * 3; > > > > > > and drop this line, and keep pitch_ const. > > > > > > > + rgbdata_ = (unsigned char *)malloc(pitch_ * rect_.h); > > > > > > Please use C++-style memory allocation > > > > > > rgbdata_ = new unsigned char[pitch_ * rect_.h]; > > > > > > The destructor should then use > > > > > > delete[] rgbdata_; > > > > > > Even better, I think you can turn rgbdata_ into a > > > > > > std::unique_ptr<unsigned char[]> rgbdata_; > > > > > > and drop the delete. > > > > > > Another option would have been to turn rgbdata_ into an > > > std::vector<unsigned char> to also remove the need for a custom > > > destructor. The drawback is that, unlike new that performs > > > default-initialization, std::vector::resize() performs > > > default-insertion, which uses value-initialization, effectively > > > memsetting the vector to 0. There's a way around that by providing a > > > custom allocator to the vector, but I think we're getting into the "not > > > worth it" territory (at least as long as libcamera-base doesn't provide > > > a helper for this). Let's thus use a unique_ptr. > > > > > > > +} > > > > + > > > > +SDLTextureMJPG::~SDLTextureMJPG() > > > > +{ > > > > + free(rgbdata_); > > > > +} > > > > + > > > > +void SDLTextureMJPG::decompress(const unsigned char *jpeg, > > > > + unsigned long jpeg_size) > > > > +{ > > > > + struct jpeg_error_mgr jerr; > > > > + struct jpeg_decompress_struct cinfo; > > > > + cinfo.err = jpeg_std_error(&jerr); > > > > > > There's a risk the JPEG data could be corrupted (that's quite common > > > with UVC cameras). Error handling should be a bit more robust, you > > > should create your own error handler, store that an error occurred, and > > > return an error from this function and skip the SDL_UpdateTexture() in > > > that case. > > > > > > Furthermore, jpg_std_error() will by default call exit() when a fatal > > > error occurs. That's not good. The recommended way to deal with this is > > > to use setjmp() and longjmp() to exit cleanly instead. > > > > > > > + > > > > + jpeg_create_decompress(&cinfo); > > > > + > > > > + jpeg_mem_src(&cinfo, jpeg, jpeg_size); > > > > + > > > > + jpeg_read_header(&cinfo, TRUE); > > > > + > > > > + jpeg_start_decompress(&cinfo); > > > > + > > > > + int row_stride = cinfo.output_width * cinfo.output_components; > > > > > > rowStride (libcamera coding style) > > > > > > > + unsigned char *buffer = (unsigned char *)malloc(row_stride); > > > > > > Here too, std::unique_ptr and new[], or just > > > > > > unsigned char buffer[row_stride); > > > > For buffer, I need to pass the address of the variable storing the > > pointer to jpeg_read_scanlines, not sure unique_ptr is possible in > > this very specific case? (although I can easily change rgbdata_ to > > unique_ptr, thanks for spotting that). > > > > "unsigned char buffer[row_stride];" won't work for the same reason, > > needs to be a raw C pointer to pass &buffer to jpeg_read_scanlines. > > Good point. You could have a separate variable: > > unsigned char row[rowStride]; > unsigned char *rows[1] = { &row }; > > Not sure if it's worth it. This makes me wonder, is there a performance > benefit from reading multiple scanlines in one go ? For the record I tried to read every scanline in one go and the decompression failed, then I left it as is, one scanline at a time seems to be the most common approach, it's simple and seems to have decent performance. > > > All the other feedback makes sense and will be addressed. > > > > > > + for (int i = 0; cinfo.output_scanline < cinfo.output_height; ++i) { > > > > + jpeg_read_scanlines(&cinfo, &buffer, 1); > > > > + memcpy(rgbdata_ + i * pitch_, buffer, row_stride); > > > > + } > > > > + > > > > + free(buffer); > > > > + jpeg_finish_decompress(&cinfo); > > > > + > > > > + jpeg_destroy_decompress(&cinfo); > > > > } > > > > > > > > void SDLTextureMJPG::update(const Span<uint8_t> &data) > > > > { > > > > - SDL_RWops *bufferStream = SDL_RWFromMem(data.data(), data.size()); > > > > - SDL_Surface *frame = IMG_Load_RW(bufferStream, 0); > > > > - SDL_UpdateTexture(ptr_, nullptr, frame->pixels, frame->pitch); > > > > - SDL_FreeSurface(frame); > > > > + decompress(data.data(), data.size()); > > > > + SDL_UpdateTexture(ptr_, nullptr, rgbdata_, pitch_); > > > > } > > > > diff --git a/src/cam/sdl_texture_mjpg.h b/src/cam/sdl_texture_mjpg.h > > > > index b103f801..73c7fb68 100644 > > > > --- a/src/cam/sdl_texture_mjpg.h > > > > +++ b/src/cam/sdl_texture_mjpg.h > > > > @@ -13,5 +13,10 @@ class SDLTextureMJPG : public SDLTexture > > > > { > > > > public: > > > > SDLTextureMJPG(const SDL_Rect &rect); > > > > + virtual ~SDLTextureMJPG(); > > > > > > No need for virtual here, and while at it, you can insert a blank line. > > > > > > > void update(const libcamera::Span<uint8_t> &data) override; > > > > + > > > > +private: > > > > + unsigned char *rgbdata_ = 0; > > > > > > Member data after member functions. I'd name the member rgbData_ or just > > > rgb_, up to you. > > > > > > > + void decompress(const unsigned char *jpeg, unsigned long jpeg_size); > > > > > > void decompress(const unsigned char *jpeg, std::size size); > > > > > > > }; > > > > > -- > Regards, > > Laurent Pinchart >
diff --git a/README.rst b/README.rst index b9e72d81..5e8bc1cc 100644 --- a/README.rst +++ b/README.rst @@ -93,7 +93,7 @@ for cam: [optional] - libdrm-dev: Enables the KMS sink - libsdl2-dev: Enables the SDL sink - - libsdl2-image-dev: Supports MJPEG on the SDL sink + - libjpeg-dev: Supports MJPEG on the SDL sink for qcam: [optional] qtbase5-dev libqt5core5a libqt5gui5 libqt5widgets5 qttools5-dev-tools libtiff-dev diff --git a/src/cam/meson.build b/src/cam/meson.build index 5957ce14..27cbd0de 100644 --- a/src/cam/meson.build +++ b/src/cam/meson.build @@ -25,7 +25,7 @@ cam_cpp_args = [] libdrm = dependency('libdrm', required : false) libsdl2 = dependency('SDL2', required : false) -libsdl2_image = dependency('SDL2_image', required : false) +libjpeg = dependency('libjpeg', required : false) if libdrm.found() cam_cpp_args += [ '-DHAVE_KMS' ] @@ -43,8 +43,8 @@ if libsdl2.found() 'sdl_texture_yuyv.cpp' ]) - if libsdl2_image.found() - cam_cpp_args += ['-DHAVE_SDL_IMAGE'] + if libjpeg.found() + cam_cpp_args += ['-DHAVE_LIBJPEG'] cam_sources += files([ 'sdl_texture_mjpg.cpp' ]) @@ -58,7 +58,7 @@ cam = executable('cam', cam_sources, libdrm, libevent, libsdl2, - libsdl2_image, + libjpeg, libyaml, ], cpp_args : cam_cpp_args, diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp index f8e3e95d..19fdfd6d 100644 --- a/src/cam/sdl_sink.cpp +++ b/src/cam/sdl_sink.cpp @@ -21,7 +21,7 @@ #include "event_loop.h" #include "image.h" -#ifdef HAVE_SDL_IMAGE +#ifdef HAVE_LIBJPEG #include "sdl_texture_mjpg.h" #endif #include "sdl_texture_yuyv.h" @@ -62,7 +62,7 @@ int SDLSink::configure(const libcamera::CameraConfiguration &config) rect_.h = cfg.size.height; switch (cfg.pixelFormat) { -#ifdef HAVE_SDL_IMAGE +#ifdef HAVE_LIBJPEG case libcamera::formats::MJPEG: texture_ = std::make_unique<SDLTextureMJPG>(rect_); break; diff --git a/src/cam/sdl_texture.h b/src/cam/sdl_texture.h index 90974798..42c906f2 100644 --- a/src/cam/sdl_texture.h +++ b/src/cam/sdl_texture.h @@ -25,5 +25,5 @@ protected: SDL_Texture *ptr_; const SDL_Rect rect_; const SDL_PixelFormatEnum pixelFormat_; - const int pitch_; + int pitch_; }; diff --git a/src/cam/sdl_texture_mjpg.cpp b/src/cam/sdl_texture_mjpg.cpp index 69e99ad3..232af673 100644 --- a/src/cam/sdl_texture_mjpg.cpp +++ b/src/cam/sdl_texture_mjpg.cpp @@ -7,19 +7,52 @@ #include "sdl_texture_mjpg.h" -#include <SDL2/SDL_image.h> +#include <jpeglib.h> using namespace libcamera; SDLTextureMJPG::SDLTextureMJPG(const SDL_Rect &rect) : SDLTexture(rect, SDL_PIXELFORMAT_RGB24, 0) { + pitch_ = rect_.w * 3; + rgbdata_ = (unsigned char *)malloc(pitch_ * rect_.h); +} + +SDLTextureMJPG::~SDLTextureMJPG() +{ + free(rgbdata_); +} + +void SDLTextureMJPG::decompress(const unsigned char *jpeg, + unsigned long jpeg_size) +{ + struct jpeg_error_mgr jerr; + struct jpeg_decompress_struct cinfo; + cinfo.err = jpeg_std_error(&jerr); + + jpeg_create_decompress(&cinfo); + + jpeg_mem_src(&cinfo, jpeg, jpeg_size); + + jpeg_read_header(&cinfo, TRUE); + + jpeg_start_decompress(&cinfo); + + int row_stride = cinfo.output_width * cinfo.output_components; + unsigned char *buffer = (unsigned char *)malloc(row_stride); + for (int i = 0; cinfo.output_scanline < cinfo.output_height; ++i) { + jpeg_read_scanlines(&cinfo, &buffer, 1); + memcpy(rgbdata_ + i * pitch_, buffer, row_stride); + } + + free(buffer); + jpeg_finish_decompress(&cinfo); + + jpeg_destroy_decompress(&cinfo); } void SDLTextureMJPG::update(const Span<uint8_t> &data) { - SDL_RWops *bufferStream = SDL_RWFromMem(data.data(), data.size()); - SDL_Surface *frame = IMG_Load_RW(bufferStream, 0); - SDL_UpdateTexture(ptr_, nullptr, frame->pixels, frame->pitch); - SDL_FreeSurface(frame); + decompress(data.data(), data.size()); + SDL_UpdateTexture(ptr_, nullptr, rgbdata_, pitch_); } diff --git a/src/cam/sdl_texture_mjpg.h b/src/cam/sdl_texture_mjpg.h index b103f801..73c7fb68 100644 --- a/src/cam/sdl_texture_mjpg.h +++ b/src/cam/sdl_texture_mjpg.h @@ -13,5 +13,10 @@ class SDLTextureMJPG : public SDLTexture { public: SDLTextureMJPG(const SDL_Rect &rect); + virtual ~SDLTextureMJPG(); void update(const libcamera::Span<uint8_t> &data) override; + +private: + unsigned char *rgbdata_ = 0; + void decompress(const unsigned char *jpeg, unsigned long jpeg_size); };
We were using the libjpeg functionality of SDL2_image only, instead just use libjpeg directly to reduce our dependancy count, it is a more commonly available library. Signed-off-by: Eric Curtin <ecurtin@redhat.com> --- README.rst | 2 +- src/cam/meson.build | 8 +++---- src/cam/sdl_sink.cpp | 4 ++-- src/cam/sdl_texture.h | 2 +- src/cam/sdl_texture_mjpg.cpp | 43 +++++++++++++++++++++++++++++++----- src/cam/sdl_texture_mjpg.h | 5 +++++ 6 files changed, 51 insertions(+), 13 deletions(-)