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

[Kieran Bingham]

Hi Laurent,

On 20/09/2020 14:40, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Mon, Aug 24, 2020 at 11:39:00AM +0100, Kieran Bingham wrote:
>> On 23/08/2020 21:43, Laurent Pinchart wrote:
>>> 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.

Anytime, when all other references are released ;-)

>>>>> + */
>>>>> + self_.reset();
>>>>> }
>>
>> On initial glances, that sounds like a neat trick!, as long as the
>> open/close calls are symmetrical (or rather unique).

Which is fine, because this can only be reached when we successfully
call camera_->acquire().


>>
>>>>>
>>>>> 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.
> 
> We probably could, but as software format conversion is in the pipe, I'm
> not sure it makes much sense to invest lots of energy in this. I can
> keep the patch in my tree for now (unless you think review is enough to
> upstream it).


I loathe putting in something completely untested, but looking through
this it sounds good (and factually correct, the object takes a reference
to itself while the device is open, because it's being used while it's
open).


If you're confident that it's fine beyond a compile-check, then
integrate it.

Code review wise, I'm happy:

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>


-- 
Regards
--
Kieran