| Message ID | 20190625162353.30434-1-helen.koike@collabora.com |
|---|---|
| State | Accepted |
| Commit | 059ed93bebbac5710fffd4714ee20cc02d3212aa |
| Headers | show |
| Series |
|
| Related | show |
Hi Helen, Thank you for the patch, and welcome to libcamera ! On 25/06/2019 17:23, Helen Koike wrote: > queueRequest is called after starting the stream. Very minor, We normally put '()' to denote functions so it would be good to put queueRequest() I can add those while applying. > If it fails, the stream should be stopped, otherwise it can get a > "Device or resource busy" error, due to VIDIOC_REQBUFS ioctls being > called after VIDIOC_STREAMON without VIDIOC_STREAMOFF in-between. Sounds like a good find. I hope this means that libcamera is starting to be useful already. Out of curiosity, what platform are you running this on? RK3399 perhaps? > Signed-off-by: Helen Koike <helen.koike@collabora.com> > --- > src/cam/capture.cpp | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp > index e455612..6b842d7 100644 > --- a/src/cam/capture.cpp > +++ b/src/cam/capture.cpp > @@ -117,6 +117,7 @@ int Capture::capture(EventLoop *loop) > ret = camera_->queueRequest(request); > if (ret < 0) { > std::cerr << "Can't queue request" << std::endl; > + camera_->stop(); This looks good to me, It seems clear that this is an unbalanced start()/stop() code path in the cam utility. I started to wonder while reviewing this if we should have a call somewhere that stops the camera on an error, but I think that goes beyond the scope of this patch and handling errors with magic will be some extra policy :-) Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > return ret; > } > } >
Hi Helen, On 25/06/2019 20:51, Kieran Bingham wrote: > Hi Helen, > > Thank you for the patch, and welcome to libcamera ! > > On 25/06/2019 17:23, Helen Koike wrote: >> queueRequest is called after starting the stream. > > Very minor, We normally put '()' to denote functions so it would be good > to put queueRequest() > > I can add those while applying. This works and passes the tests (and manual testing of cam) so I've pushed it to master. Thank you. Regards, Kieran >> If it fails, the stream should be stopped, otherwise it can get a >> "Device or resource busy" error, due to VIDIOC_REQBUFS ioctls being >> called after VIDIOC_STREAMON without VIDIOC_STREAMOFF in-between. > > Sounds like a good find. I hope this means that libcamera is starting to > be useful already. > > Out of curiosity, what platform are you running this on? RK3399 perhaps? > > >> Signed-off-by: Helen Koike <helen.koike@collabora.com> >> --- >> src/cam/capture.cpp | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp >> index e455612..6b842d7 100644 >> --- a/src/cam/capture.cpp >> +++ b/src/cam/capture.cpp >> @@ -117,6 +117,7 @@ int Capture::capture(EventLoop *loop) >> ret = camera_->queueRequest(request); >> if (ret < 0) { >> std::cerr << "Can't queue request" << std::endl; >> + camera_->stop(); > > This looks good to me, > > It seems clear that this is an unbalanced start()/stop() code path in > the cam utility. I started to wonder while reviewing this if we should > have a call somewhere that stops the camera on an error, but I think > that goes beyond the scope of this patch and handling errors with magic > will be some extra policy :-) > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > >> return ret; >> } >> } >> >
Hi Kieran, On 6/25/19 4:51 PM, Kieran Bingham wrote: > Hi Helen, > > Thank you for the patch, and welcome to libcamera ! > > On 25/06/2019 17:23, Helen Koike wrote: >> queueRequest is called after starting the stream. > > Very minor, We normally put '()' to denote functions so it would be good > to put queueRequest() Good to know. > > I can add those while applying. Thanks! > > >> If it fails, the stream should be stopped, otherwise it can get a >> "Device or resource busy" error, due to VIDIOC_REQBUFS ioctls being >> called after VIDIOC_STREAMON without VIDIOC_STREAMOFF in-between. > > Sounds like a good find. I hope this means that libcamera is starting to > be useful already. It is :) > > Out of curiosity, what platform are you running this on? RK3399 perhaps? Yes, rk3399. Regards, Helen > > >> Signed-off-by: Helen Koike <helen.koike@collabora.com> >> --- >> src/cam/capture.cpp | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp >> index e455612..6b842d7 100644 >> --- a/src/cam/capture.cpp >> +++ b/src/cam/capture.cpp >> @@ -117,6 +117,7 @@ int Capture::capture(EventLoop *loop) >> ret = camera_->queueRequest(request); >> if (ret < 0) { >> std::cerr << "Can't queue request" << std::endl; >> + camera_->stop(); > > This looks good to me, > > It seems clear that this is an unbalanced start()/stop() code path in > the cam utility. I started to wonder while reviewing this if we should > have a call somewhere that stops the camera on an error, but I think > that goes beyond the scope of this patch and handling errors with magic > will be some extra policy :-) > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > >> return ret; >> } >> } >> >
diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp index e455612..6b842d7 100644 --- a/src/cam/capture.cpp +++ b/src/cam/capture.cpp @@ -117,6 +117,7 @@ int Capture::capture(EventLoop *loop) ret = camera_->queueRequest(request); if (ret < 0) { std::cerr << "Can't queue request" << std::endl; + camera_->stop(); return ret; } }
queueRequest is called after starting the stream. If it fails, the stream should be stopped, otherwise it can get a "Device or resource busy" error, due to VIDIOC_REQBUFS ioctls being called after VIDIOC_STREAMON without VIDIOC_STREAMOFF in-between. Signed-off-by: Helen Koike <helen.koike@collabora.com> --- src/cam/capture.cpp | 1 + 1 file changed, 1 insertion(+)