calling MainMenuAction() from an other plugin

  Hi,

Udo Richter wrote:

>>> if (ShowMessage && !Skins.IsOpen() && !cOsd::IsOpen()) {
>>>    ShowMessage = false;
>>>    cRemote::CallPlugin("myShowMessage");
>>>    }
>>
>> this looks racy to me. What if two different threads do this?
>> The OSD could open between the if () and ::CallPlugin
> 
> CallPlugin is always racy, as only one plugin call will be remembered.
> 
> The only safe way is to set up a signal and wait for MainMenuAction 
> being called. If it is not called within timeout, try again.
> 
> The IsOpen() calls should ideally be made from foreground thread, but 
> usually there's no way to get there. Testing from within MainMenuAction 
> is pointless, as OSD and menu get closed right before MainMenuAction.

I did something similar in my vdr-xine plugin. Things could be improved 
using the attached patch.

It allows to remember up to 16 calls to different plugins and the caller 
can detect, whether putting the call in the remote key fifo was successful.

Bye.
  

Reinhard Nissl wrote:
> Hi,
> 
> Udo Richter wrote:
> 
>>>> if (ShowMessage && !Skins.IsOpen() && !cOsd::IsOpen()) {
>>>>    ShowMessage = false;
>>>>    cRemote::CallPlugin("myShowMessage");
>>>>    }
>>>
>>>
>>> this looks racy to me. What if two different threads do this?
>>> The OSD could open between the if () and ::CallPlugin
>>
>>
>> CallPlugin is always racy, as only one plugin call will be remembered.
>>
>> The only safe way is to set up a signal and wait for MainMenuAction 
>> being called. If it is not called within timeout, try again.
>>
>> The IsOpen() calls should ideally be made from foreground thread, but 
>> usually there's no way to get there. Testing from within 
>> MainMenuAction is pointless, as OSD and menu get closed right before 
>> MainMenuAction.
> 
> 
> I did something similar in my vdr-xine plugin. Things could be improved 
> using the attached patch.
> 
> It allows to remember up to 16 calls to different plugins and the caller 
> can detect, whether putting the call in the remote key fifo was successful.

Just so I get this right: with your patch it will be possible
to put several plugin calls into the key queue. Let's assume that
there are 3 plugin calls currently in the queue. When VDR goes through
the next main loop, it will encounter a k_Plugin in the queue and call
the first plugin's MainMenuAction(). Since there is more input in the
queue, that plugin's OSD won't be displayed, yet (that's only
done in cInterface::GetKey() in case the queue is empty). So it takes
the next k_Plugin out of the queue, deletes the menu it just created
and call the second plugin's MainMenuAction(). Before displaying that one
it will get the third k_Plugin out of the queue, call that plugin's
MainMenuAction(), and that's what will be finally displayed.

Unless I'm missing something here, queueing (up to 16) k_Plugins
doesn't have any different result (from the user's point of view)
than just overwriting any previously stored plugin call. The only thing
that appears to be important is the (currently missing) mutex lock
in cRemote::CallPlugin().

Am I missing something here?

Klaus
  

Hi,

Klaus Schmidinger wrote:

>>>>> if (ShowMessage && !Skins.IsOpen() && !cOsd::IsOpen()) {
>>>>>    ShowMessage = false;
>>>>>    cRemote::CallPlugin("myShowMessage");
>>>>>    }
>>>>
>>>>
>>>> this looks racy to me. What if two different threads do this?
>>>> The OSD could open between the if () and ::CallPlugin
>>>
>>>
>>> CallPlugin is always racy, as only one plugin call will be remembered.
>>>
>>> The only safe way is to set up a signal and wait for MainMenuAction 
>>> being called. If it is not called within timeout, try again.
>>>
>>> The IsOpen() calls should ideally be made from foreground thread, but 
>>> usually there's no way to get there. Testing from within 
>>> MainMenuAction is pointless, as OSD and menu get closed right before 
>>> MainMenuAction.
>>
>>
>> I did something similar in my vdr-xine plugin. Things could be 
>> improved using the attached patch.
>>
>> It allows to remember up to 16 calls to different plugins and the 
>> caller can detect, whether putting the call in the remote key fifo was 
>> successful.
> 
> Just so I get this right: with your patch it will be possible
> to put several plugin calls into the key queue. Let's assume that
> there are 3 plugin calls currently in the queue. When VDR goes through
> the next main loop, it will encounter a k_Plugin in the queue and call
> the first plugin's MainMenuAction(). Since there is more input in the
> queue, that plugin's OSD won't be displayed, yet (that's only
> done in cInterface::GetKey() in case the queue is empty). So it takes
> the next k_Plugin out of the queue, deletes the menu it just created
> and call the second plugin's MainMenuAction(). Before displaying that one
> it will get the third k_Plugin out of the queue, call that plugin's
> MainMenuAction(), and that's what will be finally displayed.
> 
> Unless I'm missing something here, queueing (up to 16) k_Plugins
> doesn't have any different result (from the user's point of view)
> than just overwriting any previously stored plugin call. The only thing
> that appears to be important is the (currently missing) mutex lock
> in cRemote::CallPlugin().
> 
> Am I missing something here?

In my case, I don't show an OSD. I just use this functionality as a 
trampoline to have the VDR main thread execute my code for switching the 
primary device, as it doesn't work reliably when it is done in any other 
thread.

So you are right, when a plugin opens an OSD (which is the typical 
case), one will only see the OSD of the last plugin. On the other hand, 
it would be useful to know for the caller, that the MenuMenuAction of 
the specified plugin will be called when CallPlugin() returned true. 
Otherwise it would need more "intelligent" code at the caller to achieve 
the call under race conditions.

In the case where the above is of no interest, there is no need to have 
an additional mutex lock in CallPlugin(), as Put() has one in remote.c:79.

Bye.
  

Klaus Schmidinger wrote:
> Am I missing something here?

This is mostly about plugins that expect their MainMenuAction to be 
called. A plugin may catch this call and display a message instead of 
the default menu. If the call is canceled, the message will unexpectedly 
appear the next time the user selects the main menu item.
(As I said before, I would suggest signaling and timeouts anyway)

Things might get useful with another patch I wouldn't dare to suggest 
before 1.4 ;)

This is current code:

    DELETE_MENU;
    if (cControl::Control())
       cControl::Control()->Hide();
    cPlugin *plugin = cPluginManager::GetPlugin(cRemote::GetPlugin());
    if (plugin) {
       Menu = plugin->MainMenuAction();
       if (Menu)
          Menu->Show();
       }

This is my idea:

    cPlugin *plugin = cPluginManager::GetPlugin(cRemote::GetPlugin());
    if (plugin) {
       cMenu *NewMenu = plugin->MainMenuAction();
       if (NewMenu) {
          DELETE_MENU;
          if (cControl::Control())
             cControl::Control()->Hide();
          Menu = NewMenu;
          Menu->Show();
          }
       }

That way, the old OSD/menu will be destroyed only if MainMenuAction 
returned a new menu. If MainMenuAction returns NULL, nothing should 
happen. This would be a nice dirty trick to catch the main thread from a 
plugin.
I don't know whether that change has bad side effects though.

Cheers,

Udo