| Message ID | d0udas$sfb$1@fritz38552.news.dfncis.de |
|---|---|
| State | New |
| Headers |
Received: from mout0.freenet.de ([194.97.50.131]) by www.linuxtv.org with esmtp (Exim 4.34) id 1DA36g-00035n-92 for vdr@linuxtv.org; Sat, 12 Mar 2005 10:40:26 +0100 Received: from [194.97.55.191] (helo=mx7.freenet.de) by mout0.freenet.de with esmtpa (Exim 4.51) id 1DA36I-0002cp-Cw for vdr@linuxtv.org; Sat, 12 Mar 2005 10:40:02 +0100 Received: from pd9573839.dip0.t-ipconnect.de ([217.87.56.57] helo=gurke.wofritz.de) by mx7.freenet.de with esmtpa (ID wofritz@freenet.de) (Exim 4.43 #13) id 1DA36I-0007kW-4p for vdr@linuxtv.org; Sat, 12 Mar 2005 10:40:02 +0100 Received: from localhost (localhost [127.0.0.1]) by gurke.wofritz.de (Postfix) with ESMTP id EBB9F263C7 for <vdr@linuxtv.org>; Sat, 12 Mar 2005 10:40:08 +0100 (CET) Received: by gurke.wofritz.de (Postfix, from userid 500) id 63DE126ABA; Sat, 12 Mar 2005 10:39:09 +0100 (CET) To: vdr@linuxtv.org X-Original-To: moderator@gurke.wofritz.de Delivered-To: moderator@gurke.wofritz.de by gurke.wofritz.de (Postfix) with ESMTP id E60FE263C7 for <moderator@gurke.wofritz.de>; Sat, 12 Mar 2005 10:39:08 +0100 (CET) id 84A4826ABA; Sat, 12 Mar 2005 10:38:36 +0100 (CET) From: Wolfgang Fritz <wolfgang.fritz@gmx.net> Date: Sat, 12 Mar 2005 10:38:39 +0100 Organization: None Lines: 22 Message-ID: <d0udas$sfb$1@fritz38552.news.dfncis.de> NNTP-Posting-Host: eddie.wofritz.de Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit User-Agent: Mozilla Thunderbird 1.0 (X11/20041206) X-Accept-Language: en-us, en X-AntiVirus: checked by AntiVir MailGate (version: 2.0.2-8; AVE: 6.30.0.5; VDF: 6.30.0.26; host: gurke) Subject: [vdr] [PATCH] mp3-0.9.11 signals end of replay too late X-BeenThere: vdr@linuxtv.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: Wolfgang Fritz <wolfgang.fritz@gmx.net>, 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: Sat, 12 Mar 2005 09:40:26 -0000 Status: O X-Status: X-Keywords: X-UID: 647 |
Commit Message
Wolfgang Fritz
March 12, 2005, 9:38 a.m. UTC
Graphlcd crashes with segfault when stopping an MP3 replay because it calls cMP3Control::GetIndex after the MP3 player object is already destroyed. Reason: MP3 plugin signals end of replay too late. The attaches patch fixes it.
Comments
On 12 Mar 2005 Wolfgang Fritz <wolfgang.fritz@gmx.net> wrote: > Graphlcd crashes with segfault when stopping an MP3 replay because it > calls cMP3Control::GetIndex after the MP3 player object is already > destroyed. > > Reason: MP3 plugin signals end of replay too late. The attaches patch > fixes it. Ok, I changed the location, but I think that the real flaw is in VDRs player.h where the "player" var isn't checked before access. Regards.
Stefan Huelswitt wrote: > On 12 Mar 2005 Wolfgang Fritz <wolfgang.fritz@gmx.net> wrote: > > >>Graphlcd crashes with segfault when stopping an MP3 replay because it >>calls cMP3Control::GetIndex after the MP3 player object is already >>destroyed. >> >>Reason: MP3 plugin signals end of replay too late. The attaches patch >>fixes it. > > > Ok, I changed the location, but I think that the real flaw is in > VDRs player.h where the "player" var isn't checked before access. > If tested this (temporarily inserted a test for player == 0 in player.h), but it didn't help. maybe void cMP3Control::Stop(void) { delete player; player=0; mgr->Halt(); mgr->Flush(); //XXX remove later } has a concurrency problem? Who guarantees that no other thread interrupts the mp3 thread while is in the destructor? Wouldn't be something like this a little bit safer: void cMP3Control::Stop(void) { cMP3player *p = player; player = 0; delete p; mgr->Halt(); mgr->Flush(); //XXX remove later } There seem to be more concurrency problems which crop up with the graphlcd plugin. In rare cases I get a segfault when exiting a normal vdr replay. Wolfgang > Regards. >
On 15 Mar 2005 Wolfgang Fritz <wolfgang.fritz@gmx.net> wrote: > Stefan Huelswitt wrote: >> Ok, I changed the location, but I think that the real flaw is in >> VDRs player.h where the "player" var isn't checked before access. > > If tested this (temporarily inserted a test for player == 0 in > player.h), but it didn't help. > > maybe > > void cMP3Control::Stop(void) > { > delete player; player=0; > mgr->Halt(); > mgr->Flush(); //XXX remove later > } > > has a concurrency problem? Who guarantees that no other thread > interrupts the mp3 thread while is in the destructor? > > Wouldn't be something like this a little bit safer: > > void cMP3Control::Stop(void) > { > cMP3player *p = player; > player = 0; > delete p; > mgr->Halt(); > mgr->Flush(); //XXX remove later > } > > There seem to be more concurrency problems which crop up with the > graphlcd plugin. In rare cases I get a segfault when exiting a normal > vdr replay. Of course it's a concurrency problem. >From the fact that there is no mutex protection for the player var (neither in MP3 plugin nor in any other player code), I would guess that the cControl API was designed for accesses from the VDR foreground thread only (no concurrency problem in this case). If graphlcd is running on another thread, this could be the explanation... Regards.
Stefan Huelswitt wrote: > On 15 Mar 2005 Wolfgang Fritz <wolfgang.fritz@gmx.net> wrote: > > >>Stefan Huelswitt wrote: >> >>>Ok, I changed the location, but I think that the real flaw is in >>>VDRs player.h where the "player" var isn't checked before access. >> >>If tested this (temporarily inserted a test for player == 0 in >>player.h), but it didn't help. >> >>maybe >> >>void cMP3Control::Stop(void) >>{ >> delete player; player=0; >> mgr->Halt(); >> mgr->Flush(); //XXX remove later >>} >> >>has a concurrency problem? Who guarantees that no other thread >>interrupts the mp3 thread while is in the destructor? >> >>Wouldn't be something like this a little bit safer: >> >>void cMP3Control::Stop(void) >>{ >> cMP3player *p = player; >> player = 0; >> delete p; >> mgr->Halt(); >> mgr->Flush(); //XXX remove later >>} >> >>There seem to be more concurrency problems which crop up with the >>graphlcd plugin. In rare cases I get a segfault when exiting a normal >>vdr replay. > > > Of course it's a concurrency problem. >>From the fact that there is no mutex protection for the player > var (neither in MP3 plugin nor in any other player code), I would > guess that the cControl API was designed for accesses from the > VDR foreground thread only (no concurrency problem in this case). That's right - a player is not supposed to be accessed from anything else than the foreground thread. Klaus > If graphlcd is running on another thread, this could be the > explanation... > > Regards. >
Klaus Schmidinger wrote: > Stefan Huelswitt wrote: > >>On 15 Mar 2005 Wolfgang Fritz <wolfgang.fritz@gmx.net> wrote: >> >> >> >>>Stefan Huelswitt wrote: >>> >>> >>>>Ok, I changed the location, but I think that the real flaw is in >>>>VDRs player.h where the "player" var isn't checked before access. >>> >>>If tested this (temporarily inserted a test for player == 0 in >>>player.h), but it didn't help. >>> >>>maybe >>> >>>void cMP3Control::Stop(void) >>>{ >>> delete player; player=0; >>> mgr->Halt(); >>> mgr->Flush(); //XXX remove later >>>} >>> >>>has a concurrency problem? Who guarantees that no other thread >>>interrupts the mp3 thread while is in the destructor? >>> >>>Wouldn't be something like this a little bit safer: >>> >>>void cMP3Control::Stop(void) >>>{ >>> cMP3player *p = player; >>> player = 0; >>> delete p; >>> mgr->Halt(); >>> mgr->Flush(); //XXX remove later >>>} >>> >>>There seem to be more concurrency problems which crop up with the >>>graphlcd plugin. In rare cases I get a segfault when exiting a normal >>>vdr replay. >> >> >>Of course it's a concurrency problem. >>>From the fact that there is no mutex protection for the player >>var (neither in MP3 plugin nor in any other player code), I would >>guess that the cControl API was designed for accesses from the >>VDR foreground thread only (no concurrency problem in this case). > > > That's right - a player is not supposed to be accessed from anything > else than the foreground thread. > As far I can see, cControl::GetIndex() is the only method graphlcd calls in its own thread context to get periodic information about the replay progress (for updating its progress bar display). What would be the correct way to get that information? Wolfgang > Klaus > > >>If graphlcd is running on another thread, this could be the >>explanation... >> >>Regards. >> > > >
Wolfgang Fritz wrote: > Klaus Schmidinger wrote: > >>Stefan Huelswitt wrote: >> >> >>>On 15 Mar 2005 Wolfgang Fritz <wolfgang.fritz@gmx.net> wrote: >>> >>> >>> >>> >>>>Stefan Huelswitt wrote: >>>> >>>> >>>> >>>>>Ok, I changed the location, but I think that the real flaw is in >>>>>VDRs player.h where the "player" var isn't checked before access. >>>> >>>>If tested this (temporarily inserted a test for player == 0 in >>>>player.h), but it didn't help. >>>> >>>>maybe >>>> >>>>void cMP3Control::Stop(void) >>>>{ >>>>delete player; player=0; >>>>mgr->Halt(); >>>>mgr->Flush(); //XXX remove later >>>>} >>>> >>>>has a concurrency problem? Who guarantees that no other thread >>>>interrupts the mp3 thread while is in the destructor? >>>> >>>>Wouldn't be something like this a little bit safer: >>>> >>>>void cMP3Control::Stop(void) >>>>{ >>>>cMP3player *p = player; >>>>player = 0; >>>>delete p; >>>>mgr->Halt(); >>>>mgr->Flush(); //XXX remove later >>>>} >>>> >>>>There seem to be more concurrency problems which crop up with the >>>>graphlcd plugin. In rare cases I get a segfault when exiting a normal >>>>vdr replay. >>> >>> >>>Of course it's a concurrency problem. >>>>From the fact that there is no mutex protection for the player >>>var (neither in MP3 plugin nor in any other player code), I would >>>guess that the cControl API was designed for accesses from the >>>VDR foreground thread only (no concurrency problem in this case). >> >> >>That's right - a player is not supposed to be accessed from anything >>else than the foreground thread. >> > > > As far I can see, cControl::GetIndex() is the only method graphlcd calls > in its own thread context to get periodic information about the replay > progress (for updating its progress bar display). What would be the > correct way to get that information? > > Wolfgang I guess the correct way to do this would be to introduce a dedicated function in cStatus. Just grabbing into the cControl from another thread is not the right thing to do. Klaus
=================================================================== --- mp3.c (Revision 228) +++ mp3.c (Arbeitskopie) @@ -261,10 +261,10 @@ cMP3Control::~cMP3Control() { + cStatus::MsgReplaying(this, NULL); delete lastMode; Hide(); Stop(); - cStatus::MsgReplaying(this, NULL); } void cMP3Control::Stop(void)