Bug 190674 – [GStreamer][WebRTC] Add webrtcencoder bin to cleanup and refactor the way we set encoders

• Component: New Bugs
• Status: RESOLVED FIXED

[Thibault Saunier]

[GStreamer][WebRTC] Add webrtcencoder bin to cleanup and refactor the way we set encoders

[Thibault Saunier]

Created attachment 352603 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 352603 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=352603&action=review

> Source/WebCore/ChangeLog:12
> +        The added files follow GStreamer coding style as we aim at upstreaming the element
> +        in the future.

In the future is quite vague. I won't block this because of this issue but I really think that coding style can change during the upstreaming process and currently we should respect WebKit's.

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoder.cpp:52
> +  const char *name;
> +  const char *parser_name;

This is inconsistent. Either use char or gchar.

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoder.cpp:101
> +     static void gst_webrtc_video_encoder_finalize (GObject * object)

Indentation

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoder.cpp:140
> +  if (priv->encoder)
> +    encoders[priv->encoderId].setBitrate (G_OBJECT (priv->encoder),
> +        encoders[priv->encoderId].bitrate_propname, priv->bitrate);

Please correct me if I am wrong, but I think GStreamer would recommend { } enclosing what is running after evaluation the if clause.

[Philippe Normand]

Comment on attachment 352603 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=352603&action=review

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoder.cpp:185
> +        GstPad *tmppad2 = gst_element_get_static_pad (priv->capsfilter, "sink");

This pad leaks, I think.

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoder.cpp:230
> +  GstPluginFeature *feature =
> +      gst_registry_lookup_feature (gst_registry_get (), name);

Leak? :)

[Thibault Saunier]

Created attachment 353486 [details]
Patch

[Thibault Saunier]

(In reply to Xabier Rodríguez Calvar from comment #2)
> Comment on attachment 352603 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=352603&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        The added files follow GStreamer coding style as we aim at upstreaming the element
> > +        in the future.
> 
> In the future is quite vague. I won't block this because of this issue but I
> really think that coding style can change during the upstreaming process and
> currently we should respect WebKit's.

We discussed that last week, and I believe you ended up "agreeing" we should do it that way.

> > Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoder.cpp:52
> > +  const char *name;
> > +  const char *parser_name;
> 
> This is inconsistent. Either use char or gchar.

Done, using GLib variants all around now.

> > Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoder.cpp:101
> > +     static void gst_webrtc_video_encoder_finalize (GObject * object)
> 
> Indentation

Fixed.

> > Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoder.cpp:140
> > +  if (priv->encoder)
> > +    encoders[priv->encoderId].setBitrate (G_OBJECT (priv->encoder),
> > +        encoders[priv->encoderId].bitrate_propname, priv->bitrate);
> 
> Please correct me if I am wrong, but I think GStreamer would recommend { }
> enclosing what is running after evaluation the if clause.

Fixed, though it is accepted in GStreamer.

(In reply to Philippe Normand from comment #3)
> Comment on attachment 352603 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=352603&action=review
> 
> > Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoder.cpp:185
> > +        GstPad *tmppad2 = gst_element_get_static_pad (priv->capsfilter, "sink");
> 
> This pad leaks, I think.

Fixed.

> > Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoder.cpp:230
> > +  GstPluginFeature *feature =
> > +      gst_registry_lookup_feature (gst_registry_get (), name);
> 
> Leak? :)

Fixed.

[Philippe Normand]

Comment on attachment 353486 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353486&action=review

> Source/WebCore/platform/GStreamer.cmake:38
>          platform/mediastream/libwebrtc/GStreamerVideoDecoderFactory.cpp
> +        platform/mediastream/libwebrtc/GStreamerVideoEncoder.cpp
>          platform/mediastream/libwebrtc/GStreamerVideoEncoderFactory.cpp

It's a minor detail but I think those files should move to platform/mediastream/gstreamer/ at some point.

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoder.cpp:61
> +  ENCODER_NONE,

Might be good to explicitly assign to 0?

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoder.cpp:77
> +  ENCODER_LAST,
> +} EncoderId;
> +
> +EncoderDefinition encoders[ENCODER_LAST] = {
> +  FALSE,
> +  NULL,
> +  NULL,
> +  NULL,
> +  NULL,
> +  NULL,
> +  NULL,
> +  NULL,
> +};

