[libcamera-devel] [PATCH 8/8] cam: Add support for viewfinder through DRM/KMS

[Laurent Pinchart]

Hi Kieran,

On Tue, May 19, 2020 at 09:35:59AM +0100, Kieran Bingham wrote:
> Hi Laurent,
> 
> On 19/05/2020 04:25, Laurent Pinchart wrote:
> > Use the KMSSink class to display the viewfinder stream, if any, through
> > DRM/KMS. The output connector is selected through the new -D/--display
> > argument.
> 
> Fantastic. Live previewing from non-GUI system is really helpful.
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  src/cam/capture.cpp | 21 +++++++++++++++++++++
> >  src/cam/main.cpp    | 11 +++++++++++
> >  src/cam/main.h      |  1 +
> >  3 files changed, 33 insertions(+)
> > 
> > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> > index 6982d89fabe7..5b25b2c239f8 100644
> > --- a/src/cam/capture.cpp
> > +++ b/src/cam/capture.cpp
> > @@ -13,6 +13,9 @@
> >  
> >  #include "capture.h"
> >  #include "file_sink.h"
> > +#ifdef HAVE_KMS
> 
> Where is this defined?
> Should there be an addition to meson.build?

In patch 5/8.

> > +#include "kms_sink.h"
> > +#endif
> >  #include "main.h"
> >  
> >  using namespace libcamera;
> > @@ -46,6 +49,24 @@ int Capture::run(EventLoop *loop, const OptionsParser::Options &options)
> >  
> >  	camera_->requestCompleted.connect(this, &Capture::requestComplete);
> >  
> > +#ifdef HAVE_KMS
> 
> Could we avoid ifdefs if we define a c-like macro in kms_sink.h to
> retain compile time checks?
> 
> > +	if (options.isSet(OptDisplay)) {
> 
> So we get something like:
> 	if (options.isSet(OptDisplay) && have_kms()) {

Except that we would link against the KMSSink class API, even if we
never call it, resulting in link errors. To work around that we could
have a dummy implementation when !HAVE_KMS, but we would then test the
dummy implementation, not the real one. Its API may differ slightly (it
shouldn't of course, but mistakes happen), so I'm not sure it really
gives us much.

> > +		if (config_->size() != 1) {
> > +			std::cout << "Display doesn't support multiple streams"
> > +				  << std::endl;
> > +			return -EINVAL;
> > +		}
> > +
> > +		if (roles_[0] != StreamRole::Viewfinder) {
> > +			std::cout << "Display requires a viewfinder stream"
> > +				  << std::endl;
> > +			return -EINVAL;
> > +		}
> > +
> > +		sink_ = new KMSSink(options[OptDisplay].toString());
> > +	}
> > +#endif
> > +
> >  	if (options.isSet(OptFile)) {
> >  		if (!options[OptFile].toString().empty())
> >  			sink_ = new FileSink(options[OptFile]);
> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> > index cdd29d500202..5cd1e929bd88 100644
> > --- a/src/cam/main.cpp
> > +++ b/src/cam/main.cpp
> > @@ -78,6 +78,12 @@ int CamApp::init(int argc, char **argv)
> >  	if (ret < 0)
> >  		return ret;
> >  
> > +	if (options_.isSet(OptDisplay) && options_.isSet(OptFile)) {
> > +		std::cout << "--display and --file options are mutually exclusive"
> > +			  << std::endl;
> 
> Is this a performance restriction? Or because we can only have one
> FrameSink currently.

The latter. This can be extended of course, but I wanted to start
simple.

> I mean we don't necessarily want to be writing lots (thousands) of RAW
> frames while a display is running, but *viewing* the 'N<20' raw frames
> while they are being captured to disk is useful (to see them live before
> you further inspect the files).

And even capture frames when pressing enter. Possibilities are endless
;-) I'm sure this can be done, but I'd prefer doing it on top of this
series.

> > +		return -EINVAL;
> > +	}
> > +
> >  	cm_ = new CameraManager();
> >  
> >  	ret = cm_->start();
> > @@ -164,6 +170,11 @@ int CamApp::parseOptions(int argc, char *argv[])
> >  			 ArgumentRequired, "camera");
> >  	parser.addOption(OptCapture, OptionNone,
> >  			 "Capture until interrupted by user", "capture");
> > +#ifdef HAVE_KMS
> > +	parser.addOption(OptDisplay, OptionString,
> > +			 "Display viewfinder through DRM/KMS on specified connector",
> > +			 "display", ArgumentOptional, "connector");
> > +#endif
> >  	parser.addOption(OptFile, OptionString,
> >  			 "Write captured frames to disk\n"
> >  			 "The first '#' character in the file name is expanded to the stream name and frame sequence number.\n"
> > diff --git a/src/cam/main.h b/src/cam/main.h
> > index 4a130d8dd290..57eece507a16 100644
> > --- a/src/cam/main.h
> > +++ b/src/cam/main.h
> > @@ -10,6 +10,7 @@
> >  enum {
> >  	OptCamera = 'c',
> >  	OptCapture = 'C',
> > +	OptDisplay = 'D',
> >  	OptFile = 'F',
> >  	OptHelp = 'h',
> >  	OptInfo = 'I',

-- 
Regards,

Laurent Pinchart