Bug 101349 – [GStreamer] Floating reference handling fix

• Component: Platform
• Status: RESOLVED FIXED

[Thiago Santos]

GStreamer reference handling fix

[Thiago Santos]

Created attachment 172586 [details]
Patch

[Martin Robinson]

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.

[ChangSeok Oh]

*** Bug 101603 has been marked as a duplicate of this bug. ***

[Philippe Normand]

Do you plan to update this patch Thiago?

[Thiago Santos]

Created attachment 174439 [details]
Patch

[Thiago Santos]

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

[Philippe Normand]

There's just one issue now I think, if appsrc can't be created the padTemplate is leaked. It's unlikely but still ;)

[Philippe Normand]

(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

[Philippe Normand]

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.

[Sebastian Dröge (slomo)]

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.

[Thiago Marcos P. Santos]

Comment on attachment 174439 [details]
Patch

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

> Source/WebCore/ChangeLog:1
> [phone]  Thiago Santos  <[e-mail]>

Looks like I can't grep for my name anymore when trying finding my commits. :)

[Philippe Normand]

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?

[Xabier Rodríguez Calvar]

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.

[Thiago Santos]

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

[Xabier Rodríguez Calvar]

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

[Thiago Santos]

Created attachment 175872 [details]
Patch

[WebKit Review Bot]

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.

[Philippe Normand]

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

[Thiago Santos]

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

[Thiago Santos]

Created attachment 175880 [details]
Patch

[Philippe Normand]

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

[Thiago Santos]

Created attachment 175882 [details]
Patch

[Thiago Santos]

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

[Philippe Normand]

(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

[Philippe Normand]

Please let us know how you want to commit the patch, usually for not-yet-committers the commit-queue bot takes care of it.

[Philippe Normand]

Comment on attachment 175882 [details]
Patch

Ok let's use the cq, Thiago doesn't appear to be committer anyway.

[WebKit Review Bot]

Comment on attachment 175882 [details]
Patch

Clearing flags on attachment: 175882

Committed r135705: <http://trac.webkit.org/changeset/135705>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.