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

[Laurent Pinchart]

Hi Niklas,

On Wed, Apr 03, 2019 at 01:52:18AM +0200, Niklas Söderlund wrote:
> On 2019-04-02 18:06:54 +0300, Laurent Pinchart wrote:
> > 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.

I think it's worth a try, let's see how it will look like in
applications.

-- 
Regards,

Laurent Pinchart