raspberrypi / picamera2

New libcamera based python library
BSD 2-Clause "Simplified" License
891 stars 188 forks source link

[BUG] Can't change tuning after configuring camera #886

Closed rwb27 closed 11 months ago

rwb27 commented 11 months ago

Describe the bug I want to be able to run the camera, do some analysis, then run it again with modified tuning parameters. I'm fairly sure in a previous version (I'm afraid I didn't note the version) this was possible - but at least using what's current on pypi and in Bookworm's python3-picamera2 package, it currently won't work.

The symptom is that the tuning is simply ignored for subsequent initialisations. I'm guessing the tuning gets somehow "baked in" but I am afraid I can't figure out how, after some amount of poring over the source code. It's possible something has changed in libcamera rather than picamera2 but I'm not really sure how I'd figure that out.

To Reproduce I run the following (as test_tuning.py) with pytest, but hopefully it's easy enough to follow what's happening.


import os
from picamera2 import Picamera2
import time

import pytest

def load_default_tuning():
    with Picamera2() as cam:
        cp = cam.camera_properties
        fname = f"{cp['Model']}.json"
        return cam.load_tuning_file(fname)

def generate_bad_tuning():
    default_tuning = load_default_tuning()
    bad_tuning = default_tuning.copy()
    bad_tuning["version"] = 999
    return bad_tuning

def _test_bad_tuning_after_good_tuning(configure):
    bad_tuning = generate_bad_tuning()
    default_tuning = load_default_tuning()
    print("opening camera with explicitly specified tuning")
    with Picamera2(tuning=default_tuning) as cam:
        if configure:
            cam.configure(cam.create_preview_configuration())
    del cam
    with pytest.raises(IndexError):
        # The bad version should cause a problem
        with Picamera2(tuning=bad_tuning):
            print("Success (not expected)!")

def test_bad_tuning_after_good_tuning_noconfigure():
    _test_bad_tuning_after_good_tuning(False)

def test_bad_tuning_after_good_tuning_configure():
    _test_bad_tuning_after_good_tuning(True)

Expected behaviour All tests should pass: each time we load a known-bad tuning, it should notice and fail. What actually happens is that test_bad_tuning_after_good_tuning_configure fails, because no exception is raised when we initialise the camera with the bad tuning.

Hardware : Pi 4 2Gb, running Bookworm, and Raspberry Pi Camera Module v2.1

davidplowman commented 11 months ago

@naushir Could I ask you to comment on this? The observation is that once you've configured the camera, even after closing it, you can't re-open it with a different tuning file. My impression is actually that it's always been like this, though this report suggests that maybe I am mis-remembering. What's your recollection of this? Thanks!

naushir commented 11 months ago

The observation is that once you've configured the camera, even after closing it, you can't re-open it with a different tuning file. My impression is actually that it's always been like this,

I think this is correct, and has always been the behavior. When you start the camera manager (at the libcamera API level), you open the IPA with a particular tuning file. Starting/stopping/restarting the camera will keep the same IPA instance, previously opened with the given tuning file through the camera manager.

At present, the only way to reload another tuning file would be to destroy and reconstruct the camera manager. Maybe a stop/start might also work? We may want to consider changing this and allow a new tuning file to be used when camera start is called. but that to me sounds very very scary as all the algorithm state will need to be very carefully handled.

davidplowman commented 11 months ago

Presumably when we open the camera there might be the opportunity to load a different tuning file?

davidplowman commented 11 months ago

I wonder if there's been a change inside libcamera where the camera manager was getting (effectively) garbage collected when the Python handles were dropped...?

naushir commented 11 months ago

Presumably when we open the camera there might be the opportunity to load a different tuning file?

I think only CameraManager::stop() followed by CameraManager::start() will enable us to reload the tuning file.

davidplowman commented 11 months ago

As far as I can tell, there's a gCameraManager weak global pointer in the Python bindings layer that will hold onto the camera manager, unless everyone "drops" it. When the first Picamera2 instance is closed, however, this weak pointer doesn't get cleared, suggesting to me that something is still holding on to the camera manger - and therefore it doesn't get recreated for the second "open".

I assume that at one time, presumably, it was getting dropped "correctly", so the camera manager was being shut down and restarted. At the moment I can't see what it is that might be holding on to it.

