Load GStreamer at runtime on Linux

RESOLVED FIXED in mozilla24

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: eflores, Assigned: eflores)

Tracking

unspecified
mozilla24
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Loading the GStreamer libraries at runtime will let us ship with --enable-gstreamer on Linux by default.
Posted patch Paaaattttccchhhhhhhhhhhh (obsolete) — Splinter Review
This patch loads the GStreamer libraries at runtime, so one shouldn't need to have GStreamer installed to run it.

This only works with GStreamer 0.10 for the meantime.
Attachment #734463 - Flags: review?(alessandro.d)
Posted patch Patch Numero Dos (obsolete) — Splinter Review
I am sometimes silly.
Attachment #734463 - Attachment is obsolete: true
Attachment #734463 - Flags: review?(alessandro.d)
Attachment #734464 - Flags: review?(alessandro.d)

Updated

6 years ago
Blocks: 794282
Oops, doesn't look like the try servers have the gstreamer-devel packages yet.
Depends on: 855492
An alternative might be to make the gstreamer implementation an XPCOM component, link gstreamer normally to it, and then gracefully handle the failure to load the XPCOM component.
What is the advantage of doing that?
Instead of the whole patch you have there, the load handling is simply an |if (!component) return;|
The patch hardcodes the functions needed. I assume if new functions are needed, they need to be explicitly added to the list. That's not exactly nice.
Essentially, this is what XPCOM is for: dynamically adding libraries.
/me starts reading XPCOM manual
Basically:
* Continue to link gstreamer statically to content/media/gstreamer/
* Make content/media/gstreamer/ a proper XPCOM component (instead of a statically linked lib)
* The API will be the same functions as in DecoderTraits, just as IDL.
* Do the same for the other platform implementations
* Change the implementation of DecoderTraits to
  * attempt to load the platform implementations
  * if the load succeeds, store the reference to the component in the DecoderTraits class
  * the functions in the DecoderTraits class call the above implementation API

Granted, that would be a larger change. I had thought that gstreamer implementation and DecoderTraits was already using XPCOM. I'd definitely recommend it in the mid term, because that's what XPCOM is for - dynamically plug in implementations, and handle the lack of them.
I've asked roc for comment, whether he thinks that's a good idea or not.
We should not use XPCOM here. XPCOM requires IDL and a lot of other goop that we don't want.

