[Spice-devel] [PATCH spice-streaming-agent 1/2] mjpeg plugin: split off the declaration into a header
Frediano Ziglio
fziglio at redhat.com
Thu Feb 1 10:23:37 UTC 2018
>
> > 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.com> 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
> >
> > 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_and_Order_of_Includes
>
>
More information about the Spice-devel
mailing list