Last Comment Bug 836243 - Relax gstreamer version requirement
: Relax gstreamer version requirement
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: unspecified
: All Linux
: -- normal (vote)
: mozilla21
Assigned To: Ivaylo Dimitrov
:
: Maire Reavy [:mreavy]
Mentors:
Depends on: 856308
Blocks: 794282
  Show dependency treegraph
 
Reported: 2013-01-30 03:46 PST by Ivaylo Dimitrov
Modified: 2013-03-30 06:04 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (2.56 KB, patch)
2013-01-30 03:47 PST, Ivaylo Dimitrov
giles: feedback+
Details | Diff | Splinter Review
relax_gst_version_v1.patch (2.56 KB, patch)
2013-02-13 05:53 PST, Ivaylo Dimitrov
alessandro.d: review+
Details | Diff | Splinter Review

Description Ivaylo Dimitrov 2013-01-30 03:46:36 PST
Building mozilla with gstreamer support requires gstreamer version to be at least 0.10.33 as in this version playbin2 "source-setup" signal was added. The proposed patch uses "notify::source" signal instead of "source-setup" while keeping the functionality intact. The usage of "notify::source" allows mozilla gstreamer support to be enabled on systems with gstreamer version lower than 0.10.33.
Comment 1 Ivaylo Dimitrov 2013-01-30 03:47:22 PST
Created attachment 708081 [details] [diff] [review]
patch
Comment 2 Oleg Romashin (:romaxa) 2013-02-09 10:56:58 PST
Any updates on this bug? can we get it reviewed
Comment 3 Alessandro Decina 2013-02-12 09:20:45 PST
Comment on attachment 708081 [details] [diff] [review]
patch

Review of attachment 708081 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, with only one minor thing to fix

::: content/media/gstreamer/GStreamerReader.cpp
@@ +176,3 @@
>    GStreamerReader *reader = reinterpret_cast<GStreamerReader*>(aUserData);
> +
> +  g_object_get(aPlayBin, "source", &source, NULL);

this is missing a gst_object_unref(source) after the ->PlayBinSourceSetup call
Comment 4 Alessandro Decina 2013-02-12 09:26:56 PST
I don't seem to have permissions to approve the patch btw
Comment 5 Alessandro Decina 2013-02-12 09:48:52 PST
If I could, i'd make it r+ assuming the minor issue i pointed out gets fixed :)
Comment 6 Ralph Giles (:rillian) needinfo me 2013-02-12 14:50:53 PST
Comment on attachment 708081 [details] [diff] [review]
patch

Review of attachment 708081 [details] [diff] [review]:
-----------------------------------------------------------------

Fine with me. Please upload a patch addressing the two nits.

::: content/media/gstreamer/GStreamerReader.cpp
@@ +163,5 @@
>        NULL);
>  
> +  g_signal_connect(G_OBJECT(mPlayBin), "notify::source",
> +    G_CALLBACK(GStreamerReader::PlayBinSourceSetupCb), this);
> +  

Please fix the trailing whitespace here.
Comment 7 Ralph Giles (:rillian) needinfo me 2013-02-12 14:52:20 PST
Oops, didn't mean to clobber alessandro's r+. So much for mid-air collision detection.
Comment 8 Ivaylo Dimitrov 2013-02-13 05:35:30 PST
(In reply to Alessandro Decina from comment #3)
> Comment on attachment 708081 [details] [diff] [review]
> patch
> 
> Review of attachment 708081 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, with only one minor thing to fix
> 
> ::: content/media/gstreamer/GStreamerReader.cpp
> @@ +176,3 @@
> >    GStreamerReader *reader = reinterpret_cast<GStreamerReader*>(aUserData);
> > +
> > +  g_object_get(aPlayBin, "source", &source, NULL);
> 
> this is missing a gst_object_unref(source) after the ->PlayBinSourceSetup
> call

gst_object_unref is called in the class destructor:

>      gst_app_src_end_of_stream(mSource);
> +    if (mSource)
> +      gst_object_unref(mSource);
Comment 9 Ivaylo Dimitrov 2013-02-13 05:53:28 PST
Created attachment 713368 [details] [diff] [review]
relax_gst_version_v1.patch

Removed extra whitespaces
Comment 10 Oleg Romashin (:romaxa) 2013-02-13 09:21:51 PST
Comment on attachment 713368 [details] [diff] [review]
relax_gst_version_v1.patch

Alessandro are you agree with comment #8 ?
Comment 12 Ed Morley [:emorley] 2013-02-14 03:02:06 PST
https://hg.mozilla.org/mozilla-central/rev/041328ec9651

Note You need to log in before you can comment on or make changes to this bug.