Of course, this is still not good behaviour, because you couldn't change the tuning for one camera if a second camera was open. The real solution needs to be passing some kind of "blob" in when a camera is opened/acquired.

rwb27 commented 11 months ago

Thanks for this - I've located a previously-used SD card in the lab, with older dependencies. Using libcamera v0.0.4+22-923f5d70 and picamera2==0.3.12 (the latter installed with pip), I can confirm that both tests pass, albeit I had to modify them slightly as the exceptions raised were different. Modified version below for completeness.

import os
from picamera2 import Picamera2
import time

import pytest

def load_default_tuning():
    with Picamera2() as cam:
        cp = cam.camera_properties
        fname = f"{cp['Model']}.json"
    try:
        return cam.load_tuning_file(fname)
    except RuntimeError:
        dir="/usr/share/libcamera/ipa/raspberrypi"  # from picamera2 v0.3.9
        # The directory above has been removed from the search path, which I
        # find odd - as that's where the files currently are on a default
        # Raspbian image. This may need updating if the files have moved
        # in future updates to the system libcamera package
        return cam.load_tuning_file(fname, dir=dir)

def generate_bad_tuning():
    default_tuning = load_default_tuning()
    bad_tuning = default_tuning.copy()
    bad_tuning["version"] = 999
    return bad_tuning

def _test_bad_tuning_after_good_tuning(configure):
    bad_tuning = generate_bad_tuning()
    default_tuning = load_default_tuning()
    print("opening camera with explicitly specified tuning")
    with Picamera2(tuning=default_tuning) as cam:
        if configure:
            cam.configure(cam.create_preview_configuration())
    del cam
    with pytest.raises(Exception):
        # The bad version should cause a problem
        with Picamera2(tuning=bad_tuning):
            print("Success (not expected)!")

def test_bad_tuning_after_good_tuning_noconfigure():
    _test_bad_tuning_after_good_tuning(False)

def test_bad_tuning_after_good_tuning_configure():
    _test_bad_tuning_after_good_tuning(True)

I had to run the tests separately, as otherwise the camera was left in a half-initialised state and wouldn't work for the second test, but I can confirm that pytest test_tuning.py -k _noconfigure and pytest test_tuning.py -k _configure both pass.

rwb27 commented 11 months ago

I assume that at one time, presumably, it was getting dropped "correctly", so the camera manager was being shut down and restarted. At the moment I can't see what it is that might be holding on to it.

This makes perfect sense, thank you for explaining. I agree it would be lovely to pass in a blob explicitly rather than relying on tempfiles and environment variables, but I also realise that requires quite a lot of moving parts to fix what is probably a bit of an edge case.

For now, I will look for a way to ensure we call CameraManager::stop() and CameraManager::start() in between the two calls. If I can come up with a pure-Python work-around, that would be brilliant. Any pointers from either of you would be gratefully received!

rwb27 commented 11 months ago

I've done some poking around (see attached) and as far as the output of the gc module is concerned, the number of references to the cameramanager singleton does drop to zero when the camera is closed, even after I configure it (or get the sensor modes. I can see some debug output from libcamera when I create the singleton instance manually, and if I open and close a camera (without configuring it) this debug output repeats if I delete the instance and recreate it.

However, if I configure the camera, while the number of referrers still drops to zero, I don't see the debug output a second time - that suggests that it's not being garbage collected, as you suspected. I guess I will need to delve a level down to figure out what's keeping it alive - garbage collection in C python extensions is new to me, I'm afraid...

My script and output are below, for the record.

