[libcamera-devel] [PATCH v4 1/6] libcamera: CameraManager: Drop the vector of created PipelineHandlers
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Jun 15 16:01:51 CEST 2020
Hi Umang,
On 15/06/2020 14:40, Umang Jain wrote:
> Hi Laurent,
>
> On 6/12/20 3:31 PM, Laurent Pinchart wrote:
>> Hi Umang,
>>
>> Thank you for the patch.
>>
>> On Thu, Jun 11, 2020 at 05:16:02PM +0000, Umang Jain wrote:
>>> This placeholder for the pipeline-handlers created by CameraManager,
>>> was responsible to hold a reference to pipeline-handlers which were
>>> not getting dropped anywhere in the code-path. Instead of introducing
>>> a fix of decrementing the reference, it is decided to eliminate this
>>> vector entirely from the CameraManager. This is due to the fact that
>>> the vector does not seem to have much use in CameraManager and thus
>>> reducing futile book-keeping.
>> I think the change is good, but "does not seem to have much use" sounds
>> like you're not really sure :-)
>>
>> The pipes_ vector has been there pretty much since day 1, and served the
>> purpose of storing pipeline handler instances when they were not yet
>> referenced by the Camera class. This was used to retrieve cameras, and
>> to delete pipeline handler instances when stopping the camera manager.
>> In
>>
>> commit f3695e9b09ce4a88d6e0480d9e5058673af34a49
>> Author: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>> Date: Thu Jan 3 13:10:37 2019 +0200
>>
>> libcamera: camera_manager: Register cameras with the camera manager
>>
>> cameras were registered directly with the camera manager, and the pipes_
>> vector was only used to delete pipeline handler instances. In
>>
>> commit 5b02e03199b79165086135236d8fb9b2c673aae1
>> Author: Niklas Söderlund <niklas.soderlund at ragnatech.se>
>> Date: Tue Jan 22 16:31:39 2019 +0100
>>
>> libcamera: camera: Associate cameras with their pipeline handler
>>
>> the pipeline handler instances got stored in a shared_ptr, referenced
>> from both the pipes_ vector and the Camera class. The explicit delete
>> was removed, and pipes_ was simply cleared instead. We could have
>> removed the pipes_ vector at that point, as pipeline handlers were
>> referenced from the Camera class.
>>
>> It would be nice to summarize this in the commit message to explain why
>> the pipes_ vector isn't needed anymore.
>>
>>> Since the vector is eliminated, there are no unchecked reference of the
>>> pipeline-handlers. This fixes the initial issue of dangling driver
>>> directories
>>> (for e.g. for UVC camera - in /sys/bus/usb/drivers/uvcvideo) on
>>> 'unbind' → 'bind'
>>> operation while the CameraManager is running. The directories were
>>> still kept
>>> around even after 'unbind' because of the pipeline-handler's
>>> unchecked reference
>>> holding onto them.
>> Have you tried to run the hotplug test under valgrind ? There are lots
>> of use-after-free issues with this patch applied:
>>
>> ==18826== Thread 2:
>> ==18826== Invalid read of size 8
>> ==18826== at 0x4A5A06C:
>> std::__1::vector<std::__1::weak_ptr<libcamera::Camera>,
>> std::__1::allocator<std::__1::weak_ptr<libcamera::Camera> > >::size()
>> const (vector:656)
>> ==18826== by 0x4A59778:
>> std::__1::vector<std::__1::weak_ptr<libcamera::Camera>,
>> std::__1::allocator<std::__1::weak_ptr<libcamera::Camera> > >::clear()
>> (vector:771)
>> ==18826== by 0x4A5887D: libcamera::PipelineHandler::disconnect()
>> (pipeline_handler.cpp:571)
>> ==18826== by 0x4A586C6:
>> libcamera::PipelineHandler::mediaDeviceDisconnected(libcamera::MediaDevice*)
>> (pipeline_handler.cpp:545)
>> ==18826== by 0x4A5EC3D:
>> libcamera::BoundMethodMember<libcamera::PipelineHandler, void,
>> libcamera::MediaDevice*>::invoke(libcamera::MediaDevice*)
>> (bound_method.h:198)
>> ==18826== by 0x4A5ECC4: void libcamera::BoundMethodArgs<void,
>> libcamera::MediaDevice*>::invokePack<0ul>(libcamera::BoundMethodPackBase*,
>> std::__1::integer_sequence<unsigned long, 0ul>) (bound_method.h:124)
>> ==18826== by 0x4A5EA9C: libcamera::BoundMethodArgs<void,
>> libcamera::MediaDevice*>::invokePack(libcamera::BoundMethodPackBase*)
>> (bound_method.h:133)
>> ==18826== by 0x49ABB26:
>> libcamera::BoundMethodBase::activatePack(std::__1::shared_ptr<libcamera::BoundMethodPackBase>,
>> bool) (bound_method.cpp:85)
>> ==18826== by 0x4A5EB85:
>> libcamera::BoundMethodMember<libcamera::PipelineHandler, void,
>> libcamera::MediaDevice*>::activate(libcamera::MediaDevice*, bool)
>> (bound_method.h:193)
>> ==18826== by 0x4A0F600:
>> libcamera::Signal<libcamera::MediaDevice*>::emit(libcamera::MediaDevice*)
>> (signal.h:127)
>> ==18826== by 0x4A0E589:
>> libcamera::DeviceEnumerator::removeDevice(std::__1::basic_string<char,
>> std::__1::char_traits<char>, std::__1::allocator<char> > const&)
>> (device_enumerator.cpp:290)
>> ==18826== by 0x4B0C5C1:
>> libcamera::DeviceEnumeratorUdev::udevNotify(libcamera::EventNotifier*)
>> (device_enumerator_udev.cpp:344)
>> ==18826== Address 0x5bd0af8 is 136 bytes inside a block of size 184
>> free'd
>> ==18826== at 0x48379CB: free (vg_replace_malloc.c:540)
>> ==18826== by 0x4AFD391:
>> libcamera::PipelineHandlerUVC::~PipelineHandlerUVC() (uvcvideo.cpp:61)
>> ==18826== by 0x4A61E9E:
>> std::__1::default_delete<libcamera::PipelineHandler>::operator()(libcamera::PipelineHandler*)
>> const (memory:2363)
>> ==18826== by 0x4A61C3F:
>> std::__1::__shared_ptr_pointer<libcamera::PipelineHandler*,
>> std::__1::default_delete<libcamera::PipelineHandler>,
>> std::__1::allocator<libcamera::PipelineHandler> >::__on_zero_shared()
>> (memory:3536)
>> ==18826== by 0x49AC479:
>> std::__1::__shared_count::__release_shared() (memory:3440)
>> ==18826== by 0x49AC41E:
>> std::__1::__shared_weak_count::__release_shared() (memory:3482)
>> ==18826== by 0x49B35DB:
>> std::__1::shared_ptr<libcamera::PipelineHandler>::~shared_ptr()
>> (memory:4207)
>> ==18826== by 0x49B0982: libcamera::Camera::Private::~Private()
>> (camera.cpp:300)
>> ==18826== by 0x49C213A:
>> std::__1::default_delete<libcamera::Camera::Private>::operator()(libcamera::Camera::Private*)
>> const (memory:2363)
>> ==18826== by 0x49C20BE:
>> std::__1::unique_ptr<libcamera::Camera::Private,
>> std::__1::default_delete<libcamera::Camera::Private>
>> >::reset(libcamera::Camera::Private*) (memory:2618)
>> ==18826== by 0x49B3878:
>> std::__1::unique_ptr<libcamera::Camera::Private,
>> std::__1::default_delete<libcamera::Camera::Private> >::~unique_ptr()
>> (memory:2572)
>> ==18826== by 0x49B12D2: libcamera::Camera::~Camera() (camera.cpp:517)
>> ==18826== Block was alloc'd at
>> ==18826== at 0x483679F: malloc (vg_replace_malloc.c:309)
>> ==18826== by 0x4D0F76B: operator new(unsigned long) (in
>> /usr/lib64/libc++.so.1.0)
>> ==18826== by 0x4AFD409:
>> libcamera::PipelineHandlerUVCFactory::createInstance(libcamera::CameraManager*)
>> (uvcvideo.cpp:562)
>> ==18826== by 0x4A58A4F:
>> libcamera::PipelineHandlerFactory::create(libcamera::CameraManager*)
>> (pipeline_handler.cpp:640)
>> ==18826== by 0x49C9A83:
>> libcamera::CameraManager::Private::createPipelineHandlers()
>> (camera_manager.cpp:148)
>> ==18826== by 0x49C9952: libcamera::CameraManager::Private::init()
>> (camera_manager.cpp:127)
>> ==18826== by 0x49C97F2: libcamera::CameraManager::Private::run()
>> (camera_manager.cpp:104)
>> ==18826== by 0x4A71CDC: libcamera::Thread::startThread()
>> (thread.cpp:288)
>> ==18826== by 0x4A76520:
>> _ZNSt3__18__invokeIMN9libcamera6ThreadEFvvEPS2_JEvEEDTcldsdeclsr3std3__1E7forwardIT0_Efp0_Efp_spclsr3std3__1E7forwardIT1_Efp1_EEEOT_OS6_DpOS7_
>> (type_traits:3480)
>> ==18826== by 0x4A7642D: void
>> std::__1::__thread_execute<std::__1::unique_ptr<std::__1::__thread_struct,
>> std::__1::default_delete<std::__1::__thread_struct> >, void
>> (libcamera::Thread::*)(), libcamera::Thread*,
>> 2ul>(std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct,
>> std::__1::default_delete<std::__1::__thread_struct> >, void
>> (libcamera::Thread::*)(), libcamera::Thread*>&,
>> std::__1::__tuple_indices<2ul>) (thread:273)
>> ==18826== by 0x4A75D95: void*
>> std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct,
>> std::__1::default_delete<std::__1::__thread_struct> >, void
>> (libcamera::Thread::*)(), libcamera::Thread*> >(void*) (thread:284)
>> ==18826== by 0x52B8F3F: start_thread (pthread_create.c:479)
>>
>> The change below should fix this. I'll let you add a comment at the
>> beginning of PipelineHandler::disconnect() to explain what is going on
>> :-)
>
> I will go with something like the following:
>
> + /* Since cameras_ holds a weak reference of the cameras,
> + * we just need to empty the vector here instead of
> cameras_.clear() it.
> + */
I'm not sure that's quite describing what's happening.
>> diff --git a/src/libcamera/pipeline_handler.cpp
>> b/src/libcamera/pipeline_handler.cpp
>> index bad79dcb6219..8358266d83ff 100644
>> --- a/src/libcamera/pipeline_handler.cpp
>> +++ b/src/libcamera/pipeline_handler.cpp
>> @@ -559,7 +559,9 @@ void
>> PipelineHandler::mediaDeviceDisconnected(MediaDevice *media)
>> */
>> void PipelineHandler::disconnect()
>> {
>> - for (std::weak_ptr<Camera> ptr : cameras_) {
>> + std::vector<std::weak_ptr<Camera>> cameras{ std::move(cameras_) };
This moves all cameras out of the cameras_ vector - into a new
'temporary' holding container.
Moving them into the new container, ensures that the references are kept
constant, but they are no longer 'held' by the pipeline.
I haven't checked, but I presume that this means below in the loop, the
code that was unable to remove something because a reference was held -
is now able to - because essentially the cameras_ object has been
cleared before entering the loop - instead of after.
>> +
>> + for (std::weak_ptr<Camera> ptr : cameras) {
>> std::shared_ptr<Camera> camera = ptr.lock();
>> if (!camera)
>> continue;
>> @@ -567,8 +569,6 @@ void PipelineHandler::disconnect()
>> camera->disconnect();
>> manager_->removeCamera(camera);
>> }
>> -
>> - cameras_.clear();
And because the cameras vector above was created with local scope - all
of it's contents get cleared at this point now automatically.
>> }
>> /**
>
> Quick question: You mentioned on IRC that this can be folded into a patch.
>
> Did you mean squash it into existing patch or treat this as a separate
> patch?
That usually means squash it into the existing relevant (this current?)
patch.
>>
>>> Signed-off-by: Umang Jain <email at uajain.com>
>>> ---
>>> src/libcamera/camera_manager.cpp | 3 ---
>>> 1 file changed, 3 deletions(-)
>>>
>>> diff --git a/src/libcamera/camera_manager.cpp
>>> b/src/libcamera/camera_manager.cpp
>>> index 576856a..b8128df 100644
>>> --- a/src/libcamera/camera_manager.cpp
>>> +++ b/src/libcamera/camera_manager.cpp
>>> @@ -63,7 +63,6 @@ private:
>>> bool initialized_;
>>> int status_;
>>> - std::vector<std::shared_ptr<PipelineHandler>> pipes_;
>>> std::unique_ptr<DeviceEnumerator> enumerator_;
>>> IPAManager ipaManager_;
>>> @@ -144,7 +143,6 @@ int CameraManager::Private::init()
>>> LOG(Camera, Debug)
>>> << "Pipeline handler \"" << factory->name()
>>> << "\" matched";
>>> - pipes_.push_back(std::move(pipe));
>>> }
>>> }
>>> @@ -162,7 +160,6 @@ void CameraManager::Private::cleanup()
>>> * they all get destroyed before the device enumerator deletes the
>>> * media devices.
>>> */
>> The above comment needs to be updated.
>>
>> - * Release all references to cameras and pipeline handlers to ensure
>> - * they all get destroyed before the device enumerator deletes the
>> - * media devices.
>> + * Release all references to cameras to ensure they all get
>> destroyed
>> + * before the device enumerator deletes the media devices.
>>
>>> - pipes_.clear();
>>> cameras_.clear();
>>> enumerator_.reset(nullptr);
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list