Bug 101349 - [GStreamer] Floating reference handling fix
Summary: [GStreamer] Floating reference handling fix
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 101603 (view as bug list)
Depends on:
Blocks: 91727
  Show dependency treegraph
 
Reported: 2012-11-06 07:35 PST by Thiago Santos
Modified: 2012-11-26 04:11 PST (History)
10 users (show)

See Also:


Attachments
Patch (1.83 KB, patch)
2012-11-06 07:42 PST, Thiago Santos
no flags Details | Formatted Diff | Diff
Patch (2.09 KB, patch)
2012-11-15 07:42 PST, Thiago Santos
no flags Details | Formatted Diff | Diff
Patch (5.15 KB, patch)
2012-11-24 20:44 PST, Thiago Santos
no flags Details | Formatted Diff | Diff
Patch (5.39 KB, patch)
2012-11-25 04:19 PST, Thiago Santos
no flags Details | Formatted Diff | Diff
Patch (5.41 KB, patch)
2012-11-25 05:06 PST, Thiago Santos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thiago Santos 2012-11-06 07:35:47 PST
GStreamer reference handling fix
Comment 1 Thiago Santos 2012-11-06 07:42:20 PST
Created attachment 172586 [details]
Patch
Comment 2 Martin Robinson 2012-11-06 07:47:23 PST
Comment on attachment 172586 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:220
> -    GRefPtr<GstPadTemplate> padTemplate = adoptGRef(gst_static_pad_template_get(&srcTemplate));
> +    GRefPtr<GstPadTemplate> padTemplate = adoptGRef(WTF::refGPtr(gst_static_pad_template_get(&srcTemplate)));

