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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 
> > On 31 Jan 2018, at 12:26, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
> > 
> >>> 
> >>> On 30 Jan 2018, at 17:54, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
> >>> 
> >>>>> 
> >>>>> On 30 Jan 2018, at 13:29, Lukáš Hrázký <lhrazky@xxxxxxxxxx> 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@xxxxxxxxxx>
> >>>>>>> ---
> >>>>>>> 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
> 
> 
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]