WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
173655
[GStreamer][MiniBrowser] Honor GStreamer command line parameters in MiniBrowser
https://bugs.webkit.org/show_bug.cgi?id=173655
Summary
[GStreamer][MiniBrowser] Honor GStreamer command line parameters in MiniBrowser
Enrique Ocaña
Reported
2017-06-21 09:14:16 PDT
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.
Attachments
Patch
(6.21 KB, patch)
2017-06-23 10:33 PDT
,
Vanessa Chipirrás Navalón
no flags
Details
Formatted Diff
Diff
Patch
(14.88 KB, patch)
2017-09-03 10:49 PDT
,
Vanessa Chipirrás Navalón
no flags
Details
Formatted Diff
Diff
Patch
(13.85 KB, patch)
2018-02-19 01:57 PST
,
Philippe Normand
calvaris
: review+
calvaris
: commit-queue-
Details
Formatted Diff
Diff
Patch
(10.99 KB, patch)
2018-02-23 04:00 PST
,
Carlos Garcia Campos
pnormand
: review+
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Vanessa Chipirrás Navalón
Comment 1
2017-06-23 10:33:39 PDT
Created
attachment 313727
[details]
Patch
Xabier Rodríguez Calvar
Comment 2
2017-06-26 02:25:25 PDT
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
Comment 3
2017-06-26 02:26:42 PDT
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
Comment 4
2017-06-26 02:40:51 PDT
(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
Comment 5
2017-06-26 02:43:18 PDT
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 6
2017-06-26 02:43:42 PDT
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
Comment 7
2017-06-26 03:10:49 PDT
(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
Comment 8
2017-06-26 03:17:17 PDT
(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
Comment 9
2017-06-26 03:39:31 PDT
(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
Comment 10
2017-06-26 03:52:20 PDT
(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
Comment 11
2017-06-26 04:21:41 PDT
(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
Comment 12
2017-06-26 04:29:50 PDT
(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
Comment 13
2017-06-26 04:39:54 PDT
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 14
2017-06-26 07:45:50 PDT
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
Comment 15
2017-06-26 07:49:24 PDT
(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
Comment 16
2017-06-26 08:12:39 PDT
(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
Comment 17
2017-07-05 11:51:37 PDT
(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
Comment 18
2017-09-03 10:49:44 PDT
Created
attachment 319788
[details]
Patch
Michael Catanzaro
Comment 19
2017-09-03 16:30:05 PDT
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 20
2017-09-04 02:41:56 PDT
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
Comment 21
2017-09-04 08:44:46 PDT
(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
Comment 22
2018-02-19 01:57:14 PST
Created
attachment 334147
[details]
Patch
Carlos Garcia Campos
Comment 23
2018-02-19 03:01:18 PST
(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
Comment 24
2018-02-19 03:11:50 PST
MiniBrowser exits in those cases: Cannot parse arguments: Unknown option --gaaast-version2 Cannot parse arguments: Unknown option --gst-version2
Carlos Garcia Campos
Comment 25
2018-02-19 03:14:58 PST
(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
Comment 26
2018-02-19 03:22:07 PST
(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 27
2018-02-19 05:18:24 PST
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
Comment 28
2018-02-20 06:16:21 PST
Committed
r228818
: <
https://trac.webkit.org/changeset/228818
>
Radar WebKit Bug Importer
Comment 29
2018-02-20 06:28:41 PST
<
rdar://problem/37706341
>
Carlos Garcia Campos
Comment 30
2018-02-23 03:58:36 PST
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
Comment 31
2018-02-23 04:00:11 PST
(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
Comment 32
2018-02-23 04:00:32 PST
Reopening, I'll submit a new patch.
Carlos Garcia Campos
Comment 33
2018-02-23 04:00:55 PST
Created
attachment 334515
[details]
Patch
Philippe Normand
Comment 34
2018-02-23 04:40:20 PST
Comment on
attachment 334515
[details]
Patch Thanks.
Carlos Garcia Campos
Comment 35
2018-02-23 04:53:16 PST
Committed
r228946
: <
https://trac.webkit.org/changeset/228946
>
Zan Dobersek
Comment 36
2018-02-24 03:33:12 PST
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
Comment 37
2018-02-25 03:05:39 PST
(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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug