| Message ID | 20220721103020.1066512-1-umang.jain@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, On Thu, Jul 21, 2022 at 04:00:20PM +0530, Umang Jain via libcamera-devel wrote: > Before running or setting up the pipeline, check for cameras availablity > first. If no cameras are available, skip the test. > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> Is this to fix the recurring failing test on rpi ? Nit: could you add a Fixes tag ? I also used Reported-by: with a link to a failing build from buildbot. Not that we use it now, but it's nice for statistics ? > --- > test/gstreamer/gstreamer_single_stream_test.cpp | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp > index a0dd12cf..1e0801e8 100644 > --- a/test/gstreamer/gstreamer_single_stream_test.cpp > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp > @@ -8,6 +8,8 @@ > #include <iostream> > #include <unistd.h> > > +#include <libcamera/libcamera.h> > + > #include <gst/gst.h> > > #include "gstreamer_test.h" > @@ -29,6 +31,17 @@ protected: > if (status_ != TestPass) > return status_; > > + libcamera::CameraManager cm; > + cm.start(); > + > + bool cameraFound = cm.cameras().size() > 1 ? true : false; > + if (!cameraFound) { > + cm.stop(); > + return TestSkip; > + } > + > + cm.stop(); > + The multistream test has a very similar: /* Check if platform supports multistream capture */ libcamera::CameraManager cm; cm.start(); bool cameraFound = false; for (auto &camera : cm.cameras()) { if (camera->streams().size() > 1) { cameraName_ = camera->id(); cameraFound = true; cm.stop(); break; } } if (!cameraFound) { cm.stop(); return TestSkip; } Is this something that should go in the base class ? > const gchar *streamDescription = "videoconvert ! fakesink"; > g_autoptr(GError) error0 = NULL; > stream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE, > -- > 2.31.1 >
Hi Jacopo, On 7/21/22 16:16, Jacopo Mondi wrote: > Hi Umang, > > On Thu, Jul 21, 2022 at 04:00:20PM +0530, Umang Jain via libcamera-devel wrote: >> Before running or setting up the pipeline, check for cameras availablity >> first. If no cameras are available, skip the test. >> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > Is this to fix the recurring failing test on rpi ? Yes, I tested on Rpi that no cameras will fail the test in the same way as https://buildbot.libcamera.org/#/builders/16/builds/168 Having a camera passes the test, so the test assumes that a camera is present on the system. > > Nit: could you add a Fixes tag ? I also used Reported-by: with a link > to a failing build from buildbot. Not that we use it now, but it's > nice for statistics ? > >> --- >> test/gstreamer/gstreamer_single_stream_test.cpp | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp >> index a0dd12cf..1e0801e8 100644 >> --- a/test/gstreamer/gstreamer_single_stream_test.cpp >> +++ b/test/gstreamer/gstreamer_single_stream_test.cpp >> @@ -8,6 +8,8 @@ >> #include <iostream> >> #include <unistd.h> >> >> +#include <libcamera/libcamera.h> >> + >> #include <gst/gst.h> >> >> #include "gstreamer_test.h" >> @@ -29,6 +31,17 @@ protected: >> if (status_ != TestPass) >> return status_; >> >> + libcamera::CameraManager cm; >> + cm.start(); >> + >> + bool cameraFound = cm.cameras().size() > 1 ? true : false; >> + if (!cameraFound) { >> + cm.stop(); >> + return TestSkip; >> + } >> + >> + cm.stop(); >> + > The multistream test has a very similar: > > /* Check if platform supports multistream capture */ > libcamera::CameraManager cm; > cm.start(); > bool cameraFound = false; > for (auto &camera : cm.cameras()) { > if (camera->streams().size() > 1) { > cameraName_ = camera->id(); > cameraFound = true; > cm.stop(); > break; > } > } > > if (!cameraFound) { > cm.stop(); > return TestSkip; > } > > Is this something that should go in the base class ? I did contemplate putting in the base class itself. That approach might end up with multiple cameraManager's start()/stop() cycles (which I think is Okay?). For e.g. in the multistream test, the base class would start() -> find cameras ->stop() and the multistream class would start() -> finding cameras with multistream support -> stop(). Then the actual test starts with libcamerasrc element. So there's additional one cycle in this case. whereas single stream test wouldn't need to have any start() -> stop() sequence as base class would handle it. Probably it's worth to do it in the base class ... Let me see and get back to you >> const gchar *streamDescription = "videoconvert ! fakesink"; >> g_autoptr(GError) error0 = NULL; >> stream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE, >> -- >> 2.31.1 >>
Hi Umang On Thu, Jul 21, 2022 at 05:04:24PM +0530, Umang Jain wrote: > Hi Jacopo, > > On 7/21/22 16:16, Jacopo Mondi wrote: > > Hi Umang, > > > > On Thu, Jul 21, 2022 at 04:00:20PM +0530, Umang Jain via libcamera-devel wrote: > > > Before running or setting up the pipeline, check for cameras availablity > > > first. If no cameras are available, skip the test. > > > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > Is this to fix the recurring failing test on rpi ? > > > Yes, I tested on Rpi that no cameras will fail the test in the same way as > https://buildbot.libcamera.org/#/builders/16/builds/168 > > Having a camera passes the test, so the test assumes that a camera is > present on the system. > > > > > Nit: could you add a Fixes tag ? I also used Reported-by: with a link > > to a failing build from buildbot. Not that we use it now, but it's > > nice for statistics ? > > > > > --- > > > test/gstreamer/gstreamer_single_stream_test.cpp | 13 +++++++++++++ > > > 1 file changed, 13 insertions(+) > > > > > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp > > > index a0dd12cf..1e0801e8 100644 > > > --- a/test/gstreamer/gstreamer_single_stream_test.cpp > > > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp > > > @@ -8,6 +8,8 @@ > > > #include <iostream> > > > #include <unistd.h> > > > > > > +#include <libcamera/libcamera.h> > > > + > > > #include <gst/gst.h> > > > > > > #include "gstreamer_test.h" > > > @@ -29,6 +31,17 @@ protected: > > > if (status_ != TestPass) > > > return status_; > > > > > > + libcamera::CameraManager cm; > > > + cm.start(); > > > + > > > + bool cameraFound = cm.cameras().size() > 1 ? true : false; > > > + if (!cameraFound) { > > > + cm.stop(); > > > + return TestSkip; > > > + } > > > + > > > + cm.stop(); > > > + > > The multistream test has a very similar: > > > > /* Check if platform supports multistream capture */ > > libcamera::CameraManager cm; > > cm.start(); > > bool cameraFound = false; > > for (auto &camera : cm.cameras()) { > > if (camera->streams().size() > 1) { > > cameraName_ = camera->id(); > > cameraFound = true; > > cm.stop(); > > break; > > } > > } > > > > if (!cameraFound) { > > cm.stop(); > > return TestSkip; > > } > > > > Is this something that should go in the base class ? > > > I did contemplate putting in the base class itself. That approach might end > up with multiple cameraManager's start()/stop() cycles (which I think is > Okay?). > > For e.g. in the multistream test, the base class would start() -> find > cameras ->stop() and the multistream class would start() -> finding cameras > with multistream support -> stop(). Then the actual test starts with > libcamerasrc element. So there's additional one cycle in this case. > > whereas single stream test wouldn't need to have any start() -> stop() > sequence as base class would handle it. > Isn't it just a numStream parameter to the base class init() function ? > Probably it's worth to do it in the base class ... Let me see and get back > to you > > > > const gchar *streamDescription = "videoconvert ! fakesink"; > > > g_autoptr(GError) error0 = NULL; > > > stream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE, > > > -- > > > 2.31.1 > > >
On 7/21/22 17:09, Jacopo Mondi wrote: > Hi Umang > > On Thu, Jul 21, 2022 at 05:04:24PM +0530, Umang Jain wrote: >> Hi Jacopo, >> >> On 7/21/22 16:16, Jacopo Mondi wrote: >>> Hi Umang, >>> >>> On Thu, Jul 21, 2022 at 04:00:20PM +0530, Umang Jain via libcamera-devel wrote: >>>> Before running or setting up the pipeline, check for cameras availablity >>>> first. If no cameras are available, skip the test. >>>> >>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >>> Is this to fix the recurring failing test on rpi ? >> >> Yes, I tested on Rpi that no cameras will fail the test in the same way as >> https://buildbot.libcamera.org/#/builders/16/builds/168 >> >> Having a camera passes the test, so the test assumes that a camera is >> present on the system. >> >>> Nit: could you add a Fixes tag ? I also used Reported-by: with a link >>> to a failing build from buildbot. Not that we use it now, but it's >>> nice for statistics ? >>> >>>> --- >>>> test/gstreamer/gstreamer_single_stream_test.cpp | 13 +++++++++++++ >>>> 1 file changed, 13 insertions(+) >>>> >>>> diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp >>>> index a0dd12cf..1e0801e8 100644 >>>> --- a/test/gstreamer/gstreamer_single_stream_test.cpp >>>> +++ b/test/gstreamer/gstreamer_single_stream_test.cpp >>>> @@ -8,6 +8,8 @@ >>>> #include <iostream> >>>> #include <unistd.h> >>>> >>>> +#include <libcamera/libcamera.h> >>>> + >>>> #include <gst/gst.h> >>>> >>>> #include "gstreamer_test.h" >>>> @@ -29,6 +31,17 @@ protected: >>>> if (status_ != TestPass) >>>> return status_; >>>> >>>> + libcamera::CameraManager cm; >>>> + cm.start(); >>>> + >>>> + bool cameraFound = cm.cameras().size() > 1 ? true : false; >>>> + if (!cameraFound) { >>>> + cm.stop(); >>>> + return TestSkip; >>>> + } >>>> + >>>> + cm.stop(); >>>> + >>> The multistream test has a very similar: >>> >>> /* Check if platform supports multistream capture */ >>> libcamera::CameraManager cm; >>> cm.start(); >>> bool cameraFound = false; >>> for (auto &camera : cm.cameras()) { >>> if (camera->streams().size() > 1) { >>> cameraName_ = camera->id(); >>> cameraFound = true; >>> cm.stop(); >>> break; >>> } >>> } >>> >>> if (!cameraFound) { >>> cm.stop(); >>> return TestSkip; >>> } >>> >>> Is this something that should go in the base class ? >> >> I did contemplate putting in the base class itself. That approach might end >> up with multiple cameraManager's start()/stop() cycles (which I think is >> Okay?). >> >> For e.g. in the multistream test, the base class would start() -> find >> cameras ->stop() and the multistream class would start() -> finding cameras >> with multistream support -> stop(). Then the actual test starts with >> libcamerasrc element. So there's additional one cycle in this case. >> >> whereas single stream test wouldn't need to have any start() -> stop() >> sequence as base class would handle it. >> > Isn't it just a numStream parameter to the base class init() function ? Not sure what you mean, grepping numStreams denotes: [uajain@fedora libcamera]$ git grep -ni "numStream" src/libcamera/pipeline/simple/simple.cpp:198: unsigned int numStreams, src/libcamera/pipeline/simple/simple.cpp:350: unsigned int numStreams, src/libcamera/pipeline/simple/simple.cpp:352: : Camera::Private(pipe), streams_(numStreams) src/libcamera/pipeline/simple/simple.cpp:1266: unsigned int numStreams = 1; src/libcamera/pipeline/simple/simple.cpp:1284: numStreams = streams; src/libcamera/pipeline/simple/simple.cpp:1307: std::make_unique<SimpleCameraData>(this, numStreams, sensor); > >> Probably it's worth to do it in the base class ... Let me see and get back >> to you >> >>>> const gchar *streamDescription = "videoconvert ! fakesink"; >>>> g_autoptr(GError) error0 = NULL; >>>> stream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE, >>>> -- >>>> 2.31.1 >>>>
On Thu, Jul 21, 2022 at 05:43:31PM +0530, Umang Jain wrote: > > On 7/21/22 17:09, Jacopo Mondi wrote: > > Hi Umang > > > > On Thu, Jul 21, 2022 at 05:04:24PM +0530, Umang Jain wrote: > > > Hi Jacopo, > > > > > > On 7/21/22 16:16, Jacopo Mondi wrote: > > > > Hi Umang, > > > > > > > > On Thu, Jul 21, 2022 at 04:00:20PM +0530, Umang Jain via libcamera-devel wrote: > > > > > Before running or setting up the pipeline, check for cameras availablity > > > > > first. If no cameras are available, skip the test. > > > > > > > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > > > Is this to fix the recurring failing test on rpi ? > > > > > > Yes, I tested on Rpi that no cameras will fail the test in the same way as > > > https://buildbot.libcamera.org/#/builders/16/builds/168 > > > > > > Having a camera passes the test, so the test assumes that a camera is > > > present on the system. > > > > > > > Nit: could you add a Fixes tag ? I also used Reported-by: with a link > > > > to a failing build from buildbot. Not that we use it now, but it's > > > > nice for statistics ? > > > > > > > > > --- > > > > > test/gstreamer/gstreamer_single_stream_test.cpp | 13 +++++++++++++ > > > > > 1 file changed, 13 insertions(+) > > > > > > > > > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp > > > > > index a0dd12cf..1e0801e8 100644 > > > > > --- a/test/gstreamer/gstreamer_single_stream_test.cpp > > > > > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp > > > > > @@ -8,6 +8,8 @@ > > > > > #include <iostream> > > > > > #include <unistd.h> > > > > > > > > > > +#include <libcamera/libcamera.h> > > > > > + > > > > > #include <gst/gst.h> > > > > > > > > > > #include "gstreamer_test.h" > > > > > @@ -29,6 +31,17 @@ protected: > > > > > if (status_ != TestPass) > > > > > return status_; > > > > > > > > > > + libcamera::CameraManager cm; > > > > > + cm.start(); > > > > > + > > > > > + bool cameraFound = cm.cameras().size() > 1 ? true : false; > > > > > + if (!cameraFound) { > > > > > + cm.stop(); > > > > > + return TestSkip; > > > > > + } > > > > > + > > > > > + cm.stop(); > > > > > + > > > > The multistream test has a very similar: > > > > > > > > /* Check if platform supports multistream capture */ > > > > libcamera::CameraManager cm; > > > > cm.start(); > > > > bool cameraFound = false; > > > > for (auto &camera : cm.cameras()) { > > > > if (camera->streams().size() > 1) { > > > > cameraName_ = camera->id(); > > > > cameraFound = true; > > > > cm.stop(); > > > > break; > > > > } > > > > } > > > > > > > > if (!cameraFound) { > > > > cm.stop(); > > > > return TestSkip; > > > > } > > > > > > > > Is this something that should go in the base class ? > > > > > > I did contemplate putting in the base class itself. That approach might end > > > up with multiple cameraManager's start()/stop() cycles (which I think is > > > Okay?). > > > > > > For e.g. in the multistream test, the base class would start() -> find > > > cameras ->stop() and the multistream class would start() -> finding cameras > > > with multistream support -> stop(). Then the actual test starts with > > > libcamerasrc element. So there's additional one cycle in this case. > > > > > > whereas single stream test wouldn't need to have any start() -> stop() > > > sequence as base class would handle it. > > > > > Isn't it just a numStream parameter to the base class init() function ? > > > Not sure what you mean, grepping numStreams denotes: I mean that the desired number of streams to filter the cameras whith can be a paramter to a base class function. bool GstreamerTest::cameraInit(unsigned int numStreams) { /* Check if platform supports multistream capture */ libcamera::CameraManager cm; bool cameraFound = false; cm.start(); for (auto &camera : cm.cameras()) { if (camera->streams().size() < numStreams) continue; cameraFound = true; break; } cm.stop(); return cameraFound; } > > [uajain@fedora libcamera]$ git grep -ni "numStream" > src/libcamera/pipeline/simple/simple.cpp:198: unsigned int numStreams, > src/libcamera/pipeline/simple/simple.cpp:350: unsigned int numStreams, > src/libcamera/pipeline/simple/simple.cpp:352: : Camera::Private(pipe), > streams_(numStreams) > src/libcamera/pipeline/simple/simple.cpp:1266: unsigned int numStreams = 1; > src/libcamera/pipeline/simple/simple.cpp:1284: numStreams = streams; > src/libcamera/pipeline/simple/simple.cpp:1307: > std::make_unique<SimpleCameraData>(this, numStreams, sensor); > > > > > > Probably it's worth to do it in the base class ... Let me see and get back > > > to you > > > > > > > > const gchar *streamDescription = "videoconvert ! fakesink"; > > > > > g_autoptr(GError) error0 = NULL; > > > > > stream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE, > > > > > -- > > > > > 2.31.1 > > > > >
Hi Jacopo, On 7/21/22 18:02, Jacopo Mondi wrote: > On Thu, Jul 21, 2022 at 05:43:31PM +0530, Umang Jain wrote: >> On 7/21/22 17:09, Jacopo Mondi wrote: >>> Hi Umang >>> >>> On Thu, Jul 21, 2022 at 05:04:24PM +0530, Umang Jain wrote: >>>> Hi Jacopo, >>>> >>>> On 7/21/22 16:16, Jacopo Mondi wrote: >>>>> Hi Umang, >>>>> >>>>> On Thu, Jul 21, 2022 at 04:00:20PM +0530, Umang Jain via libcamera-devel wrote: >>>>>> Before running or setting up the pipeline, check for cameras availablity >>>>>> first. If no cameras are available, skip the test. >>>>>> >>>>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >>>>> Is this to fix the recurring failing test on rpi ? >>>> Yes, I tested on Rpi that no cameras will fail the test in the same way as >>>> https://buildbot.libcamera.org/#/builders/16/builds/168 >>>> >>>> Having a camera passes the test, so the test assumes that a camera is >>>> present on the system. >>>> >>>>> Nit: could you add a Fixes tag ? I also used Reported-by: with a link >>>>> to a failing build from buildbot. Not that we use it now, but it's >>>>> nice for statistics ? >>>>> >>>>>> --- >>>>>> test/gstreamer/gstreamer_single_stream_test.cpp | 13 +++++++++++++ >>>>>> 1 file changed, 13 insertions(+) >>>>>> >>>>>> diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp >>>>>> index a0dd12cf..1e0801e8 100644 >>>>>> --- a/test/gstreamer/gstreamer_single_stream_test.cpp >>>>>> +++ b/test/gstreamer/gstreamer_single_stream_test.cpp >>>>>> @@ -8,6 +8,8 @@ >>>>>> #include <iostream> >>>>>> #include <unistd.h> >>>>>> >>>>>> +#include <libcamera/libcamera.h> >>>>>> + >>>>>> #include <gst/gst.h> >>>>>> >>>>>> #include "gstreamer_test.h" >>>>>> @@ -29,6 +31,17 @@ protected: >>>>>> if (status_ != TestPass) >>>>>> return status_; >>>>>> >>>>>> + libcamera::CameraManager cm; >>>>>> + cm.start(); >>>>>> + >>>>>> + bool cameraFound = cm.cameras().size() > 1 ? true : false; >>>>>> + if (!cameraFound) { >>>>>> + cm.stop(); >>>>>> + return TestSkip; >>>>>> + } >>>>>> + >>>>>> + cm.stop(); >>>>>> + >>>>> The multistream test has a very similar: >>>>> >>>>> /* Check if platform supports multistream capture */ >>>>> libcamera::CameraManager cm; >>>>> cm.start(); >>>>> bool cameraFound = false; >>>>> for (auto &camera : cm.cameras()) { >>>>> if (camera->streams().size() > 1) { >>>>> cameraName_ = camera->id(); >>>>> cameraFound = true; >>>>> cm.stop(); >>>>> break; >>>>> } >>>>> } >>>>> >>>>> if (!cameraFound) { >>>>> cm.stop(); >>>>> return TestSkip; >>>>> } >>>>> >>>>> Is this something that should go in the base class ? >>>> I did contemplate putting in the base class itself. That approach might end >>>> up with multiple cameraManager's start()/stop() cycles (which I think is >>>> Okay?). >>>> >>>> For e.g. in the multistream test, the base class would start() -> find >>>> cameras ->stop() and the multistream class would start() -> finding cameras >>>> with multistream support -> stop(). Then the actual test starts with >>>> libcamerasrc element. So there's additional one cycle in this case. >>>> >>>> whereas single stream test wouldn't need to have any start() -> stop() >>>> sequence as base class would handle it. >>>> >>> Isn't it just a numStream parameter to the base class init() function ? >> >> Not sure what you mean, grepping numStreams denotes: > I mean that the desired number of streams to filter the cameras whith can > be a paramter to a base class function. > > bool GstreamerTest::cameraInit(unsigned int numStreams) > { > /* Check if platform supports multistream capture */ > libcamera::CameraManager cm; > bool cameraFound = false; > > cm.start(); > > for (auto &camera : cm.cameras()) { > if (camera->streams().size() < numStreams) > continue; > > cameraFound = true; > break; > } > > cm.stop(); > > return cameraFound; > } I expect someone saying this is like moving away from the "base" class. As we introduced some specifies here itself. I guess it should be fine given the test cases we have right now(single and multistream). Let me cook this up. Thanks for the suggestion. > >> [uajain@fedora libcamera]$ git grep -ni "numStream" >> src/libcamera/pipeline/simple/simple.cpp:198: unsigned int numStreams, >> src/libcamera/pipeline/simple/simple.cpp:350: unsigned int numStreams, >> src/libcamera/pipeline/simple/simple.cpp:352: : Camera::Private(pipe), >> streams_(numStreams) >> src/libcamera/pipeline/simple/simple.cpp:1266: unsigned int numStreams = 1; >> src/libcamera/pipeline/simple/simple.cpp:1284: numStreams = streams; >> src/libcamera/pipeline/simple/simple.cpp:1307: >> std::make_unique<SimpleCameraData>(this, numStreams, sensor); >> >>>> Probably it's worth to do it in the base class ... Let me see and get back >>>> to you >>>> >>>>>> const gchar *streamDescription = "videoconvert ! fakesink"; >>>>>> g_autoptr(GError) error0 = NULL; >>>>>> stream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE, >>>>>> -- >>>>>> 2.31.1 >>>>>>
Hi Umang On Thu, Jul 21, 2022 at 06:09:20PM +0530, Umang Jain wrote: > Hi Jacopo, > > On 7/21/22 18:02, Jacopo Mondi wrote: > > On Thu, Jul 21, 2022 at 05:43:31PM +0530, Umang Jain wrote: > > > On 7/21/22 17:09, Jacopo Mondi wrote: > > > > Hi Umang > > > > > > > > On Thu, Jul 21, 2022 at 05:04:24PM +0530, Umang Jain wrote: > > > > > Hi Jacopo, > > > > > > > > > > On 7/21/22 16:16, Jacopo Mondi wrote: > > > > > > Hi Umang, > > > > > > > > > > > > On Thu, Jul 21, 2022 at 04:00:20PM +0530, Umang Jain via libcamera-devel wrote: > > > > > > > Before running or setting up the pipeline, check for cameras availablity > > > > > > > first. If no cameras are available, skip the test. > > > > > > > > > > > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > > > > > Is this to fix the recurring failing test on rpi ? > > > > > Yes, I tested on Rpi that no cameras will fail the test in the same way as > > > > > https://buildbot.libcamera.org/#/builders/16/builds/168 > > > > > > > > > > Having a camera passes the test, so the test assumes that a camera is > > > > > present on the system. > > > > > > > > > > > Nit: could you add a Fixes tag ? I also used Reported-by: with a link > > > > > > to a failing build from buildbot. Not that we use it now, but it's > > > > > > nice for statistics ? > > > > > > > > > > > > > --- > > > > > > > test/gstreamer/gstreamer_single_stream_test.cpp | 13 +++++++++++++ > > > > > > > 1 file changed, 13 insertions(+) > > > > > > > > > > > > > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp > > > > > > > index a0dd12cf..1e0801e8 100644 > > > > > > > --- a/test/gstreamer/gstreamer_single_stream_test.cpp > > > > > > > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp > > > > > > > @@ -8,6 +8,8 @@ > > > > > > > #include <iostream> > > > > > > > #include <unistd.h> > > > > > > > > > > > > > > +#include <libcamera/libcamera.h> > > > > > > > + > > > > > > > #include <gst/gst.h> > > > > > > > > > > > > > > #include "gstreamer_test.h" > > > > > > > @@ -29,6 +31,17 @@ protected: > > > > > > > if (status_ != TestPass) > > > > > > > return status_; > > > > > > > > > > > > > > + libcamera::CameraManager cm; > > > > > > > + cm.start(); > > > > > > > + > > > > > > > + bool cameraFound = cm.cameras().size() > 1 ? true : false; > > > > > > > + if (!cameraFound) { > > > > > > > + cm.stop(); > > > > > > > + return TestSkip; > > > > > > > + } > > > > > > > + > > > > > > > + cm.stop(); > > > > > > > + > > > > > > The multistream test has a very similar: > > > > > > > > > > > > /* Check if platform supports multistream capture */ > > > > > > libcamera::CameraManager cm; > > > > > > cm.start(); > > > > > > bool cameraFound = false; > > > > > > for (auto &camera : cm.cameras()) { > > > > > > if (camera->streams().size() > 1) { > > > > > > cameraName_ = camera->id(); > > > > > > cameraFound = true; > > > > > > cm.stop(); > > > > > > break; > > > > > > } > > > > > > } > > > > > > > > > > > > if (!cameraFound) { > > > > > > cm.stop(); > > > > > > return TestSkip; > > > > > > } > > > > > > > > > > > > Is this something that should go in the base class ? > > > > > I did contemplate putting in the base class itself. That approach might end > > > > > up with multiple cameraManager's start()/stop() cycles (which I think is > > > > > Okay?). > > > > > > > > > > For e.g. in the multistream test, the base class would start() -> find > > > > > cameras ->stop() and the multistream class would start() -> finding cameras > > > > > with multistream support -> stop(). Then the actual test starts with > > > > > libcamerasrc element. So there's additional one cycle in this case. > > > > > > > > > > whereas single stream test wouldn't need to have any start() -> stop() > > > > > sequence as base class would handle it. > > > > > > > > > Isn't it just a numStream parameter to the base class init() function ? > > > > > > Not sure what you mean, grepping numStreams denotes: > > I mean that the desired number of streams to filter the cameras whith can > > be a paramter to a base class function. > > > > bool GstreamerTest::cameraInit(unsigned int numStreams) > > { > > /* Check if platform supports multistream capture */ > > libcamera::CameraManager cm; > > bool cameraFound = false; > > > > cm.start(); > > > > for (auto &camera : cm.cameras()) { > > if (camera->streams().size() < numStreams) > > continue; > > > > cameraFound = true; > > break; > > } > > > > cm.stop(); > > > > return cameraFound; > > } > > > I expect someone saying this is like moving away from the "base" class. As > we introduced some specifies here itself. I admit this is not exactly software engineering at its best, as this basically adds a convenience method to the base class. But between this and repeating the same test in both derived classes, I guess this is slightly better. And anyway, this is a test, so your first solution was ok as well if it fixes a bug, so up to you > > I guess it should be fine given the test cases we have right now(single and > multistream). Let me cook this up. Thanks for the suggestion. > > > > > > [uajain@fedora libcamera]$ git grep -ni "numStream" > > > src/libcamera/pipeline/simple/simple.cpp:198: unsigned int numStreams, > > > src/libcamera/pipeline/simple/simple.cpp:350: unsigned int numStreams, > > > src/libcamera/pipeline/simple/simple.cpp:352: : Camera::Private(pipe), > > > streams_(numStreams) > > > src/libcamera/pipeline/simple/simple.cpp:1266: unsigned int numStreams = 1; > > > src/libcamera/pipeline/simple/simple.cpp:1284: numStreams = streams; > > > src/libcamera/pipeline/simple/simple.cpp:1307: > > > std::make_unique<SimpleCameraData>(this, numStreams, sensor); > > > > > > > > Probably it's worth to do it in the base class ... Let me see and get back > > > > > to you > > > > > > > > > > > > const gchar *streamDescription = "videoconvert ! fakesink"; > > > > > > > g_autoptr(GError) error0 = NULL; > > > > > > > stream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE, > > > > > > > -- > > > > > > > 2.31.1 > > > > > > >
Hi Umang, thanks for the patch ... Le jeudi 21 juillet 2022 à 16:00 +0530, Umang Jain via libcamera-devel a écrit : > Before running or setting up the pipeline, check for cameras availablity > first. If no cameras are available, skip the test. > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > test/gstreamer/gstreamer_single_stream_test.cpp | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp > index a0dd12cf..1e0801e8 100644 > --- a/test/gstreamer/gstreamer_single_stream_test.cpp > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp > @@ -8,6 +8,8 @@ > #include <iostream> > #include <unistd.h> > > +#include <libcamera/libcamera.h> > + > #include <gst/gst.h> > > #include "gstreamer_test.h" > @@ -29,6 +31,17 @@ protected: > if (status_ != TestPass) > return status_; > > + libcamera::CameraManager cm; > + cm.start(); > + > + bool cameraFound = cm.cameras().size() > 1 ? true : false; > + if (!cameraFound) { > + cm.stop(); > + return TestSkip; > + } > + > + cm.stop(); > + I think starting a CM here might be problematic (even racy), specially that it is not destroyed before GStreamer starts using it. But perhaps things have improved ... At the same time, I feel like this is avoiding some obvious testing. Instead of that, perhaps you should let if fail and handle the error code ? Or even better, perhaps you should use the device provider instead of custom libcamera code ? regards, Nicolas > const gchar *streamDescription = "videoconvert ! fakesink"; > g_autoptr(GError) error0 = NULL; > stream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE,
Hi Nicolas, I just posted a updated version for this under "[PATCH 2/2] test: gstreamer: Check availability of cameras before running" On 7/21/22 19:19, Nicolas Dufresne wrote: > Hi Umang, > > thanks for the patch ... > > Le jeudi 21 juillet 2022 à 16:00 +0530, Umang Jain via libcamera-devel a écrit : >> Before running or setting up the pipeline, check for cameras availablity >> first. If no cameras are available, skip the test. >> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> --- >> test/gstreamer/gstreamer_single_stream_test.cpp | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp >> index a0dd12cf..1e0801e8 100644 >> --- a/test/gstreamer/gstreamer_single_stream_test.cpp >> +++ b/test/gstreamer/gstreamer_single_stream_test.cpp >> @@ -8,6 +8,8 @@ >> #include <iostream> >> #include <unistd.h> >> >> +#include <libcamera/libcamera.h> >> + >> #include <gst/gst.h> >> >> #include "gstreamer_test.h" >> @@ -29,6 +31,17 @@ protected: >> if (status_ != TestPass) >> return status_; >> >> + libcamera::CameraManager cm; >> + cm.start(); >> + >> + bool cameraFound = cm.cameras().size() > 1 ? true : false; >> + if (!cameraFound) { >> + cm.stop(); >> + return TestSkip; >> + } >> + >> + cm.stop(); >> + > I think starting a CM here might be problematic (even racy), specially that it > is not destroyed before GStreamer starts using it. But perhaps things have > improved ... Right, I did overlook that part. In the new version, [PATCH 2/2] test: gstreamer: Check availability of cameras before running the CameraManager instance is limited to a function (local instance), so probably this is solved with the new patch... > > At the same time, I feel like this is avoiding some obvious testing. Instead of > that, perhaps you should let if fail and handle the error code ? Or even better, The generated error is: g_printerr("Unable to set the pipeline to the playing state.\n"); I haven't looked deep, but handling the error code directly from the pipeline i.e. GST_STATE_CHANGE_FAILURE might mean a lot of things, including unavailability of the camera. Not sure how we can bifurcate and handle it specifically > perhaps you should use the device provider instead of custom libcamera code ? I looked into that earlier, but didn't get much far with device provider. I guess I need to circle back. > > regards, > Nicolas > >> const gchar *streamDescription = "videoconvert ! fakesink"; >> g_autoptr(GError) error0 = NULL; >> stream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE,
diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp index a0dd12cf..1e0801e8 100644 --- a/test/gstreamer/gstreamer_single_stream_test.cpp +++ b/test/gstreamer/gstreamer_single_stream_test.cpp @@ -8,6 +8,8 @@ #include <iostream> #include <unistd.h> +#include <libcamera/libcamera.h> + #include <gst/gst.h> #include "gstreamer_test.h" @@ -29,6 +31,17 @@ protected: if (status_ != TestPass) return status_; + libcamera::CameraManager cm; + cm.start(); + + bool cameraFound = cm.cameras().size() > 1 ? true : false; + if (!cameraFound) { + cm.stop(); + return TestSkip; + } + + cm.stop(); + const gchar *streamDescription = "videoconvert ! fakesink"; g_autoptr(GError) error0 = NULL; stream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE,
Before running or setting up the pipeline, check for cameras availablity first. If no cameras are available, skip the test. Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- test/gstreamer/gstreamer_single_stream_test.cpp | 13 +++++++++++++ 1 file changed, 13 insertions(+)