| Message ID | 20190127005206.20901-1-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, Thank you for the patch. 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@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). > + > media_->release(); > return false; > }
Hi Laurent, Thanks for your feedback. On 2019-01-27 18:04:26 +0200, Laurent Pinchart wrote: > Hi Niklas, > > Thank you for the patch. > > 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@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? > > > + > > media_->release(); > > return false; > > } > > -- > Regards, > > Laurent Pinchart
Hi Niklas, 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@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. > >> + > >> media_->release(); > >> return false; > >> }
Heya, On 27/01/2019 20:21, Laurent Pinchart wrote: > Hi Niklas, > > 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@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. >>>> + >>>> media_->release(); >>>> return false; >>>> } >
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@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; > >>>> }
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"; + media_->release(); return false; }
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@ragnatech.se> --- src/libcamera/pipeline/uvcvideo.cpp | 6 ++++++ 1 file changed, 6 insertions(+)