859199 - Load GStreamer at runtime on Linux

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Loading the GStreamer libraries at runtime will let us ship with --enable-gstreamer on Linux by default.

Comment 1

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Paaaattttccchhhhhhhhhhhh

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.

Comment 2

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Try run:
https://tbpl.mozilla.org/?tree=Try&rev=0b3d80586b6b

Comment 3

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch Numero Dos

I am sometimes silly.

Comment 4

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

https://tbpl.mozilla.org/?tree=Try&rev=2acc84eb7bb3

Comment 5

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Aaaand one more try push with it actually on:
https://tbpl.mozilla.org/?tree=Try&rev=f2557f16bf52

Comment 6

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Oops, doesn't look like the try servers have the gstreamer-devel packages yet.

Comment 7

[Ben Bucksch (:BenB)]

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.

Comment 8

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

What is the advantage of doing that?

Comment 9

[Ben Bucksch (:BenB)]

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.

Comment 10

[Ben Bucksch (:BenB)]

Essentially, this is what XPCOM is for: dynamically adding libraries.

Comment 11

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

/me starts reading XPCOM manual

Comment 12

[Ben Bucksch (:BenB)]

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.

Comment 13

[Ben Bucksch (:BenB)]

I've asked roc for comment, whether he thinks that's a good idea or not.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 15

[Ben Bucksch (:BenB)]

OK, nevermind, then.

Comment 16

[Mike Hommey [:glandium]]

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)

Comment 17

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

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.

Comment 18

[Alessandro Decina]

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.

Comment 19

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

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

Comment 20

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch v3

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.

Comment 21

[Alessandro Decina]

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]

Comment 22

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

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.

Comment 23

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Seems to be the correct patch. Builds fine for me on OSX 8 with clang 4.1.

Comment 24

[Alessandro Decina]

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

Comment 25

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

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

Comment 26

[Alessandro Decina]

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

Comment 27

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(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

Comment 28

[Alessandro Decina]

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

Comment 29

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

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 30

[Mike Hommey [:glandium]]

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.

Comment 31

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

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

Comment 32

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

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

Comment 33

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch v4

Comment 34

[Mike Hommey [:glandium]]

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

[Mike Hommey [:glandium]]

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)

Comment 36

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch v5

Addressed comment 34 and comment 35.

Comment 37

[Alessandro Decina]

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

Comment 38

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7881391b201f

Comment 39

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7881391b201f