| Message ID | 20231130155323.13259-4-jaslo@ziska.de |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Le jeudi 30 novembre 2023 à 16:43 +0100, Jaslo Ziska via libcamera-devel a écrit : > This commit implements renegotiation of the camera configuration and > source pad caps. A renegotiation can happen when a downstream element > decides to change caps or the pipeline is dynamically changed. > > To handle a renegotiation the GST_FLOW_NOT_NEGOTIATED return value has > to be handled in GstLibcameraSrcState::processRequest. Otherwise the > default would be to print an error and stop streaming. > To archive this in a clean way the if statement is altered into a switch > statement which now also has a case for GST_FLOW_NOT_NEGOTIATED. > In the case of GST_FLOW_NOT_NEGOTIATED every source pad is checked for > the reconfiguration flag with gst_pad_needs_reconfigure which does not > clear this flag. If at least one pad requested a reconfiguration the > function returns without an error and the renegotiation will happen > later in the running task. If no pad requested a reconfiguration then > the function will return with an error. > > In gst_libcamera_src_task_run the source pads are checked for the > reconfigure flag by calling gst_pad_check_reconfigure and if one pad > returns true and the caps are not sufficient anymore then the > negotiation is triggered. It is fine to trigger the negotiation after > only a single pad returns true for gst_pad_check_reconfigure because the > reconfigure flags are cleared in the gst_libcamera_src_negotiate > function. > If any pad requested a reconfiguration the following will happen: > 1. The camera is stopped because changing the configuration may not > happen while running. > 2. The completedRequests queue will be cleared by calling > GstLibcameraSrcState::clearRequests because the completed buffers > have the wrong configuration. > 3. The new caps are negotiated by calling gst_libcamera_src_negotiate. > When the negotiation fails streaming will stop. > 4. The camera is started again. > > Signed-off-by: Jaslo Ziska <jaslo@ziska.de> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > --- > src/gstreamer/gstlibcamerasrc.cpp | 72 ++++++++++++++++++++++++++----- > 1 file changed, 61 insertions(+), 11 deletions(-) > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > index d448a750..8bd32306 100644 > --- a/src/gstreamer/gstlibcamerasrc.cpp > +++ b/src/gstreamer/gstlibcamerasrc.cpp > @@ -11,7 +11,6 @@ > * - Implement GstElement::send_event > * + Allowing application to use FLUSH/FLUSH_STOP > * + Prevent the main thread from accessing streaming thread > - * - Implement renegotiation (even if slow) > * - Implement GstElement::request-new-pad (multi stream) > * + Evaluate if a single streaming thread is fine > * - Add application driven request (snapshot) > @@ -302,18 +301,42 @@ int GstLibcameraSrcState::processRequest() > srcpad, ret); > } > > - if (ret != GST_FLOW_OK) { > - if (ret == GST_FLOW_EOS) { > - g_autoptr(GstEvent) eos = gst_event_new_eos(); > - guint32 seqnum = gst_util_seqnum_next(); > - gst_event_set_seqnum(eos, seqnum); > - for (GstPad *srcpad : srcpads_) > - gst_pad_push_event(srcpad, gst_event_ref(eos)); > - } else if (ret != GST_FLOW_FLUSHING) { > - GST_ELEMENT_FLOW_ERROR(src_, ret); > + switch (ret) { > + case GST_FLOW_OK: > + break; > + case GST_FLOW_NOT_NEGOTIATED: { > + bool reconfigure = false; > + for (GstPad *srcpad : srcpads_) { > + if (gst_pad_needs_reconfigure(srcpad)) { > + reconfigure = true; > + break; > + } > } > > - return -EPIPE; > + // if no pads needs a reconfiguration something went wrong > + if (!reconfigure) > + err = -EPIPE; > + > + break; > + } > + case GST_FLOW_EOS: { > + g_autoptr(GstEvent) eos = gst_event_new_eos(); > + guint32 seqnum = gst_util_seqnum_next(); > + gst_event_set_seqnum(eos, seqnum); > + for (GstPad *srcpad : srcpads_) > + gst_pad_push_event(srcpad, gst_event_ref(eos)); > + > + err = -EPIPE; > + break; > + } > + case GST_FLOW_FLUSHING: > + err = -EPIPE; > + break; > + default: > + GST_ELEMENT_FLOW_ERROR(src_, ret); > + > + err = -EPIPE; > + break; > } > > return err; > @@ -463,6 +486,8 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self) > G_CALLBACK(gst_task_resume), self->task); > > gst_libcamera_pad_set_pool(srcpad, pool); > + > + gst_pad_check_reconfigure(srcpad); // clear all reconfigure flags > } > > return true; > @@ -496,6 +521,31 @@ gst_libcamera_src_task_run(gpointer user_data) > return; > } > > + // check if a srcpad requested a renegotiation > + bool reconfigure = false; > + for (GstPad *srcpad : state->srcpads_) { > + if (gst_pad_check_reconfigure(srcpad)) { > + // check whether the caps even need changing > + g_autoptr(GstCaps) caps = gst_pad_get_current_caps(srcpad); > + if (!gst_pad_peer_query_accept_caps(srcpad, caps)) { > + reconfigure = true; > + break; > + } > + } > + } > + > + if (reconfigure) { > + state->cam_->stop(); > + state->clearRequests(); > + > + if (!gst_libcamera_src_negotiate(self)) { > + GST_ELEMENT_FLOW_ERROR(self, GST_FLOW_NOT_NEGOTIATED); > + gst_task_stop(self->task); > + } > + > + state->cam_->start(&state->initControls_); > + } > + > /* > * Create and queue one request. If no buffers are available the > * function returns -ENOBUFS, which we ignore here as that's not a
Hi Jaslo, Thank you for the patch. On Thu, Nov 30, 2023 at 04:43:10PM +0100, Jaslo Ziska via libcamera-devel wrote: > This commit implements renegotiation of the camera configuration and > source pad caps. A renegotiation can happen when a downstream element > decides to change caps or the pipeline is dynamically changed. > > To handle a renegotiation the GST_FLOW_NOT_NEGOTIATED return value has > to be handled in GstLibcameraSrcState::processRequest. Otherwise the > default would be to print an error and stop streaming. > To archive this in a clean way the if statement is altered into a switch > statement which now also has a case for GST_FLOW_NOT_NEGOTIATED. > In the case of GST_FLOW_NOT_NEGOTIATED every source pad is checked for > the reconfiguration flag with gst_pad_needs_reconfigure which does not > clear this flag. If at least one pad requested a reconfiguration the > function returns without an error and the renegotiation will happen > later in the running task. If no pad requested a reconfiguration then > the function will return with an error. > > In gst_libcamera_src_task_run the source pads are checked for the > reconfigure flag by calling gst_pad_check_reconfigure and if one pad > returns true and the caps are not sufficient anymore then the > negotiation is triggered. It is fine to trigger the negotiation after > only a single pad returns true for gst_pad_check_reconfigure because the > reconfigure flags are cleared in the gst_libcamera_src_negotiate > function. > If any pad requested a reconfiguration the following will happen: > 1. The camera is stopped because changing the configuration may not > happen while running. > 2. The completedRequests queue will be cleared by calling > GstLibcameraSrcState::clearRequests because the completed buffers > have the wrong configuration. > 3. The new caps are negotiated by calling gst_libcamera_src_negotiate. > When the negotiation fails streaming will stop. > 4. The camera is started again. > > Signed-off-by: Jaslo Ziska <jaslo@ziska.de> > --- > src/gstreamer/gstlibcamerasrc.cpp | 72 ++++++++++++++++++++++++++----- > 1 file changed, 61 insertions(+), 11 deletions(-) > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > index d448a750..8bd32306 100644 > --- a/src/gstreamer/gstlibcamerasrc.cpp > +++ b/src/gstreamer/gstlibcamerasrc.cpp > @@ -11,7 +11,6 @@ > * - Implement GstElement::send_event > * + Allowing application to use FLUSH/FLUSH_STOP > * + Prevent the main thread from accessing streaming thread > - * - Implement renegotiation (even if slow) > * - Implement GstElement::request-new-pad (multi stream) > * + Evaluate if a single streaming thread is fine > * - Add application driven request (snapshot) > @@ -302,18 +301,42 @@ int GstLibcameraSrcState::processRequest() > srcpad, ret); > } > > - if (ret != GST_FLOW_OK) { > - if (ret == GST_FLOW_EOS) { > - g_autoptr(GstEvent) eos = gst_event_new_eos(); > - guint32 seqnum = gst_util_seqnum_next(); > - gst_event_set_seqnum(eos, seqnum); > - for (GstPad *srcpad : srcpads_) > - gst_pad_push_event(srcpad, gst_event_ref(eos)); > - } else if (ret != GST_FLOW_FLUSHING) { > - GST_ELEMENT_FLOW_ERROR(src_, ret); > + switch (ret) { > + case GST_FLOW_OK: > + break; We usually add blank lines between cases. > + case GST_FLOW_NOT_NEGOTIATED: { > + bool reconfigure = false; > + for (GstPad *srcpad : srcpads_) { > + if (gst_pad_needs_reconfigure(srcpad)) { > + reconfigure = true; > + break; > + } > } > > - return -EPIPE; > + // if no pads needs a reconfiguration something went wrong We use C-style comments: /* If no pads need a reconfiguration something went wrong. */ Same comment below. I'll fix these minor issues when applying the series, no need to submit a new version. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + if (!reconfigure) > + err = -EPIPE; > + > + break; > + } > + case GST_FLOW_EOS: { > + g_autoptr(GstEvent) eos = gst_event_new_eos(); > + guint32 seqnum = gst_util_seqnum_next(); > + gst_event_set_seqnum(eos, seqnum); > + for (GstPad *srcpad : srcpads_) > + gst_pad_push_event(srcpad, gst_event_ref(eos)); > + > + err = -EPIPE; > + break; > + } > + case GST_FLOW_FLUSHING: > + err = -EPIPE; > + break; > + default: > + GST_ELEMENT_FLOW_ERROR(src_, ret); > + > + err = -EPIPE; > + break; > } > > return err; > @@ -463,6 +486,8 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self) > G_CALLBACK(gst_task_resume), self->task); > > gst_libcamera_pad_set_pool(srcpad, pool); > + > + gst_pad_check_reconfigure(srcpad); // clear all reconfigure flags > } > > return true; > @@ -496,6 +521,31 @@ gst_libcamera_src_task_run(gpointer user_data) > return; > } > > + // check if a srcpad requested a renegotiation > + bool reconfigure = false; > + for (GstPad *srcpad : state->srcpads_) { > + if (gst_pad_check_reconfigure(srcpad)) { > + // check whether the caps even need changing > + g_autoptr(GstCaps) caps = gst_pad_get_current_caps(srcpad); > + if (!gst_pad_peer_query_accept_caps(srcpad, caps)) { > + reconfigure = true; > + break; > + } > + } > + } > + > + if (reconfigure) { > + state->cam_->stop(); > + state->clearRequests(); > + > + if (!gst_libcamera_src_negotiate(self)) { > + GST_ELEMENT_FLOW_ERROR(self, GST_FLOW_NOT_NEGOTIATED); > + gst_task_stop(self->task); > + } > + > + state->cam_->start(&state->initControls_); > + } > + > /* > * Create and queue one request. If no buffers are available the > * function returns -ENOBUFS, which we ignore here as that's not a
diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp index d448a750..8bd32306 100644 --- a/src/gstreamer/gstlibcamerasrc.cpp +++ b/src/gstreamer/gstlibcamerasrc.cpp @@ -11,7 +11,6 @@ * - Implement GstElement::send_event * + Allowing application to use FLUSH/FLUSH_STOP * + Prevent the main thread from accessing streaming thread - * - Implement renegotiation (even if slow) * - Implement GstElement::request-new-pad (multi stream) * + Evaluate if a single streaming thread is fine * - Add application driven request (snapshot) @@ -302,18 +301,42 @@ int GstLibcameraSrcState::processRequest() srcpad, ret); } - if (ret != GST_FLOW_OK) { - if (ret == GST_FLOW_EOS) { - g_autoptr(GstEvent) eos = gst_event_new_eos(); - guint32 seqnum = gst_util_seqnum_next(); - gst_event_set_seqnum(eos, seqnum); - for (GstPad *srcpad : srcpads_) - gst_pad_push_event(srcpad, gst_event_ref(eos)); - } else if (ret != GST_FLOW_FLUSHING) { - GST_ELEMENT_FLOW_ERROR(src_, ret); + switch (ret) { + case GST_FLOW_OK: + break; + case GST_FLOW_NOT_NEGOTIATED: { + bool reconfigure = false; + for (GstPad *srcpad : srcpads_) { + if (gst_pad_needs_reconfigure(srcpad)) { + reconfigure = true; + break; + } } - return -EPIPE; + // if no pads needs a reconfiguration something went wrong + if (!reconfigure) + err = -EPIPE; + + break; + } + case GST_FLOW_EOS: { + g_autoptr(GstEvent) eos = gst_event_new_eos(); + guint32 seqnum = gst_util_seqnum_next(); + gst_event_set_seqnum(eos, seqnum); + for (GstPad *srcpad : srcpads_) + gst_pad_push_event(srcpad, gst_event_ref(eos)); + + err = -EPIPE; + break; + } + case GST_FLOW_FLUSHING: + err = -EPIPE; + break; + default: + GST_ELEMENT_FLOW_ERROR(src_, ret); + + err = -EPIPE; + break; } return err; @@ -463,6 +486,8 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self) G_CALLBACK(gst_task_resume), self->task); gst_libcamera_pad_set_pool(srcpad, pool); + + gst_pad_check_reconfigure(srcpad); // clear all reconfigure flags } return true; @@ -496,6 +521,31 @@ gst_libcamera_src_task_run(gpointer user_data) return; } + // check if a srcpad requested a renegotiation + bool reconfigure = false; + for (GstPad *srcpad : state->srcpads_) { + if (gst_pad_check_reconfigure(srcpad)) { + // check whether the caps even need changing + g_autoptr(GstCaps) caps = gst_pad_get_current_caps(srcpad); + if (!gst_pad_peer_query_accept_caps(srcpad, caps)) { + reconfigure = true; + break; + } + } + } + + if (reconfigure) { + state->cam_->stop(); + state->clearRequests(); + + if (!gst_libcamera_src_negotiate(self)) { + GST_ELEMENT_FLOW_ERROR(self, GST_FLOW_NOT_NEGOTIATED); + gst_task_stop(self->task); + } + + state->cam_->start(&state->initControls_); + } + /* * Create and queue one request. If no buffers are available the * function returns -ENOBUFS, which we ignore here as that's not a
This commit implements renegotiation of the camera configuration and source pad caps. A renegotiation can happen when a downstream element decides to change caps or the pipeline is dynamically changed. To handle a renegotiation the GST_FLOW_NOT_NEGOTIATED return value has to be handled in GstLibcameraSrcState::processRequest. Otherwise the default would be to print an error and stop streaming. To archive this in a clean way the if statement is altered into a switch statement which now also has a case for GST_FLOW_NOT_NEGOTIATED. In the case of GST_FLOW_NOT_NEGOTIATED every source pad is checked for the reconfiguration flag with gst_pad_needs_reconfigure which does not clear this flag. If at least one pad requested a reconfiguration the function returns without an error and the renegotiation will happen later in the running task. If no pad requested a reconfiguration then the function will return with an error. In gst_libcamera_src_task_run the source pads are checked for the reconfigure flag by calling gst_pad_check_reconfigure and if one pad returns true and the caps are not sufficient anymore then the negotiation is triggered. It is fine to trigger the negotiation after only a single pad returns true for gst_pad_check_reconfigure because the reconfigure flags are cleared in the gst_libcamera_src_negotiate function. If any pad requested a reconfiguration the following will happen: 1. The camera is stopped because changing the configuration may not happen while running. 2. The completedRequests queue will be cleared by calling GstLibcameraSrcState::clearRequests because the completed buffers have the wrong configuration. 3. The new caps are negotiated by calling gst_libcamera_src_negotiate. When the negotiation fails streaming will stop. 4. The camera is started again. Signed-off-by: Jaslo Ziska <jaslo@ziska.de> --- src/gstreamer/gstlibcamerasrc.cpp | 72 ++++++++++++++++++++++++++----- 1 file changed, 61 insertions(+), 11 deletions(-)