[Spice-devel] [PATCH spice-streaming-agent 1/2] mjpeg plugin: split off the declaration into a header
Christophe de Dinechin
cdupontd at redhat.com
Wed Jan 31 11:51:08 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.
If that’s the case, then your header is by definition not using config.h,
and can therefore be safely placed before config.h in the .cpp file.
I’d argue it should, since that allows you to check that some
config.h dependency did not mistakenly creep in.
>
> 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).
>
> 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