I think it would make more sense to simply avoid using adoptGRef here.
Comment 3 ChangSeok Oh 2012-11-08 08:38:27 PST
*** Bug 101603 has been marked as a duplicate of this bug. ***
Comment 4 Philippe Normand 2012-11-15 02:54:12 PST
Do you plan to update this patch Thiago?
Comment 5 Thiago Santos 2012-11-15 07:42:51 PST
Created attachment 174439 [details]
Patch
Comment 6 Thiago Santos 2012-11-15 07:45:00 PST
(In reply to comment #5)
> Created an attachment (id=174439) [details]
> Patch

Sorry it took too long, my machine died last week and I'm unable to compile webkit in this spare one I'm using.

In any case, here's the updated patch. As I said, I can't build it here, so it hasn't been compiled/tested yet.
Comment 7 Philippe Normand 2012-11-15 11:57:06 PST
There's just one issue now I think, if appsrc can't be created the padTemplate is leaked. It's unlikely but still ;)
Comment 8 Philippe Normand 2012-11-15 12:01:51 PST
(In reply to comment #7)
> There's just one issue now I think, if appsrc can't be created the padTemplate is leaked. It's unlikely but still ;)

Oh hum sorry, ignore this comment
Comment 9 Philippe Normand 2012-11-16 02:08:36 PST
Comment on attachment 174439 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:241
> +    gst_object_unref(padTemplate);

Is the unref needed?
And to avoid any ambiguity with the early return we could simply create the pad template only when needed, after the early return and do something like:
priv->srcpad = gst_ghost_pad_new_from_template("src", targetPad.get(), gst_static_pad_template_get(&srcTemplate));

I had a look at some other gst elements and that's what they do most of the time when creating a ghost pad from a static pad template, all in one line.
Comment 10 Sebastian Dröge (slomo) 2012-11-16 02:40:24 PST
The unref is needed, yes. In 0.10 get() returned a non-floating ref, in 1.0 a floating ref.

You could simply use gst_pad_new_from_static_template() for 0.10 (was added in a later version IIRC) and 1.0. Alternatively you could just gst_object_ref_sink() before passing it to the smart pointer.
Comment 11 Thiago Marcos P. Santos 2012-11-20 06:16:27 PST
Comment on attachment 174439 [details]
Patch

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

> Source/WebCore/ChangeLog:1
> +2012-11-15  Thiago Santos  <thiago.sousa.santos@collabora.com>

Looks like I can't grep for my name anymore when trying finding my commits. :)
Comment 12 Philippe Normand 2012-11-21 04:20:51 PST
Ok so we need to handle this depending on which GStreamer version we build against. And it needs to be done for the WebAudio src element which we have in platform/audio/gstreamer too.

Can you please add a new function in GStreamerVersioning.cpp to handle this?
Comment 13 Xabier Rodríguez Calvar 2012-11-22 06:36:53 PST
Thiago, I will have a look at this because it is blocking 91727. Feel free to tell me otherwise if you can finish this soon.
Comment 14 Thiago Santos 2012-11-22 07:01:34 PST
(In reply to comment #13)
> Thiago, I will have a look at this because it is blocking 91727. Feel free to tell me otherwise if you can finish this soon.

My new computer arrived (this one should be able to build webkit), but I'm still configuring it. I should be able to provide a new patch later today or early tomorrow.

Of course you can take this and fix it yourself if tomorrow is not soon enough.
Comment 15 Xabier Rodríguez Calvar 2012-11-22 07:14:10 PST
(In reply to comment #14)
> (In reply to comment #13)
> > Thiago, I will have a look at this because it is blocking 91727. Feel free to tell me otherwise if you can finish this soon.
> 
> My new computer arrived (this one should be able to build webkit), but I'm still configuring it. I should be able to provide a new patch later today or early tomorrow.
> 
> Of course you can take this and fix it yourself if tomorrow is not soon enough.

It is enough, we are not in such a hurry :) It was more about you not being able to do it because you didn't have time or something like that.

Thanks!
Comment 16 Thiago Santos 2012-11-24 20:44:52 PST
Created attachment 175872 [details]
Patch
Comment 17 WebKit Review Bot 2012-11-24 20:46:54 PST
Attachment 175872 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 4 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 Philippe Normand 2012-11-25 03:19:05 PST
Comment on attachment 175872 [details]
Patch

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

Awesome, thanks Thiago! Now just the ChangeLog to fix up and the missing include to add!

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:181
> +    priv->sourcePad = webkitGstGhostPadFromStaticTemplate(&srcTemplate, "src", 0);

GStreamerVersioning.h include is missing I think. BTW this code is not yet enabled by default in the build, you need to use the --web-audio build-webkit option. I have a patch almost ready for the 1.0 port to upload soon :)
Comment 19 Thiago Santos 2012-11-25 04:16:56 PST
(In reply to comment #18)
> (From update of attachment 175872 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175872&action=review
> 
> Awesome, thanks Thiago! Now just the ChangeLog to fix up and the missing include to add!
> 
> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:181
> > +    priv->sourcePad = webkitGstGhostPadFromStaticTemplate(&srcTemplate, "src", 0);
> 
> GStreamerVersioning.h include is missing I think. BTW this code is not yet enabled by default in the build, you need to use the --web-audio build-webkit option. I have a patch almost ready for the 1.0 port to upload soon :)

Great. So I'm only adding the missing include as trying to build this file with gstreamer 1.0 causes errors as it has not been ported yet.
Comment 20 Thiago Santos 2012-11-25 04:19:58 PST
Created attachment 175880 [details]
Patch
Comment 21 Philippe Normand 2012-11-25 04:38:17 PST
Comment on attachment 175880 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        No new tests (OOPS!).

Oh sorry I forgot to mention you should replace the OOPS with something like "existing media tests cover this change".
Comment 22 Thiago Santos 2012-11-25 05:06:19 PST
Created attachment 175882 [details]
Patch
Comment 23 Thiago Santos 2012-11-25 05:07:43 PST
(In reply to comment #21)
> (From update of attachment 175880 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175880&action=review
> 
> > Source/WebCore/ChangeLog:14
> > +        No new tests (OOPS!).
> 
> Oh sorry I forgot to mention you should replace the OOPS with something like "existing media tests cover this change".

Updated, but I couldn't run the tests and it seems I'd need to do a lot of setup and I didn't find any instructions for it. I tested by using the launcher to open pages with <video> tags.
Comment 24 Philippe Normand 2012-11-25 05:15:26 PST
(In reply to comment #23)
> (In reply to comment #21)
> > (From update of attachment 175880 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=175880&action=review
> > 
> > > Source/WebCore/ChangeLog:14
> > > +        No new tests (OOPS!).
> > 
> > Oh sorry I forgot to mention you should replace the OOPS with something like "existing media tests cover this change".
> 
> Updated, but I couldn't run the tests and it seems I'd need to do a lot of setup and I didn't find any instructions for it. I tested by using the launcher to open pages with <video> tags.

Nowadays if you use our jhbuild environment it shouldn't be too hard to run the tests, it's documented in http://trac.webkit.org/wiki/WebKitGtkLayoutTests
Comment 25 Philippe Normand 2012-11-25 06:33:06 PST
Please let us know how you want to commit the patch, usually for not-yet-committers the commit-queue bot takes care of it.
Comment 26 Philippe Normand 2012-11-26 04:05:52 PST
Comment on attachment 175882 [details]
Patch

Ok let's use the cq, Thiago doesn't appear to be committer anyway.
Comment 27 WebKit Review Bot 2012-11-26 04:11:15 PST
Comment on attachment 175882 [details]
Patch

Clearing flags on attachment: 175882

Committed r135705: <http://trac.webkit.org/changeset/135705>
Comment 28 WebKit Review Bot 2012-11-26 04:11:20 PST
All reviewed patches have been landed.  Closing bug.