| Message ID | 20201021080806.46636-2-email@uajain.com |
|---|---|
| State | Superseded |
| Delegated to: | Umang Jain |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, On 21/10/2020 09:08, Umang Jain wrote: > Allow encoding frame which are directly handed over to the encoder > via a span or vector i.e. a raw frame bytes. Introduce an overloaded > EncoderLibJpeg::encode() with libcamera::Span source parameter to > achieve this functionality. This makes the libjpeg-encoder a bit > flexible for use case such as compressing a thumbnail generated for > Exif. > This looks good to me, It changes the internals of this to support only single-plane formats, but we only support single-plane formats so I'm not worried by that. It also gives the compressRGB, compressNV12 better input parameters in my opinion, as it's clear what data they are operating on. In other words, I like that they deal with span internally, rather than operating on a MappedBuffer type. Particularly as the MappedBuffer topics will likely need more thought later. > Signed-off-by: Umang Jain <email@uajain.com> > --- > src/android/jpeg/encoder_libjpeg.cpp | 37 +++++++++++++++++----------- > src/android/jpeg/encoder_libjpeg.h | 7 ++++-- > 2 files changed, 28 insertions(+), 16 deletions(-) > > diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp > index f11e004..bfaf9d5 100644 > --- a/src/android/jpeg/encoder_libjpeg.cpp > +++ b/src/android/jpeg/encoder_libjpeg.cpp > @@ -104,9 +104,9 @@ int EncoderLibJpeg::configure(const StreamConfiguration &cfg) > return 0; > } > > -void EncoderLibJpeg::compressRGB(const libcamera::MappedBuffer *frame) > +void EncoderLibJpeg::compressRGB(const libcamera::Span<uint8_t> &frame) > { > - unsigned char *src = static_cast<unsigned char *>(frame->maps()[0].data()); > + unsigned char *src = static_cast<unsigned char *>(frame.data()); > /* \todo Stride information should come from buffer configuration. */ > unsigned int stride = pixelFormatInfo_->stride(compress_.image_width, 0); > > @@ -122,7 +122,7 @@ void EncoderLibJpeg::compressRGB(const libcamera::MappedBuffer *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(const libcamera::MappedBuffer *frame) > +void EncoderLibJpeg::compressNV(const libcamera::Span<uint8_t> &frame) > { > uint8_t tmprowbuf[compress_.image_width * 3]; > > @@ -144,7 +144,7 @@ void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame) > unsigned int cb_pos = nvSwap_ ? 1 : 0; > unsigned int cr_pos = nvSwap_ ? 0 : 1; > > - const unsigned char *src = static_cast<unsigned char *>(frame->maps()[0].data()); > + const unsigned char *src = static_cast<unsigned char *>(frame.data()); > const unsigned char *src_c = src + y_stride * compress_.image_height; > > JSAMPROW row_pointer[1]; > @@ -179,17 +179,10 @@ void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame) > } > } > > -int EncoderLibJpeg::encode(const FrameBuffer *source, > +int EncoderLibJpeg::encode(const libcamera::Span<uint8_t> &frame, > const libcamera::Span<uint8_t> &dest, > const libcamera::Span<const uint8_t> &exifData) > { > - MappedFrameBuffer frame(source, PROT_READ); > - if (!frame.isValid()) { > - LOG(JPEG, Error) << "Failed to map FrameBuffer : " > - << strerror(frame.error()); > - return frame.error(); > - } > - > unsigned char *destination = dest.data(); > unsigned long size = dest.size(); > > @@ -215,11 +208,27 @@ int EncoderLibJpeg::encode(const FrameBuffer *source, > << "x" << compress_.image_height; > > if (nv_) > - compressNV(&frame); > + compressNV(frame); > else > - compressRGB(&frame); > + compressRGB(frame); > > jpeg_finish_compress(&compress_); > > return size; > } > + > +int EncoderLibJpeg::encode(const FrameBuffer *source, > + const libcamera::Span<uint8_t> &dest, > + const libcamera::Span<const uint8_t> &exifData) > +{ > + MappedFrameBuffer frame(source, PROT_READ); > + if (!frame.isValid()) { > + LOG(JPEG, Error) << "Failed to map FrameBuffer : " > + << strerror(frame.error()); > + return frame.error(); > + } > + > + libcamera::Span<uint8_t> src = frame.maps()[0]; > + > + return encode(src, dest, exifData); This could also be: return encode(frame.maps()[0], dest, exifData); I don't think there's a need to declare a new span for this, but it's fine if you prefer it / find it more readable. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > +} > diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h > index 934caef..bf2e512 100644 > --- a/src/android/jpeg/encoder_libjpeg.h > +++ b/src/android/jpeg/encoder_libjpeg.h > @@ -21,13 +21,16 @@ public: > ~EncoderLibJpeg(); > > int configure(const libcamera::StreamConfiguration &cfg) override; > + int encode(const libcamera::Span<uint8_t> &frames, > + const libcamera::Span<uint8_t> &destination, > + const libcamera::Span<const uint8_t> &exifData); > int encode(const libcamera::FrameBuffer *source, > const libcamera::Span<uint8_t> &destination, > const libcamera::Span<const uint8_t> &exifData) override; > > private: > - void compressRGB(const libcamera::MappedBuffer *frame); > - void compressNV(const libcamera::MappedBuffer *frame); > + void compressRGB(const libcamera::Span<uint8_t> &frame); > + void compressNV(const libcamera::Span<uint8_t> &frame); > > struct jpeg_compress_struct compress_; > struct jpeg_error_mgr jerr_; >
Hello Umang, Thank you for the patch. On Wed, Oct 21, 2020 at 09:27:05AM +0100, Kieran Bingham wrote: > On 21/10/2020 09:08, Umang Jain wrote: > > Allow encoding frame which are directly handed over to the encoder s/frame/frames/ > > via a span or vector i.e. a raw frame bytes. Introduce an overloaded > > EncoderLibJpeg::encode() with libcamera::Span source parameter to > > achieve this functionality. This makes the libjpeg-encoder a bit > > flexible for use case such as compressing a thumbnail generated for > > Exif. > > This looks good to me, It changes the internals of this to support only > single-plane formats, but we only support single-plane formats so I'm > not worried by that. Fine with me as well, we can address this later if/when needed. > It also gives the compressRGB, compressNV12 better input parameters in > my opinion, as it's clear what data they are operating on. In other > words, I like that they deal with span internally, rather than operating > on a MappedBuffer type. Particularly as the MappedBuffer topics will > likely need more thought later. > > > Signed-off-by: Umang Jain <email@uajain.com> > > --- > > src/android/jpeg/encoder_libjpeg.cpp | 37 +++++++++++++++++----------- > > src/android/jpeg/encoder_libjpeg.h | 7 ++++-- > > 2 files changed, 28 insertions(+), 16 deletions(-) > > > > diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp > > index f11e004..bfaf9d5 100644 > > --- a/src/android/jpeg/encoder_libjpeg.cpp > > +++ b/src/android/jpeg/encoder_libjpeg.cpp > > @@ -104,9 +104,9 @@ int EncoderLibJpeg::configure(const StreamConfiguration &cfg) > > return 0; > > } > > > > -void EncoderLibJpeg::compressRGB(const libcamera::MappedBuffer *frame) > > +void EncoderLibJpeg::compressRGB(const libcamera::Span<uint8_t> &frame) > > { > > - unsigned char *src = static_cast<unsigned char *>(frame->maps()[0].data()); > > + unsigned char *src = static_cast<unsigned char *>(frame.data()); I think you can drop the static_cast<>. Same in compressNV(). It would be nice to turn the argument to the function into a const libcamera::Span<const uint8_t> &frame. However, src will likely need to remain mutable, as the libjpeg API doesn't take a const pointer :-( You will thus need a const_cast<>(). > > /* \todo Stride information should come from buffer configuration. */ > > unsigned int stride = pixelFormatInfo_->stride(compress_.image_width, 0); > > > > @@ -122,7 +122,7 @@ void EncoderLibJpeg::compressRGB(const libcamera::MappedBuffer *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(const libcamera::MappedBuffer *frame) > > +void EncoderLibJpeg::compressNV(const libcamera::Span<uint8_t> &frame) > > { > > uint8_t tmprowbuf[compress_.image_width * 3]; > > > > @@ -144,7 +144,7 @@ void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame) > > unsigned int cb_pos = nvSwap_ ? 1 : 0; > > unsigned int cr_pos = nvSwap_ ? 0 : 1; > > > > - const unsigned char *src = static_cast<unsigned char *>(frame->maps()[0].data()); > > + const unsigned char *src = static_cast<unsigned char *>(frame.data()); > > const unsigned char *src_c = src + y_stride * compress_.image_height; > > > > JSAMPROW row_pointer[1]; > > @@ -179,17 +179,10 @@ void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame) > > } > > } > > > > -int EncoderLibJpeg::encode(const FrameBuffer *source, > > +int EncoderLibJpeg::encode(const libcamera::Span<uint8_t> &frame, > > const libcamera::Span<uint8_t> &dest, > > const libcamera::Span<const uint8_t> &exifData) > > { > > - MappedFrameBuffer frame(source, PROT_READ); > > - if (!frame.isValid()) { > > - LOG(JPEG, Error) << "Failed to map FrameBuffer : " > > - << strerror(frame.error()); > > - return frame.error(); > > - } > > - > > unsigned char *destination = dest.data(); > > unsigned long size = dest.size(); > > > > @@ -215,11 +208,27 @@ int EncoderLibJpeg::encode(const FrameBuffer *source, > > << "x" << compress_.image_height; > > > > if (nv_) > > - compressNV(&frame); > > + compressNV(frame); > > else > > - compressRGB(&frame); > > + compressRGB(frame); > > > > jpeg_finish_compress(&compress_); > > > > return size; > > } > > + > > +int EncoderLibJpeg::encode(const FrameBuffer *source, > > + const libcamera::Span<uint8_t> &dest, > > + const libcamera::Span<const uint8_t> &exifData) > > +{ > > + MappedFrameBuffer frame(source, PROT_READ); > > + if (!frame.isValid()) { > > + LOG(JPEG, Error) << "Failed to map FrameBuffer : " > > + << strerror(frame.error()); > > + return frame.error(); > > + } > > + > > + libcamera::Span<uint8_t> src = frame.maps()[0]; > > + > > + return encode(src, dest, exifData); > > This could also be: > return encode(frame.maps()[0], dest, exifData); > > I don't think there's a need to declare a new span for this, but it's > fine if you prefer it / find it more readable. I like Kieran's suggestion. > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > +} > > diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h > > index 934caef..bf2e512 100644 > > --- a/src/android/jpeg/encoder_libjpeg.h > > +++ b/src/android/jpeg/encoder_libjpeg.h > > @@ -21,13 +21,16 @@ public: > > ~EncoderLibJpeg(); > > > > int configure(const libcamera::StreamConfiguration &cfg) override; > > + int encode(const libcamera::Span<uint8_t> &frames, s/frames/frame/ Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > + const libcamera::Span<uint8_t> &destination, > > + const libcamera::Span<const uint8_t> &exifData); > > int encode(const libcamera::FrameBuffer *source, > > const libcamera::Span<uint8_t> &destination, > > const libcamera::Span<const uint8_t> &exifData) override; > > > > private: > > - void compressRGB(const libcamera::MappedBuffer *frame); > > - void compressNV(const libcamera::MappedBuffer *frame); > > + void compressRGB(const libcamera::Span<uint8_t> &frame); > > + void compressNV(const libcamera::Span<uint8_t> &frame); > > > > struct jpeg_compress_struct compress_; > > struct jpeg_error_mgr jerr_;
diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp index f11e004..bfaf9d5 100644 --- a/src/android/jpeg/encoder_libjpeg.cpp +++ b/src/android/jpeg/encoder_libjpeg.cpp @@ -104,9 +104,9 @@ int EncoderLibJpeg::configure(const StreamConfiguration &cfg) return 0; } -void EncoderLibJpeg::compressRGB(const libcamera::MappedBuffer *frame) +void EncoderLibJpeg::compressRGB(const libcamera::Span<uint8_t> &frame) { - unsigned char *src = static_cast<unsigned char *>(frame->maps()[0].data()); + unsigned char *src = static_cast<unsigned char *>(frame.data()); /* \todo Stride information should come from buffer configuration. */ unsigned int stride = pixelFormatInfo_->stride(compress_.image_width, 0); @@ -122,7 +122,7 @@ void EncoderLibJpeg::compressRGB(const libcamera::MappedBuffer *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(const libcamera::MappedBuffer *frame) +void EncoderLibJpeg::compressNV(const libcamera::Span<uint8_t> &frame) { uint8_t tmprowbuf[compress_.image_width * 3]; @@ -144,7 +144,7 @@ void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame) unsigned int cb_pos = nvSwap_ ? 1 : 0; unsigned int cr_pos = nvSwap_ ? 0 : 1; - const unsigned char *src = static_cast<unsigned char *>(frame->maps()[0].data()); + const unsigned char *src = static_cast<unsigned char *>(frame.data()); const unsigned char *src_c = src + y_stride * compress_.image_height; JSAMPROW row_pointer[1]; @@ -179,17 +179,10 @@ void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame) } } -int EncoderLibJpeg::encode(const FrameBuffer *source, +int EncoderLibJpeg::encode(const libcamera::Span<uint8_t> &frame, const libcamera::Span<uint8_t> &dest, const libcamera::Span<const uint8_t> &exifData) { - MappedFrameBuffer frame(source, PROT_READ); - if (!frame.isValid()) { - LOG(JPEG, Error) << "Failed to map FrameBuffer : " - << strerror(frame.error()); - return frame.error(); - } - unsigned char *destination = dest.data(); unsigned long size = dest.size(); @@ -215,11 +208,27 @@ int EncoderLibJpeg::encode(const FrameBuffer *source, << "x" << compress_.image_height; if (nv_) - compressNV(&frame); + compressNV(frame); else - compressRGB(&frame); + compressRGB(frame); jpeg_finish_compress(&compress_); return size; } + +int EncoderLibJpeg::encode(const FrameBuffer *source, + const libcamera::Span<uint8_t> &dest, + const libcamera::Span<const uint8_t> &exifData) +{ + MappedFrameBuffer frame(source, PROT_READ); + if (!frame.isValid()) { + LOG(JPEG, Error) << "Failed to map FrameBuffer : " + << strerror(frame.error()); + return frame.error(); + } + + libcamera::Span<uint8_t> src = frame.maps()[0]; + + return encode(src, dest, exifData); +} diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h index 934caef..bf2e512 100644 --- a/src/android/jpeg/encoder_libjpeg.h +++ b/src/android/jpeg/encoder_libjpeg.h @@ -21,13 +21,16 @@ public: ~EncoderLibJpeg(); int configure(const libcamera::StreamConfiguration &cfg) override; + int encode(const libcamera::Span<uint8_t> &frames, + const libcamera::Span<uint8_t> &destination, + const libcamera::Span<const uint8_t> &exifData); int encode(const libcamera::FrameBuffer *source, const libcamera::Span<uint8_t> &destination, const libcamera::Span<const uint8_t> &exifData) override; private: - void compressRGB(const libcamera::MappedBuffer *frame); - void compressNV(const libcamera::MappedBuffer *frame); + void compressRGB(const libcamera::Span<uint8_t> &frame); + void compressNV(const libcamera::Span<uint8_t> &frame); struct jpeg_compress_struct compress_; struct jpeg_error_mgr jerr_;
Allow encoding frame which are directly handed over to the encoder via a span or vector i.e. a raw frame bytes. Introduce an overloaded EncoderLibJpeg::encode() with libcamera::Span source parameter to achieve this functionality. This makes the libjpeg-encoder a bit flexible for use case such as compressing a thumbnail generated for Exif. Signed-off-by: Umang Jain <email@uajain.com> --- src/android/jpeg/encoder_libjpeg.cpp | 37 +++++++++++++++++----------- src/android/jpeg/encoder_libjpeg.h | 7 ++++-- 2 files changed, 28 insertions(+), 16 deletions(-)