[libcamera-devel] [PATCH v2 0/3] android: jpeg: exif: Embed a JPEG-encoded thumbnail

[Kieran Bingham]

Hi Umang,

On 27/10/2020 21:24, Umang Jain wrote:
> Changes in v2:
> - Drop patch 1/3 from v1.
> - Use pointers instead of reference if the function is making change to
>   the parameter.
> - Split Thumbnailer class and its usage into 2 patches.
> - More rigorous const correct-ness in patch 1/3
> 
> There are two major open-ended topics I would like further commenta on:
> 
> 1. Whether to keep thumbnailEncoder_ inside PostProcessorJpeg or move it
>    to Thumbnailer class itself? Hiroh suggests that it might be useful
>    to keep it in the latter, so let's discuss it widely of what makes
>    sense.

I still think encoding the thumbnail is separate to rescaling it. Your
thumbnail class is really just a simple, fixed-output-size scaler class.

I think I saw Laurent say somewhere that as thumbnails can also be
stored as uncompressed, we could also choose to do that at some point,
which would keep it separate.

> 2. Fate of EncoderLibJpeg::encode() introduced in patch 1/3. This is
>    basically allowing us to encode from raw bytes, but is still internal
>    to EncoderLibJpeg(and uses the same naming as Encoder::encode()
>    virtual function). Should I rename it to something else, while
>    keeping it internal to EncoderLibJpeg? Also, I wonder if this is a
>    potential candidate for Encoder interface too? In the sense that
>    Encoder(s) have a way to encode stuff directly handed to them as
>    bytes. 


There's going to be more churn to these interfaces as we extend the
PostProcessors, and we don't yet know what we need, or how that will
play out.

Particularly in regards to the fact that we'll also likely need to make
updates to the FrameBuffer / MappedFrameBuffer type interfaces - so I
think we should stick with what we have for now, and we can update if
needed when those issues get resolved.

--
Kieran


> 
> Thanks for the reviews all of you!
> 
> Umang Jain (3):
>   android: jpeg: encoder_libjpeg: Allow encoding raw frame bytes
>   android: jpeg: Introduce a simple image thumbnailer
>   android: jpeg: post_processor_jpeg: Embed thumbnail into Exif metadata
> 
>  src/android/jpeg/encoder_libjpeg.cpp     |  18 ++--
>  src/android/jpeg/encoder_libjpeg.h       |   7 +-
>  src/android/jpeg/exif.cpp                |  24 ++++-
>  src/android/jpeg/exif.h                  |   2 +
>  src/android/jpeg/post_processor_jpeg.cpp |  34 +++++++
>  src/android/jpeg/post_processor_jpeg.h   |   8 +-
>  src/android/jpeg/thumbnailer.cpp         | 113 +++++++++++++++++++++++
>  src/android/jpeg/thumbnailer.h           |  36 ++++++++
>  src/android/meson.build                  |   1 +
>  9 files changed, 233 insertions(+), 10 deletions(-)
>  create mode 100644 src/android/jpeg/thumbnailer.cpp
>  create mode 100644 src/android/jpeg/thumbnailer.h
> 

-- 
Regards
--
Kieran