Created attachment 352603 [details] Patch

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.

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? :)

Created attachment 353486 [details] Patch

(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.

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.

Created attachment 353856 [details] Patch

(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.

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

Created attachment 353858 [details] Patch for landing

Comment on attachment 353858 [details] Patch for landing Clearing flags on attachment: 353858 Committed r237801: <https://trac.webkit.org/changeset/237801>

All reviewed patches have been landed. Closing bug.

<rdar://problem/45806634>