[libcamera-devel] [PATCH] android: camera_device: Hold reference to self when open

[Kieran Bingham]

Hi Laurent,

On 23/08/2020 21:43, Laurent Pinchart wrote:
> Hi Umang,
> 
> On Sun, Aug 23, 2020 at 08:03:52PM +0000, email at uajain.com wrote:
>> On Aug 23, 2020 11:59 PM, Laurent Pinchart wrote:
>>> CameraDevice instances are managed through shared pointers to support
>>> hptplug. No reference is however taken on
>>
>> s/hptplug/hotplug/
>>
>>> the device when opened by the
>>> camera service, which could cause an open device to be deleted if
>>> unplugged, leading to a crash when the device is next accessed.
>>>
>>> Fix this by taking a reference when opening a device, and releasing it
>>> at close time. The reference is stored in the CameraDevice instance
>>> itself, which is more efficient than storing it in a container in the
>>> CameraHalManager as that would require lookup operations.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>> ---
>>> src/android/camera_device.cpp | 12 ++++++++++++
>>> src/android/camera_device.h   |  4 +++-
>>> 2 files changed, 15 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>>> index de6f86f7cb9b..52468be70781 100644
>>> --- a/src/android/camera_device.cpp
>>> +++ b/src/android/camera_device.cpp
>>> @@ -459,6 +459,12 @@ int CameraDevice::open(const hw_module_t
>>> *hardwareModule)
>>> camera3Device_.ops = &hal_dev_ops;
>>> camera3Device_.priv = this;
>>>
>>> + /*
>>> + * Hold a reference to ourselves, to protect against deletion if the
>>> + * camera is unplugged before being closed.
>>> + */
>>> + self_ = shared_from_this();
>>> +
>>> return 0;
>>> }
>>>
>>> @@ -468,6 +474,12 @@ void CameraDevice::close()
>>> camera_->release();
>>>
>>> running_ = false;
>>> +
>>> + /*
>>> + * Drop our self reference. From this point the CameraDevice may get
>>> + * deleted at any time.
>>> + */
>>> + self_.reset();
>>> }

On initial glances, that sounds like a neat trick!, as long as the
open/close calls are symmetrical (or rather unique).

>>>
>>> void CameraDevice::setCallbacks(const camera3_callback_ops_t *callbacks)
>>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
>>> index 3934f194f1b5..fd991c76b90d 100644
>>> --- a/src/android/camera_device.h
>>> +++ b/src/android/camera_device.h
>>> @@ -44,7 +44,8 @@ struct CameraStream {
>>> Encoder *jpeg;
>>> };
>>>
>>> -class CameraDevice : protected libcamera::Loggable
>>> +class CameraDevice : public std::enable_shared_from_this<CameraDevice>,
>>> +      protected libcamera::Loggable
>>> {
>>> public:
>>> static std::shared_ptr<CameraDevice> create(unsigned int id,
>>> @@ -104,6 +105,7 @@ private:
>>>
>>> unsigned int id_;
>>> camera3_device_t camera3Device_;
>>> + std::shared_ptr<CameraDevice> self_;
>>>
>>> bool running_;
>>> std::shared_ptr<libcamera::Camera> camera_;
>>
>> Is this the patch after testing behaviour of a stream-able camera(probably non-
>> UVC) on CrOS with master? 
> 
> No it hasn't been tested. Sorry for not sending a cover letter to
> explain this. I should also have marked this as RFC.
> 
>> I am not sure what this fix really achieves, part of the problem is because of
>> my build/test setup :(. I believe right now, the cros_camera_service exits
>> ("sledge hammer") when a ongoing streaming camera is unplugged, hence I am not
>> able to follow if ::Close() is called anywhere in hot unplug path. By that
>> extension, I am not sure if at all we are going to drop the manual ref we take,
>> as per this patch.
> 
> The sledge hammer is triggered in the Chrome OS UVC HAL, in
> CameraHal::OnDeviceRemoved(). The Chrome OS camera service itself
> doesn't exit or abort explicitly when an open camera is disconnected,
> but that code path has probably received little testing.
> 
> In order to test this we need to support software format conversion in
> the HAL to make UVC cameras usable on Chrome OS.

I still wonder if we can quickly 'fake' this by creating a mapping for
the YUV or MJPEG formats on a camera to the NV12 format used by
AndroidHAL - the image wouldn't be visible, but as long as the camera
only writes smaller buffers (hence thinking the MJPEG might be better)
it could be a means to testing the hotplug functionality.


-- 
Regards
--
Kieran