Bug 125488 – [GStreamer] Use g_object_notify_by_pspec in custom elements

• Status: RESOLVED
• Resolution: INVALID
• Component: Media

[Brendan Long]

WebKitWebSource and WebKitTextCombiner both use g_object_emit. g_object_emit_by_psec would be more efficient, and not really any more complicated. I haven't done much of a search, so there may be other cases as well.

[Brendan Long]

I think I meant we should replace g_object_notify with g_object_notify_by_pspec. It might also be worth replacing g_signal_emit_by_name with g_signal_emit where we can, but I haven't looked into that more.

[Brendan Long]

Created attachment 221273 [details]
Patch

[WebKit Commit Bot]

Attachment 221273 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:49:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:50:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brendan Long]

Comment on attachment 221273 [details]
Patch

There are a lot of other places where we could make similar changes, but since they're not media-related, I'm not sure how effectively I could test them.

[Brendan Long]

The style failure is because GStreamer enums don't match WebKit's style.

[Philippe Normand]

Comment on attachment 221273 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:216
> -        g_object_notify(G_OBJECT(pad), "tags");
> +        g_object_notify_by_pspec(G_OBJECT(pad), padProperties[PROP_PAD_TAGS]);

This specific change doesn't seem really worth to me. Pad events don't occur that often (I think) and g_object_notify() for this object should already be quite efficient since the class has only one property.

It's true that _by_pspec is advised but I think it depends how often that code path would be used and also on the number of properties of the class.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:861
> -        g_object_notify(G_OBJECT(src), "iradio-name");
> +        g_object_notify_by_pspec(G_OBJECT(src), properties[PROP_IRADIO_NAME]);

About this and the following ones, I think I agree with the changes. How often is handleResponseReceived called though?

[Brendan Long]

(In reply to comment #6)
> > Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:216
> > -        g_object_notify(G_OBJECT(pad), "tags");
> > +        g_object_notify_by_pspec(G_OBJECT(pad), padProperties[PROP_PAD_TAGS]);
> 
> This specific change doesn't seem really worth to me. Pad events don't occur that often (I think) and g_object_notify() for this object should already be quite efficient since the class has only one property.
> 
> It's true that _by_pspec is advised but I think it depends how often that code path would be used and also on the number of properties of the class.

That makes sense. The GLib documentation indicated that you should always use g_object_notify_by_pspec if you can, but I guess it doesn't really matter much in this case.

We actually do get quite a few tag events in a row while the video is loading, but the string comparison in g_object_notify probably isn't a bottleneck.
 
> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:861
> > -        g_object_notify(G_OBJECT(src), "iradio-name");
> > +        g_object_notify_by_pspec(G_OBJECT(src), properties[PROP_IRADIO_NAME]);
> 
> About this and the following ones, I think I agree with the changes. How often is handleResponseReceived called though?

I'm not really sure. I'll do some tests to see.

[Brendan Long]

(In reply to comment #6)
> About this and the following ones, I think I agree with the changes. How often is handleResponseReceived called though?

It only gets called once, near the beginning of the stream, apparently.

[Brendan Long]

I can just close this if you don't think it's worthwhile.

[Philippe Normand]

Yes, I don't think the benefit would be worth it.