[Spice-devel] [PATCH spice-streaming-agent 1/2] mjpeg plugin: split off the declaration into a header

[Lukáš Hrázký]

On Tue, 2018-01-30 at 06:32 -0500, Frediano Ziglio wrote:
> > 
> > So that the plugin can later be included in a unit test.
> > 
> > Signed-off-by: Lukáš Hrázký <lhrazky at redhat.com>
> > ---
> >  src/mjpeg-fallback.cpp | 26 ++++++--------------------
> >  src/mjpeg-fallback.hpp | 34 ++++++++++++++++++++++++++++++++++
> >  2 files changed, 40 insertions(+), 20 deletions(-)
> >  create mode 100644 src/mjpeg-fallback.hpp
> > 
> > diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> > index f41e68a..fac34bc 100644
> > --- a/src/mjpeg-fallback.cpp
> > +++ b/src/mjpeg-fallback.cpp
> > @@ -4,6 +4,8 @@
> >   * Copyright 2017 Red Hat Inc. All rights reserved.
> >   */
> >  
> > +#include "mjpeg-fallback.hpp"
> > +
> >  #include <config.h>
> >  #include <cstring>
> >  #include <exception>
> > @@ -13,9 +15,6 @@
> >  #include <syslog.h>
> >  #include <X11/Xlib.h>
> >  
> > -#include <spice-streaming-agent/plugin.hpp>
> > -#include <spice-streaming-agent/frame-capture.hpp>
> > -
> >  #include "static-plugin.hpp"
> >  #include "jpeg.hpp"
> >  
> 
> The include order looks weird.
> Surely you want the config include before anything else as it could
> change some system header behaviour.
> But I think including mjpeg-fallback.hpp after the system headers
> here is more coherent.

This is an interesting topic :)

The reason for including the header corresponding to a .cpp file first
in it is that you will get undefined symbol errors if you are missing
includes in the header which are present in the .cpp (and which you
would put in front of it). This is described for example in the Google
C++ Style Guide [1].

If there's something I should be aware of in C, please educate me :)

> > @@ -41,11 +40,6 @@ static inline uint64_t get_time()
> >  }
> >  
> >  namespace {
> > -struct MjpegSettings
> > -{
> > -    int fps;
> > -    int quality;
> > -};
> >  
> >  class MjpegFrameCapture final: public FrameCapture
> >  {
> > @@ -69,18 +63,6 @@ private:
> >      uint64_t last_time = 0;
> >  };
> >  
> > -class MjpegPlugin final: public Plugin
> > -{
> > -public:
> > -    FrameCapture *CreateCapture() override;
> > -    unsigned Rank() override;
> > -    void ParseOptions(const ConfigureOption *options);
> > -    SpiceVideoCodecType VideoCodecType() const {
> > -        return SPICE_VIDEO_CODEC_TYPE_MJPEG;
> > -    }
> > -private:
> > -    MjpegSettings settings = { 10, 80 };
> > -};
> >  }
> >  
> >  MjpegFrameCapture::MjpegFrameCapture(const MjpegSettings&
> > settings):
> > @@ -205,6 +187,10 @@ void MjpegPlugin::ParseOptions(const
> > ConfigureOption
> > *options)
> >      }
> >  }
> >  
> > +SpiceVideoCodecType MjpegPlugin::VideoCodecType() const {
> > +    return SPICE_VIDEO_CODEC_TYPE_MJPEG;
> > +}
> > +
> >  static bool
> >  mjpeg_plugin_init(Agent* agent)
> >  {
> > diff --git a/src/mjpeg-fallback.hpp b/src/mjpeg-fallback.hpp
> > new file mode 100644
> > index 0000000..8044244
> > --- /dev/null
> > +++ b/src/mjpeg-fallback.hpp
> > @@ -0,0 +1,34 @@
> > +/* Plugin implementation for Mjpeg
> > + *
> > + * \copyright
> > + * Copyright 2017 Red Hat Inc. All rights reserved.
> > + */
> > +#ifndef SPICE_STREAMING_AGENT_MJPEG_FALLBACK_HPP
> > +#define SPICE_STREAMING_AGENT_MJPEG_FALLBACK_HPP
> > +
> > +#include <spice-streaming-agent/plugin.hpp>
> > +#include <spice-streaming-agent/frame-capture.hpp>
> > +
> > +
> > +namespace SpiceStreamingAgent {
> > +
> > +struct MjpegSettings
> > +{
> > +    int fps;
> > +    int quality;
> > +};
> > +
> > +class MjpegPlugin final: public Plugin
> > +{
> > +public:
> > +    FrameCapture *CreateCapture() override;
> > +    unsigned Rank() override;
> > +    void ParseOptions(const ConfigureOption *options);
> > +    SpiceVideoCodecType VideoCodecType() const;
> > +private:
> > +    MjpegSettings settings = { 10, 80 };
> > +};
> > +
> > +} // namespace SpiceStreamingAgent
> > +
> > +#endif // SPICE_STREAMING_AGENT_MJPEG_FALLBACK_HPP
> 
> Frediano

Lukas


[1] https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes