>
> The FrameLog class provides RAII for the FILE and encapsulates the
> logging functionality.
>
> Signed-off-by: Lukáš Hrázký <lhrazky@xxxxxxxxxx>
Why "FrameLog" if, as you said too, is managing two types of logs?
> ---
> While I retained all the behaviour of the original implementation, I
> don't like it as it is.
>
> 1. The class is doing two different things:
> a) logging the binary frames for later replay
> b) writing a time-stamped log for diagnostics
>
You can have both outputs on the same file.
> 2. The time-stamped diagnostics lines seem quite rudimentary, like
> some short helper messages I put in my code when debugging a problem.
> Doesn't look like something we'd want to release, god forbid we'd have
> to keep the log format backwards compatible in the future :)
>
I don't think nobody is planning to have is backward compatible and as
far as I know this is common for logging. Probably for the same reasoning
you should remove logging from 70% of the applications...
> However, I don't know how the frame log is used (I guess it's mostly
> Frediano who uses it?), so I didn't try to improve it, as I don't really
> know how. I'd like to:
>
Uri used the full binary to replay as videos.
I use for statistics like frame average in bytes or average time to
process a frame and such things.
> a) either get some input on how to improve it - address the two mentioned
> issues, or (much rather)
>
Issues?? As said looks like issues of probably 70% applications of the world.
> b) merge this separation and have someone post a followup that cleans it
> up.
>
Cleans what? Looks like you are suggesting to handle binary file and
other logs as completely separate thing. I don't see this as a cleanup,
more a split.
>
> src/Makefile.am | 2 +
> src/frame-log.cpp | 76 +++++++++++++++++++++++++++++++++++
> src/frame-log.hpp | 39 ++++++++++++++++++
> src/spice-streaming-agent.cpp | 71 ++++++++------------------------
> 4 files changed, 134 insertions(+), 54 deletions(-)
> create mode 100644 src/frame-log.cpp
> create mode 100644 src/frame-log.hpp
>
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 18ed22c..fead680 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -54,6 +54,8 @@ spice_streaming_agent_SOURCES = \
> concrete-agent.hpp \
> error.cpp \
> error.hpp \
> + frame-log.cpp \
> + frame-log.hpp \
> mjpeg-fallback.cpp \
> mjpeg-fallback.hpp \
> jpeg.cpp \
> diff --git a/src/frame-log.cpp b/src/frame-log.cpp
> new file mode 100644
> index 0000000..b0bd09e
> --- /dev/null
> +++ b/src/frame-log.cpp
> @@ -0,0 +1,76 @@
> +/* A utility logger for logging frames and/or time information.
> + *
> + * \copyright
> + * Copyright 2018 Red Hat Inc. All rights reserved.
When you move code you have to retain the original copyright
(in this case would be 2016-2018 as the rest is the same).
> + */
> +
> +#include "frame-log.hpp"
> +
> +#include "error.hpp"
> +#include "hexdump.h"
> +
> +#include <cstdarg>
> +#include <string.h>
> +#include <sys/time.h>
> +
> +
> +namespace spice {
> +namespace streaming_agent {
> +
> +FrameLog::FrameLog(const char *log_name, bool log_binary, bool log_frames) :
> + log_binary(log_binary),
> + log_frames(log_frames)
> +{
> + if (log_name) {
> + log_file = fopen(log_name, "wb");
> + if (!log_file) {
> + throw Error(std::string("Failed to open log file '") + log_name
> + "': " + strerror(errno));
> + }
> + if (!log_binary) {
> + setlinebuf(log_file);
> + }
> + }
> +}
> +
> +FrameLog::~FrameLog()
> +{
> + if (log_file) {
> + fclose(log_file);
> + }
> +}
> +
> +void FrameLog::log_stat(const char* format, ...)
> +{
> + if (log_file && !log_binary) {
> + fprintf(log_file, "%" PRIu64 ": ", get_time());
> + va_list ap;
> + va_start(ap, format);
> + vfprintf(log_file, format, ap);
> + va_end(ap);
> + fputs("\n", log_file);
> + }
> +}
> +
> +void FrameLog::log_frame(const void* buffer, size_t buffer_size)
> +{
> + if (log_file) {
> + if (log_binary) {
> + fwrite(buffer, buffer_size, 1, log_file);
> + } else if (log_frames) {
> + hexdump(buffer, buffer_size, log_file);
> + }
> + }
> +}
> +
> +/**
> + * Returns current time in microseconds.
> + */
> +uint64_t FrameLog::get_time()
> +{
> + struct timeval now;
> + gettimeofday(&now, NULL);
> +
> + return (uint64_t)now.tv_sec * 1000000 + (uint64_t)now.tv_usec;
> +}
> +
> +}} // namespace spice::streaming_agent
> diff --git a/src/frame-log.hpp b/src/frame-log.hpp
> new file mode 100644
> index 0000000..2ebe01b
> --- /dev/null
> +++ b/src/frame-log.hpp
> @@ -0,0 +1,39 @@
> +/* A utility logger for logging frames and/or time information.
> + *
> + * \copyright
> + * Copyright 2018 Red Hat Inc. All rights reserved.
> + */
> +
> +#ifndef SPICE_STREAMING_AGENT_FRAME_LOG_HPP
> +#define SPICE_STREAMING_AGENT_FRAME_LOG_HPP
> +
> +#include <cinttypes>
> +#include <stddef.h>
> +#include <stdio.h>
> +
> +
> +namespace spice {
> +namespace streaming_agent {
> +
> +class FrameLog {
> +public:
> + FrameLog(const char *log_name, bool log_binary, bool log_frames);
> + FrameLog(const FrameLog &) = delete;
> + FrameLog &operator=(const FrameLog &) = delete;
> + ~FrameLog();
> +
> + __attribute__ ((format (printf, 2, 3)))
> + void log_stat(const char* format, ...);
> + void log_frame(const void* buffer, size_t buffer_size);
> +
> + static uint64_t get_time();
> +
> +private:
> + FILE *log_file = NULL;
nullptr?
> + bool log_binary = false;
> + bool log_frames = false;
> +};
> +
> +}} // namespace spice::streaming_agent
> +
> +#endif // SPICE_STREAMING_AGENT_FRAME_LOG_HPP
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 1121f35..aef49c7 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -5,8 +5,8 @@
> */
>
> #include "concrete-agent.hpp"
> -#include "hexdump.h"
> #include "mjpeg-fallback.hpp"
> +#include "frame-log.hpp"
> #include "stream-port.hpp"
> #include "error.hpp"
>
> @@ -57,8 +57,6 @@ struct SpiceStreamDataMessage
>
> static bool streaming_requested = false;
> static bool quit_requested = false;
> -static bool log_binary = false;
> -static bool log_frames = false;
> static std::set<SpiceVideoCodecType> client_codecs;
>
> static bool have_something_to_read(StreamPort &stream_port, bool blocking)
> @@ -221,17 +219,6 @@ static void spice_stream_send_frame(StreamPort
> &stream_port, const void *buf, co
> syslog(LOG_DEBUG, "Sent a frame of size %u\n", size);
> }
>
> -/* returns current time in micro-seconds */
> -static uint64_t get_time(void)
> -{
> - struct timeval now;
> -
> - gettimeofday(&now, NULL);
> -
> - return (uint64_t)now.tv_sec * 1000000 + (uint64_t)now.tv_usec;
> -
> -}
> -
> static void handle_interrupt(int intr)
> {
> syslog(LOG_INFO, "Got signal %d, exiting", intr);
> @@ -330,14 +317,8 @@ static void cursor_changes(StreamPort *stream_port,
> Display *display, int event_
> }
> }
>
> -#define STAT_LOG(format, ...) do { \
> - if (f_log && !log_binary) { \
> - fprintf(f_log, "%" PRIu64 ": " format "\n", get_time(), ##
> __VA_ARGS__); \
> - } \
> -} while(0)
> -
This could have performance impact. Don't think is a problem with
the current code.
> static void
> -do_capture(StreamPort &stream_port, FILE *f_log)
> +do_capture(StreamPort &stream_port, FrameLog &frame_log)
> {
> unsigned int frame_count = 0;
> while (!quit_requested) {
> @@ -361,13 +342,13 @@ do_capture(StreamPort &stream_port, FILE *f_log)
> if (++frame_count % 100 == 0) {
> syslog(LOG_DEBUG, "SENT %d frames\n", frame_count);
> }
> - uint64_t time_before = get_time();
> + uint64_t time_before = FrameLog::get_time();
>
> - STAT_LOG("Capturing...");
> + frame_log.log_stat("Capturing...");
> FrameInfo frame = capture->CaptureFrame();
> - STAT_LOG("Captured");
> + frame_log.log_stat("Captured");
>
> - uint64_t time_after = get_time();
> + uint64_t time_after = FrameLog::get_time();
> syslog(LOG_DEBUG,
> "got a frame -- size is %zu (%" PRIu64 " ms) "
> "(%" PRIu64 " ms from last frame)(%" PRIu64 " us)\n",
> @@ -385,18 +366,12 @@ do_capture(StreamPort &stream_port, FILE *f_log)
> codec = capture->VideoCodecType();
>
> syslog(LOG_DEBUG, "wXh %uX%u codec=%u\n", width, height,
> codec);
> - STAT_LOG("Started new stream wXh %uX%u codec=%u", width,
> height, codec);
> + frame_log.log_stat("Started new stream wXh %uX%u codec=%u",
> width, height, codec);
>
> spice_stream_send_format(stream_port, width, height, codec);
> }
> - STAT_LOG("Frame of %zu bytes:", frame.buffer_size);
> - if (f_log) {
> - if (log_binary) {
> - fwrite(frame.buffer, frame.buffer_size, 1, f_log);
> - } else if (log_frames) {
> - hexdump(frame.buffer, frame.buffer_size, f_log);
> - }
> - }
> + frame_log.log_stat("Frame of %zu bytes:", frame.buffer_size);
> + frame_log.log_frame(frame.buffer, frame.buffer_size);
>
> try {
> spice_stream_send_frame(stream_port, frame.buffer,
> frame.buffer_size);
> @@ -404,7 +379,7 @@ do_capture(StreamPort &stream_port, FILE *f_log)
> syslog(e);
> break;
> }
> - STAT_LOG("Sent");
> + frame_log.log_stat("Sent");
>
> read_command(stream_port, false);
> }
> @@ -418,6 +393,8 @@ int main(int argc, char* argv[])
> const char *stream_port_name =
> "/dev/virtio-ports/org.spice-space.stream.0";
> int opt;
> const char *log_filename = NULL;
> + bool log_binary = false;
> + bool log_frames = false;
> int logmask = LOG_UPTO(LOG_WARNING);
> const char *pluginsdir = PLUGINSDIR;
> enum {
> @@ -493,20 +470,10 @@ int main(int argc, char* argv[])
>
> register_interrupts();
>
> - FILE *f_log = NULL;
> - if (log_filename) {
> - f_log = fopen(log_filename, "wb");
> - if (!f_log) {
> - syslog(LOG_ERR, "Failed to open log file '%s': %s\n",
> - log_filename, strerror(errno));
> - return EXIT_FAILURE;
> - }
> - if (!log_binary) {
> - setlinebuf(f_log);
> - }
> - for (const std::string& arg: old_args) {
> - STAT_LOG("Args: %s", arg.c_str());
> - }
> + FrameLog frame_log(log_filename, log_binary, log_frames);
> +
> + for (const std::string& arg: old_args) {
> + frame_log.log_stat("Args: %s", arg.c_str());
> }
> old_args.clear();
>
> @@ -531,17 +498,13 @@ int main(int argc, char* argv[])
> std::thread cursor_th(cursor_changes, &stream_port, display,
> event_base);
> cursor_th.detach();
>
> - do_capture(stream_port, f_log);
> + do_capture(stream_port, frame_log);
> }
> catch (std::exception &err) {
> syslog(LOG_ERR, "%s\n", err.what());
> ret = EXIT_FAILURE;
> }
>
> - if (f_log) {
> - fclose(f_log);
> - f_log = NULL;
> - }
> closelog();
> return ret;
> }
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel