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

[Kieran Bingham]

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?

> +#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()) {

> +		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.

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).

> +		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
--
Kieran