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

Lukáš Hrázký lhrazky at redhat.com
Thu Feb 1 13:40:15 UTC 2018 

On Thu, 2018-02-01 at 05:23 -0500, Frediano Ziglio wrote:
> > 
> > > On 31 Jan 2018, at 12:26, Frediano Ziglio <fziglio at redhat.com>
> > > wrote:
> > > 
> > > > > 
> > > > > On 30 Jan 2018, at 17:54, Frediano Ziglio <fziglio at redhat.com
> > > > > > wrote:
> > > > > 
> > > > > > > 
> > > > > > > On 30 Jan 2018, at 13:29, Lukáš Hrázký <lhrazky at redhat.co
> > > > > > > m> wrote:
> > > > > > > 
> > > > > > > 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 :)
> > > > > > 
> > > > > > +1 for Lukas’ rationale.
> > > > > > 
> > > > > > LLVM enforces this order too, see
> > > > > > https://llvm.org/docs/CodingStandards.html#include-style.
> > > > > > It’s actually enforced by clang-format in that case.
> > > > > > 
> > > > > 
> > > > > I think I inherited the order from many time ago, is a good
> > > > > point.
> > > > > In spice-server I have a script that creates for each header
> > > > > a C file
> > > > > that
> > > > > include a single header just to test that the headers can be
> > > > > included.
> > > > > Would save me this manual job! Note that the spice-server
> > > > > style is
> > > > > different
> > > > > but this does not mean cannot be changed.
> > > > > 
> > > > > > Also, it is weird is to include <config.h> and not
> > > > > > “config.h”… It’s a
> > > > > > project
> > > > > > header, not system header.
> > > > > > 
> > > > > 
> > > > > I think this inherited too but you are right, should be ""
> > > > > syntax.
> > > > > 
> > > > > > 
> > > > > > Thanks
> > > > > > Christophe
> > > > > > 
> > > > > 
> > > > > About including config.h first:
> > > > > 
> > > > > 
> > > > > Short explanation: can save lot of pain.
> > > > > 
> > > > > 
> > > > > Long explanation follow!
> > > > > 
> > > > > The config.h is generated by configure script for different
> > > > > reasons:
> > > > > - allow to configure some options;
> > > > > - detect available libraries;
> > > > > - detect portability issues.
> > > > 
> > > > I understand. But if you use these config.h features in some
> > > > header,
> > > > then it is my opinion that the header itself should include
> > > > config.h.
> > > > Any reason why not?
> > > > 
> > > 
> > > One reason is that if the header needs to be installed on the
> > > system
> > > should not include config.h as config.h should be usually used by
> > > your program to avoid changing other program API/ABI.
> > > Another is that you are not sure the file is included ASAP.
> 
> There was some discussion off list, for instance was mentioned:
> https://www.gnu.org/software/autoconf/manual/autoconf-2.66/html_node/
> Configuration-Headers.html
> 
> > If that’s the case, then your header is by definition not using
> > config.h,
> 
> yes
> 
> > and can therefore be safely placed before config.h in the .cpp
> > file.
> 
> no, if your header include some system header (even recursively)
> the definitions could change.
> 
> > I’d argue it should, since that allows you to check that some
> > config.h dependency did not mistakenly creep in.
> > 
> 
> yes, this would means including config.h at the beginning of all
> headers however as you don't want for public headers this would
> apply only to private ones that would seem a bit not coherent.
> 
> > > 
> > > Another question that come to my mind is why we use always
> > > "config.h"
> > > in the main directory instead for instance putting inside a
> > > include/my_program/config.h for example to be able to do a
> > > #include "my_program/config.h" with less conflicts possible.
> > 
> > I think this is good practice, since it makes it possible to
> > include your
> > code in a larger project, where the config.h may be generated at
> > a higher level (and selected by passing -I options).
> > 
> 
> The -I was mentioned also in autoconf manual linked above. They also
> mention that config.h is just the usual name.
> As we already have an include/spice-streaming-agent we could use
> that directory.
> 
> For the sake of this single patch I would start the .cpp file with
> 
> #include "config.h"
> #include "mjpeg-fallback.hpp"
> 
> or
> 
> #include <config.h>
> #include "mjpeg-fallback.hpp"
> 
> maybe better for consistency with the actual code the second option.
> 
> Frediano

