WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
Bug 101349
[GStreamer] Floating reference handling fix
https://bugs.webkit.org/show_bug.cgi?id=101349
Summary
[GStreamer] Floating reference handling fix
Thiago Santos
Reported
2012-11-06 07:35:47 PST
GStreamer reference handling fix
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
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Thiago Santos
Comment 1
2012-11-06 07:42:20 PST
Created
attachment 172586
[details]
Patch
Martin Robinson
Comment 2
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.
ChangSeok Oh
Comment 3
2012-11-08 08:38:27 PST
***
Bug 101603
has been marked as a duplicate of this bug. ***
Philippe Normand
Comment 4
2012-11-15 02:54:12 PST
Do you plan to update this patch Thiago?
Thiago Santos
Comment 5
2012-11-15 07:42:51 PST
Created
attachment 174439
[details]
Patch
Thiago Santos
Comment 6
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.
Philippe Normand
Comment 7
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 ;)
Philippe Normand
Comment 8
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
Philippe Normand
Comment 9
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.
Sebastian Dröge (slomo)
Comment 10
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.
Thiago Marcos P. Santos
Comment 11
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. :)
Philippe Normand
Comment 12
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?
Xabier Rodríguez Calvar
Comment 13
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.
Thiago Santos
Comment 14
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.
Xabier Rodríguez Calvar
Comment 15
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!
Thiago Santos
Comment 16
2012-11-24 20:44:52 PST
Created
attachment 175872
[details]
Patch
WebKit Review Bot
Comment 17
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.
Philippe Normand
Comment 18
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 :)
Thiago Santos
Comment 19
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.
Thiago Santos
Comment 20
2012-11-25 04:19:58 PST
Created
attachment 175880
[details]
Patch
Philippe Normand
Comment 21
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".
Thiago Santos
Comment 22
2012-11-25 05:06:19 PST
Created
attachment 175882
[details]
Patch
Thiago Santos
Comment 23
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.
Philippe Normand
Comment 24
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
Philippe Normand
Comment 25
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.
Philippe Normand
Comment 26
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.
WebKit Review Bot
Comment 27
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
>
WebKit Review Bot
Comment 28
2012-11-26 04:11:20 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug