[libcamera-devel] [RFC 5/5] libcamera: camera: Add support for stream usage hints

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2019-04-02 18:06:54 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Tue, Apr 02, 2019 at 02:53:32AM +0200, Niklas Söderlund wrote:
> > Instead of requesting the default configuration for a set of streams
> > where the application have to figure out which streams provided by the
> 
> s/have/has/
> 
> > camera is best suited for its intended usage have the library figure
> 
> s/usage/usage,/
> 
> > this out by using stream usage hints.
> > 
> > The application asks the library for a list of streams and a suggested
> > default configuration for them by supplying a list of stream usage
> > hints. Once the list is retrieved the application can try and fine-tune
> > the returned configuration and then try to apply it to the camera.
> > 
> > Currently no pipeline handler is prepared to handle usage hints but nor
> > did it make use of the list of Stream IDs which was the previous
> > interface. The main reason for this is that all cameras currently only
> > provide one stream each. This will still be the case but the API will be
> > prepared to expand both pipeline handlers and applications to support
> > multiple streams.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  include/libcamera/camera.h               |  3 ++-
> >  src/cam/main.cpp                         |  2 +-
> 
> qcam ? :-)
> 
> >  src/libcamera/camera.cpp                 | 30 ++++++++----------------
> >  src/libcamera/include/pipeline_handler.h |  2 +-
> >  src/libcamera/pipeline/ipu3/ipu3.cpp     |  4 ++--
> >  src/libcamera/pipeline/uvcvideo.cpp      |  6 ++---
> >  src/libcamera/pipeline/vimc.cpp          |  6 ++---
> >  src/libcamera/pipeline_handler.cpp       |  4 ++--
> >  src/qcam/main_window.cpp                 |  5 ++--

I see qcam being address in this patch ;-)

> >  test/camera/capture.cpp                  |  5 ++--
> >  test/camera/configuration_default.cpp    | 17 +++++---------
> >  test/camera/configuration_set.cpp        |  3 +--
> >  test/camera/statemachine.cpp             |  4 +---
> >  13 files changed, 34 insertions(+), 57 deletions(-)

[snip]

> >  /**
> > - * \brief Retrieve a group of stream configurations
> > - * \param[in] streams A map of stream IDs and configurations to setup
> > + * \brief Retrieve a group of stream configurations according to usage hints
> > + * \param[in] hints A list of stream usage hints
> >   *
> > - * Retrieve the camera's configuration for a specified group of streams. The
> > - * caller can specifies which of the camera's streams to retrieve configuration
> > - * from by populating \a streams.
> > + * Retrieve configuration for a set of desired usages. The caller specifies a
> > + * list of usage hints and the camera returns a map of suitable streams and
> > + * a suggested default configuration.
> 
> s/a suggested default configuration/their suggested default configurations/
> 
> We'll have to expand this eventually.

I agree this will probably needs to be expanded as we learn what pieces 
are missing.

> 
> There's already one problem I can identify : there's no way for the
> caller to associate streams with the usage hints. One option would be to
> return a std::vector<std::pair<Stream *, StreamConfiguration>> with the
> requirement that the returned vector will have the same order as the
> hints vector. Other options are possible too.

Good catch, for some reason I assumed this was already the case... 

I agree your suggestion of using std::vector<std::pair<>> would solve 
the issue but it's not a very nice interface for applications to use.  
After toying around a bit I think I found an other possible solution.

How about adding a Stream * to StreamConfiguration and returning a 
std::vector<StreamConfiguration> where the order is the same as the 
hints vector? It would also simplify the API towards applications which 
I really like.

The reason this is a std::map currently is due to our organic growth and 
that we used Stream * as the way to retrieve default configurations in a 
non so smart way as we always only had one stream that worked but now it 
seems less smart.

If you think this is a semi good idea I propose I add that on top of 
this change to ease review. Let me know what you think.

-- 
Regards,
Niklas Söderlund