| Message ID | 20250618120204.3047218-1-chenghaoyang@google.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Harvey, Thank you for the patch. On Wed, Jun 18, 2025 at 12:01:59PM +0000, Harvey Yang wrote: > Previously, JPEGPixelFormatInfo::PixelFormatInfo is a const reference. > However, the use cases of it are mostly from temporarily constructed > instances, which will be destructed right after setting up > JPEGPixelFormatInfo. Could you please explain in more details where those temporary instances are destroyed ? JPEGPixelFormatInfo::pixelFormatInfo is populated from PixelFormatInfo::info() calls, which should always return a reference to a const global instance with a lifetime matching the libcamera lifetime. > To avoid data corruption, JPEGPixelFormatInfo::PixelFormatInfo should be > a pure constant member variable instead. > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > --- > src/android/jpeg/encoder_libjpeg.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp > index cb242b5ec6a8..0c088535f0be 100644 > --- a/src/android/jpeg/encoder_libjpeg.cpp > +++ b/src/android/jpeg/encoder_libjpeg.cpp > @@ -34,7 +34,7 @@ namespace { > > struct JPEGPixelFormatInfo { > J_COLOR_SPACE colorSpace; > - const PixelFormatInfo &pixelFormatInfo; > + const PixelFormatInfo pixelFormatInfo; > bool nvSwap; > }; >
Hi Laurent, On Thu, Jun 19, 2025 at 12:09 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Harvey, > > Thank you for the patch. > > On Wed, Jun 18, 2025 at 12:01:59PM +0000, Harvey Yang wrote: > > Previously, JPEGPixelFormatInfo::PixelFormatInfo is a const reference. > > However, the use cases of it are mostly from temporarily constructed > > instances, which will be destructed right after setting up > > JPEGPixelFormatInfo. > > Could you please explain in more details where those temporary instances > are destroyed ? JPEGPixelFormatInfo::pixelFormatInfo is populated from > PixelFormatInfo::info() calls, which should always return a reference to > a const global instance with a lifetime matching the libcamera lifetime. Ah yes, you're right. I misunderstood the failure I encountered: I'm running libcamera on Android, and it's that 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. It seems that C++ initialization order doesn't follow the linking process... Please check if it makes sense to you, and I'll upload another patch to put `pixelFormatInfo` in a static function in PixelFormatInfo. Let me know if there's a more proper fix. Thanks, Harvey > > > To avoid data corruption, JPEGPixelFormatInfo::PixelFormatInfo should be > > a pure constant member variable instead. > > > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > > --- > > src/android/jpeg/encoder_libjpeg.cpp | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp > > index cb242b5ec6a8..0c088535f0be 100644 > > --- a/src/android/jpeg/encoder_libjpeg.cpp > > +++ b/src/android/jpeg/encoder_libjpeg.cpp > > @@ -34,7 +34,7 @@ namespace { > > > > struct JPEGPixelFormatInfo { > > J_COLOR_SPACE colorSpace; > > - const PixelFormatInfo &pixelFormatInfo; > > + const PixelFormatInfo pixelFormatInfo; > > bool nvSwap; > > }; > > > > -- > Regards, > > Laurent Pinchart
diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp index cb242b5ec6a8..0c088535f0be 100644 --- a/src/android/jpeg/encoder_libjpeg.cpp +++ b/src/android/jpeg/encoder_libjpeg.cpp @@ -34,7 +34,7 @@ namespace { struct JPEGPixelFormatInfo { J_COLOR_SPACE colorSpace; - const PixelFormatInfo &pixelFormatInfo; + const PixelFormatInfo pixelFormatInfo; bool nvSwap; };
Previously, JPEGPixelFormatInfo::PixelFormatInfo is a const reference. However, the use cases of it are mostly from temporarily constructed instances, which will be destructed right after setting up JPEGPixelFormatInfo. To avoid data corruption, JPEGPixelFormatInfo::PixelFormatInfo should be a pure constant member variable instead. Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> --- src/android/jpeg/encoder_libjpeg.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)