[libcamera-devel] [PATCH v4 1/6] libcamera: CameraManager: Drop the vector of created PipelineHandlers
Umang Jain
email at uajain.com
Mon Jun 15 17:02:23 CEST 2020
Hi Kieran,
On 6/15/20 7:31 PM, Kieran Bingham wrote:
> 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.
hmm, cameras_ is a vector of weak_ptr references, right? According to
the concepts I have
in my head (:D), pipeline doesn't _really_ hold a (strong)reference
through this vector.
Hence, isn't moving from cameras_ to temporary container merely to avoid
.clear() call below?
>
> 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.
Yes, but according to docs online, .clear() also tries to destroy the
objects in the vector
and I am under the impression that, since they are weak-references here,
they are
_not_ supposed to be destroyed(or decrement the refcount) the objects.
(that should happen
automatically when refcount drops to 0).
>
>
>
>>> }
>>> /**
>> 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://u15657259.ct.sendgrid.net/ls/click?upn=8H1KCc2bev8KdIveckpOEBeWjI3THEr-2F8W-2FrEpvXj1fUaD8nZSgfyCwFn-2BKX4QPmXiU9H2IGCLkMp0Cw1NTkpQ-3D-3DDJ-N_C3wFy2Q4UgRsRLDAYieRZ5Z3EhAWyy0-2FkOzyYc6FPc1dn6ROcAJqKXb9hjP566uPJMdELFe3GohTLWwC0myBlrYgTXKbgNWKhCFl6ePGYmSF4nYd6A9-2BWivkIa1mO-2BS8MCKqKHfu-2BFzZmuiY-2FqbM7xe-2FL9w6rjernZd59Uz-2Bc8ddTJ6ISK6R0qrMzttzQX9ED9I-2F9ipg1ivJPnxtOFZW07b2-2Ft3Yxf6CJbdHCKcJ2u3NFuupig2e61AhqFLdvMTA
More information about the libcamera-devel
mailing list