bugGnash - The GNU Flash player - Bugs: bug #20046, assertion failing in...

 
 

bug #20046: assertion failing in NetStreamFFmpeg.cpp

Submitted by:  Sandro Santilli <strk>
Submitted on:  Sat 02 Jun 2007 05:13:00 PM UTC  
 
Category: video-ffmpegSeverity: 5 - Blocker
Release: NoneStatus: Ready For Test
Privacy: PublicAssigned to: Sandro Santilli <strk>
Open/Closed: Closed

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Mon 04 Jun 2007 02:32:38 PM UTC, comment #12:

Committed a sleep in the decoder loop. This fixes the intensive CPU use. Can you confirm ?

Sandro Santilli <strk>
Project MemberIn charge of this item.
Mon 04 Jun 2007 01:17:17 PM UTC, comment #11:

Btw, if FLVParser was used always (not only on short queues) AND the FLVParser call was blocking, CPU usage might actually become less intensive by optimizing the polling or waiting mechanism in lower-level objects (LoadThread/curl_adapter/tu_file).

Failing to have a blocking call forces us to try in a loop (possibly sleeping) till the data is available.
Note that other sleep will still be in the called functions, again looping in try&check iterations..

Sandro Santilli <strk>
Project MemberIn charge of this item.
Mon 04 Jun 2007 10:25:56 AM UTC, comment #10:

I'd think it should not be "only" used when queues are short, but rather it should be used as soon as it provides new data.
See the mail on gnash-dev about other problems with that...

Sandro Santilli <strk>
Project MemberIn charge of this item.
Mon 04 Jun 2007 10:20:16 AM UTC, comment #9:

No, the reason for 100% CPU is that the decoding loop in av_streamer is looping like crazy without a conditional variable or a sleep. The problem is not on FLVParser since that is "only" used when the queues are getting "close to empty", which is defined as less than 10 elements in the loop, iirc.

Tomas Groth <tgc>
Mon 04 Jun 2007 10:02:02 AM UTC, comment #8:

Am I right that the 100% CPU thing is due to FLVParser::next{Video,Audio}Frame ?

These snippets in particular:

// Make sure that there are parsed enough
// frames to return the need frame
while(_videoFrames.size() <= _nextVideoFrame && !_parsingComplete)
{.
if (!parseNextFrame()) break;
// WE COULD SLEEP HERE ...
}

Sandro Santilli <strk>
Project MemberIn charge of this item.
Mon 04 Jun 2007 09:45:42 AM UTC, comment #7:

We might sleep, assuming the playback won't reach us so easily in that sleep time. A better solution would be waiting for the parser to have more data to decode (not on the main thread).
Even better would be having non-blocking reads interface on FLVParser (which would go deep into tu_file).

Sandro Santilli <strk>
Project MemberIn charge of this item.
Mon 04 Jun 2007 09:31:47 AM UTC, comment #6:

Dropping the conditional variable will make gnash use 100% CPU while decoding/playing, is that acceptable? I know it's better than deadlocking, but still... A possible solution could be to make the loop sleep when the queues are filled, though that might cause some other issues. I'm short on time today, so i wont be able to test it right now.

Tomas Groth <tgc>
Mon 04 Jun 2007 09:07:08 AM UTC, comment #5:

I actually dropped the conditional variable as a whole, I never liked it and I'm sure its not correct for the decoder to stop in any circumstance except reaching end of stream.

The decoder mutex is also been dropped now.
My experience is much better now with video.
Please test on your side.

BTW, I still have a problem with shared version of gnash, but its the same as before (except no deadlock now..)

Sandro Santilli <strk>
Project MemberIn charge of this item.
Sun 03 Jun 2007 06:14:18 PM UTC, comment #4:

Sorry, breaking there will NOT do the job.

The problem is that the check for m_go AND the wait() call must
happen atomically. This is what the mutex associated with conditional variables is for.
In partucular, the wait() call will unlock a mutex to give other threads a chance to reach the condition we're waiting for.

So, either we do NOT rely on m_go, or we ensure m_go can't be changed until the mutex associated with decoder_wait is released
(which is at wait() time).

Code that relies on m_go in order to signal the condition is in ::close(). Didn't check other places

Sandro Santilli <strk>
Project MemberIn charge of this item.
Sun 03 Jun 2007 04:24:11 PM UTC, comment #3:

Yes, the break should do, if you comment out the abort() right before the break I committed it should be fine for now.
Just wanted to point out we don't check m_go enough

Sandro Santilli <strk>
Project MemberIn charge of this item.
Sun 03 Jun 2007 03:35:50 PM UTC, comment #2:

What does it take to reproduce the assertion fault?

Anyway, couldn't a quick and dirty hack be to place a "if (!m_go) break;" before calling wait()?

Tomas Groth <tgc>
Sat 02 Jun 2007 05:25:59 PM UTC, comment #1:

Actually, I'm thinking of a getDecoderRequestedState() function, locking and returning one of:
RUN, STOP, QUIT

That way we would also have a mechanism for premature destruction
of the thread (requesting a QUIT by a setDecoderStateRequest()).

Sandro Santilli <strk>
Project MemberIn charge of this item.
Sat 02 Jun 2007 05:13:00 PM UTC, original submission:

server/asobj/NetStreamFfmpeg.cpp:668: static void gnash::NetStreamFfmpeg::av_streamer(gnash::NetStreamFfmpeg*): Assertion `ns->m_go' failed.

The assertion is there to prevent a deadlock, as it checks
the m_go flag (also checked as the loop condition) inside
the iteration, right before calling wait().
Some code signaling the condition variable won't signal it if
m_go is false, so if it is before calling wait() I think it will
result in a deadlock..

This was a known problem and we were trying to address it with a redesign. Anyway, for 0.8.0 we'd better have a method to invoke
to fetch and set m_go locking it's own mutex (it worked fine for another similar blocker).

Sandro Santilli <strk>
Project MemberIn charge of this item.

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by tgc (Posted a comment)
  • -unavailable- added by strk (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 3 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon 04 Jun 2007 08:06:10 PM UTCstrkOpen/ClosedOpen=>Closed
    Mon 04 Jun 2007 09:07:08 AM UTCstrkStatusNone=>Ready For Test
      Assigned totgc=>strk

    Back to the top


    Powered by Savane 3.1-cleanup1