Message ID | 200506301159.20072.wolfgang@rohdewald.de |
---|---|
State | New |
Headers |
Received: from natsmtp00.rzone.de ([81.169.145.165]) by www.linuxtv.org with esmtp (Exim 4.34) id 1DnvpQ-0005u9-PW for vdr@linuxtv.org; Thu, 30 Jun 2005 11:59:28 +0200 Received: from wr.rohdewald.de (p548F8FAC.dip0.t-ipconnect.de [84.143.143.172]) (authenticated bits=0) by post.webmailer.de (8.13.1/8.13.1) with ESMTP id j5U9xRGx001117 for <vdr@linuxtv.org>; Thu, 30 Jun 2005 11:59:27 +0200 (MEST) Received: by wr.rohdewald.de (Postfix, from userid 107) id 1686A270022; Thu, 30 Jun 2005 11:59:27 +0200 (CEST) Received: from localhost (localhost [127.0.0.1]) by wr.rohdewald.de (Postfix) with ESMTP id ECC9127001E for <vdr@linuxtv.org>; Thu, 30 Jun 2005 11:59:20 +0200 (CEST) From: Wolfgang Rohdewald <wolfgang@rohdewald.de> To: vdr@linuxtv.org Subject: Re: [vdr] [PATCH] fix segfault in cSkins::Message Date: Thu, 30 Jun 2005 11:59:19 +0200 User-Agent: KMail/1.7.2 References: <200506301031.10756.wolfgang@rohdewald.de> In-Reply-To: <200506301031.10756.wolfgang@rohdewald.de> MIME-Version: 1.0 Message-Id: <200506301159.20072.wolfgang@rohdewald.de> Content-Type: Multipart/Mixed; boundary="Boundary-00=_4J8wCsCnMRvmkDN" X-Spam-Checker-Version: SpamAssassin 3.0.4 (2005-06-05) on wr.rohdewald.de X-Spam-Level: X-Spam-Status: No, score=-2.8 required=5.0 tests=ALL_TRUSTED,AWL autolearn=ham version=3.0.4 X-BeenThere: vdr@linuxtv.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: wolfgang@rohdewald.de, Klaus Schmidinger's VDR <vdr@linuxtv.org> List-Id: Klaus Schmidinger's VDR <vdr.linuxtv.org> List-Unsubscribe: <http://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr>, <mailto:vdr-request@linuxtv.org?subject=unsubscribe> List-Archive: <http://www.linuxtv.org/pipermail/vdr> List-Post: <mailto:vdr@linuxtv.org> List-Help: <mailto:vdr-request@linuxtv.org?subject=help> List-Subscribe: <http://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr>, <mailto:vdr-request@linuxtv.org?subject=subscribe> X-List-Received-Date: Thu, 30 Jun 2005 09:59:28 -0000 Status: O X-Status: X-Keywords: X-UID: 3314 |
Commit Message
Wolfgang Rohdewald
June 30, 2005, 9:59 a.m. UTC
On Donnerstag 30 Juni 2005 10:31, Wolfgang Rohdewald wrote: > this happens if the MENUTIMEOUT makes the OSD menu > disappear while a message is displayed. more is needed - all delete osd should also nullify it. See gdb output below. also, I can only get rid of the segfault shown by valgrind below if constructors with osd as private member nullify osd right at the beginning, before calling NewOsd(). (I did not check which constructor causes my segfault, just applied it to all of them). I suppose other skins might have the same problem. This looks to me as if access to osd might not be threadsafe, but I am by no means a thread expert Extended patch attached. While being at it, I replaced a few more delete x by DELETENULL(x) Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 16384 (LWP 8769)] 0x080fec70 in cSkinClassicDisplayMessage::Flush (this=0x9f38ec0) at skinclassic.c:644 644 osd->Flush(); Current language: auto; currently c++ (gdb) p osd $1 = (class cOsd *) 0x10 (gdb) bt #0 0x080fec70 in cSkinClassicDisplayMessage::Flush (this=0x9f38ec0) at skinclassic.c:644 #1 0x081015dc in cSkins::Flush (this=0x81ef5a4) at skins.c:210 #2 0x080b95ab in cInterface::GetKey (this=0x9e5b6b8, Wait=true) at interface.c:35 #3 0x081223c5 in main (argc=14, argv=0xbf85ffd4) at vdr.c:639 line 644 is: if (osd) osd->Flush(); ==20735== Conditional jump or move depends on uninitialised value(s) ==20735== at 0x80FED8D: cSkinClassicDisplayMessage::Flush() (skinclassic.c:644) ==20735== by 0x81016FF: cSkins::Flush() (skins.c:210) ==20735== by 0x80B9612: cInterface::GetKey(bool) (interface.c:35) ==20735== by 0x81224E8: main (vdr.c:639) ==20735== ==20735== Use of uninitialised value of size 4 ==20735== at 0x80FED95: cSkinClassicDisplayMessage::Flush() (skinclassic.c:645) ==20735== by 0x81016FF: cSkins::Flush() (skins.c:210) ==20735== by 0x80B9612: cInterface::GetKey(bool) (interface.c:35) ==20735== by 0x81224E8: main (vdr.c:639) ==20735== ==20735== Invalid read of size 4 ==20735== at 0x80FEDA3: cSkinClassicDisplayMessage::Flush() (skinclassic.c:645) ==20735== by 0x81016FF: cSkins::Flush() (skins.c:210) ==20735== by 0x80B9612: cInterface::GetKey(bool) (interface.c:35) ==20735== by 0x81224E8: main (vdr.c:639) ==20735== Address 0x6564698A is not stack'd, malloc'd or (recently) free'd ==20735== ==20735== Process terminating with default action of signal 11 (SIGSEGV) ==20735== GPF (Pointer out of bounds?) ==20735== at 0x80FEDA3: cSkinClassicDisplayMessage::Flush() (skinclassic.c:645) ==20735== by 0x81016FF: cSkins::Flush() (skins.c:210) ==20735== by 0x80B9612: cInterface::GetKey(bool) (interface.c:35) ==20735== by 0x81224E8: main (vdr.c:639) ==20735==
Comments
Wolfgang Rohdewald wrote: > On Donnerstag 30 Juni 2005 10:31, Wolfgang Rohdewald wrote: > >>this happens if the MENUTIMEOUT makes the OSD menu >>disappear while a message is displayed. > > > more is needed - all delete osd should also nullify it. See > gdb output below. > > also, I can only get rid of the segfault shown by valgrind below > if constructors with osd as private member nullify osd right > at the beginning, before calling NewOsd(). (I did not check which > constructor causes my segfault, just applied it to all of them). > > I suppose other skins might have the same problem. > > This looks to me as if access to osd might not be threadsafe, > but I am by no means a thread expert IIRC we already had this discussion some time ago. The point is that you're not supposed to call any of the skin functions from a thread. These functions are only supposed to be called by VDR itself. Klaus
On 30 Jun 2005 Klaus Schmidinger <Klaus.Schmidinger@cadsoft.de> wrote: > The point is that you're not supposed to call any of the skin > functions from a thread. These functions are only supposed to be called > by VDR itself. I assume that this means "to be called from the foreground thread only". No matter if it's VDR itself or a plugin. Or am I wrong? Regards.
Stefan Huelswitt wrote: > On 30 Jun 2005 Klaus Schmidinger <Klaus.Schmidinger@cadsoft.de> wrote: > > >>The point is that you're not supposed to call any of the skin >>functions from a thread. These functions are only supposed to be called >>by VDR itself. > > > I assume that this means "to be called from the foreground thread > only". No matter if it's VDR itself or a plugin. Yes, that´s what I meant. Klaus
On Donnerstag 30 Juni 2005 16:53, Klaus Schmidinger wrote: > IIRC we already had this discussion some time ago. > The point is that you're not supposed to call any of the skin > functions from a thread. These functions are only supposed to be called > by VDR itself. how else can a plugin display a message or ask a question? since cInterface::Confirm() also calls Skins.Message(), I suppose Confirm() is also illegal for plugins? this is how muggle has always been doing this: #if VDRVERSNUM >= 10307 Skins.Message (mtInfo, buffer,duration); Skins.Flush (); #else Interface->Status (buffer); Interface->Flush (); #endif and if (!Interface->Confirm(tr("Import items?"))) return false;
Wolfgang Rohdewald wrote: > On Donnerstag 30 Juni 2005 16:53, Klaus Schmidinger wrote: > >>IIRC we already had this discussion some time ago. >>The point is that you're not supposed to call any of the skin >>functions from a thread. These functions are only supposed to be called >>by VDR itself. > > > how else can a plugin display a message or ask a question? since > cInterface::Confirm() also calls Skins.Message(), I suppose Confirm() > is also illegal for plugins? > > this is how muggle has always been doing this: > > #if VDRVERSNUM >= 10307 > Skins.Message (mtInfo, buffer,duration); > Skins.Flush (); > #else > Interface->Status (buffer); > Interface->Flush (); > #endif > > and > > if (!Interface->Confirm(tr("Import items?"))) > return false; Sorry, I was a little too vague here. What I meant to say was that these functions shall only be called from the _foreground_ thread (the one that VDR's main loop runs in). Klaus
On Donnerstag 30 Juni 2005 17:37, Klaus Schmidinger wrote: > Stefan Huelswitt wrote: > > On 30 Jun 2005 Klaus Schmidinger <Klaus.Schmidinger@cadsoft.de> wrote: > > > > > >>The point is that you're not supposed to call any of the skin > >>functions from a thread. These functions are only supposed to be called > >>by VDR itself. > > > > > > I assume that this means "to be called from the foreground thread > > only". No matter if it's VDR itself or a plugin. > > Yes, that´s what I meant. in the case of muggle: There is a menu entry in the muggle setup which starts a thread importing tracks. This thread shows status messages like "Imported 1000 item". When the OSD times out while such a status message is displayed, segfault. So will that importing thread have to pass the message to vdr through port 2001 or is there an easier way? from cInterface::GetKey(): if (!Skins.IsOpen()) { char *message = SVDRP->GetMessage(); if (message) { Skins.Message(mtInfo, message); free(message); }
On Donnerstag, 30. Juni 2005 17:43, Klaus Schmidinger wrote: > Wolfgang Rohdewald wrote: > > On Donnerstag 30 Juni 2005 16:53, Klaus Schmidinger wrote: > > > >>IIRC we already had this discussion some time ago. > >>The point is that you're not supposed to call any of the skin > >>functions from a thread. These functions are only supposed to be called > >>by VDR itself. > > > > > > how else can a plugin display a message or ask a question? since > > cInterface::Confirm() also calls Skins.Message(), I suppose Confirm() > > is also illegal for plugins? > > > > this is how muggle has always been doing this: > > > > #if VDRVERSNUM >= 10307 > > Skins.Message (mtInfo, buffer,duration); > > Skins.Flush (); > > #else > > Interface->Status (buffer); > > Interface->Flush (); > > #endif > > > > and > > > > if (!Interface->Confirm(tr("Import items?"))) > > return false; > > Sorry, I was a little too vague here. > What I meant to say was that these functions shall only be called > from the _foreground_ thread (the one that VDR's main loop runs in). Is there another way for a plugin to display a message during background processing ? Preferably a _non_ blocking way,as I noticed that Skins.Message() does it's job blocking.
Stefan Lucke wrote: > On Donnerstag, 30. Juni 2005 17:43, Klaus Schmidinger wrote: > >>Wolfgang Rohdewald wrote: >> >>>On Donnerstag 30 Juni 2005 16:53, Klaus Schmidinger wrote: >>> >>> >>>>IIRC we already had this discussion some time ago. >>>>The point is that you're not supposed to call any of the skin >>>>functions from a thread. These functions are only supposed to be called >>>>by VDR itself. >>> >>> >>>how else can a plugin display a message or ask a question? since >>>cInterface::Confirm() also calls Skins.Message(), I suppose Confirm() >>>is also illegal for plugins? >>> >>>this is how muggle has always been doing this: >>> >>>#if VDRVERSNUM >= 10307 >>> Skins.Message (mtInfo, buffer,duration); >>> Skins.Flush (); >>>#else >>> Interface->Status (buffer); >>> Interface->Flush (); >>>#endif >>> >>>and >>> >>> if (!Interface->Confirm(tr("Import items?"))) >>> return false; >> >>Sorry, I was a little too vague here. >>What I meant to say was that these functions shall only be called >>from the _foreground_ thread (the one that VDR's main loop runs in). > > > Is there another way for a plugin to display a message during > background processing ? > Preferably a _non_ blocking way,as I noticed that Skins.Message() > does it's job blocking. Is there a solution for this problem? I now also need to notify the user of a background event, and the Housekeeping method (a possible candidate since it's called from the foreground thread) isn't suitable, since it's called only once every 60 seconds. Bye
On Dienstag 26 Juli 2005 14:29, Luca Olivetti wrote: > Stefan Lucke wrote: > > Is there another way for a plugin to display a message during > > background processing ? > > Preferably a _non_ blocking way,as I noticed that Skins.Message() > > does it's job blocking. > > Is there a solution for this problem? > I now also need to notify the user of a background event, and the > Housekeeping method (a possible candidate since it's called from the > foreground thread) isn't suitable, since it's called only once every 60 > seconds. as far as I can see the only other method is to connect to port 2001. For now we disabled those messages in muggle.
On Dienstag, 26. Juli 2005 18:52, Wolfgang Rohdewald wrote: > On Dienstag 26 Juli 2005 14:29, Luca Olivetti wrote: > > Stefan Lucke wrote: > > > Is there another way for a plugin to display a message during > > > background processing ? > > > Preferably a _non_ blocking way,as I noticed that Skins.Message() > > > does it's job blocking. > > > > Is there a solution for this problem? > > I now also need to notify the user of a background event, and the > > Housekeeping method (a possible candidate since it's called from the > > foreground thread) isn't suitable, since it's called only once every 60 > > seconds. > > as far as I can see the only other method is to connect to port 2001. The SVDRP port is not hardcoded, it can be specified via "-p number". So the next question would be, how can a plugin read the port our main program is listening on ? So plugins need the -p option too. Isn't it more suitable to make Skins.Message() usable for plugins ? Klaus ? > > For now we disabled those messages in muggle. >
Stefan Lucke wrote: > On Dienstag, 26. Juli 2005 18:52, Wolfgang Rohdewald wrote: > >>On Dienstag 26 Juli 2005 14:29, Luca Olivetti wrote: >> >>>Stefan Lucke wrote: >>> >>>>Is there another way for a plugin to display a message during >>>>background processing ? >>>>Preferably a _non_ blocking way,as I noticed that Skins.Message() >>>>does it's job blocking. >>> >>>Is there a solution for this problem? >>>I now also need to notify the user of a background event, and the >>>Housekeeping method (a possible candidate since it's called from the >>>foreground thread) isn't suitable, since it's called only once every 60 >>>seconds. >> >>as far as I can see the only other method is to connect to port 2001. > > > The SVDRP port is not hardcoded, it can be specified via "-p number". > So the next question would be, how can a plugin read the port > our main program is listening on ? So plugins need the -p option too. > > Isn't it more suitable to make Skins.Message() usable for plugins ? > Klaus ? A plugin may well call Skins.Message(), but only from the _main_ thread. I'm thinking about a function like cOsdObject *cPlugin::Popup(void); which would be called in every turn of the main loop (i.e. from the foreground thread) whenever there is no OSD display open. A plugin could either return a cOsdObject that will be presented to the user, or could directly display a message (even with user interaction, if necessary) and retrun NULL. Of course, if a plugin's thread wants to issue a message, it will have to store the message inside the plugin and wait until asked through a call to its Popup() function. Would that be a useful solution? Klaus
On Sonntag, 31. Juli 2005 16:56, Klaus Schmidinger wrote: > Stefan Lucke wrote: > > On Dienstag, 26. Juli 2005 18:52, Wolfgang Rohdewald wrote: > > > >>On Dienstag 26 Juli 2005 14:29, Luca Olivetti wrote: > >> > >>>Stefan Lucke wrote: > >>> > >>>>Is there another way for a plugin to display a message during > >>>>background processing ? > >>>>Preferably a _non_ blocking way,as I noticed that Skins.Message() > >>>>does it's job blocking. > >>> > >>>Is there a solution for this problem? > >>>I now also need to notify the user of a background event, and the > >>>Housekeeping method (a possible candidate since it's called from the > >>>foreground thread) isn't suitable, since it's called only once every 60 > >>>seconds. > >> > >>as far as I can see the only other method is to connect to port 2001. > > > > > > The SVDRP port is not hardcoded, it can be specified via "-p number". > > So the next question would be, how can a plugin read the port > > our main program is listening on ? So plugins need the -p option too. > > > > Isn't it more suitable to make Skins.Message() usable for plugins ? > > Klaus ? > > A plugin may well call Skins.Message(), but only from the _main_ > thread. > > I'm thinking about a function like > > cOsdObject *cPlugin::Popup(void); > > which would be called in every turn of the main loop (i.e. from the > foreground thread) whenever there is no OSD display open. A plugin > could either return a cOsdObject that will be presented to the user, > or could directly display a message (even with user interaction, if > necessary) and retrun NULL. Of course, if a plugin's thread wants to > issue a message, it will have to store the message inside the plugin > and wait until asked through a call to its Popup() function. > > Would that be a useful solution? > In some way yes, as it is something like InteractiveHousekeeping(). But that would introduce message queueing code into each plugin, which wants to display a message. The condition "whenever there is no OSD display open" will cause an undetermined delay. "normal" OSDs have an area where such messages could be displayed. I would prefer something like Skins.QueueMessage() so that there is only one peace of code and each plugin can use it. For more complex OSDs (interactive ones) Popup() is more suitable: "You've new mail: read now (y (green) / n (red)) ?"
Klaus Schmidinger wrote: > A plugin may well call Skins.Message(), but only from the _main_ > thread. > > I'm thinking about a function like > > cOsdObject *cPlugin::Popup(void); > > which would be called in every turn of the main loop (i.e. from the > foreground thread) whenever there is no OSD display open. A plugin > could either return a cOsdObject that will be presented to the user, In this case the function wouldn't be called again, would it? Isn't it possible to use Skins.Message even with an osd already open? Maybe cOsdObject *cPlugin::Popup(bool canpopup) called regardless of an osd open (and canpopup set to false if the plugin cannot create a cOsdObject)? > or could directly display a message (even with user interaction, if > necessary) and retrun NULL. Of course, if a plugin's thread wants to > issue a message, it will have to store the message inside the plugin > and wait until asked through a call to its Popup() function. > > Would that be a useful solution? I think it would be useful. For example, I could use it in my actuator plugin to show the dish position instead of patching vdr as I do now, and as soon as the user presses a button I could close my cOsdObject and reinject the key to vdr. But it's not a complete solution, what if more than one plugin needs to show something on screen? I reckon there's no simple solution to that, but maybe the solution would be implementing something like the ttxsubs or subtitles patches do (something like more than one osd opened at once, with only the highest priority one actually shown), or some kind of windowing system...I really don't know. Bye
On Sonntag 31 Juli 2005 16:56, Klaus Schmidinger wrote: > Stefan Lucke wrote: > > Isn't it more suitable to make Skins.Message() usable for plugins ? > > Klaus ? > > A plugin may well call Skins.Message(), but only from the _main_ > thread. > > I'm thinking about a function like > > cOsdObject *cPlugin::Popup(void); > > which would be called in every turn of the main loop (i.e. from the > foreground thread) whenever there is no OSD display open. A plugin > could either return a cOsdObject that will be presented to the user, > or could directly display a message (even with user interaction, if > necessary) and retrun NULL. Of course, if a plugin's thread wants to > issue a message, it will have to store the message inside the plugin > and wait until asked through a call to its Popup() function. > > Would that be a useful solution? Sounds good to me. How long would the cOsdObject be displayed? Could this be definable by a parameter like in cSkins::Message()? Does vdr have a way to notice if a background thread accesses the OSD? If so, and if this happens thru cSkins::Message() or cInterface::Confirm(), could vdr automatically handle this? Or just issue a warning like "You should use ::Popup()"
Wolfgang Rohdewald wrote: > On Sonntag 31 Juli 2005 16:56, Klaus Schmidinger wrote: > >>Stefan Lucke wrote: >> >>>Isn't it more suitable to make Skins.Message() usable for plugins ? >>>Klaus ? >> >>A plugin may well call Skins.Message(), but only from the _main_ >>thread. >> >>I'm thinking about a function like >> >> cOsdObject *cPlugin::Popup(void); >> >>which would be called in every turn of the main loop (i.e. from the >>foreground thread) whenever there is no OSD display open. A plugin >>could either return a cOsdObject that will be presented to the user, >>or could directly display a message (even with user interaction, if >>necessary) and retrun NULL. Of course, if a plugin's thread wants to >>issue a message, it will have to store the message inside the plugin >>and wait until asked through a call to its Popup() function. >> >>Would that be a useful solution? > > > Sounds good to me. > > How long would the cOsdObject be displayed? Could this be definable > by a parameter like in cSkins::Message()? The cOsdObject would be displayed until its ProcessKey() function returns osEnd. Note that this would be like any other menu, and it would run in the _foreground_ thread. > Does vdr have a way to notice if a background thread accesses the > OSD? A background thread should never access the OSD directly. > If so, and if this happens thru cSkins::Message() or cInterface::Confirm(), > could vdr automatically handle this? Or just issue a warning > like "You should use ::Popup()" Again: if a plugin wants to interact with the user "out of the blue" (meaning: not through the plugins main menu entry), cPlugin::Popup() can either return a cOsdObject (which can be anything) that will be further handled by VDR's main loop and will end when its ProcessKey() function tells VDR to do so (or when the user hits kMenu - maybe there are even other reasons), or it can do something like directly call Skins::Message() and display a message (and return NULL). But all this will be done from the _foreground_ thread, so any information a plugin's _background_ thread wants to display needs to be stored in the plugin somehow, until the plugin is asked for it by a call to its Popup() function. I don't think that this will be much of a problem, because typically such messages would be progress messages, like "processed x out of y items", so the background thread could simply increment some counter and the foreground thread could display the message whenever the difference between the previous message and the current counter exceeds some limit. OMHO there is most certainly no need for a "message queue" for background thread messages. Klaus
Stefan Lucke wrote: > On Sonntag, 31. Juli 2005 16:56, Klaus Schmidinger wrote: > ... >>I'm thinking about a function like >> >> cOsdObject *cPlugin::Popup(void); >> >>which would be called in every turn of the main loop (i.e. from the >>foreground thread) whenever there is no OSD display open. A plugin >>could either return a cOsdObject that will be presented to the user, >>or could directly display a message (even with user interaction, if >>necessary) and retrun NULL. Of course, if a plugin's thread wants to >>issue a message, it will have to store the message inside the plugin >>and wait until asked through a call to its Popup() function. >> >>Would that be a useful solution? >> > > > In some way yes, as it is something like InteractiveHousekeeping(). > But that would introduce message queueing code into each plugin, which > wants to display a message. The condition "whenever there is no OSD > display open" will cause an undetermined delay. "normal" OSDs have > an area where such messages could be displayed. > I would prefer something like Skins.QueueMessage() so that there is > only one peace of code and each plugin can use it. > For more complex OSDs (interactive ones) Popup() is more suitable: > "You've new mail: read now (y (green) / n (red)) ?" Why would you want to queue such messages? If I understand this correctly, the original purpose of this was to display some sort of progress indication. So, if a plugins's background thread has something to say (like "processed x out of y items"), it can do so at any time. There's no need to queue all messages, like processed 10 out of 1000 items processed 20 out of 1000 items processed 30 out of 1000 items processed 40 out of 1000 items processed 50 out of 1000 items processed 60 out of 1000 items processed 70 out of 1000 items processed 80 out of 1000 items If VDR's main loop decides to ask the plugin whether it has something to say, it doesn't really matter whether this is processed 20 out of 1000 items or processed 70 out of 1000 items The key point here is that the background thread does _not_ direcly display a message. It rather keeps some indication of its current state, and whenever suitable, the foreground thread will be asked to display it. Klaus
Luca Olivetti wrote: > Klaus Schmidinger wrote: > >> A plugin may well call Skins.Message(), but only from the _main_ >> thread. >> >> I'm thinking about a function like >> >> cOsdObject *cPlugin::Popup(void); >> >> which would be called in every turn of the main loop (i.e. from the >> foreground thread) whenever there is no OSD display open. A plugin >> could either return a cOsdObject that will be presented to the user, > > > In this case the function wouldn't be called again, would it? The Popup() function would be called whenever there is nothing else on teh OSD. If it returns a cOsdObject, then that object's ProcessKey() function will be called to actually process the display, and while doing so the Plugin() function would, of course, not be called, since there is already an object on the OSD. > Isn't it possible to use Skins.Message even with an osd already open? > Maybe > > cOsdObject *cPlugin::Popup(bool canpopup) > > called regardless of an osd open (and canpopup set to false if the > plugin cannot create a cOsdObject)? No. Such messages from plugins come "out of the blue", so if there is currently a menu open, the user shouldn't be irritated by messages that don't have anything to do with this menu. >> or could directly display a message (even with user interaction, if >> necessary) and retrun NULL. Of course, if a plugin's thread wants to >> issue a message, it will have to store the message inside the plugin >> and wait until asked through a call to its Popup() function. >> >> Would that be a useful solution? > > > I think it would be useful. For example, I could use it in my actuator > plugin to show the dish position instead of patching vdr as I do now, > and as soon as the user presses a button I could close my cOsdObject and > reinject the key to vdr. > But it's not a complete solution, what if more than one plugin needs to > show something on screen? I reckon there's no simple solution to that, > but maybe the solution would be implementing something like the ttxsubs > or subtitles patches do (something like more than one osd opened at > once, with only the highest priority one actually shown), or some kind > of windowing system...I really don't know. Only one plugin's Popup() function will be called at a time, and if that plugin decides to display something, it will have the OSD for itself until it gives it up (or VDR closes it for other reasons). The subtitle plugins should be able to use this mechanism just as well. Klaus
On Sonntag 31 Juli 2005 18:29, Klaus Schmidinger wrote: > > Does vdr have a way to notice if a background thread accesses the > > OSD? > > A background thread should never access the OSD directly. I do understand that. I just think it might be helpful to not simply say it is forbidden but to enforce it instead of segfaulting. Considering the enormous amount of plugins there will always be some plugin developers trying to do forbidden things and if a user tells them "sometimes vdr restarts but I have no idea why" it is almost impossible to debug that. If you take my original two patches you could do void cSkinClassicDisplayMessage::Flush(void) { if (osd) osd->Flush(); else warn: background thread is not allowed to access osd } Otherwise I believe I understand what you propose and it seems to be a good solution to me.
Wolfgang Rohdewald wrote: > On Sonntag 31 Juli 2005 18:29, Klaus Schmidinger wrote: > >>>Does vdr have a way to notice if a background thread accesses the >>>OSD? >> >>A background thread should never access the OSD directly. > > > I do understand that. I just think it might be helpful to not > simply say it is forbidden but to enforce it instead of > segfaulting. > > Considering the enormous amount of plugins there will always be > some plugin developers trying to do forbidden things and if a > user tells them "sometimes vdr restarts but I have no idea why" > it is almost impossible to debug that. > > If you take my original two patches you could do > > void cSkinClassicDisplayMessage::Flush(void) > { > if (osd) > osd->Flush(); > else > warn: background thread is not allowed to access osd > } You don't _know_ that this is because of a background thread. Besides, I'd hate to have to check 'osd' all over the place (and every plugin that implements a skin would have to do the same). I guess I'll simply put a test into cSkinDisplay::cSkinDisplay() that aborts in case it gets called from a thread other than the foreground thread. > Otherwise I believe I understand what you propose and it seems > to be a good solution to me. Ok, so I'll implement it that way. Klaus
Klaus Schmidinger wrote: > Wolfgang Rohdewald wrote: > >> On Sonntag 31 Juli 2005 18:29, Klaus Schmidinger wrote: >> >>>> Does vdr have a way to notice if a background thread accesses the >>>> OSD? >>> >>> >>> A background thread should never access the OSD directly. >> >> >> >> I do understand that. I just think it might be helpful to not >> simply say it is forbidden but to enforce it instead of >> segfaulting. >> >> Considering the enormous amount of plugins there will always be >> some plugin developers trying to do forbidden things and if a >> user tells them "sometimes vdr restarts but I have no idea why" >> it is almost impossible to debug that. >> >> If you take my original two patches you could do >> >> void cSkinClassicDisplayMessage::Flush(void) >> { >> if (osd) >> osd->Flush(); >> else >> warn: background thread is not allowed to access osd >> } > > > You don't _know_ that this is because of a background thread. > Besides, I'd hate to have to check 'osd' all over the place > (and every plugin that implements a skin would have to do the same). > > I guess I'll simply put a test into cSkinDisplay::cSkinDisplay() > that aborts in case it gets called from a thread other than the > foreground thread. > >> Otherwise I believe I understand what you propose and it seems >> to be a good solution to me. > > > Ok, so I'll implement it that way. > > Klaus Well, after a weekend of consideration I decided to implement a function that can be used by background threads to queue a message for display (the thread can even wait for user input) in VDR 1.3.37. Since a plugin's MainMenuFunction can now (since VDR 1.3.32) be launched programmatically, a plugin that wants to do more than just display a message can always launch itself and interact with the user in the foreground. I've abandoned the idea of implementing something like cPlugin::Popup() because it would only make things very complicated. Since handling subtitles shall be made an integral part of the core VDR code in version 1.5.x I'll postpone all further OSD handling considerations until then. Klaus
En/na Klaus Schmidinger ha escrit: > > Well, after a weekend of consideration I decided to implement > a function that can be used by background threads to queue a > message for display (the thread can even wait for user input) > in VDR 1.3.37. Since a plugin's MainMenuFunction can > now (since VDR 1.3.32) be launched programmatically, a plugin that > wants to do more than just display a message can always launch > itself and interact with the user in the foreground. It isn't working really well for my use case (dish position): while the dish is moving I'm trying to call QueueMessage either with Seconds=0 and Seconds=-1. Timeout is -1 (so that vdr should display the message no matter what). Instead of a steady message with the position updating in real time I just see a flash with the (same) position for a couple of messages, then the message disappears to show a new one with a new position. > > I've abandoned the idea of implementing something like cPlugin::Popup() > because it would only make things very complicated. this one would have worked :-( Bye
En/na Luca Olivetti ha escrit: > It isn't working really well for my use case (dish position): while the > dish is moving I'm trying to call QueueMessage either with Seconds=0 and > Seconds=-1. Timeout is -1 (so that vdr should display the message no > matter what). Instead of a steady message with the position updating in > real time I just see a flash with the (same) position for a couple of > messages, then the message disappears to show a new one with a new position. that should read "for a couple of seconds". Bye
diff -up org27/dvbspu.c src/dvbspu.c --- org27/dvbspu.c 2005-05-07 13:13:48.000000000 +0200 +++ src/dvbspu.c 2005-06-30 11:12:53.000000000 +0200 @@ -235,9 +235,9 @@ cDvbSpuDecoder::cDvbSpuDecoder() cDvbSpuDecoder::~cDvbSpuDecoder() { - delete spubmp; - delete spu; - delete osd; + DELETENULL(spubmp); + DELETENULL(spu); + DELETENULL(osd); } void cDvbSpuDecoder::processSPU(uint32_t pts, uint8_t * buf, bool AllowedShow) @@ -246,8 +246,7 @@ void cDvbSpuDecoder::processSPU(uint32_t DEBUG("SPU pushData: pts: %d\n", pts); - delete spubmp; - spubmp = NULL; + DELETENULL(spubmp); delete[]spu; spu = buf; spupts = pts; @@ -390,16 +389,14 @@ void cDvbSpuDecoder::Draw(void) void cDvbSpuDecoder::Hide(void) { - delete osd; - osd = NULL; + DELETENULL(osd); } void cDvbSpuDecoder::Empty(void) { Hide(); - delete spubmp; - spubmp = NULL; + DELETENULL(spubmp); delete[]spu; spu = NULL; diff -up org27/skinclassic.c src/skinclassic.c --- org27/skinclassic.c 2005-05-16 12:45:07.000000000 +0200 +++ src/skinclassic.c 2005-06-30 11:30:55.000000000 +0200 @@ -88,6 +88,7 @@ public: cSkinClassicDisplayChannel::cSkinClassicDisplayChannel(bool WithInfo) { + osd = NULL; int Lines = WithInfo ? 5 : 1; const cFont *font = cFont::GetFont(fontOsd); lineHeight = font->Height(); @@ -101,7 +102,7 @@ cSkinClassicDisplayChannel::cSkinClassic cSkinClassicDisplayChannel::~cSkinClassicDisplayChannel() { - delete osd; + DELETENULL(osd); } void cSkinClassicDisplayChannel::SetChannel(const cChannel *Channel, int Number) @@ -174,6 +175,7 @@ public: cSkinClassicDisplayMenu::cSkinClassicDisplayMenu(void) { + osd = NULL; const cFont *font = cFont::GetFont(fontOsd); lineHeight = font->Height(); x0 = 0; @@ -200,7 +202,7 @@ cSkinClassicDisplayMenu::cSkinClassicDis cSkinClassicDisplayMenu::~cSkinClassicDisplayMenu() { - delete osd; + DELETENULL(osd); } void cSkinClassicDisplayMenu::SetScrollbar(void) @@ -394,6 +396,7 @@ public: cSkinClassicDisplayReplay::cSkinClassicDisplayReplay(bool ModeOnly) { + osd = NULL; const cFont *font = cFont::GetFont(fontOsd); int lineHeight = font->Height(); lastCurrentWidth = 0; @@ -412,7 +415,7 @@ cSkinClassicDisplayReplay::cSkinClassicD cSkinClassicDisplayReplay::~cSkinClassicDisplayReplay() { - delete osd; + DELETENULL(osd); } void cSkinClassicDisplayReplay::SetTitle(const char *Title) @@ -494,6 +497,7 @@ public: cSkinClassicDisplayVolume::cSkinClassicDisplayVolume(void) { + osd = NULL; const cFont *font = cFont::GetFont(fontOsd); int lineHeight = font->Height(); osd = cOsdProvider::NewOsd(Setup.OSDLeft, Setup.OSDTop + Setup.OSDHeight - lineHeight); @@ -503,7 +507,7 @@ cSkinClassicDisplayVolume::cSkinClassicD cSkinClassicDisplayVolume::~cSkinClassicDisplayVolume() { - delete osd; + DELETENULL(osd); } void cSkinClassicDisplayVolume::SetVolume(int Current, int Total, bool Mute) @@ -548,6 +552,7 @@ public: cSkinClassicDisplayTracks::cSkinClassicDisplayTracks(const char *Title, int NumTracks, const char * const *Tracks) { + osd = NULL; const cFont *font = cFont::GetFont(fontOsd); lineHeight = font->Height(); currentIndex = -1; @@ -575,7 +580,7 @@ cSkinClassicDisplayTracks::cSkinClassicD cSkinClassicDisplayTracks::~cSkinClassicDisplayTracks() { - delete osd; + DELETENULL(osd); } void cSkinClassicDisplayTracks::SetItem(const char *Text, int Index, bool Current) @@ -621,6 +626,7 @@ public: cSkinClassicDisplayMessage::cSkinClassicDisplayMessage(void) { + osd = NULL; const cFont *font = cFont::GetFont(fontOsd); int lineHeight = font->Height(); osd = cOsdProvider::NewOsd(Setup.OSDLeft, Setup.OSDTop + Setup.OSDHeight - lineHeight); @@ -630,7 +636,7 @@ cSkinClassicDisplayMessage::cSkinClassic cSkinClassicDisplayMessage::~cSkinClassicDisplayMessage() { - delete osd; + DELETENULL(osd); } void cSkinClassicDisplayMessage::SetMessage(eMessageType Type, const char *Text) @@ -641,7 +647,8 @@ void cSkinClassicDisplayMessage::SetMess void cSkinClassicDisplayMessage::Flush(void) { - osd->Flush(); + if (osd) + osd->Flush(); } // --- cSkinClassic ---------------------------------------------------------- Only in src: skinclassic.o diff -up org27/skins.c src/skins.c --- org27/skins.c 2005-01-14 14:07:19.000000000 +0100 +++ src/skins.c 2005-06-30 10:17:10.000000000 +0200 @@ -189,7 +191,8 @@ eKeys cSkins::Message(eMessageType Type, cStatus::MsgOsdClear(); } else { - cSkinDisplay::Current()->SetMessage(Type, NULL); + if (cSkinDisplay::Current()) + cSkinDisplay::Current()->SetMessage(Type, NULL); cStatus::MsgOsdStatusMessage(NULL); } }