[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 10:13:06 UTC 2018
> 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?
> 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list