This FIXME in GStreamerUtilities.cpp asks to pass the command line parameters to the GStreamer initialization function: https://github.com/WebKit/webkit/blob/80403285b787da5677372e320efca24b304ea5b4/Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp#L154 Having support for GStreamer command line parameters would be nice for debugging MiniBrowser.
Created attachment 313727 [details] Patch
Comment on attachment 313727 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313727&action=review > Source/WebCore/ChangeLog:12 > + (WebCore::initializeGStreamer): Here gets the Gstreamer's option from GStreamer should be spelled like this. > Source/WebCore/ChangeLog:13 > + the main() and create the char "argv" to Initialize the GStreamer library. "the main() function" or just "main()". Initialized does not need to be capitalized. > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:155 > + Please remove this empty line. > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:156 > + const gchar* gstArgv = g_getenv("GST_ARGUMENTS"); const char* gstreamerArgumentsString > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:157 > + bool gstInitialized = false; bool isGStreamerInitialized > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:160 > + Please remove this empty line. > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:161 > + gchar ** argvSplit = g_strsplit(gstArgv, " ", -1); char** splitGStreamerArguments > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:162 > + guint argcSizeSplit = g_strv_length(argvSplit); unsigned gstreamerArgumentsSize > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:164 > + char ** nArgv = g_new0(char*, argcSizeSplit+1); char** argv = g_new0(char*, gstreamerArgumentsSize + 1); > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:167 > + for (guint i = 0; i < (argcSizeSplit+1); i++) unsigned i > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:173 > + argcSizeSplit++; > + int* argc = (int*)(&argcSizeSplit); > + > + gstInitialized = gst_init_check(argc, &nArgv, &error.outPtr()); int argc = gstreamerArgumentsSize + 1; isGStreamerInitialized = gst_init_check(&argc, &argv, &error.outPtr()); > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:175 > + ASSERT_WITH_MESSAGE(gstInitialized, "GStreamer initialization failed: %s", error ? error->message : > + "unknown error occurred"); As we allow long lines, you can make this so. > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:176 > + Please remove this empty line. > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:184 > + Please remove this empty line. > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:187 > + ASSERT_WITH_MESSAGE(gstInitialized, "GStreamer initialization failed: %s", error ? error->message : > + "unknown error occurred"); ditto > Tools/ChangeLog:12 > + I need to use them in this part of the project to returns there's a typo "to returns" -> "to return". > Tools/ChangeLog:15 > + (main): Detect the Gstreamer's input options to pass GStreamer is the proper capitalization. > Tools/ChangeLog:16 > + them to the GStreamer initialization function. ditto. > Tools/MiniBrowser/gtk/main.c:471 > + GString *strbuf = g_string_new(NULL); Please use WTF::StringBuilder and name it stringBuilder (WK coding style: full words as variable names) > Tools/MiniBrowser/gtk/main.c:472 > + gboolean gstParam = false; Please use bool and name the variable as gstreamerParameters. > Tools/MiniBrowser/gtk/main.c:482 > + gchar * arguments = g_string_free(strbuf, FALSE); I suspect you won't need this line with WTF::StringBuilder but you should have used char instead gchar and placing the * next to the type: char* arguments; > Tools/MiniBrowser/gtk/main.c:491 > + g_option_context_add_group(context, gst_init_get_option_group()); Doc says: If you use this function, you should make sure you initialise the GLib threading system as one of the very first things in your program (see the example at the beginning of this section). I guess you should call gst_init here which is going to be fun as it will steal your GStreamer parameter and you'll need to copy them first.
In general I don't like passing the information from one process to the other by using an environment variable. I think there must some some better way that Carlos García knows better than me (CCed).
(In reply to Xabier Rodríguez Calvar from comment #2) > > Tools/MiniBrowser/gtk/main.c:471 > > + GString *strbuf = g_string_new(NULL); > > Please use WTF::StringBuilder and name it stringBuilder (WK coding style: > full words as variable names) According to Enrique, this is plain C and we can't use WTF here, so forget this. > > Tools/MiniBrowser/gtk/main.c:482 > > + gchar * arguments = g_string_free(strbuf, FALSE); > > I suspect you won't need this line with WTF::StringBuilder but you should > have used char instead gchar and placing the * next to the type: char* > arguments; In this case, as we are keeing GString, better to access stringBuilder->str in the next line and free the GString with g_string_free(stringBuilder, TRUE).
Are the gst command line options only for debugging? Isn't it possible to do the same things with GST en vars? Do we expect that other application will be able to pass command line options to gst? or is this an internal MiniBrowser thing for debugging?
Comment on attachment 313727 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313727&action=review > Tools/MiniBrowser/gtk/main.c:483 > + g_setenv("GST_ARGUMENTS", arguments, FALSE); The variable name, should we keep it in case Carlos does not provide a better solution, should be called GSTREAMER_ARGUMENTS
(In reply to Carlos Garcia Campos from comment #5) > Are the gst command line options only for debugging? Isn't it possible to do > the same things with GST en vars? Do we expect that other application will > be able to pass command line options to gst? or is this an internal > MiniBrowser thing for debugging? The purpose, as Philippe originally commented to me when we thought about the task, is about debugging. All the GStreamer-related tools have a --help-gst command line parameter which explains what gst parameters you can pass. If other applications using WebKit want to honor the parameters, it's up to them. By now, the most useful usage for debugging is to have support just in MiniBrowser.
(In reply to Enrique Ocaña from comment #7) > (In reply to Carlos Garcia Campos from comment #5) > > > Are the gst command line options only for debugging? Isn't it possible to do > > the same things with GST en vars? Do we expect that other application will > > be able to pass command line options to gst? or is this an internal > > MiniBrowser thing for debugging? > > The purpose, as Philippe originally commented to me when we thought about > the task, is about debugging. All the GStreamer-related tools have a > --help-gst command line parameter which explains what gst parameters you can > pass. > > If other applications using WebKit want to honor the parameters, it's up to > them. It's not up to them, how are they going to do it if we don't provide a way to do it? That's exactly why I'm asking. > By now, the most useful usage for debugging is to have support just in > MiniBrowser. What cab be done with command line argument that can't be done with GST_DEBUG?
(In reply to Carlos Garcia Campos from comment #8) > It's not up to them, how are they going to do it if we don't provide a way > to do it? That's exactly why I'm asking. Ask Philippe. He's the one who wrote the FIXME in the first place. > > By now, the most useful usage for debugging is to have support just in > > MiniBrowser. > > What cab be done with command line argument that can't be done with > GST_DEBUG? These are all the options which can't be emulated via GST_DEBUG: --gst-version Print the GStreamer version --gst-fatal-warnings Make all warnings fatal --gst-debug-help Print available debug categories and exit --gst-debug-no-color Disable colored debugging output --gst-debug-color-mode Changes coloring mode of the debug log. Possible modes: off, on, disable, auto, unix --gst-plugin-spew Enable verbose plugin loading diagnostics --gst-plugin-path=PATHS Colon-separated paths containing plugins --gst-plugin-load=PLUGINS Comma-separated list of plugins to preload in addition to the list stored in environment variable GST_PLUGIN_PATH --gst-disable-segtrap Disable trapping of segmentation faults during plugin loading --gst-disable-registry-update Disable updating the registry --gst-disable-registry-fork Disable spawning a helper process while scanning the registry
(In reply to Enrique Ocaña from comment #9) > (In reply to Carlos Garcia Campos from comment #8) > > > It's not up to them, how are they going to do it if we don't provide a way > > to do it? That's exactly why I'm asking. > > Ask Philippe. He's the one who wrote the FIXME in the first place. Let's wait for Philippe to do this them, because I can't ask him.
(In reply to Carlos Garcia Campos from comment #10) > (In reply to Enrique Ocaña from comment #9) > > (In reply to Carlos Garcia Campos from comment #8) > > > > > It's not up to them, how are they going to do it if we don't provide a way > > > to do it? That's exactly why I'm asking. > > > > Ask Philippe. He's the one who wrote the FIXME in the first place. > > Let's wait for Philippe to do this them, because I can't ask him. We don't need to wait for Philippe. I can tell you that this is not absolutely necessary but very good to have. Our problem is that a user can call the UI process and pass these options but we need to put it through to the web process. My question to you was not if this was necessary or not, my question is which is the best way to share the information between the UI and web processes in this case.
(In reply to Xabier Rodríguez Calvar from comment #11) > (In reply to Carlos Garcia Campos from comment #10) > > (In reply to Enrique Ocaña from comment #9) > > > (In reply to Carlos Garcia Campos from comment #8) > > > > > > > It's not up to them, how are they going to do it if we don't provide a way > > > > to do it? That's exactly why I'm asking. > > > > > > Ask Philippe. He's the one who wrote the FIXME in the first place. > > > > Let's wait for Philippe to do this them, because I can't ask him. > > We don't need to wait for Philippe. I can tell you that this is not > absolutely necessary but very good to have. Our problem is that a user can > call the UI process and pass these options but we need to put it through to > the web process. > > My question to you was not if this was necessary or not, my question is > which is the best way to share the information between the UI and web > processes in this case. But the best way depends on what this is for, and how it's going to be used, that's why I'm asking. If it's only a debug thing, that we will enable for developer buuolds only, the env var is a good enough solution, and very convenient. If this si somethign we want other applications to use for whatever purpose, then we would need to add public API and then the proper way is to pass them via IPC as web process initialization parameters
One possible solution would be to check /proc/self/cmdline in the UI process, collect all gst options and pass them to the web process as initialization parameters (WebProcessCreationParameters). That way we don't need changes in MB/apps, new API nor using env vars.
Comment on attachment 313727 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313727&action=review > Tools/MiniBrowser/gtk/main.c:469 > g_setenv("WEBKIT_INJECTED_BUNDLE_PATH", WEBKIT_INJECTED_BUNDLE_PATH, FALSE); I know this was here before, but it needs to move up above the call to gtk_init... >> Tools/MiniBrowser/gtk/main.c:483 >> + g_setenv("GST_ARGUMENTS", arguments, FALSE); > > The variable name, should we keep it in case Carlos does not provide a better solution, should be called GSTREAMER_ARGUMENTS ...but I don't know what you can do about this one. r- from me for using setenv after threads have been created. We just had a crash in GNOME a couple years ago where this brought down the entire desktop session. Last month we had another one in Endless where it randomly crashed our inital-setup process, so users couldn't boot their computers. Here it's just going to crash MB, but still. setenv is not threadsafe unless no threads are calling getenv, and getenv is used all over the place, deep in libraries. It's unavoidable. So setenv has to happen at the very very top of main, before doing things like library initialization. Unfortunately I do not have any alternate recommendation as to how to achieve the behavior you want. :/
(In reply to Michael Catanzaro from comment #14) > Unfortunately I do not have any alternate recommendation as to how to > achieve the behavior you want. :/ Ah, I didn't realize I hadn't read the bug before commenting: (In reply to Carlos Garcia Campos from comment #13) > One possible solution would be to check /proc/self/cmdline in the UI > process, collect all gst options and pass them to the web process as > initialization parameters (WebProcessCreationParameters). That way we don't > need changes in MB/apps, new API nor using env vars. Yes, that would be safe.
(In reply to Carlos Garcia Campos from comment #13) > One possible solution would be to check /proc/self/cmdline in the UI > process, collect all gst options and pass them to the web process as > initialization parameters (WebProcessCreationParameters). That way we don't > need changes in MB/apps, new API nor using env vars. This sounds good.
(In reply to Carlos Garcia Campos from comment #13) > One possible solution would be to check /proc/self/cmdline in the UI > process, collect all gst options and pass them to the web process as > initialization parameters (WebProcessCreationParameters). That way we don't > need changes in MB/apps, new API nor using env vars. Ok, I will apply this solution by following these steps: 1- Check /proc/self/cmdline and collect all gst options inside WebProcessCreationParameters. This will be done within the WebProcessPool::createNewWebProcess() method. (UIProcess) 2- After, I will receive the WebProcessCreationParameters within the WebProcess::initializeWebProcess() method. (WebProcess) - BUT!!, the problem is how to make WebProcess::initializeWebProcess() and the MediaPlayerPrivateGStreamer talk to each other, because gst_init() is located in the "GstreamerUtilities.cpp" file, and here it needs to get some information from the WebProcessCreationParameters. This is possible? Does anyone know any possible solution? Thanks!
Created attachment 319788 [details] Patch
Comment on attachment 319788 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=319788&action=review Hi! I have a lot of comments, but don't be discouraged: that's normal. Getting patches into WebKit isn't easy. > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:28 > +#include "WebKit/WebProcess/InjectedBundle/API/gtk/DOM/ConvertToUTF8String.h" There's a reason that the WebKit subproject directories are not in the include path: WebCore is a lower layer. You can't use the WebKit subproject here. (In fact, WebCore/platform is supposed to be a lower layer than the rest of WebCore, too, though that's not enforced yet.) It looks like you don't use the functions from this header anymore, since you found WTF::String::utf8, so you can just remove it. > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:153 > +#if ENABLE(VIDEO) && USE(GSTREAMER) What if WebKit is built with USE(GSTREAMER) and ENABLE(WEB_AUDIO) but not ENABLE(VIDEO)? Now GStreamer won't be initialized. I think you need to drop the ENABLE(VIDEO) check here. > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:163 > + nArgv = g_new0(char*, argcSizeSplit+1); > + > + for (guint i = 0; i < parameters.size(); i++) > + nArgv[i] = g_strdup(parameters[i].utf8().data()); You need to free nArgv with g_strfreev(). > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:167 > + argc = (int*) (&size); > + gstInitialized = gst_init_check(argc, &nArgv, &error.outPtr()); You don't need argc here. Just pass &size to gst_init_check(). > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:169 > + ASSERT_WITH_MESSAGE(gstInitialized, "GStreamer initialization failed: %s", error ? error->message : > + "unknown error occurred"); This would be nicer on one long line. Anyway, this is a preexisting problem, but it doesn't make sense to use ASSERT_WITH_MESSAGE() here because we really want this assert in release mode, not just debug mode. We could use RELEASE_ASSERT(), but it would be easier to just use gst_init() instead of gst_init_check(). The point of gst_init_check() is to allow the application to gracefully handle a situation where GStreamer is broken, but that's fatal for WebKit: we really just want to crash always. If you make this change, then this function will need to be changed to return void instead of a bool. > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:177 > return gstInitialized; > +#endif So now the function has no return value in the !ENABLE(VIDEO) || !USE(GSTREAMER) case. Easiest solution is to change the function to return void like I suggest above, but alternatively you could declare gstInitialized outside the conditional. > Source/WebKit/UIProcess/gtk/WebProcessPoolGtk.cpp:96 > + gboolean isGStreamerParameters = false; Use normal bool. If you need to use gboolean, which is necessary when interacting with GNOME's C APIs, then beware that it's an int rather than one byte, and use GLib's TRUE/FALSE instead of C++'s true/false. But otherwise, it's best to use C++ bool. > Source/WebKit/UIProcess/gtk/WebProcessPoolGtk.cpp:99 > + enum { > + BufferSize = 2048 > + }; Don't create an enum with only one member, just declare a constant: const unsigned BufferSize = 2048; > Source/WebKit/UIProcess/gtk/WebProcessPoolGtk.cpp:105 > + while (fgets(buffer, BufferSize, file)) { I see you've tried to account for the command line being longer than 2048 characters, but it doesn't work: it misses any GStreamer options that might be split over the boundary. So this is going to need a rethink. The standard library fgets() makes this very difficult because it forces you to guess the size of the buffer in advance. I'm going to suggest using g_file_get_contents() instead of fopen(), that way you get the result in one string of the right size allocated by Gio (remember to free it with g_free()) and everything will become much simpler. > Source/WebKit/UIProcess/gtk/WebProcessPoolGtk.cpp:108 > + isGStreamerParameters = g_str_has_prefix(buffer + n, "--gst"); There's actually no reason to filter out parameters here. Just pass everything on to GStreamer. Let GStreamer figure out for itself what to do with the options. > Source/WebKit/UIProcess/gtk/WebProcessPoolGtk.cpp:111 > + char* var = &buffer[n]; > + parameters.gstreamerOptions.append(String::fromUTF8(var)); In WebKit we prefer to avoid unnecessary local variables unless it significantly improves readability. In this case it's better to do it in one line: parameters.gstreamerOptions.append(String::fromUTF8(&buffer[n]). > Source/WebKit/WebProcess/soup/WebProcessSoup.cpp:30 > +#include "WebCore/platform/graphics/gstreamer/GStreamerUtilities.h" We don't use full paths in WebKit, see https://webkit.org/code-style-guidelines/#include-statements. Just #include <GStreamerUtilities.h>. Add WebCore/platform/graphics/gstreamer to the include path in PlatformGTK.cmake and PlatformWPE.cmake if you need to.
Comment on attachment 319788 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=319788&action=review Besides Michael's comments, I have some of my own. > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:154 > + bool gstInitialized = false; We should name this isGStreamerInitialized. > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:155 > + char** nArgv; nArgv -> argv. I don't see the need of prepending the n here. > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:157 > + guint size = 0; This should be unsigned. > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:159 > + guint argcSizeSplit = parameters.size(); This should be size_t > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:162 > + for (guint i = 0; i < parameters.size(); i++) guint -> unsigned. >> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:163 >> + nArgv[i] = g_strdup(parameters[i].utf8().data()); > > You need to free nArgv with g_strfreev(). Or not duplicating the memory cause gst_init_check can modify the number of options but it won't free the strings. I would maybe g_newa argv (to allocate it in the stack and avoid freeing it later) and just assign parameters[i].utf8().data() to it.
(In reply to Xabier Rodríguez Calvar from comment #20) > Or not duplicating the memory cause gst_init_check can modify the number of > options but it won't free the strings. Hm, good catch. > I would maybe g_newa argv (to allocate it in the stack and avoid freeing it > later) and just assign parameters[i].utf8().data() to it. I hate to use g_newa for user input, but this is a very tricky problem and it seems like the best option. It's not really an unbounded amount of input because there is a command line length limit. And clearly the API is not usable with memory that is not stack-allocated.
Created attachment 334147 [details] Patch
(In reply to Michael Catanzaro from comment #19) [...] > I see you've tried to account for the command line being longer than 2048 > characters, but it doesn't work: it misses any GStreamer options that might > be split over the boundary. So this is going to need a rethink. The standard > library fgets() makes this very difficult because it forces you to guess the > size of the buffer in advance. I'm going to suggest using > g_file_get_contents() instead of fopen(), that way you get the result in one > string of the right size allocated by Gio (remember to free it with > g_free()) and everything will become much simpler. > > > Source/WebKit/UIProcess/gtk/WebProcessPoolGtk.cpp:108 > > + isGStreamerParameters = g_str_has_prefix(buffer + n, "--gst"); > > There's actually no reason to filter out parameters here. Just pass > everything on to GStreamer. Let GStreamer figure out for itself what to do > with the options. Are you sure gst doesn't fail if it receives options it doesn't recognize?
MiniBrowser exits in those cases: Cannot parse arguments: Unknown option --gaaast-version2 Cannot parse arguments: Unknown option --gst-version2
(In reply to Philippe Normand from comment #24) > MiniBrowser exits in those cases: > > Cannot parse arguments: Unknown option --gaaast-version2 > Cannot parse arguments: Unknown option --gst-version2 I don't mean in case of invalid options, but when passing valid minibrowser options to gst init.
(In reply to Carlos Garcia Campos from comment #25) > (In reply to Philippe Normand from comment #24) > > MiniBrowser exits in those cases: > > > > Cannot parse arguments: Unknown option --gaaast-version2 > > Cannot parse arguments: Unknown option --gst-version2 > > I don't mean in case of invalid options, but when passing valid minibrowser > options to gst init. That works here
Comment on attachment 334147 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=334147&action=review I don't know if carlos has any other comments and needs to freely undo my r+, but other that the nits I wrote, LGTM. > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:245 > + char** nArgv; nArgv -> newArgv or just argv > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:247 > + size_t argcSizeSplit = parameters.size(); You don't need this variable > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:248 > + nArgv = g_new0(char*, argcSizeSplit+1); parameters.size() + 1 > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:251 > + char** nArgv; > + parameters.insert(0, "WebProcess"); > + size_t argcSizeSplit = parameters.size(); > + nArgv = g_new0(char*, argcSizeSplit+1); > + > + for (unsigned i = 0; i < parameters.size(); i++) > + nArgv[i] = g_strdup(parameters[i].utf8().data()); If I had to write this, I would: 1. make the function take const Vector& 2. g_new0 size + 2 instead of 1 3. g_strdup("WebProcess") to newArgs[0]. 4. begin the loop at 1.
Committed r228818: <https://trac.webkit.org/changeset/228818>
<rdar://problem/37706341>
Comment on attachment 334147 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=334147&action=review I'm sorry to be late here, but yes, I have a few comments. The most important thing is that this patch does nothing and breaks several unit tests. The gst options passed to MiniBrowser only work in the UI process because gst_init_get_option_group() ends up initializing gst, but then we are just passing the program name to the web process. It breaks several unit tests that use the MIMETypeRegistry in the UI process, because we are now assuming that gst is only used in the web process. The MIMETypeRegistry uses it to get the list of supported media types. In the UI process gst is initialized by gst_init_get_option_group() in MiniBrowser, but not in the unit tests or any other application not using gst_init_get_option_group(). > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:241 > GUniqueOutPtr<GError> error; Now this is unused if !ENABLE(VIDEO) && !ENABLE(WEB_AUDIO) > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:246 > + parameters.insert(0, "WebProcess"); Why WebProcess? that's not even the name in the gtk port. We could use g_get_prgname() > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:253 > + int size = g_strv_length(nArgv); This iterates the array again when we already know the size is parameters.size() > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:-142 > - if (!initializeGStreamer()) > - return false; This is still needed, this is called in the UI process too. > Source/WebKit/UIProcess/gtk/WebProcessPoolGtk.cpp:98 > + GUniqueOutPtr<gchar> contents; > + gsize length; > + if (g_file_get_contents("/proc/self/cmdline", &contents.outPtr(), &length, nullptr)) > + parameters.gstreamerOptions.append(String::fromUTF8(contents.get())); Options in /proc/self/cmdline are separated by null characters. Here we are using a Vector to always pass only one option, the whole contents of /proc/self/cmdline, but String::fromUTF8() will read until the first null character, so we are always passing the program name here. > Source/WebKit/WebProcess/soup/WebProcessSoup.cpp:57 > +#if USE(GSTREAMER) > + WebCore::initializeGStreamer(parameters.gstreamerOptions); > +#endif This is done for both WPE and GTK, but only GTK is sending the gst parameters to the WebProcess.
(In reply to Philippe Normand from comment #26) > (In reply to Carlos Garcia Campos from comment #25) > > (In reply to Philippe Normand from comment #24) > > > MiniBrowser exits in those cases: > > > > > > Cannot parse arguments: Unknown option --gaaast-version2 > > > Cannot parse arguments: Unknown option --gst-version2 > > > > I don't mean in case of invalid options, but when passing valid minibrowser > > options to gst init. > > That works here I would still feel more confortable if we only pass the gst options, if gst_init is changed in the future to fail in case of unrecognized options, this will break. And there's no reason to send options via IPC when we know they are going to be ignored.
Reopening, I'll submit a new patch.
Created attachment 334515 [details] Patch
Comment on attachment 334515 [details] Patch Thanks.
Committed r228946: <https://trac.webkit.org/changeset/228946>
Not exactly sure how GTK+ sets up the prgname thing, probably through gtk_main(). But with WPE we have to do it manually: https://trac.webkit.org/changeset/228973/webkit
(In reply to Zan Dobersek from comment #36) > Not exactly sure how GTK+ sets up the prgname thing, probably through > gtk_main(). But with WPE we have to do it manually: > https://trac.webkit.org/changeset/228973/webkit Ah, right, that's done in gtk_init. I think it's a good idea that wpe sets it, but we should still fallback to getting the last component of getCurrentExecutablePath() in case we get nullptr.