python investigate_cameramanager.py (investigate_cameramanager.py - renamed with .patch because I can't attach .py) yields:

Making singleton (should see debug output below)
[45:16:09.653402009] [23326]  INFO Camera camera_manager.cpp:284 libcamera v0.1.0+118-563cd78e
[45:16:09.699163713] [23332]  WARN RPiSdn sdn.cpp:39 Using legacy SDN tuning - please consider moving SDN inside rpi.denoise
[45:16:09.701348250] [23332]  WARN RPI vc4.cpp:390 Mismatch between Unicam and CamHelper for embedded data usage!
[45:16:09.702212787] [23332]  INFO RPI vc4.cpp:444 Registered camera /base/soc/i2c0mux/i2c@1/imx219@10 to Unicam device /dev/media4 and ISP device /dev/media0
[45:16:09.702272102] [23332]  INFO RPI pipeline_base.cpp:1142 Using configuration file '/usr/share/libcamera/pipeline/rpi/vc4/rpi_apps.yaml'
Referrers: __main__, 
Making Picamera2
Referrers: <picamera2.picamera2.CameraManager object at 0xebb4acf0>, __main__, 
cam.close()
Referrers: __main__, 
deleted cam, manager -- should start afresh from here.
Making singleton (should see debug output below)
[45:16:09.922438028] [23326]  INFO Camera camera_manager.cpp:284 libcamera v0.1.0+118-563cd78e
[45:16:09.966077454] [23345]  WARN RPiSdn sdn.cpp:39 Using legacy SDN tuning - please consider moving SDN inside rpi.denoise
[45:16:09.968047120] [23345]  WARN RPI vc4.cpp:390 Mismatch between Unicam and CamHelper for embedded data usage!
[45:16:09.968900102] [23345]  INFO RPI vc4.cpp:444 Registered camera /base/soc/i2c0mux/i2c@1/imx219@10 to Unicam device /dev/media4 and ISP device /dev/media0
[45:16:09.968957639] [23345]  INFO RPI pipeline_base.cpp:1142 Using configuration file '/usr/share/libcamera/pipeline/rpi/vc4/rpi_apps.yaml'
Referrers: __main__, 
Making Picamera2
Referrers: <picamera2.picamera2.CameraManager object at 0xebb4acf0>, __main__, 
cam.configure()
[45:16:09.983732361] [23326]  INFO Camera camera.cpp:1183 configuring streams: (0) 640x480-XBGR8888 (1) 640x480-SBGGR10_CSI2P
[45:16:09.984295843] [23345]  INFO RPI vc4.cpp:608 Sensor: /base/soc/i2c0mux/i2c@1/imx219@10 - Selected sensor format: 640x480-SBGGR10_1X10 - Selected unicam format: 640x480-pBAA
Referrers: <picamera2.picamera2.CameraManager object at 0xebb4acf0>, __main__, 
cam.close()
Referrers: __main__, 
deleted cam, manager -- should start afresh from here.
Making singleton (should see debug output below)
Referrers: __main__, 

The lack of debug output between the last two lines indicates that the cameramanager object did not get destroyed. This was using apt-installed libcamera and picamera2:

Package: python3-libcamera
Version: 0.1.0+rpt20231122-1
Package: python3-picamera2
Version: 0.3.16-1

I can confirm that on the older system (Package: python3-libcamera Version: 0~git20230302+923f5d70-1), I do get the debug output after the last recreation of the camera manager, i.e. it is getting garbage collected as I expect.

naushir commented 11 months ago

I wonder if we need a open and close function defined in the pybind11 wrapper for the camera manager here: https://github.com/raspberrypi/libcamera/blob/563cd78e1c9858769f7e4cc2628e2515836fd6e7/src/py/libcamera/py_main.cpp#L140 ?

davidplowman commented 11 months ago

I think I might have a hideous workaround. After deleting the Picamera2 object (cam in your script), add:

from picamera2.picamera2 import CameraManager
import gc
del Picamera2._cm
Picamera2._cm = None
gc.collect()
Picamera2._cm = CameraManager()

before opening the next instance. At least, that seemed to do the trick for me.

I wonder if the moral of the story is that this has never been guaranteed to work because it is all totally at the mercy of when and how Python garbage collection happens (which is a mystery to me too, frankly). And it seems that our luck may finally have run out. But the better plan is obviously to work towards a proper API for passing in this information when we open a camera.

rwb27 commented 11 months ago

If my understanding of the comments above is correct, I think that would solve my issue - I would be very happy to manually call close then open on the CameraManager, having first ensured there are no open cameras. I don't know the internals of libcamera well enough to know if that's a safe thing to do, but I would be delighted to try it out.

I think I have a proof-of-principle working. As I said, I am slightly concerned I'm doing something dangerous, but it does seem to run OK. Steps to reproduce my current setup:

sudo apt install libcamera-dev
python3 -m venv .venv  # Note: system-site-packages is not enabled
source .venv/bin/activate
git clone https://github.com/rwb27/pylibcamera.git
cd pylibcamera
pip install .  # Note: I had to increase swap to 500Mb on my 2Gb Pi 4.
pip install picamera2
cp -R /usr/lib/python3/dist-packages/pykms/ .venv/lib/python3.11/site-packages/ # can't install via pip

I can then run the attached python script, which calls libcamera.CameraManager.singleton().restart() after closing the Picamera2 object. This seems to reload everything. investigate_gc.py

I've modified my test script above, and with the addition of the reset() in between, both tests now pass - i.e. it appears to correctly reload the tuning.

import os
from picamera2 import Picamera2
import libcamera
import time

import pytest

def load_default_tuning():
    with Picamera2() as cam:
        cp = cam.camera_properties
        fname = f"{cp['Model']}.json"
    try:
        return cam.load_tuning_file(fname)
    except RuntimeError:
        dir="/usr/share/libcamera/ipa/raspberrypi"  # from picamera2 v0.3.9
        # The directory above has been removed from the search path, which I
        # find odd - as that's where the files currently are on a default
        # Raspbian image. This may need updating if the files have moved
        # in future updates to the system libcamera package
        return cam.load_tuning_file(fname, dir=dir)

def generate_bad_tuning():
    default_tuning = load_default_tuning()
    bad_tuning = default_tuning.copy()
    bad_tuning["version"] = 999
    return bad_tuning

def _test_bad_tuning_after_good_tuning(configure):
    bad_tuning = generate_bad_tuning()
    default_tuning = load_default_tuning()
    print("opening camera with explicitly specified tuning")
    with Picamera2(tuning=default_tuning) as cam:
        if configure:
            cam.configure(cam.create_preview_configuration())
    del cam
    libcamera.CameraManager.singleton().restart()
    with pytest.raises(Exception):
        # The bad version should cause a problem
        with Picamera2(tuning=bad_tuning):
            print("Success (not expected)!")

def test_bad_tuning_after_good_tuning_noconfigure():
    _test_bad_tuning_after_good_tuning(False)

def test_bad_tuning_after_good_tuning_configure():
    _test_bad_tuning_after_good_tuning(True)

Thanks so much for your quick response!

rwb27 commented 11 months ago

I would be delighted to submit my reset() function to libcamera, but I'm not 100% sure the most appropriate way to do it - I am sure that my fork of pylibcamera is not the right way, but I think the actual libcamera repository is at linuxtv.org so a PR to the Raspberry Pi fork might not be helpful?

rwb27 commented 11 months ago

@davidplowman I'm not quite sure how I managed to miss your post - adding that work-around works just as well as mine, but with the substantial advantage that I don't need to change anything in libcamera. Oh, how I wish I'd seen your message before spending hours messing about with a C++ library I'm not familiar with!

I have done a little more digging to figure out if I can spot a problem. I think the issue is that there's a reference from an object in the C extension, because the reference count reported by sys.getrefcount doesn't match the list of objects returned by gc.get_referrers. Adding that to my test script, I get:

However, if I add a configure() in the middle, the numbers change:

If I go round the cycle again, I acquire another two references - i.e. an extra 2 references are held somewhere every time we make a Picamera2 object and call configure. Calling configure multiple times doesn't seem to add references.

My workaround (the restart function in PyCameraManager) produces the desired reload, but the reference count goes up by 2 each time, which is worrying. Your workaround doesn't seem to gain references, so I'm going to use it in preference.

I'm not quite sure why your work-around works: as far as I can tell, the reference held by the CameraManager object is correctly released when the camera is closed, because that object does show up in gc.get_referrers. However, it does work, so clearly something I don't yet understand is happening, and my hunch is that a reference is perhaps being held by an object in the C extension?

I actually get the same effect if I only use gc.collect() in between deleting and re-creating the camera object, recreating the CameraManager doesn't seem to help.

My problem is fixed for now, so I'm going to stop investigating - but many thanks for your help! If you're interested, this is part of my upgrade of the OpenFlexure Microscope, to use the latest OS and camera stack. The lens shading correction is a big part of its appeal, so I'm happy this will keep it working. I look forward to a neater way of doing it in the future, whenever that may be!

rwb27 commented 11 months ago

Just for the record, since closing this I've actually implemented both work-arounds in the OpenFlexure Microscope software (specifically https://github.com/rwb27/labthings-picamera2). I have learned: