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

[Lukáš Hrázký]

On Tue, 2018-02-06 at 04:53 -0500, Frediano Ziglio wrote:
> > 
> > On Tue, 2018-02-06 at 04:35 -0500, Frediano Ziglio wrote:
> > > > 
> > > > So that the plugin can later be included in a unit test.
> > > > 
> > > > Signed-off-by: Lukáš Hrázký 
> > > > ---
> > > >  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..7c918a7 100644
> > > > --- a/src/mjpeg-fallback.cpp
> > > > +++ b/src/mjpeg-fallback.cpp
> > > > @@ -5,6 +5,8 @@
> > > >   */
> > > >  
> > > >  #include 
> > > > +#include "mjpeg-fallback.hpp"
> > > > +
> > > >  #include 
> > > >  #include 
> > > >  #include 
> > > > @@ -13,9 +15,6 @@
> > > >  #include 
> > > >  #include 
> > > >  
> > > > -#include 
> > > > -#include 
> > > > -
> > > >  #include "static-plugin.hpp"
> > > >  #include "jpeg.hpp"
> > > >  
> > > > @@ -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 000..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 
> > > > +#include 
> > > > +
> > > > +
> > > > +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
> > > 
> > > You need this hunk for distribution of the new file:
> > > 
> > > diff --git a/src/Makefile.am b/src/Makefile.am
> > > index 8d5c5bd..6d37274 100644
> > > --- a/src/Makefile.am
> > > +++ b/src/Makefile.am
> > > @@ -54,6 +54,7 @@ spice_streaming_agent_SOURCES = \
> > > concrete-agent.cpp \
> > > concrete-agent.hpp \
> > > mjpeg-fallback.cpp \
> > > +   mjpeg-fallback.hpp \
> > > jpeg.cpp \
> > > jpeg.hpp \
> > > $(NULL)
> > > 
> > > 
> > > Beside that,
> > 
> > What do you mean by distribution? This file is only used for
> > compilation, right?
> > 
> > Lukas
> > 
> > > Acked-by: Frediano Ziglio 
> > > 
> > > Frediano
> 
> "make dist", the command is used to generate the tarball to distribute,
> if you don't have a similar change the package you distribute won't
> include that file and the compilation will fail (I have a script to
> generate the RPM directly so is easy to test).

I see, I didn't realize that! Thanks.

Lukas
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Frediano Ziglio]

> 
> On Tue, 2018-02-06 at 04:35 -0500, Frediano Ziglio wrote:
> > > 
> > > So that the plugin can later be included in a unit test.
> > > 
> > > Signed-off-by: Lukáš Hrázký 
> > > ---
> > >  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..7c918a7 100644
> > > --- a/src/mjpeg-fallback.cpp
> > > +++ b/src/mjpeg-fallback.cpp
> > > @@ -5,6 +5,8 @@
> > >   */
> > >  
> > >  #include 
> > > +#include "mjpeg-fallback.hpp"
> > > +
> > >  #include 
> > >  #include 
> > >  #include 
> > > @@ -13,9 +15,6 @@
> > >  #include 
> > >  #include 
> > >  
> > > -#include 
> > > -#include 
> > > -
> > >  #include "static-plugin.hpp"
> > >  #include "jpeg.hpp"
> > >  
> > > @@ -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 000..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 
> > > +#include 
> > > +
> > > +
> > > +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
> > 
> > You need this hunk for distribution of the new file:
> > 
> > diff --git a/src/Makefile.am b/src/Makefile.am
> > index 8d5c5bd..6d37274 100644
> > --- a/src/Makefile.am
> > +++ b/src/Makefile.am
> > @@ -54,6 +54,7 @@ spice_streaming_agent_SOURCES = \
> > concrete-agent.cpp \
> > concrete-agent.hpp \
> > mjpeg-fallback.cpp \
> > +   mjpeg-fallback.hpp \
> > jpeg.cpp \
> > jpeg.hpp \
> > $(NULL)
> > 
> > 
> > Beside that,
> 
> What do you mean by distribution? This file is only used for
> compilation, right?
> 
> Lukas
> 
> > Acked-by: Frediano Ziglio 
> > 
> > Frediano
> 

"make dist", the command is used to generate the tarball to distribute,
if you don't have a similar change the package you distribute won't
include that file and the compilation will fail (I have a script to
generate the RPM directly so is easy to test).

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Lukáš Hrázký]

On Tue, 2018-02-06 at 04:35 -0500, Frediano Ziglio wrote:
> > 
> > So that the plugin can later be included in a unit test.
> > 
> > Signed-off-by: Lukáš Hrázký 
> > ---
> >  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..7c918a7 100644
> > --- a/src/mjpeg-fallback.cpp
> > +++ b/src/mjpeg-fallback.cpp
> > @@ -5,6 +5,8 @@
> >   */
> >  
> >  #include 
> > +#include "mjpeg-fallback.hpp"
> > +
> >  #include 
> >  #include 
> >  #include 
> > @@ -13,9 +15,6 @@
> >  #include 
> >  #include 
> >  
> > -#include 
> > -#include 
> > -
> >  #include "static-plugin.hpp"
> >  #include "jpeg.hpp"
> >  
> > @@ -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 000..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 
> > +#include 
> > +
> > +
> > +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
> 
> You need this hunk for distribution of the new file:
> 
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 8d5c5bd..6d37274 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -54,6 +54,7 @@ spice_streaming_agent_SOURCES = \
> concrete-agent.cpp \
> concrete-agent.hpp \
> mjpeg-fallback.cpp \
> +   mjpeg-fallback.hpp \
> jpeg.cpp \
> jpeg.hpp \
> $(NULL)
> 
> 
> Beside that,

What do you mean by distribution? This file is only used for
compilation, right?

Lukas

> Acked-by: Frediano Ziglio 
> 
> Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Frediano Ziglio]

> 
> So that the plugin can later be included in a unit test.
> 
> Signed-off-by: Lukáš Hrázký 
> ---
>  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..7c918a7 100644
> --- a/src/mjpeg-fallback.cpp
> +++ b/src/mjpeg-fallback.cpp
> @@ -5,6 +5,8 @@
>   */
>  
>  #include 
> +#include "mjpeg-fallback.hpp"
> +
>  #include 
>  #include 
>  #include 
> @@ -13,9 +15,6 @@
>  #include 
>  #include 
>  
> -#include 
> -#include 
> -
>  #include "static-plugin.hpp"
>  #include "jpeg.hpp"
>  
> @@ -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 000..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 
> +#include 
> +
> +
> +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

You need this hunk for distribution of the new file:

diff --git a/src/Makefile.am b/src/Makefile.am
index 8d5c5bd..6d37274 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -54,6 +54,7 @@ spice_streaming_agent_SOURCES = \
concrete-agent.cpp \
concrete-agent.hpp \
mjpeg-fallback.cpp \
+   mjpeg-fallback.hpp \
jpeg.cpp \
jpeg.hpp \
$(NULL)


Beside that,

Acked-by: Frediano Ziglio 

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel