| Message ID | 20190226021857.28255-5-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, Thank you for the patch. On Tue, Feb 26, 2019 at 03:18:53AM +0100, Niklas Söderlund wrote: > Upcoming enforcing of order the camera shall be operate on is not > compatible with the cam utility. Requests shall be queued after the > camera is started, not before. What are the practical implications of this on camera operation, from an application point of view ? In particular, when will the acquisition be started after this change ? Will the pipeline handlers be required to start acquisition on the sensor at start() time, throwing away all images until requests are queued ? Or will acquisition start when the first request is queued ? Sensors can take a long time to start, so it's important to document the exact behaviour. > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/cam/main.cpp | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > index 4c2df583fe8e99b7..8df8844c33a2d052 100644 > --- a/src/cam/main.cpp > +++ b/src/cam/main.cpp > @@ -133,6 +133,7 @@ static int capture() > int ret; > > std::vector<Stream *> streams = camera->streams(); > + std::vector<Request *> requests; > > ret = configureStreams(camera.get(), streams); > if (ret < 0) { > @@ -169,21 +170,24 @@ static int capture() > goto out; > } > > - ret = camera->queueRequest(request); > - if (ret < 0) { > - std::cerr << "Can't queue request" << std::endl; > - goto out; > - } > + requests.push_back(request); > } > > - std::cout << "Capture until user interrupts by SIGINT" << std::endl; > - > ret = camera->start(); > if (ret) { > std::cout << "Failed to start capture" << std::endl; > goto out; > } > > + for (Request *request : requests) { > + ret = camera->queueRequest(request); > + if (ret < 0) { > + std::cerr << "Can't queue request" << std::endl; > + goto out; > + } > + } > + > + std::cout << "Capture until user interrupts by SIGINT" << std::endl; > ret = loop->exec(); > > ret = camera->stop();
Hi Laurent, Thanks for your feedback. On 2019-02-27 15:04:32 +0200, Laurent Pinchart wrote: > Hi Niklas, > > Thank you for the patch. > > On Tue, Feb 26, 2019 at 03:18:53AM +0100, Niklas Söderlund wrote: > > Upcoming enforcing of order the camera shall be operate on is not > > compatible with the cam utility. Requests shall be queued after the > > camera is started, not before. > > What are the practical implications of this on camera operation, from an > application point of view ? In particular, when will the acquisition be > started after this change ? Will the pipeline handlers be required to > start acquisition on the sensor at start() time, throwing away all > images until requests are queued ? Or will acquisition start when the > first request is queued ? Sensors can take a long time to start, so it's > important to document the exact behaviour. My understanding of the situation is that at start() time VIDIOC_STREAMON shall be called. Then it's currently not defined what other actions are expected by the camera or pipeline handler when processing start(). I agree this need to be defined. I see two possible scenarios. 1. The library grantees that once a request have been queued it will eventually complete disregarding of more requests are queued or not. 2. The library informs the application about the request queue depth it needs before requests starts to be processed. I'm hoping we can do scenario 1 but not sure if we can do that without ugly scratch buffers in the library which I feel is a bit of a hack. Whatever scenario we pick the pipeline handler would need to know about the request queue depth and act on that or report it to the application so it can act. The pipeline handlers do not know this property today so not much we can do yet. In any case I think this is outside the scope of this patch. This problem exists disregarding if we only allow queueing requests after start() or if allow it both before and after. In fact I thought of this change as a first step to address this issue as we now define that requests can only be queued after start(). > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > src/cam/main.cpp | 18 +++++++++++------- > > 1 file changed, 11 insertions(+), 7 deletions(-) > > > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > > index 4c2df583fe8e99b7..8df8844c33a2d052 100644 > > --- a/src/cam/main.cpp > > +++ b/src/cam/main.cpp > > @@ -133,6 +133,7 @@ static int capture() > > int ret; > > > > std::vector<Stream *> streams = camera->streams(); > > + std::vector<Request *> requests; > > > > ret = configureStreams(camera.get(), streams); > > if (ret < 0) { > > @@ -169,21 +170,24 @@ static int capture() > > goto out; > > } > > > > - ret = camera->queueRequest(request); > > - if (ret < 0) { > > - std::cerr << "Can't queue request" << std::endl; > > - goto out; > > - } > > + requests.push_back(request); > > } > > > > - std::cout << "Capture until user interrupts by SIGINT" << std::endl; > > - > > ret = camera->start(); > > if (ret) { > > std::cout << "Failed to start capture" << std::endl; > > goto out; > > } > > > > + for (Request *request : requests) { > > + ret = camera->queueRequest(request); > > + if (ret < 0) { > > + std::cerr << "Can't queue request" << std::endl; > > + goto out; > > + } > > + } > > + > > + std::cout << "Capture until user interrupts by SIGINT" << std::endl; > > ret = loop->exec(); > > > > ret = camera->stop(); > > -- > Regards, > > Laurent Pinchart
Hi Niklas, On Wed, Feb 27, 2019 at 07:51:28PM +0100, Niklas Söderlund wrote: > On 2019-02-27 15:04:32 +0200, Laurent Pinchart wrote: > > On Tue, Feb 26, 2019 at 03:18:53AM +0100, Niklas Söderlund wrote: > >> Upcoming enforcing of order the camera shall be operate on is not > >> compatible with the cam utility. Requests shall be queued after the > >> camera is started, not before. > > > > What are the practical implications of this on camera operation, from an > > application point of view ? In particular, when will the acquisition be > > started after this change ? Will the pipeline handlers be required to > > start acquisition on the sensor at start() time, throwing away all > > images until requests are queued ? Or will acquisition start when the > > first request is queued ? Sensors can take a long time to start, so it's > > important to document the exact behaviour. > > My understanding of the situation is that at start() time > VIDIOC_STREAMON shall be called. Then it's currently not defined what > other actions are expected by the camera or pipeline handler when > processing start(). I agree this need to be defined. > > I see two possible scenarios. > > 1. The library grantees that once a request have been queued it will > eventually complete disregarding of more requests are queued or not. I think we need to guarantee that once a request has been queued it will complete ASAP, regardless of if more requests are queued. Results may not be optimal, but the request should complete nonetheless. > 2. The library informs the application about the request queue depth it > needs before requests starts to be processed. That should be done too, as applications should try to fill the processing pipeline with request for optimal operation. > I'm hoping we can do scenario 1 but not sure if we can do that without > ugly scratch buffers in the library which I feel is a bit of a hack. Scratch buffers may be needed, depending on the hardware, and I don't think that would really be a hack, more the implementation of a best effort mode when the system becomes too slow to keep up. > Whatever scenario we pick the pipeline handler would need to know about > the request queue depth and act on that or report it to the application > so it can act. The pipeline handlers do not know this property today so > not much we can do yet. It doesn't matter much today given our simple pipeline handlers :-) None of them require any particular queue depth. It will soon become an issue we will need to tackle though. > In any case I think this is outside the scope of this patch. This > problem exists disregarding if we only allow queueing requests after > start() or if allow it both before and after. In fact I thought of this > change as a first step to address this issue as we now define that > requests can only be queued after start(). My point was that we need to document the behaviour related to stream start. As explained, sensors can take a long time to start. Do we guarantee that this delay will be synchronous with the start() call, or do we push it to the time when the first request(s) are queued ? In the former case, can we guarantee that pipeline handlers will always be able to do so even without buffers queued ? If not, we have a problem, and need to reconsider the queue-after-start order. > >> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > >> --- > >> src/cam/main.cpp | 18 +++++++++++------- > >> 1 file changed, 11 insertions(+), 7 deletions(-) > >> > >> diff --git a/src/cam/main.cpp b/src/cam/main.cpp > >> index 4c2df583fe8e99b7..8df8844c33a2d052 100644 > >> --- a/src/cam/main.cpp > >> +++ b/src/cam/main.cpp > >> @@ -133,6 +133,7 @@ static int capture() > >> int ret; > >> > >> std::vector<Stream *> streams = camera->streams(); > >> + std::vector<Request *> requests; > >> > >> ret = configureStreams(camera.get(), streams); > >> if (ret < 0) { > >> @@ -169,21 +170,24 @@ static int capture() > >> goto out; > >> } > >> > >> - ret = camera->queueRequest(request); > >> - if (ret < 0) { > >> - std::cerr << "Can't queue request" << std::endl; > >> - goto out; > >> - } > >> + requests.push_back(request); > >> } > >> > >> - std::cout << "Capture until user interrupts by SIGINT" << std::endl; > >> - > >> ret = camera->start(); > >> if (ret) { > >> std::cout << "Failed to start capture" << std::endl; > >> goto out; > >> } > >> > >> + for (Request *request : requests) { > >> + ret = camera->queueRequest(request); > >> + if (ret < 0) { > >> + std::cerr << "Can't queue request" << std::endl; > >> + goto out; > >> + } > >> + } > >> + > >> + std::cout << "Capture until user interrupts by SIGINT" << std::endl; > >> ret = loop->exec(); > >> > >> ret = camera->stop();
diff --git a/src/cam/main.cpp b/src/cam/main.cpp index 4c2df583fe8e99b7..8df8844c33a2d052 100644 --- a/src/cam/main.cpp +++ b/src/cam/main.cpp @@ -133,6 +133,7 @@ static int capture() int ret; std::vector<Stream *> streams = camera->streams(); + std::vector<Request *> requests; ret = configureStreams(camera.get(), streams); if (ret < 0) { @@ -169,21 +170,24 @@ static int capture() goto out; } - ret = camera->queueRequest(request); - if (ret < 0) { - std::cerr << "Can't queue request" << std::endl; - goto out; - } + requests.push_back(request); } - std::cout << "Capture until user interrupts by SIGINT" << std::endl; - ret = camera->start(); if (ret) { std::cout << "Failed to start capture" << std::endl; goto out; } + for (Request *request : requests) { + ret = camera->queueRequest(request); + if (ret < 0) { + std::cerr << "Can't queue request" << std::endl; + goto out; + } + } + + std::cout << "Capture until user interrupts by SIGINT" << std::endl; ret = loop->exec(); ret = camera->stop();
Upcoming enforcing of order the camera shall be operate on is not compatible with the cam utility. Requests shall be queued after the camera is started, not before. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/cam/main.cpp | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)