| Message ID | 20240305153058.1761020-3-nicolas@ndufresne.ca |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Quoting Nicolas Dufresne (2024-03-05 15:30:57) > From: Nicolas Dufresne <nicolas.dufresne@collabora.com> > Why? Is it just better? I'll try to interpret as I read the patch. Please correct me whereever I'm wrong. """ The single stream test for the gstreamer component has a simple pipeline construction using only a fakesink. The impelementation currently supports connecting to a more complex stream construction defined by the streamDescription, but this is over engineered for the simple need to start a stream to capture and discard the frames. Convert the use of gst_parse_bin_from_description_full() which uses only a single element 'fakesink' to construct the fakesink directly and link it to the libcamerasrc. """ > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > .../gstreamer_single_stream_test.cpp | 27 +++++++------------ > 1 file changed, 9 insertions(+), 18 deletions(-) > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp > index 301e4a93..815c6609 100644 > --- a/test/gstreamer/gstreamer_single_stream_test.cpp > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp > @@ -29,30 +29,21 @@ protected: > if (status_ != TestPass) > return status_; > > - const gchar *streamDescription = "fakesink"; > - g_autoptr(GError) error0 = NULL; > - stream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE, > - NULL, > - GST_PARSE_FLAG_FATAL_ERRORS, > - &error0); > - > - if (!stream0_) { > - g_printerr("Bin could not be created (%s)\n", error0->message); > + fakesink_ = gst_element_factory_make("fakesink", nullptr); > + if (!fakesink_) { > + g_printerr("Your installation is missing 'fakesink'\n"); > return TestFail; > } > - g_object_ref_sink(stream0_); > - > - if (createPipeline() != TestPass) > - return TestFail; > + g_object_ref_sink(fakesink_); > > - return TestPass; > + return createPipeline(); > } > > int run() override > { > /* Build the pipeline */ > - gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, stream0_, NULL); > - if (gst_element_link(libcameraSrc_, stream0_) != TRUE) { > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, fakesink_, nullptr); > + if (!gst_element_link(libcameraSrc_, fakesink_)) { > g_printerr("Elements could not be linked.\n"); > return TestFail; > } > @@ -68,11 +59,11 @@ protected: > > void cleanup() override > { > - g_clear_object(&stream0_); > + g_clear_object(&fakesink_); > } > > private: > - GstElement *stream0_; > + GstElement *fakesink_; > }; > > TEST_REGISTER(GstreamerSingleStreamTest) > -- > 2.43.2 >
Hi, Le jeudi 09 mai 2024 à 15:50 +0100, Kieran Bingham a écrit : > Quoting Nicolas Dufresne (2024-03-05 15:30:57) > > From: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > > > Why? Is it just better? I commit and sign with my Collabora address for all the work I do on my employer time. But I'm using my personal address for mailing lists. I did no action here, "git send-email" noticed and added the from. If you use "git am". it should transparently reflect my choice here. > > I'll try to interpret as I read the patch. Please correct me whereever > I'm wrong. > > """ > The single stream test for the gstreamer component has a simple pipeline > construction using only a fakesink. > > The impelementation currently supports connecting to a more complex > stream construction defined by the streamDescription, but this is over > engineered for the simple need to start a stream to capture and discard > the frames. > > Convert the use of gst_parse_bin_from_description_full() which uses only > a single element 'fakesink' to construct the fakesink directly and link > it to the libcamerasrc. > """ That is very good, I should have wrote something, but if the target was the GStreamer project, it would have been considered overkill due to the trivality of the changes (from a GStreamer dev point of view of course). I'll keep in mind for next time. > > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > --- > > .../gstreamer_single_stream_test.cpp | 27 +++++++------------ > > 1 file changed, 9 insertions(+), 18 deletions(-) > > > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp > > index 301e4a93..815c6609 100644 > > --- a/test/gstreamer/gstreamer_single_stream_test.cpp > > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp > > @@ -29,30 +29,21 @@ protected: > > if (status_ != TestPass) > > return status_; > > > > - const gchar *streamDescription = "fakesink"; > > - g_autoptr(GError) error0 = NULL; > > - stream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE, > > - NULL, > > - GST_PARSE_FLAG_FATAL_ERRORS, > > - &error0); > > - > > - if (!stream0_) { > > - g_printerr("Bin could not be created (%s)\n", error0->message); > > + fakesink_ = gst_element_factory_make("fakesink", nullptr); > > + if (!fakesink_) { > > + g_printerr("Your installation is missing 'fakesink'\n"); > > return TestFail; > > } > > - g_object_ref_sink(stream0_); > > - > > - if (createPipeline() != TestPass) > > - return TestFail; > > + g_object_ref_sink(fakesink_); > > > > - return TestPass; > > + return createPipeline(); > > } > > > > int run() override > > { > > /* Build the pipeline */ > > - gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, stream0_, NULL); > > - if (gst_element_link(libcameraSrc_, stream0_) != TRUE) { > > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, fakesink_, nullptr); > > + if (!gst_element_link(libcameraSrc_, fakesink_)) { > > g_printerr("Elements could not be linked.\n"); > > return TestFail; > > } > > @@ -68,11 +59,11 @@ protected: > > > > void cleanup() override > > { > > - g_clear_object(&stream0_); > > + g_clear_object(&fakesink_); > > } > > > > private: > > - GstElement *stream0_; > > + GstElement *fakesink_; > > }; > > > > TEST_REGISTER(GstreamerSingleStreamTest) > > -- > > 2.43.2 > >
Quoting Nicolas Dufresne (2024-05-09 16:12:18) > Hi, > > Le jeudi 09 mai 2024 à 15:50 +0100, Kieran Bingham a écrit : > > Quoting Nicolas Dufresne (2024-03-05 15:30:57) > > > From: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > > > > > > Why? Is it just better? > > I commit and sign with my Collabora address for all the work I do on my employer > time. But I'm using my personal address for mailing lists. I did no action here, > "git send-email" noticed and added the from. If you use "git am". it should > transparently reflect my choice here. Sorry - I meant - the commit message was empty. I don't mind (or care) which the From is from ;-) I meant "Simplify single stream test" - Why - is it better simplified? The empty commit message didn't tell us anything! > > > > > I'll try to interpret as I read the patch. Please correct me whereever > > I'm wrong. > > > > """ > > The single stream test for the gstreamer component has a simple pipeline > > construction using only a fakesink. > > > > The impelementation currently supports connecting to a more complex > > stream construction defined by the streamDescription, but this is over > > engineered for the simple need to start a stream to capture and discard > > the frames. > > > > Convert the use of gst_parse_bin_from_description_full() which uses only > > a single element 'fakesink' to construct the fakesink directly and link > > it to the libcamerasrc. > > """ > > That is very good, I should have wrote something, but if the target was the > GStreamer project, it would have been considered overkill due to the trivality > of the changes (from a GStreamer dev point of view of course). I'll keep in mind > for next time. The difficulty for me is I'm not a gstreamer dev, so I find the gstreamer element difficult to read. An introduction is very helpful to go into the patch with something in mind. If you're happy with the commit message, it can be added at applying or in a v2 - I don't know if a v2 is needed yet. > > > > > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > --- > > > .../gstreamer_single_stream_test.cpp | 27 +++++++------------ > > > 1 file changed, 9 insertions(+), 18 deletions(-) > > > > > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp > > > index 301e4a93..815c6609 100644 > > > --- a/test/gstreamer/gstreamer_single_stream_test.cpp > > > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp > > > @@ -29,30 +29,21 @@ protected: > > > if (status_ != TestPass) > > > return status_; > > > > > > - const gchar *streamDescription = "fakesink"; > > > - g_autoptr(GError) error0 = NULL; > > > - stream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE, > > > - NULL, > > > - GST_PARSE_FLAG_FATAL_ERRORS, > > > - &error0); > > > - > > > - if (!stream0_) { > > > - g_printerr("Bin could not be created (%s)\n", error0->message); > > > + fakesink_ = gst_element_factory_make("fakesink", nullptr); > > > + if (!fakesink_) { > > > + g_printerr("Your installation is missing 'fakesink'\n"); > > > return TestFail; > > > } > > > - g_object_ref_sink(stream0_); > > > - > > > - if (createPipeline() != TestPass) > > > - return TestFail; > > > + g_object_ref_sink(fakesink_); > > > > > > - return TestPass; > > > + return createPipeline(); > > > } > > > > > > int run() override > > > { > > > /* Build the pipeline */ > > > - gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, stream0_, NULL); > > > - if (gst_element_link(libcameraSrc_, stream0_) != TRUE) { > > > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, fakesink_, nullptr); > > > + if (!gst_element_link(libcameraSrc_, fakesink_)) { > > > g_printerr("Elements could not be linked.\n"); > > > return TestFail; > > > } > > > @@ -68,11 +59,11 @@ protected: > > > > > > void cleanup() override > > > { > > > - g_clear_object(&stream0_); > > > + g_clear_object(&fakesink_); > > > } > > > > > > private: > > > - GstElement *stream0_; > > > + GstElement *fakesink_; > > > }; > > > > > > TEST_REGISTER(GstreamerSingleStreamTest) > > > -- > > > 2.43.2 > > > >
On Thu, May 09, 2024 at 04:57:29PM +0100, Kieran Bingham wrote: > Quoting Nicolas Dufresne (2024-05-09 16:12:18) > > Hi, > > > > Le jeudi 09 mai 2024 à 15:50 +0100, Kieran Bingham a écrit : > > > Quoting Nicolas Dufresne (2024-03-05 15:30:57) > > > > From: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > > > > > > > > > Why? Is it just better? > > > > I commit and sign with my Collabora address for all the work I do on my employer > > time. But I'm using my personal address for mailing lists. I did no action here, > > "git send-email" noticed and added the from. If you use "git am". it should > > transparently reflect my choice here. > > Sorry - I meant - the commit message was empty. I don't mind (or care) > which the From is from ;-) > > I meant "Simplify single stream test" - Why - is it better simplified? > The empty commit message didn't tell us anything! > > > > I'll try to interpret as I read the patch. Please correct me whereever > > > I'm wrong. > > > > > > """ > > > The single stream test for the gstreamer component has a simple pipeline > > > construction using only a fakesink. > > > > > > The impelementation currently supports connecting to a more complex s/impelementation/implementation/ Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > stream construction defined by the streamDescription, but this is over > > > engineered for the simple need to start a stream to capture and discard > > > the frames. > > > > > > Convert the use of gst_parse_bin_from_description_full() which uses only > > > a single element 'fakesink' to construct the fakesink directly and link > > > it to the libcamerasrc. > > > """ > > > > That is very good, I should have wrote something, but if the target was the > > GStreamer project, it would have been considered overkill due to the trivality > > of the changes (from a GStreamer dev point of view of course). I'll keep in mind > > for next time. > > The difficulty for me is I'm not a gstreamer dev, so I find the > gstreamer element difficult to read. An introduction is very helpful to > go into the patch with something in mind. > > If you're happy with the commit message, it can be added at applying or > in a v2 - I don't know if a v2 is needed yet. I'll merge this patch already, independently from the rest of the series. > > > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > --- > > > > .../gstreamer_single_stream_test.cpp | 27 +++++++------------ > > > > 1 file changed, 9 insertions(+), 18 deletions(-) > > > > > > > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp > > > > index 301e4a93..815c6609 100644 > > > > --- a/test/gstreamer/gstreamer_single_stream_test.cpp > > > > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp > > > > @@ -29,30 +29,21 @@ protected: > > > > if (status_ != TestPass) > > > > return status_; > > > > > > > > - const gchar *streamDescription = "fakesink"; > > > > - g_autoptr(GError) error0 = NULL; > > > > - stream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE, > > > > - NULL, > > > > - GST_PARSE_FLAG_FATAL_ERRORS, > > > > - &error0); > > > > - > > > > - if (!stream0_) { > > > > - g_printerr("Bin could not be created (%s)\n", error0->message); > > > > + fakesink_ = gst_element_factory_make("fakesink", nullptr); > > > > + if (!fakesink_) { > > > > + g_printerr("Your installation is missing 'fakesink'\n"); > > > > return TestFail; > > > > } > > > > - g_object_ref_sink(stream0_); > > > > - > > > > - if (createPipeline() != TestPass) > > > > - return TestFail; > > > > + g_object_ref_sink(fakesink_); > > > > > > > > - return TestPass; > > > > + return createPipeline(); > > > > } > > > > > > > > int run() override > > > > { > > > > /* Build the pipeline */ > > > > - gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, stream0_, NULL); > > > > - if (gst_element_link(libcameraSrc_, stream0_) != TRUE) { > > > > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, fakesink_, nullptr); > > > > + if (!gst_element_link(libcameraSrc_, fakesink_)) { > > > > g_printerr("Elements could not be linked.\n"); > > > > return TestFail; > > > > } > > > > @@ -68,11 +59,11 @@ protected: > > > > > > > > void cleanup() override > > > > { > > > > - g_clear_object(&stream0_); > > > > + g_clear_object(&fakesink_); > > > > } > > > > > > > > private: > > > > - GstElement *stream0_; > > > > + GstElement *fakesink_; > > > > }; > > > > > > > > TEST_REGISTER(GstreamerSingleStreamTest)
diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp index 301e4a93..815c6609 100644 --- a/test/gstreamer/gstreamer_single_stream_test.cpp +++ b/test/gstreamer/gstreamer_single_stream_test.cpp @@ -29,30 +29,21 @@ protected: if (status_ != TestPass) return status_; - const gchar *streamDescription = "fakesink"; - g_autoptr(GError) error0 = NULL; - stream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE, - NULL, - GST_PARSE_FLAG_FATAL_ERRORS, - &error0); - - if (!stream0_) { - g_printerr("Bin could not be created (%s)\n", error0->message); + fakesink_ = gst_element_factory_make("fakesink", nullptr); + if (!fakesink_) { + g_printerr("Your installation is missing 'fakesink'\n"); return TestFail; } - g_object_ref_sink(stream0_); - - if (createPipeline() != TestPass) - return TestFail; + g_object_ref_sink(fakesink_); - return TestPass; + return createPipeline(); } int run() override { /* Build the pipeline */ - gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, stream0_, NULL); - if (gst_element_link(libcameraSrc_, stream0_) != TRUE) { + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, fakesink_, nullptr); + if (!gst_element_link(libcameraSrc_, fakesink_)) { g_printerr("Elements could not be linked.\n"); return TestFail; } @@ -68,11 +59,11 @@ protected: void cleanup() override { - g_clear_object(&stream0_); + g_clear_object(&fakesink_); } private: - GstElement *stream0_; + GstElement *fakesink_; }; TEST_REGISTER(GstreamerSingleStreamTest)