| Message ID | 20220629125551.330470-1-umang.jain@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, Thank you for the patch. On Wed, Jun 29, 2022 at 06:25:51PM +0530, Umang Jain via libcamera-devel wrote: > While the "src" pad is added to the element, it is accessed > via a index number. If multiple pads are added(in future) > and tracked in state->srcpads_, the index might need re-adjusting. Don't we already support multiple pads ? I don't foresee libcamera_src_init() adding more than one pad, the other ones should be added dynamically by gst_libcamera_src_request_new_pad() as far as I understand. > Use the std::vector::back() instead of index, which corresponds > to std::vector::push_back() for tracking of pads. It also slightly > helps with readability. > > No functional changes intended. > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/gstreamer/gstlibcamerasrc.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > index 46fd02d2..4813ab96 100644 > --- a/src/gstreamer/gstlibcamerasrc.cpp > +++ b/src/gstreamer/gstlibcamerasrc.cpp > @@ -631,7 +631,7 @@ gst_libcamera_src_init(GstLibcameraSrc *self) > gst_task_set_lock(self->task, &self->stream_lock); > > state->srcpads_.push_back(gst_pad_new_from_template(templ, "src")); > - gst_element_add_pad(GST_ELEMENT(self), state->srcpads_[0]); > + gst_element_add_pad(GST_ELEMENT(self), state->srcpads_.back()); I'm fine with the code change, but the commit message should be reworded if my understanding is right and this function will never add more than one pad. > > /* C-style friend. */ > state->src_ = self;
Hi Laurent, On 6/30/22 05:49, Laurent Pinchart wrote: > Hi Umang, > > Thank you for the patch. > > On Wed, Jun 29, 2022 at 06:25:51PM +0530, Umang Jain via libcamera-devel wrote: >> While the "src" pad is added to the element, it is accessed >> via a index number. If multiple pads are added(in future) >> and tracked in state->srcpads_, the index might need re-adjusting. > Don't we already support multiple pads ? I don't foresee > libcamera_src_init() adding more than one pad, the other ones should be > added dynamically by gst_libcamera_src_request_new_pad() as far as I > understand. Ah yes, that's correct. I didn't come across request_new_pad() earlier. > >> Use the std::vector::back() instead of index, which corresponds >> to std::vector::push_back() for tracking of pads. It also slightly >> helps with readability. >> >> No functional changes intended. >> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> --- >> src/gstreamer/gstlibcamerasrc.cpp | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp >> index 46fd02d2..4813ab96 100644 >> --- a/src/gstreamer/gstlibcamerasrc.cpp >> +++ b/src/gstreamer/gstlibcamerasrc.cpp >> @@ -631,7 +631,7 @@ gst_libcamera_src_init(GstLibcameraSrc *self) >> gst_task_set_lock(self->task, &self->stream_lock); >> >> state->srcpads_.push_back(gst_pad_new_from_template(templ, "src")); >> - gst_element_add_pad(GST_ELEMENT(self), state->srcpads_[0]); >> + gst_element_add_pad(GST_ELEMENT(self), state->srcpads_.back()); > I'm fine with the code change, but the commit message should be reworded > if my understanding is right and this function will never add more than > one pad. Submitting v1.1 with commit message change. > >> >> /* C-style friend. */ >> state->src_ = self;
Hello Umang, Thanks for the patch. here's the rb tag with Laurent's suggestion on commit message, since multi pads is already supported. Reviewed-by: Vedant Paranjape <vedantparanjape160201@gmail.com> On Wed, Jun 29, 2022 at 2:56 PM Umang Jain <umang.jain@ideasonboard.com> wrote: > > While the "src" pad is added to the element, it is accessed > via a index number. If multiple pads are added(in future) > and tracked in state->srcpads_, the index might need re-adjusting. > > Use the std::vector::back() instead of index, which corresponds > to std::vector::push_back() for tracking of pads. It also slightly > helps with readability. > > No functional changes intended. > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/gstreamer/gstlibcamerasrc.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > index 46fd02d2..4813ab96 100644 > --- a/src/gstreamer/gstlibcamerasrc.cpp > +++ b/src/gstreamer/gstlibcamerasrc.cpp > @@ -631,7 +631,7 @@ gst_libcamera_src_init(GstLibcameraSrc *self) > gst_task_set_lock(self->task, &self->stream_lock); > > state->srcpads_.push_back(gst_pad_new_from_template(templ, "src")); > - gst_element_add_pad(GST_ELEMENT(self), state->srcpads_[0]); > + gst_element_add_pad(GST_ELEMENT(self), state->srcpads_.back()); > > /* C-style friend. */ > state->src_ = self; > -- > 2.31.1 >
diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp index 46fd02d2..4813ab96 100644 --- a/src/gstreamer/gstlibcamerasrc.cpp +++ b/src/gstreamer/gstlibcamerasrc.cpp @@ -631,7 +631,7 @@ gst_libcamera_src_init(GstLibcameraSrc *self) gst_task_set_lock(self->task, &self->stream_lock); state->srcpads_.push_back(gst_pad_new_from_template(templ, "src")); - gst_element_add_pad(GST_ELEMENT(self), state->srcpads_[0]); + gst_element_add_pad(GST_ELEMENT(self), state->srcpads_.back()); /* C-style friend. */ state->src_ = self;
While the "src" pad is added to the element, it is accessed via a index number. If multiple pads are added(in future) and tracked in state->srcpads_, the index might need re-adjusting. Use the std::vector::back() instead of index, which corresponds to std::vector::push_back() for tracking of pads. It also slightly helps with readability. No functional changes intended. Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- src/gstreamer/gstlibcamerasrc.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)