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

10269 mbox series

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.
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. 

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

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
>