[libcamera-devel] [PATCH 2/3] android: jpeg: encoder_libjpeg: Allow encoding raw frame bytes

[Hirokazu Honda]

On Tue, Oct 27, 2020 at 3:49 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Umang,
>
> Thank you for the patch.
>
> On Mon, Oct 26, 2020 at 07:31:33PM +0530, Umang Jain wrote:
> > Allow encoding frames 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 at uajain.com>
> > ---
> >  src/android/jpeg/encoder_libjpeg.cpp | 18 ++++++++++++------
> >  src/android/jpeg/encoder_libjpeg.h   |  7 +++++--
> >  2 files changed, 17 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp
> > index cfa5332..129ca27 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 MappedBuffer *frame)
> > +void EncoderLibJpeg::compressRGB(Span<uint8_t> frame)
>
> Can you make this a Span<const uint8_t> to express that the function
> doesn't modify the contents of the frame ? It will likely require a few
> changes within the function, making local variables const, and/or using
> const_cast<> to cast away the const as some libjpeg functions take a
> non-const pointer to data they never modify (an unfortunate API choice,
> but that's the way it is).
>
> >  {
> > -     unsigned char *src = static_cast<unsigned char *>(frame->maps()[0].data());
> > +     unsigned char *src = 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 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 MappedBuffer *frame)
> > +void EncoderLibJpeg::compressNV(Span<uint8_t> frame)
>
> Same here.
>
> >  {
> >       uint8_t tmprowbuf[compress_.image_width * 3];
> >
> > @@ -144,7 +144,7 @@ void EncoderLibJpeg::compressNV(const 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 = frame.data();
> >       const unsigned char *src_c = src + y_stride * compress_.image_height;
> >
> >       JSAMPROW row_pointer[1];
> > @@ -189,6 +189,12 @@ int EncoderLibJpeg::encode(const FrameBuffer &source, Span<uint8_t> dest,
> >               return frame.error();
> >       }
> >
> > +     return encode(frame.maps()[0], dest, exifData);
> > +}
> > +
> > +int EncoderLibJpeg::encode(Span<uint8_t> src, Span<uint8_t> dest,
> > +                        Span<const uint8_t> exifData)
>
> And same here for src (dest obviously needs to be mutable :-)).
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> > +{
> >       unsigned char *destination = dest.data();
> >       unsigned long size = dest.size();
> >
> > @@ -214,9 +220,9 @@ int EncoderLibJpeg::encode(const FrameBuffer &source, Span<uint8_t> dest,
> >                        << "x" << compress_.image_height;
> >
> >       if (nv_)
> > -             compressNV(&frame);
> > +             compressNV(src);
> >       else
> > -             compressRGB(&frame);
> > +             compressRGB(src);
> >
> >       jpeg_finish_compress(&compress_);
> >
> > diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h
> > index 40505dd..06f884b 100644
> > --- a/src/android/jpeg/encoder_libjpeg.h
> > +++ b/src/android/jpeg/encoder_libjpeg.h
> > @@ -24,10 +24,13 @@ public:
> >       int encode(const libcamera::FrameBuffer &source,
> >                  libcamera::Span<uint8_t> destination,
> >                  libcamera::Span<const uint8_t> exifData) override;
> > +     int encode(libcamera::Span<uint8_t> source,
> > +                libcamera::Span<uint8_t> destination,
> > +                libcamera::Span<const uint8_t> exifData);
> >

The commit message says this is an overloaded function.
But it isn't actually. Because of that, this function needs to be
invoked with EncoderLibJpeg in PostProcessorJpeg.
If I remember correctly, we'all agree to expanding FrameBuffer for
dmabuf in the future.
So the interface with FrameBuffer should be a solid single overloaded function.
I think we would rather
1.) makes the new function a specific function (which is done by this
patch actually),
2.) Use the FrameBuffer interface always.
We may want to think of the tradeoff between introducing the specific
function and how simple the code becomes thanks to it.

Best Regards,
-Hiro



> >  private:
> > -     void compressRGB(const libcamera::MappedBuffer *frame);
> > -     void compressNV(const libcamera::MappedBuffer *frame);
> > +     void compressRGB(libcamera::Span<uint8_t> frame);
> > +     void compressNV(libcamera::Span<uint8_t> frame);
> >
> >       struct jpeg_compress_struct compress_;
> >       struct jpeg_error_mgr jerr_;
>
> --
> Regards,
>
> Laurent Pinchart
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel