| Message ID | 20210906134004.23485-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On Mon, Sep 6, 2021 at 10:40 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > The JPEG post-processor uses MappedFrameBuffer to access pixel data, but > only uses data from the first plane. Pass the vector of planes to the > encode() function to correctly handle multi-planar formats (currently > limited to NV12). > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> I am not against this patch. But is this necessary in this series? I have done this in https://patchwork.libcamera.org/project/libcamera/list/?series=2423. -Hiro > --- > src/android/jpeg/encoder_libjpeg.cpp | 18 +++++++++--------- > src/android/jpeg/encoder_libjpeg.h | 8 +++++--- > src/android/jpeg/post_processor_jpeg.cpp | 2 +- > src/android/jpeg/thumbnailer.cpp | 4 ++-- > 4 files changed, 17 insertions(+), 15 deletions(-) > > diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp > index b8b01e20a001..7f63f36e793e 100644 > --- a/src/android/jpeg/encoder_libjpeg.cpp > +++ b/src/android/jpeg/encoder_libjpeg.cpp > @@ -103,9 +103,9 @@ int EncoderLibJpeg::configure(const StreamConfiguration &cfg) > return 0; > } > > -void EncoderLibJpeg::compressRGB(Span<const uint8_t> frame) > +void EncoderLibJpeg::compressRGB(const std::vector<Span<uint8_t>> &planes) > { > - unsigned char *src = const_cast<unsigned char *>(frame.data()); > + unsigned char *src = const_cast<unsigned char *>(planes[0].data()); > /* \todo Stride information should come from buffer configuration. */ > unsigned int stride = pixelFormatInfo_->stride(compress_.image_width, 0); > > @@ -121,7 +121,7 @@ void EncoderLibJpeg::compressRGB(Span<const uint8_t> frame) > * Compress the incoming buffer from a supported NV format. > * This naively unpacks the semi-planar NV12 to a YUV888 format for libjpeg. > */ > -void EncoderLibJpeg::compressNV(Span<const uint8_t> frame) > +void EncoderLibJpeg::compressNV(const std::vector<Span<uint8_t>> &planes) > { > uint8_t tmprowbuf[compress_.image_width * 3]; > > @@ -133,7 +133,6 @@ void EncoderLibJpeg::compressNV(Span<const uint8_t> frame) > * Possible hints at: > * https://sourceforge.net/p/libjpeg/mailman/message/30815123/ > */ > - unsigned int y_stride = pixelFormatInfo_->stride(compress_.image_width, 0); > unsigned int c_stride = pixelFormatInfo_->stride(compress_.image_width, 1); > > unsigned int horzSubSample = 2 * compress_.image_width / c_stride; > @@ -143,8 +142,8 @@ void EncoderLibJpeg::compressNV(Span<const uint8_t> frame) > unsigned int cb_pos = nvSwap_ ? 1 : 0; > unsigned int cr_pos = nvSwap_ ? 0 : 1; > > - const unsigned char *src = frame.data(); > - const unsigned char *src_c = src + y_stride * compress_.image_height; > + const unsigned char *src = planes[0].data(); > + const unsigned char *src_c = planes[1].data(); > > JSAMPROW row_pointer[1]; > row_pointer[0] = &tmprowbuf[0]; > @@ -188,11 +187,12 @@ int EncoderLibJpeg::encode(const FrameBuffer &source, Span<uint8_t> dest, > return frame.error(); > } > > - return encode(frame.planes()[0], dest, exifData, quality); > + return encode(frame.planes(), dest, exifData, quality); > } > > -int EncoderLibJpeg::encode(Span<const uint8_t> src, Span<uint8_t> dest, > - Span<const uint8_t> exifData, unsigned int quality) > +int EncoderLibJpeg::encode(const std::vector<Span<uint8_t>> &src, > + Span<uint8_t> dest, Span<const uint8_t> exifData, > + unsigned int quality) > { > unsigned char *destination = dest.data(); > unsigned long size = dest.size(); > diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h > index 61fbd1a69278..45ffbd7fae5d 100644 > --- a/src/android/jpeg/encoder_libjpeg.h > +++ b/src/android/jpeg/encoder_libjpeg.h > @@ -9,6 +9,8 @@ > > #include "encoder.h" > > +#include <vector> > + > #include "libcamera/internal/formats.h" > > #include <jpeglib.h> > @@ -24,14 +26,14 @@ public: > libcamera::Span<uint8_t> destination, > libcamera::Span<const uint8_t> exifData, > unsigned int quality) override; > - int encode(libcamera::Span<const uint8_t> source, > + int encode(const std::vector<libcamera::Span<uint8_t>> &planes, > libcamera::Span<uint8_t> destination, > libcamera::Span<const uint8_t> exifData, > unsigned int quality); > > private: > - void compressRGB(libcamera::Span<const uint8_t> frame); > - void compressNV(libcamera::Span<const uint8_t> frame); > + void compressRGB(const std::vector<libcamera::Span<uint8_t>> &planes); > + void compressNV(const std::vector<libcamera::Span<uint8_t>> &planes); > > struct jpeg_compress_struct compress_; > struct jpeg_error_mgr jerr_; > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp > index 3160a784419c..68d74842925d 100644 > --- a/src/android/jpeg/post_processor_jpeg.cpp > +++ b/src/android/jpeg/post_processor_jpeg.cpp > @@ -72,7 +72,7 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source, > */ > thumbnail->resize(rawThumbnail.size()); > > - int jpeg_size = thumbnailEncoder_.encode(rawThumbnail, > + int jpeg_size = thumbnailEncoder_.encode({ rawThumbnail }, > *thumbnail, {}, quality); > thumbnail->resize(jpeg_size); > > diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp > index 043c7b33f06a..2c341535de9e 100644 > --- a/src/android/jpeg/thumbnailer.cpp > +++ b/src/android/jpeg/thumbnailer.cpp > @@ -62,8 +62,8 @@ void Thumbnailer::createThumbnail(const FrameBuffer &source, > ASSERT(tw % 2 == 0 && th % 2 == 0); > > /* Image scaling block implementing nearest-neighbour algorithm. */ > - unsigned char *src = static_cast<unsigned char *>(frame.planes()[0].data()); > - unsigned char *srcC = src + sh * sw; > + unsigned char *src = frame.planes()[0].data(); > + unsigned char *srcC = frame.planes()[1].data(); > unsigned char *srcCb, *srcCr; > unsigned char *dstY, *srcY; > > -- > Regards, > > Laurent Pinchart >
Hi Laurent, On 9/6/21 7:10 PM, Laurent Pinchart wrote: > The JPEG post-processor uses MappedFrameBuffer to access pixel data, but > only uses data from the first plane. Pass the vector of planes to the > encode() function to correctly handle multi-planar formats (currently > limited to NV12). > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> The diff looks good to me but I haven't the entire series yet. Also, this fixes the segfault observed with the series while trying to click a picture so, Acked-by: Umang Jain <umang.jain@ideasonboard.com> Tested-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/android/jpeg/encoder_libjpeg.cpp | 18 +++++++++--------- > src/android/jpeg/encoder_libjpeg.h | 8 +++++--- > src/android/jpeg/post_processor_jpeg.cpp | 2 +- > src/android/jpeg/thumbnailer.cpp | 4 ++-- > 4 files changed, 17 insertions(+), 15 deletions(-) > > diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp > index b8b01e20a001..7f63f36e793e 100644 > --- a/src/android/jpeg/encoder_libjpeg.cpp > +++ b/src/android/jpeg/encoder_libjpeg.cpp > @@ -103,9 +103,9 @@ int EncoderLibJpeg::configure(const StreamConfiguration &cfg) > return 0; > } > > -void EncoderLibJpeg::compressRGB(Span<const uint8_t> frame) > +void EncoderLibJpeg::compressRGB(const std::vector<Span<uint8_t>> &planes) > { > - unsigned char *src = const_cast<unsigned char *>(frame.data()); > + unsigned char *src = const_cast<unsigned char *>(planes[0].data()); > /* \todo Stride information should come from buffer configuration. */ > unsigned int stride = pixelFormatInfo_->stride(compress_.image_width, 0); > > @@ -121,7 +121,7 @@ void EncoderLibJpeg::compressRGB(Span<const uint8_t> frame) > * Compress the incoming buffer from a supported NV format. > * This naively unpacks the semi-planar NV12 to a YUV888 format for libjpeg. > */ > -void EncoderLibJpeg::compressNV(Span<const uint8_t> frame) > +void EncoderLibJpeg::compressNV(const std::vector<Span<uint8_t>> &planes) > { > uint8_t tmprowbuf[compress_.image_width * 3]; > > @@ -133,7 +133,6 @@ void EncoderLibJpeg::compressNV(Span<const uint8_t> frame) > * Possible hints at: > * https://sourceforge.net/p/libjpeg/mailman/message/30815123/ > */ > - unsigned int y_stride = pixelFormatInfo_->stride(compress_.image_width, 0); > unsigned int c_stride = pixelFormatInfo_->stride(compress_.image_width, 1); > > unsigned int horzSubSample = 2 * compress_.image_width / c_stride; > @@ -143,8 +142,8 @@ void EncoderLibJpeg::compressNV(Span<const uint8_t> frame) > unsigned int cb_pos = nvSwap_ ? 1 : 0; > unsigned int cr_pos = nvSwap_ ? 0 : 1; > > - const unsigned char *src = frame.data(); > - const unsigned char *src_c = src + y_stride * compress_.image_height; > + const unsigned char *src = planes[0].data(); > + const unsigned char *src_c = planes[1].data(); > > JSAMPROW row_pointer[1]; > row_pointer[0] = &tmprowbuf[0]; > @@ -188,11 +187,12 @@ int EncoderLibJpeg::encode(const FrameBuffer &source, Span<uint8_t> dest, > return frame.error(); > } > > - return encode(frame.planes()[0], dest, exifData, quality); > + return encode(frame.planes(), dest, exifData, quality); > } > > -int EncoderLibJpeg::encode(Span<const uint8_t> src, Span<uint8_t> dest, > - Span<const uint8_t> exifData, unsigned int quality) > +int EncoderLibJpeg::encode(const std::vector<Span<uint8_t>> &src, > + Span<uint8_t> dest, Span<const uint8_t> exifData, > + unsigned int quality) > { > unsigned char *destination = dest.data(); > unsigned long size = dest.size(); > diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h > index 61fbd1a69278..45ffbd7fae5d 100644 > --- a/src/android/jpeg/encoder_libjpeg.h > +++ b/src/android/jpeg/encoder_libjpeg.h > @@ -9,6 +9,8 @@ > > #include "encoder.h" > > +#include <vector> > + > #include "libcamera/internal/formats.h" > > #include <jpeglib.h> > @@ -24,14 +26,14 @@ public: > libcamera::Span<uint8_t> destination, > libcamera::Span<const uint8_t> exifData, > unsigned int quality) override; > - int encode(libcamera::Span<const uint8_t> source, > + int encode(const std::vector<libcamera::Span<uint8_t>> &planes, > libcamera::Span<uint8_t> destination, > libcamera::Span<const uint8_t> exifData, > unsigned int quality); > > private: > - void compressRGB(libcamera::Span<const uint8_t> frame); > - void compressNV(libcamera::Span<const uint8_t> frame); > + void compressRGB(const std::vector<libcamera::Span<uint8_t>> &planes); > + void compressNV(const std::vector<libcamera::Span<uint8_t>> &planes); > > struct jpeg_compress_struct compress_; > struct jpeg_error_mgr jerr_; > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp > index 3160a784419c..68d74842925d 100644 > --- a/src/android/jpeg/post_processor_jpeg.cpp > +++ b/src/android/jpeg/post_processor_jpeg.cpp > @@ -72,7 +72,7 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source, > */ > thumbnail->resize(rawThumbnail.size()); > > - int jpeg_size = thumbnailEncoder_.encode(rawThumbnail, > + int jpeg_size = thumbnailEncoder_.encode({ rawThumbnail }, > *thumbnail, {}, quality); > thumbnail->resize(jpeg_size); > > diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp > index 043c7b33f06a..2c341535de9e 100644 > --- a/src/android/jpeg/thumbnailer.cpp > +++ b/src/android/jpeg/thumbnailer.cpp > @@ -62,8 +62,8 @@ void Thumbnailer::createThumbnail(const FrameBuffer &source, > ASSERT(tw % 2 == 0 && th % 2 == 0); > > /* Image scaling block implementing nearest-neighbour algorithm. */ > - unsigned char *src = static_cast<unsigned char *>(frame.planes()[0].data()); > - unsigned char *srcC = src + sh * sw; > + unsigned char *src = frame.planes()[0].data(); > + unsigned char *srcC = frame.planes()[1].data(); > unsigned char *srcCb, *srcCr; > unsigned char *dstY, *srcY; >
Hi Hiro, On Mon, Sep 06, 2021 at 10:56:26PM +0900, Hirokazu Honda wrote: > On Mon, Sep 6, 2021 at 10:40 PM Laurent Pinchart wrote: > > > > The JPEG post-processor uses MappedFrameBuffer to access pixel data, but > > only uses data from the first plane. Pass the vector of planes to the > > encode() function to correctly handle multi-planar formats (currently > > limited to NV12). > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > I am not against this patch. > But is this necessary in this series? Without it the JPEG encoder crashes when capturing a still image, so I'd consider it as necessary :-) > I have done this in > https://patchwork.libcamera.org/project/libcamera/list/?series=2423. I've given this more thoughts over the weekend, and I think we need to introduce an Image class. It won't be trivial, there are quite a few issues to handle to make it suitable for the libcamera public API. Once this series gets merged I'll provide feedback on that. > > --- > > src/android/jpeg/encoder_libjpeg.cpp | 18 +++++++++--------- > > src/android/jpeg/encoder_libjpeg.h | 8 +++++--- > > src/android/jpeg/post_processor_jpeg.cpp | 2 +- > > src/android/jpeg/thumbnailer.cpp | 4 ++-- > > 4 files changed, 17 insertions(+), 15 deletions(-) > > > > diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp > > index b8b01e20a001..7f63f36e793e 100644 > > --- a/src/android/jpeg/encoder_libjpeg.cpp > > +++ b/src/android/jpeg/encoder_libjpeg.cpp > > @@ -103,9 +103,9 @@ int EncoderLibJpeg::configure(const StreamConfiguration &cfg) > > return 0; > > } > > > > -void EncoderLibJpeg::compressRGB(Span<const uint8_t> frame) > > +void EncoderLibJpeg::compressRGB(const std::vector<Span<uint8_t>> &planes) > > { > > - unsigned char *src = const_cast<unsigned char *>(frame.data()); > > + unsigned char *src = const_cast<unsigned char *>(planes[0].data()); > > /* \todo Stride information should come from buffer configuration. */ > > unsigned int stride = pixelFormatInfo_->stride(compress_.image_width, 0); > > > > @@ -121,7 +121,7 @@ void EncoderLibJpeg::compressRGB(Span<const uint8_t> frame) > > * Compress the incoming buffer from a supported NV format. > > * This naively unpacks the semi-planar NV12 to a YUV888 format for libjpeg. > > */ > > -void EncoderLibJpeg::compressNV(Span<const uint8_t> frame) > > +void EncoderLibJpeg::compressNV(const std::vector<Span<uint8_t>> &planes) > > { > > uint8_t tmprowbuf[compress_.image_width * 3]; > > > > @@ -133,7 +133,6 @@ void EncoderLibJpeg::compressNV(Span<const uint8_t> frame) > > * Possible hints at: > > * https://sourceforge.net/p/libjpeg/mailman/message/30815123/ > > */ > > - unsigned int y_stride = pixelFormatInfo_->stride(compress_.image_width, 0); > > unsigned int c_stride = pixelFormatInfo_->stride(compress_.image_width, 1); > > > > unsigned int horzSubSample = 2 * compress_.image_width / c_stride; > > @@ -143,8 +142,8 @@ void EncoderLibJpeg::compressNV(Span<const uint8_t> frame) > > unsigned int cb_pos = nvSwap_ ? 1 : 0; > > unsigned int cr_pos = nvSwap_ ? 0 : 1; > > > > - const unsigned char *src = frame.data(); > > - const unsigned char *src_c = src + y_stride * compress_.image_height; > > + const unsigned char *src = planes[0].data(); > > + const unsigned char *src_c = planes[1].data(); > > > > JSAMPROW row_pointer[1]; > > row_pointer[0] = &tmprowbuf[0]; > > @@ -188,11 +187,12 @@ int EncoderLibJpeg::encode(const FrameBuffer &source, Span<uint8_t> dest, > > return frame.error(); > > } > > > > - return encode(frame.planes()[0], dest, exifData, quality); > > + return encode(frame.planes(), dest, exifData, quality); > > } > > > > -int EncoderLibJpeg::encode(Span<const uint8_t> src, Span<uint8_t> dest, > > - Span<const uint8_t> exifData, unsigned int quality) > > +int EncoderLibJpeg::encode(const std::vector<Span<uint8_t>> &src, > > + Span<uint8_t> dest, Span<const uint8_t> exifData, > > + unsigned int quality) > > { > > unsigned char *destination = dest.data(); > > unsigned long size = dest.size(); > > diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h > > index 61fbd1a69278..45ffbd7fae5d 100644 > > --- a/src/android/jpeg/encoder_libjpeg.h > > +++ b/src/android/jpeg/encoder_libjpeg.h > > @@ -9,6 +9,8 @@ > > > > #include "encoder.h" > > > > +#include <vector> > > + > > #include "libcamera/internal/formats.h" > > > > #include <jpeglib.h> > > @@ -24,14 +26,14 @@ public: > > libcamera::Span<uint8_t> destination, > > libcamera::Span<const uint8_t> exifData, > > unsigned int quality) override; > > - int encode(libcamera::Span<const uint8_t> source, > > + int encode(const std::vector<libcamera::Span<uint8_t>> &planes, > > libcamera::Span<uint8_t> destination, > > libcamera::Span<const uint8_t> exifData, > > unsigned int quality); > > > > private: > > - void compressRGB(libcamera::Span<const uint8_t> frame); > > - void compressNV(libcamera::Span<const uint8_t> frame); > > + void compressRGB(const std::vector<libcamera::Span<uint8_t>> &planes); > > + void compressNV(const std::vector<libcamera::Span<uint8_t>> &planes); > > > > struct jpeg_compress_struct compress_; > > struct jpeg_error_mgr jerr_; > > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp > > index 3160a784419c..68d74842925d 100644 > > --- a/src/android/jpeg/post_processor_jpeg.cpp > > +++ b/src/android/jpeg/post_processor_jpeg.cpp > > @@ -72,7 +72,7 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source, > > */ > > thumbnail->resize(rawThumbnail.size()); > > > > - int jpeg_size = thumbnailEncoder_.encode(rawThumbnail, > > + int jpeg_size = thumbnailEncoder_.encode({ rawThumbnail }, > > *thumbnail, {}, quality); > > thumbnail->resize(jpeg_size); > > > > diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp > > index 043c7b33f06a..2c341535de9e 100644 > > --- a/src/android/jpeg/thumbnailer.cpp > > +++ b/src/android/jpeg/thumbnailer.cpp > > @@ -62,8 +62,8 @@ void Thumbnailer::createThumbnail(const FrameBuffer &source, > > ASSERT(tw % 2 == 0 && th % 2 == 0); > > > > /* Image scaling block implementing nearest-neighbour algorithm. */ > > - unsigned char *src = static_cast<unsigned char *>(frame.planes()[0].data()); > > - unsigned char *srcC = src + sh * sw; > > + unsigned char *src = frame.planes()[0].data(); > > + unsigned char *srcC = frame.planes()[1].data(); > > unsigned char *srcCb, *srcCr; > > unsigned char *dstY, *srcY; > >
Hi Laurent, On Mon, Sep 6, 2021 at 11:08 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Hiro, > > On Mon, Sep 06, 2021 at 10:56:26PM +0900, Hirokazu Honda wrote: > > On Mon, Sep 6, 2021 at 10:40 PM Laurent Pinchart wrote: > > > > > > The JPEG post-processor uses MappedFrameBuffer to access pixel data, but > > > only uses data from the first plane. Pass the vector of planes to the > > > encode() function to correctly handle multi-planar formats (currently > > > limited to NV12). > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > I am not against this patch. > > But is this necessary in this series? > > Without it the JPEG encoder crashes when capturing a still image, so I'd > consider it as necessary :-) Got it. > > > I have done this in > > https://patchwork.libcamera.org/project/libcamera/list/?series=2423. > > I've given this more thoughts over the weekend, and I think we need to > introduce an Image class. It won't be trivial, there are quite a few > issues to handle to make it suitable for the libcamera public API. Once > this series gets merged I'll provide feedback on that. > Look forward to it! > > > --- > > > src/android/jpeg/encoder_libjpeg.cpp | 18 +++++++++--------- > > > src/android/jpeg/encoder_libjpeg.h | 8 +++++--- > > > src/android/jpeg/post_processor_jpeg.cpp | 2 +- > > > src/android/jpeg/thumbnailer.cpp | 4 ++-- > > > 4 files changed, 17 insertions(+), 15 deletions(-) > > > > > > diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp > > > index b8b01e20a001..7f63f36e793e 100644 > > > --- a/src/android/jpeg/encoder_libjpeg.cpp > > > +++ b/src/android/jpeg/encoder_libjpeg.cpp > > > @@ -103,9 +103,9 @@ int EncoderLibJpeg::configure(const StreamConfiguration &cfg) > > > return 0; > > > } > > > > > > -void EncoderLibJpeg::compressRGB(Span<const uint8_t> frame) > > > +void EncoderLibJpeg::compressRGB(const std::vector<Span<uint8_t>> &planes) > > > { > > > - unsigned char *src = const_cast<unsigned char *>(frame.data()); > > > + unsigned char *src = const_cast<unsigned char *>(planes[0].data()); > > > /* \todo Stride information should come from buffer configuration. */ > > > unsigned int stride = pixelFormatInfo_->stride(compress_.image_width, 0); > > > > > > @@ -121,7 +121,7 @@ void EncoderLibJpeg::compressRGB(Span<const uint8_t> frame) > > > * Compress the incoming buffer from a supported NV format. > > > * This naively unpacks the semi-planar NV12 to a YUV888 format for libjpeg. > > > */ > > > -void EncoderLibJpeg::compressNV(Span<const uint8_t> frame) > > > +void EncoderLibJpeg::compressNV(const std::vector<Span<uint8_t>> &planes) > > > { > > > uint8_t tmprowbuf[compress_.image_width * 3]; > > > > > > @@ -133,7 +133,6 @@ void EncoderLibJpeg::compressNV(Span<const uint8_t> frame) > > > * Possible hints at: > > > * https://sourceforge.net/p/libjpeg/mailman/message/30815123/ > > > */ > > > - unsigned int y_stride = pixelFormatInfo_->stride(compress_.image_width, 0); y_stride should be used in computing src_y later in while-loop. Reviewed-by: Hirokazu Honda <hiroh@chromium.org> -Hiro > > > unsigned int c_stride = pixelFormatInfo_->stride(compress_.image_width, 1); > > > > > > unsigned int horzSubSample = 2 * compress_.image_width / c_stride; > > > @@ -143,8 +142,8 @@ void EncoderLibJpeg::compressNV(Span<const uint8_t> frame) > > > unsigned int cb_pos = nvSwap_ ? 1 : 0; > > > unsigned int cr_pos = nvSwap_ ? 0 : 1; > > > > > > - const unsigned char *src = frame.data(); > > > - const unsigned char *src_c = src + y_stride * compress_.image_height; > > > + const unsigned char *src = planes[0].data(); > > > + const unsigned char *src_c = planes[1].data(); > > > > > > JSAMPROW row_pointer[1]; > > > row_pointer[0] = &tmprowbuf[0]; > > > @@ -188,11 +187,12 @@ int EncoderLibJpeg::encode(const FrameBuffer &source, Span<uint8_t> dest, > > > return frame.error(); > > > } > > > > > > - return encode(frame.planes()[0], dest, exifData, quality); > > > + return encode(frame.planes(), dest, exifData, quality); > > > } > > > > > > -int EncoderLibJpeg::encode(Span<const uint8_t> src, Span<uint8_t> dest, > > > - Span<const uint8_t> exifData, unsigned int quality) > > > +int EncoderLibJpeg::encode(const std::vector<Span<uint8_t>> &src, > > > + Span<uint8_t> dest, Span<const uint8_t> exifData, > > > + unsigned int quality) > > > { > > > unsigned char *destination = dest.data(); > > > unsigned long size = dest.size(); > > > diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h > > > index 61fbd1a69278..45ffbd7fae5d 100644 > > > --- a/src/android/jpeg/encoder_libjpeg.h > > > +++ b/src/android/jpeg/encoder_libjpeg.h > > > @@ -9,6 +9,8 @@ > > > > > > #include "encoder.h" > > > > > > +#include <vector> > > > + > > > #include "libcamera/internal/formats.h" > > > > > > #include <jpeglib.h> > > > @@ -24,14 +26,14 @@ public: > > > libcamera::Span<uint8_t> destination, > > > libcamera::Span<const uint8_t> exifData, > > > unsigned int quality) override; > > > - int encode(libcamera::Span<const uint8_t> source, > > > + int encode(const std::vector<libcamera::Span<uint8_t>> &planes, > > > libcamera::Span<uint8_t> destination, > > > libcamera::Span<const uint8_t> exifData, > > > unsigned int quality); > > > > > > private: > > > - void compressRGB(libcamera::Span<const uint8_t> frame); > > > - void compressNV(libcamera::Span<const uint8_t> frame); > > > + void compressRGB(const std::vector<libcamera::Span<uint8_t>> &planes); > > > + void compressNV(const std::vector<libcamera::Span<uint8_t>> &planes); > > > > > > struct jpeg_compress_struct compress_; > > > struct jpeg_error_mgr jerr_; > > > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp > > > index 3160a784419c..68d74842925d 100644 > > > --- a/src/android/jpeg/post_processor_jpeg.cpp > > > +++ b/src/android/jpeg/post_processor_jpeg.cpp > > > @@ -72,7 +72,7 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source, > > > */ > > > thumbnail->resize(rawThumbnail.size()); > > > > > > - int jpeg_size = thumbnailEncoder_.encode(rawThumbnail, > > > + int jpeg_size = thumbnailEncoder_.encode({ rawThumbnail }, > > > *thumbnail, {}, quality); > > > thumbnail->resize(jpeg_size); > > > > > > diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp > > > index 043c7b33f06a..2c341535de9e 100644 > > > --- a/src/android/jpeg/thumbnailer.cpp > > > +++ b/src/android/jpeg/thumbnailer.cpp > > > @@ -62,8 +62,8 @@ void Thumbnailer::createThumbnail(const FrameBuffer &source, > > > ASSERT(tw % 2 == 0 && th % 2 == 0); > > > > > > /* Image scaling block implementing nearest-neighbour algorithm. */ > > > - unsigned char *src = static_cast<unsigned char *>(frame.planes()[0].data()); > > > - unsigned char *srcC = src + sh * sw; > > > + unsigned char *src = frame.planes()[0].data(); > > > + unsigned char *srcC = frame.planes()[1].data(); > > > unsigned char *srcCb, *srcCr; > > > unsigned char *dstY, *srcY; > > > > > -- > Regards, > > Laurent Pinchart
diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp index b8b01e20a001..7f63f36e793e 100644 --- a/src/android/jpeg/encoder_libjpeg.cpp +++ b/src/android/jpeg/encoder_libjpeg.cpp @@ -103,9 +103,9 @@ int EncoderLibJpeg::configure(const StreamConfiguration &cfg) return 0; } -void EncoderLibJpeg::compressRGB(Span<const uint8_t> frame) +void EncoderLibJpeg::compressRGB(const std::vector<Span<uint8_t>> &planes) { - unsigned char *src = const_cast<unsigned char *>(frame.data()); + unsigned char *src = const_cast<unsigned char *>(planes[0].data()); /* \todo Stride information should come from buffer configuration. */ unsigned int stride = pixelFormatInfo_->stride(compress_.image_width, 0); @@ -121,7 +121,7 @@ void EncoderLibJpeg::compressRGB(Span<const uint8_t> frame) * Compress the incoming buffer from a supported NV format. * This naively unpacks the semi-planar NV12 to a YUV888 format for libjpeg. */ -void EncoderLibJpeg::compressNV(Span<const uint8_t> frame) +void EncoderLibJpeg::compressNV(const std::vector<Span<uint8_t>> &planes) { uint8_t tmprowbuf[compress_.image_width * 3]; @@ -133,7 +133,6 @@ void EncoderLibJpeg::compressNV(Span<const uint8_t> frame) * Possible hints at: * https://sourceforge.net/p/libjpeg/mailman/message/30815123/ */ - unsigned int y_stride = pixelFormatInfo_->stride(compress_.image_width, 0); unsigned int c_stride = pixelFormatInfo_->stride(compress_.image_width, 1); unsigned int horzSubSample = 2 * compress_.image_width / c_stride; @@ -143,8 +142,8 @@ void EncoderLibJpeg::compressNV(Span<const uint8_t> frame) unsigned int cb_pos = nvSwap_ ? 1 : 0; unsigned int cr_pos = nvSwap_ ? 0 : 1; - const unsigned char *src = frame.data(); - const unsigned char *src_c = src + y_stride * compress_.image_height; + const unsigned char *src = planes[0].data(); + const unsigned char *src_c = planes[1].data(); JSAMPROW row_pointer[1]; row_pointer[0] = &tmprowbuf[0]; @@ -188,11 +187,12 @@ int EncoderLibJpeg::encode(const FrameBuffer &source, Span<uint8_t> dest, return frame.error(); } - return encode(frame.planes()[0], dest, exifData, quality); + return encode(frame.planes(), dest, exifData, quality); } -int EncoderLibJpeg::encode(Span<const uint8_t> src, Span<uint8_t> dest, - Span<const uint8_t> exifData, unsigned int quality) +int EncoderLibJpeg::encode(const std::vector<Span<uint8_t>> &src, + Span<uint8_t> dest, Span<const uint8_t> exifData, + unsigned int quality) { unsigned char *destination = dest.data(); unsigned long size = dest.size(); diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h index 61fbd1a69278..45ffbd7fae5d 100644 --- a/src/android/jpeg/encoder_libjpeg.h +++ b/src/android/jpeg/encoder_libjpeg.h @@ -9,6 +9,8 @@ #include "encoder.h" +#include <vector> + #include "libcamera/internal/formats.h" #include <jpeglib.h> @@ -24,14 +26,14 @@ public: libcamera::Span<uint8_t> destination, libcamera::Span<const uint8_t> exifData, unsigned int quality) override; - int encode(libcamera::Span<const uint8_t> source, + int encode(const std::vector<libcamera::Span<uint8_t>> &planes, libcamera::Span<uint8_t> destination, libcamera::Span<const uint8_t> exifData, unsigned int quality); private: - void compressRGB(libcamera::Span<const uint8_t> frame); - void compressNV(libcamera::Span<const uint8_t> frame); + void compressRGB(const std::vector<libcamera::Span<uint8_t>> &planes); + void compressNV(const std::vector<libcamera::Span<uint8_t>> &planes); struct jpeg_compress_struct compress_; struct jpeg_error_mgr jerr_; diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp index 3160a784419c..68d74842925d 100644 --- a/src/android/jpeg/post_processor_jpeg.cpp +++ b/src/android/jpeg/post_processor_jpeg.cpp @@ -72,7 +72,7 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source, */ thumbnail->resize(rawThumbnail.size()); - int jpeg_size = thumbnailEncoder_.encode(rawThumbnail, + int jpeg_size = thumbnailEncoder_.encode({ rawThumbnail }, *thumbnail, {}, quality); thumbnail->resize(jpeg_size); diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp index 043c7b33f06a..2c341535de9e 100644 --- a/src/android/jpeg/thumbnailer.cpp +++ b/src/android/jpeg/thumbnailer.cpp @@ -62,8 +62,8 @@ void Thumbnailer::createThumbnail(const FrameBuffer &source, ASSERT(tw % 2 == 0 && th % 2 == 0); /* Image scaling block implementing nearest-neighbour algorithm. */ - unsigned char *src = static_cast<unsigned char *>(frame.planes()[0].data()); - unsigned char *srcC = src + sh * sw; + unsigned char *src = frame.planes()[0].data(); + unsigned char *srcC = frame.planes()[1].data(); unsigned char *srcCb, *srcCr; unsigned char *dstY, *srcY;
The JPEG post-processor uses MappedFrameBuffer to access pixel data, but only uses data from the first plane. Pass the vector of planes to the encode() function to correctly handle multi-planar formats (currently limited to NV12). Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/android/jpeg/encoder_libjpeg.cpp | 18 +++++++++--------- src/android/jpeg/encoder_libjpeg.h | 8 +++++--- src/android/jpeg/post_processor_jpeg.cpp | 2 +- src/android/jpeg/thumbnailer.cpp | 4 ++-- 4 files changed, 17 insertions(+), 15 deletions(-)