[libcamera-devel] [PATCH] libcamera: pipeline: uvcvideo: add warning if no default video device is found

[Laurent Pinchart]

Hi Kieran,

On Mon, Jan 28, 2019 at 09:21:30AM +0000, Kieran Bingham wrote:
> On 27/01/2019 20:21, Laurent Pinchart wrote:
> > On Sun, Jan 27, 2019 at 05:14:52PM +0100, Niklas Söderlund wrote:
> >> On 2019-01-27 18:04:26 +0200, Laurent Pinchart wrote:
> >>> On Sun, Jan 27, 2019 at 01:52:06AM +0100, Niklas Söderlund wrote:
> >>>> If for any reason a default video device is not found in the media graph
> >>>> the creation of the UVC pipeline silently failed. This is not optimal
> >>>> when debugging problems with the pipeline, add a warning to notify the
> >>>> user of the issue.
> >>>>
> >>>> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> >>>> ---
> >>>>  src/libcamera/pipeline/uvcvideo.cpp | 6 ++++++
> >>>>  1 file changed, 6 insertions(+)
> >>>>
> >>>> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> >>>> index c51e8bc1f2c2bf25..4ae179d24709c856 100644
> >>>> --- a/src/libcamera/pipeline/uvcvideo.cpp
> >>>> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> >>>> @@ -8,12 +8,15 @@
> >>>>  #include <libcamera/camera.h>
> >>>>  
> >>>>  #include "device_enumerator.h"
> >>>> +#include "log.h"
> >>>>  #include "media_device.h"
> >>>>  #include "pipeline_handler.h"
> >>>>  #include "v4l2_device.h"
> >>>>  
> >>>>  namespace libcamera {
> >>>>  
> >>>> +LOG_DEFINE_CATEGORY(UVC)
> >>>> +
> >>>>  class PipelineHandlerUVC : public PipelineHandler
> >>>>  {
> >>>>  public:
> >>>> @@ -60,6 +63,9 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
> >>>>  	}
> >>>>  
> >>>>  	if (!video_ || video_->open()) {
> >>>> +		if (!video_)
> >>>> +			LOG(UVC, Warning) << "Could not find a default video device";
> >>>
> >>> I'd make it an error, as it's quite fatal. I think we should also log a
> >>> message in the video_->open() failure case, as that's equally fatal (the
> >>> permissions denied case is especially important).
> >>
> >> I will make it an error.
> >>
> >> Regarding logging the failure of video_->open() this already happens in 
> >> V4L2Device::open(). I'm open to a discussion of adding logging in each 
> >> pipeline handler for the open call, my initial position is however that 
> >> it would add little of value as such an error is already logged. What is 
> >> the rest of the groups view?
> > 
> > I agree with you, it's best to handle as much as possible in the
> > libcamera core to minimize the potential issues in the pipeline
> > handlers.
> 
> Yes, failures and errors should be reported as early as possible, and as
> close to the root component as possible.
> 
> In this case - at the point of the Open call.
> 
> It does however present an possible issue with filtering.
> If an error occurs - and someone sets something like:
> 
> LIBCAMERA_LOG_LEVELS=pipeline:Debug
> 
> - (and nothing else) Would that then /hide/ the V4L2 errors? (or does
> each category stay at it's default severity level unless explicitly changed)
> 
> As long as the V4L2 category remains unchanged, and will still print
> then that's fine.

Categories not explicitly specified keep their default log level, so
it's fine.

> >>>> +
> >>>>  		media_->release();
> >>>>  		return false;
> >>>>  	}

-- 
Regards,

Laurent Pinchart