[vlc-devel] VLC log system regression (was: Re: [PATCH] macosx: fix startup and shutdown procedure)

[David Fuhrmann]

Am 26.03.2013 um 20:01 schrieb Rémi Denis-Courmont <remi at remlab.net>:

> Le mardi 26 mars 2013 19:56:55, David Fuhrmann a écrit :
>> Am 26.03.2013 um 17:59 schrieb "Rémi Denis-Courmont" <remi at remlab.net>:
>>>>    return _o_sharedInstance;
>>>> 
>>>> diff --git a/modules/gui/macosx/intf.m b/modules/gui/macosx/intf.m
>>>> index c5fced6..398f8dc 100644
>>>> --- a/modules/gui/macosx/intf.m
>>>> +++ b/modules/gui/macosx/intf.m
>>>> @@ -280,7 +280,7 @@ static void Run(intf_thread_t *p_intf)
>>>> 
>>>>    [[VLCMain sharedInstance] setIntf: p_intf];
>>>> 
>>>>    /* subscribe to LibVLCCore's messages */
>>>> 
>>>> -    vlc_LogSet(p_intf->p_libvlc, MsgCallback, NULL);
>>>> +    //vlc_LogSet(p_intf->p_libvlc, MsgCallback, NULL);
>>> 
>>> This should be moved to wherever the message dialog or MacOS equivalent
>>> gets created, and then only after the callback data is initialized.
>> 
>> Hi Remi,
>> 
>> I also noticed this problem recently. You introduced this regression on
>> purpose?
> 
> Yes of course. I am an incarnation of the antichrist and I also have signed 
> shady deals with QuickTime and gstreamer to derail VLC on MacOS. But please do 
> not tell anyone about it.
> 
>> Why??
> 
> I am intrinsically evil and I enjoy it. To top it all, it is a small sub-sub-
> sub-bullet point within my plan A to take over the World.
> 
>> It is just inconsistent, annoying to developers and hardens
>> debugging.
> 
> Seriously, the log subscription interface that was in place previously 
> followed the same conventions. Once you called it, you had to potentially 
> handle logging callbacks up until you deregistered it. This is very much like 
> var_AddCallback and var_DelCallback by the way.

AFAIK the callback system is able to register multiple callbacks for one variable?
So it would be still the case that 2 different modules (strerr log and GUI log) get the same data?

> In other words, I think that was a latent bug in the MacOS UI code. I guess 
> the regression is not even caused by the change of interface, but by the move 
> of the early log messages.

No. These early log messages are not causing this, I just tried it.

The regression is quite obvious for me:
Either, the priv->log.cb is assigned to the GUI callback. Or, priv->log.cb is assigned to PrintColorMsg/PrintMsg (in the case you call vlc_LogSet(p_..., NULL, NULL)).
AFAIK console output is only done in these 2 functions. And they are not used outside of vlc_LogSet.
Therefore, simply only one of these 2 (3) callbacks is called.

As a side node: vlc_vaLog already calls Win32DebugOutputMessage on windows.
For unix, you could also just add a function call to Print(Color)Msg here, to always enable console output?


> 
>> In any case, IMO the current solution for the macosx interface is better,
>> as it logs everything. So if the user experiences some problems, he can
>> simply opens the log and copy the messages. No need for this strange "open
>> log window and restart playback".
> 
> It might be so, but then you need to be extra-careful with memory consumption 
> over time...

Of course. Therefore, we are only keeping the last x-hundred logs.

>> Sometimes, the user even may not be able
>> to reproduce the problem so easily, because it does not happen every time
>> or has something to do which interfaces or other persistent modules.
>> 
>> Please also note that deinitializing the log when the window closes also is
>> not implemented in qt interface as of now, as far as I tested.
> 
> That code seems to work fine for me:

I haven't checked the code. I just quickly tried it with ubuntu and found out that console logging stopped, when messages window was opened for the first time.
Perhaps, the destructor of this class is not called at the right time?