[libcamera-devel] [PATCH 4/8] cam: fix order camera is operated on

[Niklas Söderlund]

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 at 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

-- 
Regards,
Niklas Söderlund