Hi,

TLDR; I digress. Lets #include <config.h> at the beginning of every
.cpp file.

Unless we could actually make sure we don't need it at all, which might
well be the case (can anybody judge the scope of which platforms and
which libraries actually depend on some config.h definitions?).

I would also be very interested in a list of documented behaviour
changes dependent on the defines in config.h...


Read on for the gory details :)

If you look inside config.h, it actually contains following defines:

#define PACKAGE "spice-streaming-agent"
#define PACKAGE_BUGREPORT "spice-devel at lists.freedesktop.org"
#define PACKAGE_NAME "spice-streaming-agent"
// etc...

So actually packaging this file and including it in public headers is a
big nono.

It also includes defines like:

#define HAVE_STDINT_H 1
#define HAVE_STDLIB_H 1
#define HAVE_STRING_H 1

Which you can use in your code to make it portable. You may even want
to use these in your headers, so it makes sense to #include <config.h>
at the beginning of your headers. Except it would not be consistent
with your public headers (and so if you happen to have a .cpp which
only includes your public headers, you would still have to include
config.h in there). So that's why it seems best to include it at the
beggining of every .cpp.

You may also want to use the definitions from config.h in your public
headers. In that case you're out of luck and have to resolve to more
complex solutions, some described i.e. in [1].

For the issue that someone using your library with his own config.h
that could have different defines, which could create ABI
incompatibility between his code and your library, from what I gather
you just have to hope it will not happen on the same platform.

Small note about "" vs <>: It is actually recommended on the autoconf
doc page Frediano linked to use <>, which is better for the case you
have another config.h somewhere in the project. Myself I'd perhaps
still go for #include "config.h", as the header is local and we can
just not create another config.h ourselves to confuse things further.

Lukas

[1] https://stackoverflow.com/questions/28637542/ensure-config-h-is-included-once

> > > 
> > > I think that many of these syntaxes came from examples and lot
> > > of historic copy&paste here and there.
> > 
> > :-)
> > 
> > > 
> > > > > Now... porting Unix application can be really a pain.
> > > > > In config.h there can be options that change the declarations
> > > > > inside
> > > > > system headers!
> > > > > Just an example: _GNU_SOURCE. Enables some additional
> > > > > functions/structures/whatever specific to GNU. In some cases
> > > > > change
> > > > > the name of some structure fields. Now it may be possible
> > > > > that for
> > > > > instance your .hpp header include "string" which include
> > > > > "string.h"
> > > > > file which have some declaration added/removed. As you know
> > > > > headers
> > > > > are usually protected by some guards to avoid
> > > > > double/recursive
> > > > > inclusion. So including config.h after/before changes the
> > > > > available
> > > > > functions.
> > > > > So far so good, program won't compile and you can fix it.
> > > > > In my experience porting programs unfortunately what can
> > > > > happen is
> > > > > much worse. Some systems (really, I don't remember) export a
> > > > > different
> > > > > structure depending on some defines. What can happen is that
> > > > > structure
> > > > > "foo" in moduleA have some size/fields and others in moduleB.
> > > > > You link
> > > > > the two modules in the same executable library and when the
> > > > > program
> > > > > run you'll get some stack/heap corruption! Yes, this is a
> > > > > real case!
> > > > > Linux is quite developer friendly but fixing these header
> > > > > details in
> > > > > some nasty Unix environment is really a pain!
> > > > > Last year (2017) some people complained that C99 is too
> > > > > recent!
> > > > > I remember other cases where changing system include order
> > > > > make the
> > > > > same source crash.
> > > > > A case I remember are unixODBC headers where different
> > > > > defines
> > > > > make the header present 2 different ABI... but obviously the
> > > > > binary
> > > > > libraries you are linking expose just one, not hard to
> > > > > imagine the
> > > > > consequences running with the wrong defines.
> > > > > Yes, should not happen but really if to avoid mostly of these
> > > > > nasty
> > > > > issues you just have to include config.h first you do it.
> > > > > 
> > > > > Frediano
> > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > > > @@ -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_a
> > > > > > > nd_Order_of_Includes
> > 
> > 
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel

More information about the Spice-devel mailing list