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

[Kieran Bingham]

Hi Jacopo,

On 22/07/2020 11:31, Jacopo Mondi wrote:
> 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.

The lifetime of a compression object will support multiple consecutive
frames.

All frames must use the same configuration, and (for jpeg), the class
will maintain the libjpeg compression object and allocations (and
configuration), and each call to compress will then simply take one
input frame, and produce one output buffer based on all the
preconfigured parameters.

There's nothing preventing someone from constructing a compression
object each time of course, except then there need to be allocations,
and configuration for each frame which would be redundant.

(by that I mean libjpeg makes internal allocations, so keeping the
lifetime of the Compressor object allows reuse of those internal
allocations for the duration of a whole stream).


> 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

libjpeg can only perform a single compression at a time, but once the
object has a thread, it can indeed handle these as a queue and become
event driven.

But by 'concurrent' a single libjpeg object could only compress a single
frame at a time. If it runs in it's own thread, then that would then
prevent blocking of the calling thread if that's what you mean.



> mechanism to notify the caller when the conversion is done. In this
> case format information are -per conversion run- not per instance.

No, if you want a different set of configuration options, just create
another instance of the compressor...

I haven't yet looked at threading, but indeed that could be handled by
each Compressor creating a thread, which then signals completion events.

That would then form part of the design of the Compressor object base class.



>>>> +};
>>>> +
>>>> +#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

-- 
Regards
--
Kieran