[libcamera-devel] [RFC PATCH 1/6] android: Introduce JPEG compression

[Jacopo Mondi]

On Wed, Jul 22, 2020 at 11:17:50AM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>

snip

> >> +	unsigned int stride_;
> >> +
> >> +	bool nv_;
> >> +	bool nvSwap_;
> >> +	unsigned int horzSubSample_;
> >> +	unsigned int vertSubSample_;
> >
> > These information should be decoupled from the compressor class, as
> > they belong to the format the is being compressed, not to the
> > compressor instance. What do you think ? Are compressors throw away
> > objects created to run once on a format and thrown away ?
>
> This is just a caching of the configuration that was selected. When it
> can come from the PixelFormatInfo, I expect it will store just a
> reference to that instead.
>

My question was more like: do you create one compressor instance per
conversion ? Otherwise I don't see how format information, regardless
where they come from or are stored, can be class members.

As I thought it, the compressor class should be instantiated once, and
provides methods to schedule possibly concurrent conversions, in a
thread as you noted down in the cover letter, with a signal/slot
mechanism to notify the caller when the conversion is done. In this
case format information are -per conversion run- not per instance.

>
> >> +};
> >> +
> >> +#endif /* __COMPRESSOR_JPEG_H__ */
> >> diff --git a/src/android/meson.build b/src/android/meson.build
> >> index 822cad621f01..51dcd99ee62f 100644
> >> --- a/src/android/meson.build
> >> +++ b/src/android/meson.build
> >> @@ -6,6 +6,7 @@ android_hal_sources = files([
> >>      'camera_device.cpp',
> >>      'camera_metadata.cpp',
> >>      'camera_ops.cpp',
> >> +    'jpeg/compressor_jpeg.cpp',
> >>  ])
> >>
> >>  android_camera_metadata_sources = files([
> >> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> >> index 3aad4386ffc2..d78e2c1f6eb8 100644
> >> --- a/src/libcamera/meson.build
> >> +++ b/src/libcamera/meson.build
> >> @@ -124,6 +124,8 @@ if get_option('android')
> >>      libcamera_sources += android_hal_sources
> >>      includes += android_includes
> >>      libcamera_link_with += android_camera_metadata
> >> +
> >> +    libcamera_deps += dependency('libjpeg')
> >
> > Shouldn't this be a dependency introduced by compiling the android HAL
> > in ?
> >
>
> it is ... it's under the if get_option('android') section.
>

Oh sorry, missed that.


>
> > Thanks
> >   j
> >
> >>  endif
> >>
> >>  # We add '/' to the build_rpath as a 'safe' path to act as a boolean flag.
> >> --
> >> 2.25.1
> >>
> >> _______________________________________________
> >> libcamera-devel mailing list
> >> libcamera-devel at lists.libcamera.org
> >> https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards
> --
> Kieran