(In reply to Ben Bucksch (:BenB) from comment #9)
> Instead of the whole patch you have there, the load handling is simply an
> |if (!component) return;|

No it's not, because you need to write all the extra code to create the XPCOM component.

> The patch hardcodes the functions needed. I assume if new functions are
> needed, they need to be explicitly added to the list. That's not exactly
> nice.

With XPCOM you need to add them to the IDL and the component. It's not better. In fact it's worse, because you have the extra IDL layer and you have to write your interfaces using the horrible XPCOM conventions (everything returns nsresult, etc).

XPCOM should basically never be used.
OK, nevermind, then.
Comment on attachment 734464 [details] [diff] [review]
Patch Numero Dos

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

::: content/media/gstreamer/GStreamerLoader.cpp
@@ +37,5 @@
> +    return true;
> +  }
> +
> +  void *handles[] = {
> +    dlopen("libgstreamer-0.10.so", RTLD_NOW | RTLD_LOCAL),

That's not going to work unless the -dev packages are installed on the user machine. You need libgstreamer-0.10.so.0. You may also want to add libgstreamer-1.0.so.0, provided the APIs we use are compatible. Likewise for the other libs.

(BTW, an XPCOM component wouldn't allow to depend on multiple versions of gstreamer)
I wasn't aware of that convention. Will change.

gstreamer 1.0 is unfortunately not ABI compatible. We can sort that out at a later time though. Just want to get this out the door first.
Edwin, have you tried to make this work on Mac? At the very least on mac it should change the shlib suffix to .dylib.

Also, have you considered making the gst backend a MPAPI plugin? I think MPAPI plugins are dlopened, so maybe we could keep linking to gst and let firefox handle the failure at loading the plugin if it can't resolve gst symbols.
(In reply to Alessandro Decina from comment #18)
> Edwin, have you tried to make this work on Mac? At the very least on mac it
> should change the shlib suffix to .dylib.

Will do.

> Also, have you considered making the gst backend a MPAPI plugin? I think
> MPAPI plugins are dlopened, so maybe we could keep linking to gst and let
> firefox handle the failure at loading the plugin if it can't resolve gst
> symbols.

Yes, I've considered that, but for now this is the `patch of least resistance'. There is not much reason to switch it to MPAPI until uptake of GStreamer 1.0 (to the exclusion of 0.10) starts (and therefore we have to deal with proper fragmentation). Of course, I'm not sure what kind of time frame that would imply...
Posted patch Patch v3 (obsolete) — Splinter Review
On Mac we're going to include gstreamer as part of the build and statically link against that, so this patch keeps the current behaviour as it is on that platform.
Attachment #734464 - Attachment is obsolete: true
Attachment #734464 - Flags: review?(alessandro.d)
Attachment #744332 - Flags: review?(alessandro.d)
Doesn't seem to compile for me on mac using clang:

$ clang -v
Apple clang version 4.0 (tags/Apple/clang-421.0.60) (based on LLVM 3.1svn)
Target: x86_64-apple-darwin12.0.0
Thread model: posix


 0:00.19 /usr/bin/make -C content/media/gstreamer -j8 -s
 0:00.30 GStreamerLoader.cpp
 0:00.58 In file included from /Users/alessandro/src/git/mozilla-central/content/media/gstreamer/GStreamerLoader.cpp:117:
 0:00.58 [1m/Users/alessandro/src/git/mozilla-central/content/media/gstreamer/GStreamerFunctionList.h:9:10: [0m[0;1;31merror: [0m[1mredefinition of 'gst_app_sink_get_type'[0m
 0:00.58 GST_FUNC(gst_app_sink_get_type)
 0:00.58 [0;1;32m         ^
 0:00.58 [0m[1m/Users/alessandro/src/git/mozilla-central/content/media/gstreamer/GStreamerLoader.cpp:115:40: [0m[0;1;30mnote: [0mexpanded from macro 'GST_FUNC'[0m
 0:00.58 #define GST_FUNC(func) typeof(::func)* func;
 0:00.58 [0;1;32m                                       ^
 0:00.58 [0m[1m/Users/alessandro/src/git/mozilla-central/content/media/gstreamer/GStreamerFunctionList.h:9:10: [0m[0;1;30mnote: [0mprevious definition is here[0m
 0:00.58 GST_FUNC(gst_app_sink_get_type)
 0:00.58 [0;1;32m         ^
 0:00.58 [0m[1m/Users/alessandro/src/git/mozilla-central/content/media/gstreamer/GStreamerLoader.cpp:17:40: [0m[0;1;30mnote: [0mexpanded from macro 'GST_FUNC'[0m
 0:00.58 #define GST_FUNC(func) typeof(::func)* func;
 0:00.58 [0;1;32m                                       ^
 0:00.58 [0mIn file included from /Users/alessandro/src/git/mozilla-central/content/media/gstreamer/GStreamerLoader.cpp:117:
 0:00.58 [1m/Users/alessandro/src/git/mozilla-central/content/media/gstreamer/GStreamerFunctionList.h:10:10: [0m[0;1;31merror: [0m[1mredefinition of 'gst_app_sink_pull_buffer'[0m
 0:00.58 GST_FUNC(gst_app_sink_pull_buffer)
 0:00.58 [0;1;32m         ^
 0:00.58 [0m[1m/Users/alessandro/src/git/mozilla-central/content/media/gstreamer/GStreamerLoader.cpp:115:40: [0m[0;1;30mnote: [0mexpanded from macro 'GST_FUNC'[0m
 0:00.58 #define GST_FUNC(func) typeof(::func)* func;
 0:00.58 [0;1;32m                                       ^
 0:00.58 [0m[1m/Users/alessandro/src/git/mozilla-central/content/media/gstreamer/GStreamerFunctionList.h:10:10: [0m[0;1;30mnote: [0mprevious definition is here[0m
 0:00.58 GST_FUNC(gst_app_sink_pull_buffer)

[snip]
Sigh. I must have put up the patch from the wrong computer. I'll put up the right one when I'm in the office next.
Seems to be the correct patch. Builds fine for me on OSX 8 with clang 4.1.
(In reply to Edwin Flores [:eflores] [:edwin] from comment #23)
> Seems to be the correct patch. Builds fine for me on OSX 8 with clang 4.1.

You're right, I must have done something wrong fixing local conflicts when I tried the last time. The code looks ok and I can confirm it works on mac.

I don't have a linux box to test this though. Is gst deployed on the mozilla infrastructure yet?
(In reply to Alessandro Decina from comment #24)
> I don't have a linux box to test this though. Is gst deployed on the mozilla
> infrastructure yet?

Yes, we now have the gstreamer-devel packages on our try servers. Unfortunately, centos lags quite a bit so it's only 0.10.29. There are a couple of things that the gstreamer backend uses that aren't in 0.10.29, so I'll have to take a look at fixing those things before we can look at landing this. Sigh.
(In reply to Edwin Flores [:eflores] [:edwin] from comment #25)
> (In reply to Alessandro Decina from comment #24)
> > I don't have a linux box to test this though. Is gst deployed on the mozilla
> > infrastructure yet?
> 
> Yes, we now have the gstreamer-devel packages on our try servers.
> Unfortunately, centos lags quite a bit so it's only 0.10.29. There are a
> couple of things that the gstreamer backend uses that aren't in 0.10.29, so
> I'll have to take a look at fixing those things before we can look at
> landing this. Sigh.

Ah... do you already know what those are?
(In reply to Alessandro Decina from comment #26)
> Ah... do you already know what those are?

gst_buffer_get_qdata
gst_buffer_set_qdata
gst_element_factory_list_filter
gst_element_factory_list_get_elements
gst_structure_take_value
I think https://bugzilla.mozilla.org/show_bug.cgi?id=856308 took care of take_value and get and set _qdata.

Doing without the factory list API will be annoying though :/
That landed a while ago; looks like it might have missed a couple of things, or just wasn't working with anything that old.

I've asked whether we can get non-centos packages on the build boxes in the hope that we can get a more modern gstreamer to work with; we should have an answer in the next day.
Comment on attachment 744332 [details] [diff] [review]
Patch v3

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

::: content/media/gstreamer/GStreamerLoader.cpp
@@ +44,5 @@
> +    dlopen("libgstreamer-0.10.so.0", RTLD_NOW | RTLD_LOCAL),
> +    dlopen("libgstapp-0.10.so.0", RTLD_NOW | RTLD_LOCAL),
> +    dlopen("libgstbase-0.10.so.0", RTLD_NOW | RTLD_LOCAL),
> +    dlopen("libgstvideo-0.10.so.0", RTLD_NOW | RTLD_LOCAL)
> +  };

Before doing that, you should try to get dlsym(RTLD_DEFAULT, "gst_version"). That will return a pointer if gstreamer is already loaded because it is a dependency of something else we're loading. You don't want to dlopen if gstreamer is already loaded. And if it's not the right version, you don't want to use it either.

@@ +54,5 @@
> +  }
> +
> +#define GST_FUNC(symbol) \
> +  for (size_t i = 0; i < sizeof(handles) / sizeof (handles[0]); i++) { \
> +    symbol = (typeof(symbol))dlsym(handles[i], #symbol); \

It would be better to directly load symbols from the right handle instead of enumerating.
Attachment #744332 - Flags: feedback-
(In reply to Alessandro Decina from comment #28)
> I think https://bugzilla.mozilla.org/show_bug.cgi?id=856308 took care of
> take_value and get and set _qdata.

Oops, looks like you're right about these.
(In reply to Mike Hommey (high latency until June 25) [:glandium] from comment #30)
> It would be better to directly load symbols from the right handle instead of
> enumerating.

Seems like a lot more ugliness in some already pretty ugly code to save an insignificant amount of time in a function that will only ever be run once per instance of the browser.
Posted patch Patch v4 (obsolete) — Splinter Review
Attachment #744332 - Attachment is obsolete: true
Attachment #744332 - Flags: review?(alessandro.d)
Attachment #757800 - Flags: review?(alessandro.d)
Attachment #757800 - Flags: feedback?(mh+mozilla)
(In reply to Edwin Flores [:eflores] [:edwin] from comment #32)
> (In reply to Mike Hommey (high latency until June 25) [:glandium] from
> comment #30)
> > It would be better to directly load symbols from the right handle instead of
> > enumerating.
> 
> Seems like a lot more ugliness in some already pretty ugly code to save an
> insignificant amount of time in a function that will only ever be run once
> per instance of the browser.

That's not really uglier:

#define LIBGSTREAMER 0
#define LIBGSTAPP 1
#define LIBGSTVIDEO 2

GST_FUNC(LIBGSTAPP, gst_app_sink_get_type)
(...)

#define GST_FUNC(lib, symbol) do { symbol = dlsym(handles[lib], #symbol); } while(0);
Comment on attachment 757800 [details] [diff] [review]
Patch v4

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

::: content/media/gstreamer/GStreamerLoader.cpp
@@ +89,5 @@
> +fail:
> +
> +  for (size_t i = 0; i < sizeof(handles) / sizeof(handles[0]); i++) {
> +    if (handles[i]) {
> +      dlclose(handles[i]);

You probably don't want to dlclose(RTLD_DEFAULT)
Attachment #757800 - Flags: feedback?(mh+mozilla)
Posted patch Patch v5Splinter Review
Addressed comment 34 and comment 35.
Attachment #757800 - Attachment is obsolete: true
Attachment #757800 - Flags: review?(alessandro.d)
Attachment #758829 - Flags: review?(alessandro.d)
Comment on attachment 758829 [details] [diff] [review]
Patch v5

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

r+, with again the only reserve that i'm only able to test this on mac
Attachment #758829 - Flags: review?(alessandro.d) → review+
https://hg.mozilla.org/mozilla-central/rev/7881391b201f
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Depends on: 878363
Blocks: 927898
You need to log in before you can comment on or make changes to this bug.