[Spice-devel] [PATCH v2 spice-streaming-agent 2/2] gst-plugin: Shorten template declarations

[Uri Lublin]

I'd add a comment saying that
The typeUPtr templates are very similar except for the unref-function.
This patch is replacing the templates with a macro which also accepts
an unref-function and is using this macro to define all typeUPtr types.

On 8/11/19 11:54 AM, Frediano Ziglio wrote:
>>
>> Signed-off-by: Snir Sheriber <ssheribe at redhat.com>
>> ---
>> Another suggestion to shoten the templates + changing subject
> 
> It looks pretty neat this version, see below for minor comments.
> And more type safe too.
> 
>> ---
>>   src/gst-plugin.cpp | 64 +++++++++++++++-------------------------------
>>   1 file changed, 20 insertions(+), 44 deletions(-)
>>
>> diff --git a/src/gst-plugin.cpp b/src/gst-plugin.cpp
>> index 4d17dbc..703d5b2 100644
>> --- a/src/gst-plugin.cpp
>> +++ b/src/gst-plugin.cpp
>> @@ -38,43 +38,19 @@ struct GstreamerEncoderSettings
>>       std::vector<std::pair<std::string, std::string>> prop_pairs;
>>   };
>>   
>> -template <typename T>
>> -struct GstObjectDeleter {
>> -    void operator()(T* p)
>> -    {
>> -        gst_object_unref(p);
>> -    }
>> -};
>> -
>> -template <typename T>
>> -using GstObjectUPtr = std::unique_ptr<T, GstObjectDeleter<T>>;
>> -
>> -struct GstCapsDeleter {
>> -    void operator()(GstCaps* p)
>> -    {
>> -        gst_caps_unref(p);
>> -    }
>> -};
>> -
>> -using GstCapsUPtr = std::unique_ptr<GstCaps, GstCapsDeleter>;
>> -
>> -struct GstSampleDeleter {
>> -    void operator()(GstSample* p)
>> -    {
>> -        gst_sample_unref(p);
>> -    }
>> -};
>> -
>> -using GstSampleUPtr = std::unique_ptr<GstSample, GstSampleDeleter>;
>> -
>> -struct GstBufferDeleter {
>> -    void operator()(GstBuffer* p)
>> -    {
>> -        gst_buffer_unref(p);
>> -    }
>> -};
>> -
>> -using GstBufferUPtr = std::unique_ptr<GstBuffer, GstBufferDeleter>;
>> +#define DECLARE_UPTR(type, func) \
>> +struct type##Deleter { \
>> +    void operator()(type* p) \
>> +    { \
>> +        func(p); \
>> +    } \
>> +}; \
>> +using type##UPtr = std::unique_ptr<type, type##Deleter>; \
>> +
> 
> I have to say that I like this syntax more than the one at
> https://www.spice-space.org/spice-project-coding-style-and-coding-conventions.html
> and I suggested it too. On the other hand it does not conform to the style.

I'd not change the style -- too many white-space commits will
follow (although it seems less important in a macro).
(It seems this was the style of the template code before).

> I would remove the last "\" at last line, the empty like will be part of the macro
> which is confusing.

me too.

> Not sure if it's better to indent not at first column but it's fine for me.

Are you talking about 'struct' and 'using' lines ? Yes, makes sense
to indent them to show they are part of the macro.

> 
>> +DECLARE_UPTR(GstBuffer, gst_buffer_unref)
>> +DECLARE_UPTR(GstCaps, gst_caps_unref)
>> +DECLARE_UPTR(GstSample, gst_sample_unref)
>> +DECLARE_UPTR(GstElement, gst_object_unref)
>>   
>>   class GstreamerFrameCapture final : public FrameCapture
>>   {
>> @@ -96,7 +72,7 @@ private:
>>   #if XLIB_CAPTURE
>>       void xlib_capture();
>>   #endif
>> -    GstObjectUPtr<GstElement> pipeline, capture, sink;
>> +    GstElementUPtr pipeline, capture, sink;
>>       GstSampleUPtr sample;
>>       GstMapInfo map = {};
>>       uint32_t last_width = ~0u, last_height = ~0u;
>> @@ -210,7 +186,7 @@ GstElement
>> *GstreamerFrameCapture::get_encoder_plugin(const GstreamerEncoderSett
>>   
>>   // Utility to add an element to a GstBin
>>   // This checks return value and update reference correctly
>> -void gst_bin_add(GstBin *bin, const GstObjectUPtr<GstElement> &elem)
>> +void gst_bin_add(GstBin *bin, const GstElementUPtr &elem)
>>   {
>>       if (::gst_bin_add(bin, elem.get())) {
>>           // ::gst_bin_add take ownership using floating references but
>> @@ -226,24 +202,24 @@ void GstreamerFrameCapture::pipeline_init(const
>> GstreamerEncoderSettings &settin
>>   {
>>       gboolean link;
>>   
>> -    GstObjectUPtr<GstElement> pipeline(gst_pipeline_new("pipeline"));
>> +    GstElementUPtr pipeline(gst_pipeline_new("pipeline"));
>>       if (!pipeline) {
>>           throw std::runtime_error("Gstreamer's pipeline element cannot be
>>           created");
>>       }
>> -    GstObjectUPtr<GstElement> capture(get_capture_plugin(settings));
>> +    GstElementUPtr capture(get_capture_plugin(settings));
>>       if (!capture) {
>>           throw std::runtime_error("Gstreamer's capture element cannot be
>>           created");
>>       }
>> -    GstObjectUPtr<GstElement>
>> convert(gst_element_factory_make("autovideoconvert", "convert"));
>> +    GstElementUPtr convert(gst_element_factory_make("autovideoconvert",
>> "convert"));
>>       if (!convert) {
>>           throw std::runtime_error("Gstreamer's 'autovideoconvert' element
>>           cannot be created");
>>       }
>>       GstCapsUPtr sink_caps;
>> -    GstObjectUPtr<GstElement> encoder(get_encoder_plugin(settings,
>> sink_caps));
>> +    GstElementUPtr encoder(get_encoder_plugin(settings, sink_caps));
>>       if (!encoder) {
>>           throw std::runtime_error("Gstreamer's encoder element cannot be
>>           created");
>>       }
>> -    GstObjectUPtr<GstElement> sink(gst_element_factory_make("appsink",
>> "sink"));
>> +    GstElementUPtr sink(gst_element_factory_make("appsink", "sink"));
>>       if (!sink) {
>>           throw std::runtime_error("Gstreamer's appsink element cannot be
>>           created");
>>       }
> 
> Let's see what other people think about.

Looks good to me. Much shorter.
I basically suggested the same thing (but with a template if possible).

Uri.