Bug 125488 - [GStreamer] Use g_object_notify_by_pspec in custom elements
Summary: [GStreamer] Use g_object_notify_by_pspec in custom elements
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-12-10 02:06 PST by Brendan Long
Modified: 2014-02-13 06:59 PST (History)
6 users (show)

See Also:


Attachments
Patch (10.36 KB, patch)
2014-01-15 09:12 PST, Brendan Long
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brendan Long 2013-12-10 02:06:26 PST
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.
Comment 1 Brendan Long 2014-01-15 08:12:14 PST
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.
Comment 2 Brendan Long 2014-01-15 09:12:20 PST
Created attachment 221273 [details]
Patch
Comment 3 WebKit Commit Bot 2014-01-15 09:14:30 PST
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.
Comment 4 Brendan Long 2014-01-15 09:16:26 PST
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.
Comment 5 Brendan Long 2014-01-15 09:17:13 PST
The style failure is because GStreamer enums don't match WebKit's style.
Comment 6 Philippe Normand 2014-01-15 22:53:27 PST
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?
Comment 7 Brendan Long 2014-01-16 10:03:24 PST
(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.
Comment 8 Brendan Long 2014-02-04 15:39:19 PST
(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.
Comment 9 Brendan Long 2014-02-11 09:53:18 PST
I can just close this if you don't think it's worthwhile.
Comment 10 Philippe Normand 2014-02-12 23:47:42 PST
Yes, I don't think the benefit would be worth it.