[v2] EncoderLibJpeg: Migrate const global pixelInfo to static
diff mbox series

Message ID 20250619082027.4058507-1-chenghaoyang@google.com
State New
Headers show
Series
  • [v2] EncoderLibJpeg: Migrate const global pixelInfo to static
Related show

Commit Message

Cheng-Hao Yang June 19, 2025, 8:20 a.m. UTC
From: Harvey Yang <chenghaoyang@chromium.org>

Previously, const global `pixelInfo` in `encoder_libjpeg.cpp` is
initialized before const global `pixelFormatInfo` in `formats.cpp`,
even if `formats.cpp` is wrapped in the base libcamera shared library,
and the android adapter (which contains `encoder_libjpeg.cpp`)
depends on the base libcamera shared library.

The reason should be that C++ initialization order doesn't follow the
linking process.

This patch migrates const global pixelInfo to static to avoid the race
condition.

Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
---
 src/android/jpeg/encoder_libjpeg.cpp | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

Comments

Barnabás Pőcze June 19, 2025, 9:57 a.m. UTC | #1
Hi

2025. 06. 19. 10:20 keltezéssel, Harvey Yang írta:
> From: Harvey Yang <chenghaoyang@chromium.org>
> 
> Previously, const global `pixelInfo` in `encoder_libjpeg.cpp` is
> initialized before const global `pixelFormatInfo` in `formats.cpp`,
> even if `formats.cpp` is wrapped in the base libcamera shared library,
> and the android adapter (which contains `encoder_libjpeg.cpp`)
> depends on the base libcamera shared library.

I generally agree with any removal of non-constinit or mutable global state.

So were you getting `Unsupported pixel format ...` warnings previously,
or a crash or something else?


> 
> The reason should be that C++ initialization order doesn't follow the
> linking process.

I would have assumed that any reasonable linker will bring up the dependencies
before the dependents, but I suppose it is not impossible that the ordering
is different for some reason.


Regards,
Barnabás Pőcze

