| Message ID | 20200823182900.18247-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | New |
| Delegated to: | Laurent Pinchart |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, On Sun, Aug 23, 2020 at 08:03:52PM +0000, email@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@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(); > > } > > > > 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.
Hi Laurent, On 23/08/2020 21:43, Laurent Pinchart wrote: > Hi Umang, > > On Sun, Aug 23, 2020 at 08:03:52PM +0000, email@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@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.
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@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@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. 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).
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@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@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@ideasonboard.com>
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(); } 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_;
CameraDevice instances are managed through shared pointers to support hptplug. No reference is however taken on 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@ideasonboard.com> --- src/android/camera_device.cpp | 12 ++++++++++++ src/android/camera_device.h | 4 +++- 2 files changed, 15 insertions(+), 1 deletion(-)