[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: "

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: "