So ENCODER_LAST is 4 but encoders's array is bigger. How is this supposed to work? :)

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoder.cpp:97
> +G_DEFINE_TYPE_WITH_PRIVATE (GstWebrtcVideoEncoder, gst_webrtc_video_encoder,
> +    GST_TYPE_BIN)
> +     enum
> +     {
> +       PROP_0,
> +       PROP_FORMAT,
> +       PROP_ENCODER,
> +       PROP_BITRATE,
> +       N_PROPS
> +     };

The indentation is odd here

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoder.cpp:101
> +     static void gst_webrtc_video_encoder_finalize (GObject * object)

Useless whitespace here ;)

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoder.cpp:153
> +  for (i = 1; i < ENCODER_LAST; i++) {

So here you start at 1 because the first item is always FALSE anyway, right?

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoder.cpp:282
> +  g_object_set (encoder, prop_name, bitrate * 1024, NULL);

Use KBIT_TO_BIT here? Otherwise this macro is unused.

[Thibault Saunier]

Created attachment 353856 [details]
Patch

[Thibault Saunier]

(In reply to Philippe Normand from comment #6)
> Comment on attachment 353486 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=353486&action=review
> 
> > Source/WebCore/platform/GStreamer.cmake:38
> >          platform/mediastream/libwebrtc/GStreamerVideoDecoderFactory.cpp
> > +        platform/mediastream/libwebrtc/GStreamerVideoEncoder.cpp
> >          platform/mediastream/libwebrtc/GStreamerVideoEncoderFactory.cpp
> 
> It's a minor detail but I think those files should move to
> platform/mediastream/gstreamer/ at some point.
> 
> > Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoder.cpp:61
> > +  ENCODER_NONE,
> 
> Might be good to explicitly assign to 0?

Done.

> > Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoder.cpp:77
> > +  ENCODER_LAST,
> > +} EncoderId;
> > +
> > +EncoderDefinition encoders[ENCODER_LAST] = {
> > +  FALSE,
> > +  NULL,
> > +  NULL,
> > +  NULL,
> > +  NULL,
> > +  NULL,
> > +  NULL,
> > +  NULL,
> > +};
> 
> So ENCODER_LAST is 4 but encoders's array is bigger. How is this supposed to
> work? :)

Those are setting the content of the `EncoderDefinition` structure, not the whole array itself.

> > Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoder.cpp:97
> > +G_DEFINE_TYPE_WITH_PRIVATE (GstWebrtcVideoEncoder, gst_webrtc_video_encoder,
> > +    GST_TYPE_BIN)
> > +     enum
> > +     {
> > +       PROP_0,
> > +       PROP_FORMAT,
> > +       PROP_ENCODER,
> > +       PROP_BITRATE,
> > +       N_PROPS
> > +     };
> 
> The indentation is odd here

`gst-indent`' doing :-) - added necessary `INDENT-OFF/ON`.

> > Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoder.cpp:101
> > +     static void gst_webrtc_video_encoder_finalize (GObject * object)
> 
> Useless whitespace here ;)

Same.

> > Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoder.cpp:153
> > +  for (i = 1; i < ENCODER_LAST; i++) {
> 
> So here you start at 1 because the first item is always FALSE anyway, right?

Right 0 is the NONE encoder :-)

> > Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoder.cpp:282
> > +  g_object_set (encoder, prop_name, bitrate * 1024, NULL);
> 
> Use KBIT_TO_BIT here? Otherwise this macro is unused.

[WebKit Commit Bot]

Comment on attachment 353856 [details]
Patch

Rejecting attachment 353856 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 353856, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: https://webkit-queues.webkit.org/results/9864724

[Thibault Saunier]

Created attachment 353858 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 353858 [details]
Patch for landing

Clearing flags on attachment: 353858

Committed r237801: <https://trac.webkit.org/changeset/237801>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45806634>