Relax gstreamer version requirement

RESOLVED FIXED in mozilla21

Status

()

Core
Audio/Video
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Ivaylo Dimitrov, Assigned: Ivaylo Dimitrov)

Tracking

unspecified
mozilla21
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

2.56 KB, patch
Alessandro Decina
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 708081 [details] [diff] [review]
patch
Attachment #708081 - Flags: review?(alessandro.d)
Attachment #708081 - Flags: feedback?(giles)
Any updates on this bug? can we get it reviewed

Comment 3

4 years ago
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

4 years ago
I don't seem to have permissions to approve the patch btw

Comment 5

4 years ago
If I could, i'd make it r+ assuming the minor issue i pointed out gets fixed :)

Updated

4 years ago
Attachment #708081 - Flags: review?(alessandro.d) → review+
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.
Attachment #708081 - Flags: review?(alessandro.d)
Attachment #708081 - Flags: review+
Attachment #708081 - Flags: feedback?(giles)
Attachment #708081 - Flags: feedback+
Oops, didn't mean to clobber alessandro's r+. So much for mid-air collision detection.
(Assignee)

Comment 8

4 years ago
(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);
(Assignee)

Comment 9

4 years ago
Created attachment 713368 [details] [diff] [review]
relax_gst_version_v1.patch

Removed extra whitespaces
Attachment #708081 - Attachment is obsolete: true
Attachment #708081 - Flags: review?(alessandro.d)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment on attachment 713368 [details] [diff] [review]
relax_gst_version_v1.patch

Alessandro are you agree with comment #8 ?
Attachment #713368 - Flags: review?(alessandro.d)

Updated

4 years ago
Attachment #713368 - Flags: review?(alessandro.d) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/041328ec9651
https://hg.mozilla.org/mozilla-central/rev/041328ec9651
Assignee: nobody → freemangordon
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21

Updated

4 years ago
Blocks: 794282
(Assignee)

Updated

4 years ago
Depends on: 856308
You need to log in before you can comment on or make changes to this bug.