[libcamera-devel,8/8] cam: Add support for viewfinder through DRM/KMS
diff mbox series

Message ID 20200519032505.17307-9-laurent.pinchart@ideasonboard.com
State Changes Requested
Delegated to: Laurent Pinchart
Headers show
Series
  • libcamera: Add DRM/KMS viewfinder display to cam
Related show

Commit Message

Laurent Pinchart May 19, 2020, 3:25 a.m. UTC 
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.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/cam/capture.cpp | 21 +++++++++++++++++++++
 src/cam/main.cpp    | 11 +++++++++++
 src/cam/main.h      |  1 +
 3 files changed, 33 insertions(+)

Comments

Kieran Bingham May 19, 2020, 8:35 a.m. UTC | #1 
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@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',
>
Laurent Pinchart May 19, 2020, 11:06 a.m. UTC | #2 
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@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',

Patch
diff mbox series 

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
+#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
+	if (options.isSet(OptDisplay)) {
+		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;
+		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',