kbingham / libcamera

libcamera - Making complex cameras easy. This is a personal fork, please use the upstream repository at https://git.libcamera.org/libcamera/libcamera.git/
https://libcamera.org
Other
173 stars 69 forks source link

RFU: Check for pixelformat availability before accessing to avoid crash #36

Open rsglobal opened 2 years ago

rsglobal commented 2 years ago

RFU - Request for upstream :)

@kbingham ,

Hi Kieran,

Could you please send this patch to ML if relevant for other.

Currently I'm trying to run uvccamera pipeline on Android and got crash at cfg.pixelFormat = pixelFormats.front();

--- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
+++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
@@ -122,6 +122,9 @@ CameraConfiguration::Status UVCCameraConfiguration::validate()
        const Size size = cfg.size;

        const std::vector<PixelFormat> pixelFormats = formats.pixelformats();
+       if (pixelFormats.empty())
+               return Invalid;
+
        auto iter = std::find(pixelFormats.begin(), pixelFormats.end(), pixelFormat);
        if (iter == pixelFormats.end()) {
                cfg.pixelFormat = pixelFormats.front();
kbingham commented 2 years ago

Looks same initially, can you provide since more information regarding the crash that occurs and what you tried when it happened please?

rsglobal commented 2 years ago

I decided to use libcamera for external cameras support for Android instead of external camera HAL built-in into android.

Built-in HAL doesn't support a set of cheap cameras, which was my motivation to use libcamera.

But it turned-out that enabling of uvcpipeline isn't enough and there are a lot of work need to be done to accomplish my goal.

I started to put stubs in a places which caused pipeline initialization to fail expecting to get at least any results and at some point faced this crash. With this patch I can see Camera configuration invalid log which is much better than the crash.

kbingham commented 2 years ago

This https://patchwork.libcamera.org/patch/17290/ was merged in September, which I think should also fix / prevent the issue you hit here. If you're able to retest this - let me know if it's still an issue or we can close this off.