On Thu, 2018-02-01 at 16:40 +0000, Frediano Ziglio wrote:
> Check if the current server is active or not and stream only if active.
> This will allow to support multiple servers running.
> When multiple servers are running only one have the GPU associated
> and is writing to the screen.
> Stop capturing and release resources when the server is not active
> and vice versa.
In every place in this patch series where you talk about 'server', you
should replace it with 'display server' (or possibly just 'X server',
not sure if Wayland is ever going to apply here?). I was confused for
quite some time thinking you mean the SPICE server.
> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> ---
> src/spice-streaming-agent.cpp | 99 ++++++++++++++++++++++++++++++++++---------
> 1 file changed, 80 insertions(+), 19 deletions(-)
>
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index a688d1f..2c32070 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -16,12 +16,14 @@
> #include <poll.h>
> #include <syslog.h>
> #include <signal.h>
> +#include <sys/eventfd.h>
> #include <exception>
> #include <stdexcept>
> #include <memory>
> #include <mutex>
> #include <thread>
> #include <vector>
> +#include <atomic>
> #include <functional>
> #include <X11/Xlib.h>
> #include <X11/extensions/Xfixes.h>
> @@ -57,19 +59,25 @@ static int streaming_requested;
> static std::set<SpiceVideoCodecType> client_codecs;
> static bool quit;
> static int streamfd = -1;
> +static int update_fd = -1;
> static bool stdin_ok;
> static int log_binary = 0;
> static std::mutex stream_mtx;
> +static const char vt_property_name[] = "XFree86_has_VT";
const char[] here, std::string on the line below :) Just use
std::string...
> +static string streamport = "/dev/virtio-ports/com.redhat.stream.0";
> +static Atom vt_property = None;
> +static std::atomic_bool vt_active;
These static variables... Not too sure about C, in C++ it is considered
bad practice. Not an objection here at the moment, since there are some
already present, but in the future the way to go is to create a class
(or more) to hold them and properly encapsulate them.
> static int have_something_to_read(int *pfd, int timeout)
> {
> int nfds;
> - struct pollfd pollfds[2] = {
> + struct pollfd pollfds[3] = {
> {streamfd, POLLIN, 0},
> + {update_fd, POLLIN, 0},
> {0, POLLIN, 0}
> };
> *pfd = -1;
> - nfds = (stdin_ok ? 2 : 1);
> + nfds = (stdin_ok ? 3 : 2);
> if (poll(pollfds, nfds, timeout) < 0) {
> syslog(LOG_ERR, "poll FAILED\n");
> return -1;
> @@ -78,6 +86,9 @@ static int have_something_to_read(int *pfd, int timeout)
> *pfd = streamfd;
> }
> if (pollfds[1].revents == POLLIN) {
> + *pfd = update_fd;
> + }
> + if (pollfds[2].revents == POLLIN) {
> *pfd = 0;
> }
> return *pfd != -1;
> @@ -152,6 +163,28 @@ static int read_command_from_device(void)
> return 1;
> }
>
> +static void start_capture()
> +{
> + for (int attempts = 0; ; ) {
> + streamfd = open(streamport.c_str(), O_RDWR);
> + if (streamfd >= 0)
> + break;
> + if (++attempts > 10)
> + throw std::runtime_error("failed to open the streaming device (" +
> + streamport + "): " + strerror(errno));
It would be good to log the failed attempts. As a side note, we should
look for a logging library for C++.
> + usleep(50 * 1000);
> + }
> +}
> +
> +static void stop_capture()
> +{
> + if (streamfd) {
> + close(streamfd);
> + streamfd = -1;
> + }
> + streaming_requested = 0;
> +}
> +
> static int read_command(bool blocking)
> {
> int fd, n=1;
> @@ -164,8 +197,17 @@ static int read_command(bool blocking)
> sleep(1);
> continue;
> }
> - if (fd) {
> + if (fd == streamfd) {
> n = read_command_from_device();
> + } else if (fd == update_fd) {
> + uint64_t n;
> + read(update_fd, &n, sizeof(n));
> + bool vt_active = ::vt_active.load(std::memory_order_relaxed);
> + if (vt_active) {
> + start_capture();
> + } else {
> + stop_capture();
> + }
> } else {
> n = read_command_from_stdin();
> }
> @@ -328,10 +370,23 @@ send_cursor(unsigned width, unsigned height, int hotspot_x, int hotspot_y,
> static void cursor_changes(Display *display, int event_base)
> {
> unsigned long last_serial = 0;
> + Window rootwindow = DefaultRootWindow(display);
>
> while (1) {
> XEvent event;
> XNextEvent(display, &event);
> +
> + if (event.type == PropertyNotify) {
> + if (vt_property == None || event.xproperty.atom != vt_property)
> + continue;
> + // update vt property, activate screen read if needed
> + vt_active.store(get_win_prop_int(display, rootwindow, vt_property) != 0, std::memory_order_relaxed);
> + std::atomic_thread_fence(std::memory_order_acquire);
> + uint64_t n = 1;
> + write(update_fd, &n, sizeof(n));
What does writing '1' to update_fd mean? (rhetorical question :)) You
do not even check the value in the place you read it from update_fd. I
understand this is sufficient and works atm., but this way you include
random communication details in deferent places of the code, it is not
clean.
This communication should be encapsulated, in C++ for example in a
class that would have a method like toggle_streaming(), which would
send the '1' and in the place where you read the command, you would use
the same class to do so.
> + continue;
> + }
> +
> if (event.type != event_base + 1)
> continue;
>
> @@ -352,19 +407,14 @@ static void cursor_changes(Display *display, int event_base)
> }
>
> static void
> -do_capture(const string &streamport, FILE *f_log)
> +do_capture(FILE *f_log)
> {
> - streamfd = open(streamport.c_str(), O_RDWR);
> - if (streamfd < 0)
> - throw std::runtime_error("failed to open the streaming device (" +
> - streamport + "): " + strerror(errno));
> -
> unsigned int frame_count = 0;
> while (! quit) {
> while (!quit && !streaming_requested) {
> if (read_command(true) < 0) {
> syslog(LOG_ERR, "FAILED to read command\n");
> - goto done;
> + return;
> }
> }
>
> @@ -422,23 +472,16 @@ do_capture(const string &streamport, FILE *f_log)
> //usleep(1);
> if (read_command(false) < 0) {
> syslog(LOG_ERR, "FAILED to read command\n");
> - goto done;
> + return;
> }
> }
> }
> -
> -done:
> - if (streamfd >= 0) {
> - close(streamfd);
> - streamfd = -1;
> - }
> }
>
> #define arg_error(...) syslog(LOG_ERR, ## __VA_ARGS__);
>
> int main(int argc, char* argv[])
> {
> - string streamport = "/dev/virtio-ports/com.redhat.stream.0";
> char opt;
> const char *log_filename = NULL;
> int logmask = LOG_UPTO(LOG_WARNING);
> @@ -492,6 +535,12 @@ int main(int argc, char* argv[])
>
> agent.LoadPlugins(PLUGINSDIR);
>
> + update_fd = eventfd(0, EFD_CLOEXEC|EFD_NONBLOCK);
> + if (update_fd < 0) {
> + syslog(LOG_ERR, "FAILED to create eventfd\n");
> + return EXIT_FAILURE;
> + }
> +
> register_interrupts();
>
> FILE *f_log = NULL;
> @@ -517,12 +566,24 @@ int main(int argc, char* argv[])
> Window rootwindow = DefaultRootWindow(display);
> XFixesSelectCursorInput(display, rootwindow, XFixesDisplayCursorNotifyMask);
>
> + vt_property = XInternAtom(display, vt_property_name, True);
> + if (vt_property == None) {
> + syslog(LOG_ERR, "VT property not found\n");
> + return EXIT_FAILURE;
> + }
> + XSelectInput(display, rootwindow, PropertyChangeMask);
> +
> + vt_active.store(get_win_prop_int(display, rootwindow, vt_property) != 0, std::memory_order_relaxed);
> +
> std::thread cursor_th(cursor_changes, display, event_base);
> cursor_th.detach();
>
> int ret = EXIT_SUCCESS;
> try {
> - do_capture(streamport, f_log);
> + if (::vt_active.load(std::memory_order_relaxed))
> + start_capture();
> + do_capture(f_log);
> + stop_capture();
> }
> catch (std::runtime_error &err) {
> syslog(LOG_ERR, "%s\n", err.what());
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel