Bug 173655 – [GStreamer][MiniBrowser] Honor GStreamer command line parameters in MiniBrowser

• Component: Media
• Status: RESOLVED FIXED

[Enrique Ocaña]

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.

[Vanessa Chipirrás Navalón]

Created attachment 313727 [details]
Patch

[Xabier Rodríguez Calvar]

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.

[Xabier Rodríguez Calvar]

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

[Xabier Rodríguez Calvar]

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

[Carlos Garcia Campos]

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?

[Xabier Rodríguez Calvar]

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

[Enrique Ocaña]

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

[Carlos Garcia Campos]

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

[Enrique Ocaña]

(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

[Carlos Garcia Campos]

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

[Xabier Rodríguez Calvar]

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

[Carlos Garcia Campos]

(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

[Carlos Garcia Campos]

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.

[Michael Catanzaro]

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

[Michael Catanzaro]

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

[Xabier Rodríguez Calvar]

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

[Vanessa Chipirrás Navalón]

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

[Vanessa Chipirrás Navalón]

Created attachment 319788 [details]
Patch

[Michael Catanzaro]

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.

[Xabier Rodríguez Calvar]

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.

[Michael Catanzaro]

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

[Philippe Normand]

Created attachment 334147 [details]
Patch

[Carlos Garcia Campos]

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

[Philippe Normand]

MiniBrowser exits in those cases:

Cannot parse arguments: Unknown option --gaaast-version2
Cannot parse arguments: Unknown option --gst-version2

[Carlos Garcia Campos]

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

[Philippe Normand]

(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

[Xabier Rodríguez Calvar]

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.

[Philippe Normand]

Committed r228818: <https://trac.webkit.org/changeset/228818>

[Radar WebKit Bug Importer]

<rdar://problem/37706341>

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

Reopening, I'll submit a new patch.

[Carlos Garcia Campos]

Created attachment 334515 [details]
Patch

[Philippe Normand]

Comment on attachment 334515 [details]
Patch

Thanks.

[Carlos Garcia Campos]

Committed r228946: <https://trac.webkit.org/changeset/228946>

[Zan Dobersek]

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

[Carlos Garcia Campos]

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