> 
> This patch migrates const global pixelInfo to static to avoid the race
> condition.
> 
> Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> ---
>   src/android/jpeg/encoder_libjpeg.cpp | 28 ++++++++++++++--------------
>   1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp
> index cb242b5ec6a8..3ba5103d5ad5 100644
> --- a/src/android/jpeg/encoder_libjpeg.cpp
> +++ b/src/android/jpeg/encoder_libjpeg.cpp
> @@ -38,26 +38,26 @@ struct JPEGPixelFormatInfo {
>   	bool nvSwap;
>   };
>   
> -const std::map<PixelFormat, JPEGPixelFormatInfo> pixelInfo{
> -	{ formats::R8, { JCS_GRAYSCALE, PixelFormatInfo::info(formats::R8), false } },
> -
> -	{ formats::RGB888, { JCS_EXT_BGR, PixelFormatInfo::info(formats::RGB888), false } },
> -	{ formats::BGR888, { JCS_EXT_RGB, PixelFormatInfo::info(formats::BGR888), false } },
> -
> -	{ formats::NV12, { JCS_YCbCr, PixelFormatInfo::info(formats::NV12), false } },
> -	{ formats::NV21, { JCS_YCbCr, PixelFormatInfo::info(formats::NV21), true } },
> -	{ formats::NV16, { JCS_YCbCr, PixelFormatInfo::info(formats::NV16), false } },
> -	{ formats::NV61, { JCS_YCbCr, PixelFormatInfo::info(formats::NV61), true } },
> -	{ formats::NV24, { JCS_YCbCr, PixelFormatInfo::info(formats::NV24), false } },
> -	{ formats::NV42, { JCS_YCbCr, PixelFormatInfo::info(formats::NV42), true } },
> -};
> -
>   const struct JPEGPixelFormatInfo &findPixelInfo(const PixelFormat &format)
>   {
>   	static const struct JPEGPixelFormatInfo invalidPixelFormat {
>   		JCS_UNKNOWN, PixelFormatInfo(), false
>   	};
>   
> +	static const std::map<PixelFormat, JPEGPixelFormatInfo> pixelInfo{
> +		{ formats::R8, { JCS_GRAYSCALE, PixelFormatInfo::info(formats::R8), false } },
> +
> +		{ formats::RGB888, { JCS_EXT_BGR, PixelFormatInfo::info(formats::RGB888), false } },
> +		{ formats::BGR888, { JCS_EXT_RGB, PixelFormatInfo::info(formats::BGR888), false } },
> +
> +		{ formats::NV12, { JCS_YCbCr, PixelFormatInfo::info(formats::NV12), false } },
> +		{ formats::NV21, { JCS_YCbCr, PixelFormatInfo::info(formats::NV21), true } },
> +		{ formats::NV16, { JCS_YCbCr, PixelFormatInfo::info(formats::NV16), false } },
> +		{ formats::NV61, { JCS_YCbCr, PixelFormatInfo::info(formats::NV61), true } },
> +		{ formats::NV24, { JCS_YCbCr, PixelFormatInfo::info(formats::NV24), false } },
> +		{ formats::NV42, { JCS_YCbCr, PixelFormatInfo::info(formats::NV42), true } },
> +	};
> +
>   	const auto iter = pixelInfo.find(format);
>   	if (iter == pixelInfo.end()) {
>   		LOG(JPEG, Error) << "Unsupported pixel format for JPEG encoder: "
Cheng-Hao Yang June 19, 2025, 10:02 a.m. UTC | #2
Hi Barnabás,

On Thu, Jun 19, 2025 at 5:57 PM Barnabás Pőcze
<barnabas.pocze@ideasonboard.com> wrote:
>
> Hi
>
> 2025. 06. 19. 10:20 keltezéssel, Harvey Yang írta:
> > From: Harvey Yang <chenghaoyang@chromium.org>
> >
> > Previously, const global `pixelInfo` in `encoder_libjpeg.cpp` is
> > initialized before const global `pixelFormatInfo` in `formats.cpp`,
> > even if `formats.cpp` is wrapped in the base libcamera shared library,
> > and the android adapter (which contains `encoder_libjpeg.cpp`)
> > depends on the base libcamera shared library.
>
> I generally agree with any removal of non-constinit or mutable global state.
>
> So were you getting `Unsupported pixel format ...` warnings previously,
> or a crash or something else?

Yes, I got `Unsupported pixel format ...` warnings, and after adding logs,
I found `pixelFormatInfo` has size 0 when being queried.

>
>
> >
> > The reason should be that C++ initialization order doesn't follow the
> > linking process.
>
> I would have assumed that any reasonable linker will bring up the dependencies
> before the dependents, but I suppose it is not impossible that the ordering
> is different for some reason.

The testing environment is Android OS, FYI.

BR,
Harvey

>
>
> Regards,
> Barnabás Pőcze
>
> >
> > This patch migrates const global pixelInfo to static to avoid the race
> > condition.
> >
> > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> > ---
> >   src/android/jpeg/encoder_libjpeg.cpp | 28 ++++++++++++++--------------
> >   1 file changed, 14 insertions(+), 14 deletions(-)
> >
> > diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp
> > index cb242b5ec6a8..3ba5103d5ad5 100644
> > --- a/src/android/jpeg/encoder_libjpeg.cpp
> > +++ b/src/android/jpeg/encoder_libjpeg.cpp
> > @@ -38,26 +38,26 @@ struct JPEGPixelFormatInfo {
> >       bool nvSwap;
> >   };
> >
> > -const std::map<PixelFormat, JPEGPixelFormatInfo> pixelInfo{
> > -     { formats::R8, { JCS_GRAYSCALE, PixelFormatInfo::info(formats::R8), false } },
> > -
> > -     { formats::RGB888, { JCS_EXT_BGR, PixelFormatInfo::info(formats::RGB888), false } },
> > -     { formats::BGR888, { JCS_EXT_RGB, PixelFormatInfo::info(formats::BGR888), false } },
> > -
> > -     { formats::NV12, { JCS_YCbCr, PixelFormatInfo::info(formats::NV12), false } },
> > -     { formats::NV21, { JCS_YCbCr, PixelFormatInfo::info(formats::NV21), true } },
> > -     { formats::NV16, { JCS_YCbCr, PixelFormatInfo::info(formats::NV16), false } },
> > -     { formats::NV61, { JCS_YCbCr, PixelFormatInfo::info(formats::NV61), true } },
> > -     { formats::NV24, { JCS_YCbCr, PixelFormatInfo::info(formats::NV24), false } },
> > -     { formats::NV42, { JCS_YCbCr, PixelFormatInfo::info(formats::NV42), true } },
> > -};
> > -
> >   const struct JPEGPixelFormatInfo &findPixelInfo(const PixelFormat &format)
> >   {
> >       static const struct JPEGPixelFormatInfo invalidPixelFormat {
> >               JCS_UNKNOWN, PixelFormatInfo(), false
> >       };
> >
> > +     static const std::map<PixelFormat, JPEGPixelFormatInfo> pixelInfo{
> > +             { formats::R8, { JCS_GRAYSCALE, PixelFormatInfo::info(formats::R8), false } },
> > +
> > +             { formats::RGB888, { JCS_EXT_BGR, PixelFormatInfo::info(formats::RGB888), false } },
> > +             { formats::BGR888, { JCS_EXT_RGB, PixelFormatInfo::info(formats::BGR888), false } },
> > +
> > +             { formats::NV12, { JCS_YCbCr, PixelFormatInfo::info(formats::NV12), false } },
> > +             { formats::NV21, { JCS_YCbCr, PixelFormatInfo::info(formats::NV21), true } },
> > +             { formats::NV16, { JCS_YCbCr, PixelFormatInfo::info(formats::NV16), false } },
> > +             { formats::NV61, { JCS_YCbCr, PixelFormatInfo::info(formats::NV61), true } },
> > +             { formats::NV24, { JCS_YCbCr, PixelFormatInfo::info(formats::NV24), false } },
> > +             { formats::NV42, { JCS_YCbCr, PixelFormatInfo::info(formats::NV42), true } },
> > +     };
> > +
> >       const auto iter = pixelInfo.find(format);
> >       if (iter == pixelInfo.end()) {
> >               LOG(JPEG, Error) << "Unsupported pixel format for JPEG encoder: "
>
Laurent Pinchart June 19, 2025, 10:30 a.m. UTC | #3
On Thu, Jun 19, 2025 at 06:02:33PM +0800, Cheng-Hao Yang wrote:
> On Thu, Jun 19, 2025 at 5:57 PM Barnabás Pőcze wrote:
> > 2025. 06. 19. 10:20 keltezéssel, Harvey Yang írta:
> > > From: Harvey Yang <chenghaoyang@chromium.org>
> > >
> > > Previously, const global `pixelInfo` in `encoder_libjpeg.cpp` is
> > > initialized before const global `pixelFormatInfo` in `formats.cpp`,
> > > even if `formats.cpp` is wrapped in the base libcamera shared library,
> > > and the android adapter (which contains `encoder_libjpeg.cpp`)
> > > depends on the base libcamera shared library.
> >
> > I generally agree with any removal of non-constinit or mutable global state.
> >
> > So were you getting `Unsupported pixel format ...` warnings previously,
> > or a crash or something else?
> 
> Yes, I got `Unsupported pixel format ...` warnings, and after adding logs,
> I found `pixelFormatInfo` has size 0 when being queried.

There's something I don't quite get.
src/android/jpeg/encoder_libjpeg.cpp is compiled in libcamera-hal.so,
which links against libcamera.so. The latter provides
PixelFormatInfo::info(). Does this mean that the global variables of
libcamera-hal.so get initialized before those of libcamera.so ? Is that
allowed ? More investigation seems to be needed.

> > > The reason should be that C++ initialization order doesn't follow the
> > > linking process.
> >
> > I would have assumed that any reasonable linker will bring up the dependencies
> > before the dependents, but I suppose it is not impossible that the ordering
> > is different for some reason.
> 
> The testing environment is Android OS, FYI.

How to you compile libcamera on Android ?

> > > This patch migrates const global pixelInfo to static to avoid the race
> > > condition.
> > >
> > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> > > ---
> > >   src/android/jpeg/encoder_libjpeg.cpp | 28 ++++++++++++++--------------
> > >   1 file changed, 14 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp
> > > index cb242b5ec6a8..3ba5103d5ad5 100644
> > > --- a/src/android/jpeg/encoder_libjpeg.cpp
> > > +++ b/src/android/jpeg/encoder_libjpeg.cpp
> > > @@ -38,26 +38,26 @@ struct JPEGPixelFormatInfo {
> > >       bool nvSwap;
> > >   };
> > >
> > > -const std::map<PixelFormat, JPEGPixelFormatInfo> pixelInfo{
> > > -     { formats::R8, { JCS_GRAYSCALE, PixelFormatInfo::info(formats::R8), false } },
> > > -
> > > -     { formats::RGB888, { JCS_EXT_BGR, PixelFormatInfo::info(formats::RGB888), false } },
> > > -     { formats::BGR888, { JCS_EXT_RGB, PixelFormatInfo::info(formats::BGR888), false } },
> > > -
> > > -     { formats::NV12, { JCS_YCbCr, PixelFormatInfo::info(formats::NV12), false } },
> > > -     { formats::NV21, { JCS_YCbCr, PixelFormatInfo::info(formats::NV21), true } },
> > > -     { formats::NV16, { JCS_YCbCr, PixelFormatInfo::info(formats::NV16), false } },
> > > -     { formats::NV61, { JCS_YCbCr, PixelFormatInfo::info(formats::NV61), true } },
> > > -     { formats::NV24, { JCS_YCbCr, PixelFormatInfo::info(formats::NV24), false } },
> > > -     { formats::NV42, { JCS_YCbCr, PixelFormatInfo::info(formats::NV42), true } },
> > > -};
> > > -
> > >   const struct JPEGPixelFormatInfo &findPixelInfo(const PixelFormat &format)
> > >   {
> > >       static const struct JPEGPixelFormatInfo invalidPixelFormat {
> > >               JCS_UNKNOWN, PixelFormatInfo(), false
> > >       };
> > >
> > > +     static const std::map<PixelFormat, JPEGPixelFormatInfo> pixelInfo{
> > > +             { formats::R8, { JCS_GRAYSCALE, PixelFormatInfo::info(formats::R8), false } },
> > > +
> > > +             { formats::RGB888, { JCS_EXT_BGR, PixelFormatInfo::info(formats::RGB888), false } },
> > > +             { formats::BGR888, { JCS_EXT_RGB, PixelFormatInfo::info(formats::BGR888), false } },
> > > +
> > > +             { formats::NV12, { JCS_YCbCr, PixelFormatInfo::info(formats::NV12), false } },
> > > +             { formats::NV21, { JCS_YCbCr, PixelFormatInfo::info(formats::NV21), true } },
> > > +             { formats::NV16, { JCS_YCbCr, PixelFormatInfo::info(formats::NV16), false } },
> > > +             { formats::NV61, { JCS_YCbCr, PixelFormatInfo::info(formats::NV61), true } },
> > > +             { formats::NV24, { JCS_YCbCr, PixelFormatInfo::info(formats::NV24), false } },
> > > +             { formats::NV42, { JCS_YCbCr, PixelFormatInfo::info(formats::NV42), true } },
> > > +     };
> > > +
> > >       const auto iter = pixelInfo.find(format);
> > >       if (iter == pixelInfo.end()) {
> > >               LOG(JPEG, Error) << "Unsupported pixel format for JPEG encoder: "
Cheng-Hao Yang June 20, 2025, 4:33 a.m. UTC | #4
Hi Laurent,

On Thu, Jun 19, 2025 at 6:30 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Thu, Jun 19, 2025 at 06:02:33PM +0800, Cheng-Hao Yang wrote:
> > On Thu, Jun 19, 2025 at 5:57 PM Barnabás Pőcze wrote:
> > > 2025. 06. 19. 10:20 keltezéssel, Harvey Yang írta:
> > > > From: Harvey Yang <chenghaoyang@chromium.org>
> > > >
> > > > Previously, const global `pixelInfo` in `encoder_libjpeg.cpp` is
> > > > initialized before const global `pixelFormatInfo` in `formats.cpp`,
> > > > even if `formats.cpp` is wrapped in the base libcamera shared library,
> > > > and the android adapter (which contains `encoder_libjpeg.cpp`)
> > > > depends on the base libcamera shared library.
> > >
> > > I generally agree with any removal of non-constinit or mutable global state.
> > >
> > > So were you getting `Unsupported pixel format ...` warnings previously,
> > > or a crash or something else?
> >
> > Yes, I got `Unsupported pixel format ...` warnings, and after adding logs,
> > I found `pixelFormatInfo` has size 0 when being queried.
>
> There's something I don't quite get.
> src/android/jpeg/encoder_libjpeg.cpp is compiled in libcamera-hal.so,
> which links against libcamera.so. The latter provides
> PixelFormatInfo::info(). Does this mean that the global variables of
> libcamera-hal.so get initialized before those of libcamera.so ? Is that
> allowed ? More investigation seems to be needed.

I had the same question, while the experiments showed the other way
around. There are also similar discussions like:
https://stackoverflow.com/questions/3746238/c-global-initialization-order-ignores-dependencies


>
> > > > The reason should be that C++ initialization order doesn't follow the
> > > > linking process.
> > >
> > > I would have assumed that any reasonable linker will bring up the dependencies
> > > before the dependents, but I suppose it is not impossible that the ordering
> > > is different for some reason.
> >
> > The testing environment is Android OS, FYI.
>
> How to you compile libcamera on Android ?

Currently we have to rewrite with Android.bp, instead of using the available
`meson.build`s, while as I mentioned before, I built `libcamera_src_defaults`,
which contains `src/libcamera/formats.cpp`. And built
`libcamera_android_src_defaults` (depending on `libcamera_src_defaults`)
, which contains `src/android/jpeg/encoder_libjpeg.cpp`.

BR,
Harvey

>
> > > > This patch migrates const global pixelInfo to static to avoid the race
> > > > condition.
> > > >
> > > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> > > > ---
> > > >   src/android/jpeg/encoder_libjpeg.cpp | 28 ++++++++++++++--------------
> > > >   1 file changed, 14 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp
> > > > index cb242b5ec6a8..3ba5103d5ad5 100644
> > > > --- a/src/android/jpeg/encoder_libjpeg.cpp
> > > > +++ b/src/android/jpeg/encoder_libjpeg.cpp
> > > > @@ -38,26 +38,26 @@ struct JPEGPixelFormatInfo {
> > > >       bool nvSwap;
> > > >   };
> > > >
> > > > -const std::map<PixelFormat, JPEGPixelFormatInfo> pixelInfo{
> > > > -     { formats::R8, { JCS_GRAYSCALE, PixelFormatInfo::info(formats::R8), false } },
> > > > -
> > > > -     { formats::RGB888, { JCS_EXT_BGR, PixelFormatInfo::info(formats::RGB888), false } },
> > > > -     { formats::BGR888, { JCS_EXT_RGB, PixelFormatInfo::info(formats::BGR888), false } },
> > > > -
> > > > -     { formats::NV12, { JCS_YCbCr, PixelFormatInfo::info(formats::NV12), false } },
> > > > -     { formats::NV21, { JCS_YCbCr, PixelFormatInfo::info(formats::NV21), true } },
> > > > -     { formats::NV16, { JCS_YCbCr, PixelFormatInfo::info(formats::NV16), false } },
> > > > -     { formats::NV61, { JCS_YCbCr, PixelFormatInfo::info(formats::NV61), true } },
> > > > -     { formats::NV24, { JCS_YCbCr, PixelFormatInfo::info(formats::NV24), false } },
> > > > -     { formats::NV42, { JCS_YCbCr, PixelFormatInfo::info(formats::NV42), true } },
> > > > -};
> > > > -
> > > >   const struct JPEGPixelFormatInfo &findPixelInfo(const PixelFormat &format)
> > > >   {
> > > >       static const struct JPEGPixelFormatInfo invalidPixelFormat {
> > > >               JCS_UNKNOWN, PixelFormatInfo(), false
> > > >       };
> > > >
> > > > +     static const std::map<PixelFormat, JPEGPixelFormatInfo> pixelInfo{
> > > > +             { formats::R8, { JCS_GRAYSCALE, PixelFormatInfo::info(formats::R8), false } },
> > > > +
> > > > +             { formats::RGB888, { JCS_EXT_BGR, PixelFormatInfo::info(formats::RGB888), false } },
> > > > +             { formats::BGR888, { JCS_EXT_RGB, PixelFormatInfo::info(formats::BGR888), false } },
> > > > +
> > > > +             { formats::NV12, { JCS_YCbCr, PixelFormatInfo::info(formats::NV12), false } },
> > > > +             { formats::NV21, { JCS_YCbCr, PixelFormatInfo::info(formats::NV21), true } },
> > > > +             { formats::NV16, { JCS_YCbCr, PixelFormatInfo::info(formats::NV16), false } },
> > > > +             { formats::NV61, { JCS_YCbCr, PixelFormatInfo::info(formats::NV61), true } },
> > > > +             { formats::NV24, { JCS_YCbCr, PixelFormatInfo::info(formats::NV24), false } },
> > > > +             { formats::NV42, { JCS_YCbCr, PixelFormatInfo::info(formats::NV42), true } },
> > > > +     };
> > > > +
> > > >       const auto iter = pixelInfo.find(format);
> > > >       if (iter == pixelInfo.end()) {
> > > >               LOG(JPEG, Error) << "Unsupported pixel format for JPEG encoder: "
>
> --
> Regards,
>
> Laurent Pinchart
Barnabás Pőcze June 20, 2025, 7:30 a.m. UTC | #5
Hi

2025. 06. 20. 6:33 keltezéssel, Cheng-Hao Yang írta:
> Hi Laurent,
> 
> On Thu, Jun 19, 2025 at 6:30 PM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>>
>> On Thu, Jun 19, 2025 at 06:02:33PM +0800, Cheng-Hao Yang wrote:
>>> On Thu, Jun 19, 2025 at 5:57 PM Barnabás Pőcze wrote:
>>>> 2025. 06. 19. 10:20 keltezéssel, Harvey Yang írta:
>>>>> From: Harvey Yang <chenghaoyang@chromium.org>
>>>>>
>>>>> Previously, const global `pixelInfo` in `encoder_libjpeg.cpp` is
>>>>> initialized before const global `pixelFormatInfo` in `formats.cpp`,
>>>>> even if `formats.cpp` is wrapped in the base libcamera shared library,
>>>>> and the android adapter (which contains `encoder_libjpeg.cpp`)
>>>>> depends on the base libcamera shared library.
>>>>
>>>> I generally agree with any removal of non-constinit or mutable global state.
>>>>
>>>> So were you getting `Unsupported pixel format ...` warnings previously,
>>>> or a crash or something else?
>>>
>>> Yes, I got `Unsupported pixel format ...` warnings, and after adding logs,
>>> I found `pixelFormatInfo` has size 0 when being queried.
>>
>> There's something I don't quite get.
>> src/android/jpeg/encoder_libjpeg.cpp is compiled in libcamera-hal.so,
>> which links against libcamera.so. The latter provides
>> PixelFormatInfo::info(). Does this mean that the global variables of
>> libcamera-hal.so get initialized before those of libcamera.so ? Is that
>> allowed ? More investigation seems to be needed.
> 
> I had the same question, while the experiments showed the other way
> around. There are also similar discussions like:
> https://stackoverflow.com/questions/3746238/c-global-initialization-order-ignores-dependencies

The case presented there I think is clear. But here we have two shared libraries
where one depends on the other. It seems a bit odd that the dynamic linker would
start the initialization of the dependent before the initialization of the
dependency is done.


> 
> 
>>
>>>>> The reason should be that C++ initialization order doesn't follow the
>>>>> linking process.
>>>>
>>>> I would have assumed that any reasonable linker will bring up the dependencies
>>>> before the dependents, but I suppose it is not impossible that the ordering
>>>> is different for some reason.
>>>
>>> The testing environment is Android OS, FYI.
>>
>> How to you compile libcamera on Android ?
> 
> Currently we have to rewrite with Android.bp, instead of using the available
> `meson.build`s, while as I mentioned before, I built `libcamera_src_defaults`,
> which contains `src/libcamera/formats.cpp`. And built
> `libcamera_android_src_defaults` (depending on `libcamera_src_defaults`)
> , which contains `src/android/jpeg/encoder_libjpeg.cpp`.

Can you tell us where that `Android.bp` file is? I can't find it. It does not
make any of the two a static library, correct?


Regards,
Barnabás Pőcze


> 
> BR,
> Harvey
> 
> [...]
Cheng-Hao Yang June 20, 2025, 8:02 a.m. UTC | #6
Hi Barnabás,

On Fri, Jun 20, 2025 at 3:30 PM Barnabás Pőcze
<barnabas.pocze@ideasonboard.com> wrote:
>
> Hi
>
> 2025. 06. 20. 6:33 keltezéssel, Cheng-Hao Yang írta:
> > Hi Laurent,
> >
> > On Thu, Jun 19, 2025 at 6:30 PM Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> >>
> >> On Thu, Jun 19, 2025 at 06:02:33PM +0800, Cheng-Hao Yang wrote:
> >>> On Thu, Jun 19, 2025 at 5:57 PM Barnabás Pőcze wrote:
> >>>> 2025. 06. 19. 10:20 keltezéssel, Harvey Yang írta:
> >>>>> From: Harvey Yang <chenghaoyang@chromium.org>
> >>>>>
> >>>>> Previously, const global `pixelInfo` in `encoder_libjpeg.cpp` is
> >>>>> initialized before const global `pixelFormatInfo` in `formats.cpp`,
> >>>>> even if `formats.cpp` is wrapped in the base libcamera shared library,
> >>>>> and the android adapter (which contains `encoder_libjpeg.cpp`)
> >>>>> depends on the base libcamera shared library.
> >>>>
> >>>> I generally agree with any removal of non-constinit or mutable global state.
> >>>>
> >>>> So were you getting `Unsupported pixel format ...` warnings previously,
> >>>> or a crash or something else?
> >>>
> >>> Yes, I got `Unsupported pixel format ...` warnings, and after adding logs,
> >>> I found `pixelFormatInfo` has size 0 when being queried.
> >>
> >> There's something I don't quite get.
> >> src/android/jpeg/encoder_libjpeg.cpp is compiled in libcamera-hal.so,
> >> which links against libcamera.so. The latter provides
> >> PixelFormatInfo::info(). Does this mean that the global variables of
> >> libcamera-hal.so get initialized before those of libcamera.so ? Is that
> >> allowed ? More investigation seems to be needed.
> >
> > I had the same question, while the experiments showed the other way
> > around. There are also similar discussions like:
> > https://stackoverflow.com/questions/3746238/c-global-initialization-order-ignores-dependencies
>
> The case presented there I think is clear. But here we have two shared libraries
> where one depends on the other. It seems a bit odd that the dynamic linker would
> start the initialization of the dependent before the initialization of the
> dependency is done.
>
>
> >
> >
> >>
> >>>>> The reason should be that C++ initialization order doesn't follow the
> >>>>> linking process.
> >>>>
> >>>> I would have assumed that any reasonable linker will bring up the dependencies
> >>>> before the dependents, but I suppose it is not impossible that the ordering
> >>>> is different for some reason.
> >>>
> >>> The testing environment is Android OS, FYI.
> >>
> >> How to you compile libcamera on Android ?
> >
> > Currently we have to rewrite with Android.bp, instead of using the available
> > `meson.build`s, while as I mentioned before, I built `libcamera_src_defaults`,
> > which contains `src/libcamera/formats.cpp`. And built
> > `libcamera_android_src_defaults` (depending on `libcamera_src_defaults`)
> > , which contains `src/android/jpeg/encoder_libjpeg.cpp`.
>
> Can you tell us where that `Android.bp` file is? I can't find it. It does not
> make any of the two a static library, correct?

As it's in an Android vertical (for ChromeOS) repository, it's not
public now, sorry.

After some more investigation, I think I found what I missed:
I used `cc_defaults` in Android.bp to build the core sources and the Android
Adapter sources, while it doesn't produce intermediate results/shared libraries.
When I added the final shared library with Virtual pipeline handler, it combines
all the defaults dependencies into a single shared library, instead of
doing it with
linking, I believe.

Seems that it's only a special case for Android. I'll check if we
should refine our
Android.bp to avoid cc_defaults or just adopt this patch in our repository.
Doesn't need to merge this patch into the upstream though.

BR,
Harvey

>
>
> Regards,
> Barnabás Pőcze
>
>
> >
> > BR,
> > Harvey
> >
> > [...]
>

Patch
diff mbox series

diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp
index cb242b5ec6a8..3ba5103d5ad5 100644
--- a/src/android/jpeg/encoder_libjpeg.cpp
+++ b/src/android/jpeg/encoder_libjpeg.cpp
@@ -38,26 +38,26 @@  struct JPEGPixelFormatInfo {
 	bool nvSwap;
 };
 
-const std::map<PixelFormat, JPEGPixelFormatInfo> pixelInfo{
-	{ formats::R8, { JCS_GRAYSCALE, PixelFormatInfo::info(formats::R8), false } },
-
-	{ formats::RGB888, { JCS_EXT_BGR, PixelFormatInfo::info(formats::RGB888), false } },
-	{ formats::BGR888, { JCS_EXT_RGB, PixelFormatInfo::info(formats::BGR888), false } },
-
-	{ formats::NV12, { JCS_YCbCr, PixelFormatInfo::info(formats::NV12), false } },
-	{ formats::NV21, { JCS_YCbCr, PixelFormatInfo::info(formats::NV21), true } },
-	{ formats::NV16, { JCS_YCbCr, PixelFormatInfo::info(formats::NV16), false } },
-	{ formats::NV61, { JCS_YCbCr, PixelFormatInfo::info(formats::NV61), true } },
-	{ formats::NV24, { JCS_YCbCr, PixelFormatInfo::info(formats::NV24), false } },
-	{ formats::NV42, { JCS_YCbCr, PixelFormatInfo::info(formats::NV42), true } },
-};
-
 const struct JPEGPixelFormatInfo &findPixelInfo(const PixelFormat &format)
 {
 	static const struct JPEGPixelFormatInfo invalidPixelFormat {
 		JCS_UNKNOWN, PixelFormatInfo(), false
 	};
 
+	static const std::map<PixelFormat, JPEGPixelFormatInfo> pixelInfo{
+		{ formats::R8, { JCS_GRAYSCALE, PixelFormatInfo::info(formats::R8), false } },
+
+		{ formats::RGB888, { JCS_EXT_BGR, PixelFormatInfo::info(formats::RGB888), false } },
+		{ formats::BGR888, { JCS_EXT_RGB, PixelFormatInfo::info(formats::BGR888), false } },
+
+		{ formats::NV12, { JCS_YCbCr, PixelFormatInfo::info(formats::NV12), false } },
+		{ formats::NV21, { JCS_YCbCr, PixelFormatInfo::info(formats::NV21), true } },
+		{ formats::NV16, { JCS_YCbCr, PixelFormatInfo::info(formats::NV16), false } },
+		{ formats::NV61, { JCS_YCbCr, PixelFormatInfo::info(formats::NV61), true } },
+		{ formats::NV24, { JCS_YCbCr, PixelFormatInfo::info(formats::NV24), false } },
+		{ formats::NV42, { JCS_YCbCr, PixelFormatInfo::info(formats::NV42), true } },
+	};
+
 	const auto iter = pixelInfo.find(format);
 	if (iter == pixelInfo.end()) {
 		LOG(JPEG, Error) << "Unsupported pixel format for JPEG encoder: "