[PATCH 0/1] MR5407: winegstreamer: Set GST_DEBUG if not set, based on WINEDEBUG channels.
From: Rémi Bernon rbernon@codeweavers.com
--- dlls/winegstreamer/main.c | 10 +++++++++- dlls/winegstreamer/unixlib.c | 9 +++++++++ dlls/winegstreamer/unixlib.h | 7 +++++++ 3 files changed, 25 insertions(+), 1 deletion(-)
diff --git a/dlls/winegstreamer/main.c b/dlls/winegstreamer/main.c index 74a6eee5b1d..d4f5db04a15 100644 --- a/dlls/winegstreamer/main.c +++ b/dlls/winegstreamer/main.c @@ -33,6 +33,8 @@ #include "wmcodecdsp.h"
WINE_DEFAULT_DEBUG_CHANNEL(quartz); +WINE_DECLARE_DEBUG_CHANNEL(mfplat); +WINE_DECLARE_DEBUG_CHANNEL(wmvcore);
DEFINE_GUID(GUID_NULL, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0); DEFINE_GUID(MEDIASUBTYPE_VC1S,MAKEFOURCC('V','C','1','S'),0x0000,0x0010,0x80,0x00,0x00,0xaa,0x00,0x38,0x9b,0x71); @@ -810,9 +812,15 @@ HRESULT WINAPI DllGetClassObject(REFCLSID clsid, REFIID iid, void **out)
static BOOL CALLBACK init_gstreamer_proc(INIT_ONCE *once, void *param, void **ctx) { + struct wg_init_gstreamer_params params = + { + .trace_on = TRACE_ON(mfplat) || TRACE_ON(quartz) || TRACE_ON(wmvcore), + .warn_on = WARN_ON(mfplat) || WARN_ON(quartz) || WARN_ON(wmvcore), + .err_on = ERR_ON(mfplat) || ERR_ON(quartz) || ERR_ON(wmvcore), + }; HINSTANCE handle;
- if (WINE_UNIX_CALL(unix_wg_init_gstreamer, NULL)) + if (WINE_UNIX_CALL(unix_wg_init_gstreamer, ¶ms)) return FALSE;
/* Unloading glib is a bad idea.. it installs atexit handlers, diff --git a/dlls/winegstreamer/unixlib.c b/dlls/winegstreamer/unixlib.c index a2ec66e1990..175ab92ecdc 100644 --- a/dlls/winegstreamer/unixlib.c +++ b/dlls/winegstreamer/unixlib.c @@ -246,6 +246,7 @@ bool push_event(GstPad *pad, GstEvent *event)
NTSTATUS wg_init_gstreamer(void *arg) { + struct wg_init_gstreamer_params *params = arg; char arg0[] = "wine"; char arg1[] = "--gst-disable-registry-fork"; char *args[] = {arg0, arg1, NULL}; @@ -253,6 +254,14 @@ NTSTATUS wg_init_gstreamer(void *arg) char **argv = args; GError *err;
+ if (params->trace_on) + setenv("GST_DEBUG", "WINE:9,4", FALSE); + if (params->warn_on) + setenv("GST_DEBUG", "3", FALSE); + if (params->err_on) + setenv("GST_DEBUG", "1", FALSE); + setenv("GST_DEBUG_NO_COLOR", "1", FALSE); + /* GStreamer installs a temporary SEGV handler when it loads plugins * to initialize its registry calling exit(-1) when any fault is caught. * We need to make sure any signal reaches our signal handlers to catch diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h index 4ec9fce515e..5b1b6ffa7ba 100644 --- a/dlls/winegstreamer/unixlib.h +++ b/dlls/winegstreamer/unixlib.h @@ -218,6 +218,13 @@ typedef UINT64 wg_parser_stream_t; typedef UINT64 wg_transform_t; typedef UINT64 wg_muxer_t;
+struct wg_init_gstreamer_params +{ + UINT8 trace_on; + UINT8 warn_on; + UINT8 err_on; +}; + struct wg_parser_create_params { wg_parser_t parser;
Elizabeth Figura (@zfigura) commented about dlls/winegstreamer/main.c:
static BOOL CALLBACK init_gstreamer_proc(INIT_ONCE *once, void *param, void **ctx) {
- struct wg_init_gstreamer_params params =
- {
.trace_on = TRACE_ON(mfplat) || TRACE_ON(quartz) || TRACE_ON(wmvcore),.warn_on = WARN_ON(mfplat) || WARN_ON(quartz) || WARN_ON(wmvcore),.err_on = ERR_ON(mfplat) || ERR_ON(quartz) || ERR_ON(wmvcore),- };
I'm not opposed to this but I'm not sure either. Maybe a separate channel would be better?
Elizabeth Figura (@zfigura) commented about dlls/winegstreamer/unixlib.c:
char **argv = args; GError *err;
- if (params->trace_on)
setenv("GST_DEBUG", "WINE:9,4", FALSE);- if (params->warn_on)
setenv("GST_DEBUG", "3", FALSE);- if (params->err_on)
setenv("GST_DEBUG", "1", FALSE);- setenv("GST_DEBUG_NO_COLOR", "1", FALSE);
We shouldn't override variables set by the user.
Also I'm pretty sure that e.g. TRACE_ON implies ERR_ON, so this will always result in GST_DEBUG=1.
On Tue Mar 26 17:35:27 2024 +0000, Elizabeth Figura wrote:
I'm not opposed to this but I'm not sure either. Maybe a separate channel would be better?
If only GStreamer logs is desired, GST_DEBUG can be used directly? I think using a fourth channel defeats the purpose of this MR, which is to make sure that setting WINEDEBUG is enough to get useful traces for debugging.
On Tue Mar 26 17:35:27 2024 +0000, Elizabeth Figura wrote:
We shouldn't override variables set by the user. Also I'm pretty sure that e.g. TRACE_ON implies ERR_ON, so this will always result in GST_DEBUG=1.
Last setenv parameter is "overwrite", controlling whether a previously set env variable is overwritten.
On Tue Mar 26 17:48:35 2024 +0000, Rémi Bernon wrote:
Last setenv parameter is "overwrite", controlling whether a previously set env variable is overwritten.
Oh indeed, I wasn't aware of that.
On Tue Mar 26 17:48:34 2024 +0000, Rémi Bernon wrote:
If only GStreamer logs is desired, GST_DEBUG can be used directly? I think using a fourth channel defeats the purpose of this MR, which is to make sure that setting WINEDEBUG is enough to get useful traces for debugging.
My concern is more about if only DirectShow logs are desired. That's not uncommon in my experience, if I'm debugging some other part of the pipeline. To be sure we have to apply tradeoffs somewhere, but GStreamer logs tend to be *really* verbose.
On Tue Mar 26 17:57:55 2024 +0000, Elizabeth Figura wrote:
My concern is more about if only DirectShow logs are desired. That's not uncommon in my experience, if I'm debugging some other part of the pipeline. To be sure we have to apply tradeoffs somewhere, but GStreamer logs tend to be *really* verbose.
We can just emit `GST_INFO("GStreamer logging implicitly enabled by Wine debug channel %s; set env GST_DEBUG to override.", channel);` and let the user silence[^1] or voice GStreamer independently.
[^1]: Does GST_DEBUG=0 work?
On Tue Mar 26 23:39:14 2024 +0000, Jinoh Kang wrote:
We can just emit `GST_INFO("GStreamer logging implicitly enabled by Wine debug channel %s; set env GST_DEBUG to override.", channel);` and let the user silence[^1] or voice GStreamer independently. [^1]: Does GST_DEBUG=0 work?
I don't see the point. Yes traces are verbose, and adding a couple of traces from GStreamer won't make it much worse. Not including GStreamer traces when you are using +quartz/+mfplat/+wmvcore make the logs much less useful in practice.
The default setting here with +quartz/+mfplat/+wmvcore is to use 9 for WINE, in order for every trace in winegstreamer to show up, there's not so many of them and they are always useful to follow the execution, and 4/INFO for the rest which isn't excessively verbose. I'm fine using 3/WARNING for the rest if that really matters but IME extra info messages is useful too to get more context.
If you are truly uninterested by the GStreamer trace, yes you can just use GST_DEBUG=0. Also none of this is meant for users, it's meant for developers and I don't think it deserves an info message.
On Wed Mar 27 08:26:26 2024 +0000, Rémi Bernon wrote:
I don't see the point. Yes traces are verbose, and adding a couple of traces from GStreamer won't make it much worse. Not including GStreamer traces when you are using +quartz/+mfplat/+wmvcore make the logs much less useful in practice. The default setting here with +quartz/+mfplat/+wmvcore is to use 9 for WINE, in order for every trace in winegstreamer to show up, there's not so many of them and they are always useful to follow the execution, and 4/INFO for the rest which isn't excessively verbose. I'm fine using 3/WARNING for the rest if that really matters but IME extra info messages is useful too to get more context. If you are truly uninterested by the GStreamer trace, yes you can just use GST_DEBUG=0. Also none of this is meant for users, it's meant for developers and I don't think it deserves an info message.
Fwiw were it using Wine debugging facility as it probably should (but can't because GStreamer creates its own threads), it would be using TRACE everywhere and we wouldn't even have to argue about this.
it's meant for developers and I don't think it deserves an info message.
I think *developers* deserve an info message. As an outsider (or a newcomer), I don't want to be surprised by sudden change of GST_DEBUG default when I want to debug one of those channels.
Info messages immediately answer that question, without requiring digging through the code (which only a few wine developers are familiar with).
On Wed Mar 27 08:41:11 2024 +0000, Jinoh Kang wrote:
it's meant for developers and I don't think it deserves an info message.
I think *developers* deserve an info message. As an outsider (or a newcomer), I don't want to be surprised by sudden change of GST_DEBUG default when I want to debug one of those channels. Info messages immediately answer that question, without requiring digging through the code (which only a few wine developers are familiar with).
Anyways I'm not strongly arguing for the info message. I'm merely suggesting a compromise.
(As you might have noticed, I believe *in general* that we should *not* make Wine[^1] even harder to hack around with for someone who is not a CW employee or a long-time contributor. Not everyone agrees with this; you can say "if you seriously want to hack with Wine you should already read the entire codebase" and that wouldn't be entirely wrong. I'm just not sure if we (as a collective, not this MR in partiular) *really* want Wine project to be more open and friendly to new contributors, or not.)
[^1]: Or more broadly, any complex open source software.
On Wed Mar 27 08:58:20 2024 +0000, Jinoh Kang wrote:
Anyways I'm not strongly arguing for the info message. I'm merely suggesting a compromise. (As you might have noticed, I believe *in general* that we should *not* make Wine[^1] even harder to hack around with for someone who is not a CW employee or a long-time contributor. Not everyone agrees with this; you can say "if you seriously want to hack with Wine you should already read the entire codebase" and that wouldn't be entirely wrong. I'm just not sure if we (as a collective, not this MR in partiular) *really* want Wine project to be more open and friendly to new contributors, or not.) [^1]: Or more broadly, any complex open source software.
How is this making anything harder to hack around? WINEDEBUG is the only thing you need to know about when hacking Wine, this is making sure that it truly is, for a module that currently doesn't follow the usual Wine traces.
the only thing you need to know about
FWIW you also need to know about this implicit behavior if you expected +mfplat and GStreamer logging to be configured independently but the default behavior isn't.
Well, I guess it isn't hard to figure out if you can grep `GST_DEBUG` in the wine codebase. I assume `GST_DEBUG` is and will ever be the only envvar responsible for controlling the source of "GStreamer-LEVEL: [...]" messages.[^1]
In that case, I don't necessarily see a need for a separate channel? *Maybe* if Wine had hierarchical logger configuration, but that doesn't exist (yet). Wine debug channels have always been ad-hoc; I don't think there is a hard rule for strict separation.
[^1]: which you can disable with `GST_DEBUG=0`.
On Wed Mar 27 10:03:35 2024 +0000, Jinoh Kang wrote:
the only thing you need to know about
FWIW you also need to know about this implicit behavior if you expected +mfplat and GStreamer logging to be configured independently but the default behavior isn't. Well, I guess it isn't hard to figure out if you can grep `GST_DEBUG` in the wine codebase. I assume `GST_DEBUG` is and will ever be the only envvar responsible for controlling the source of "GStreamer-LEVEL: [...]" messages.[^1] In that case, I don't necessarily see a need for a separate channel? *Maybe* if Wine had hierarchical logger configuration, but that doesn't exist (yet). Wine debug channels have always been ad-hoc; I don't think there is a hard rule for strict separation. [^1]: which you can disable with `GST_DEBUG=0`.
Yes, GST_DEBUG=0 works.
After thinking about it I think I'm fine with GST_DEBUG=4,WINE:9 for a default. FWIW, it was just unsureness on my part, not trying to say it was a definitely better option.
Fwiw were it using Wine debugging facility as it probably should (but can't because GStreamer creates its own threads), it would be using TRACE everywhere and we wouldn't even have to argue about this.
Eh, well, it's less about the Wine traces than the GStreamer traces. GST_DEBUG=WINE:9 seems uncontroversial; GST_DEBUG=4,WINE:9 is maybe a little less uncontroversial, but I think it's probably fine nonetheless.
This merge request was approved by Elizabeth Figura.
-
Elizabeth Figura (＠zfigura) -
Jinoh Kang (＠iamahuman)
-
Rémi Bernon
-
Rémi Bernon (＠rbernon)