Incorporated elupus' comments including: 1) Fixed CCriticalSection exit behavior...

diff --git a/xbmc/cores/dvdplayer/DVDDemuxers/DVDDemuxFFmpeg.cpp b/xbmc/cores/dvdplayer/DVDDemuxers/DVDDemuxFFmpeg.cpp
index 660afde..ae478a8 100644 (file)
--- a/xbmc/cores/dvdplayer/DVDDemuxers/DVDDemuxFFmpeg.cpp
+++ b/xbmc/cores/dvdplayer/DVDDemuxers/DVDDemuxFFmpeg.cpp
@@ -635,159 +635,158 @@ DemuxPacket* CDVDDemuxFFmpeg::Read()
   // on some cases where the received packet is invalid we will need to return an empty packet (0 length) otherwise the main loop (in CDVDPlayer)
   // would consider this the end of stream and stop.
   bool bReturnEmpty = false;
+  { CSingleLock lock(m_critSection); // open lock scope
+  if (m_pFormatContext)
   {
-    CSingleLock lock(m_critSection);
-    if (m_pFormatContext)
+    // assume we are not eof
+    if(m_pFormatContext->pb)
+      m_pFormatContext->pb->eof_reached = 0;
+
+    // keep track if ffmpeg doesn't always set these
+    pkt.size = 0;
+    pkt.data = NULL;
+    pkt.stream_index = MAX_STREAMS;
+
+    // timeout reads after 100ms
+    m_timeout = CTimeUtils::GetTimeMS() + 20000;
+    int result = 0;
+    try
     {
-      // assume we are not eof
-      if(m_pFormatContext->pb)
-        m_pFormatContext->pb->eof_reached = 0;
-
-      // keep track if ffmpeg doesn't always set these
-      pkt.size = 0;
-      pkt.data = NULL;
-      pkt.stream_index = MAX_STREAMS;
-
-      // timeout reads after 100ms
-      m_timeout = CTimeUtils::GetTimeMS() + 20000;
-      int result = 0;
-      try
-      {
-        result = m_dllAvFormat.av_read_frame(m_pFormatContext, &pkt);
-      }
-      catch(const win32_exception &e)
-      {
-        e.writelog(__FUNCTION__);
-        result = AVERROR(EFAULT);
-      }
-      m_timeout = 0;
+      result = m_dllAvFormat.av_read_frame(m_pFormatContext, &pkt);
+    }
+    catch(const win32_exception &e)
+    {
+      e.writelog(__FUNCTION__);
+      result = AVERROR(EFAULT);
+    }
+    m_timeout = 0;
 
-      if (result == AVERROR(EINTR) || result == AVERROR(EAGAIN))
+    if (result == AVERROR(EINTR) || result == AVERROR(EAGAIN))
+    {
+      // timeout, probably no real error, return empty packet
+      bReturnEmpty = true;
+    }
+    else if (result < 0)
+    {
+      Flush();
+    }
+    else if (pkt.size < 0 || pkt.stream_index >= MAX_STREAMS)
+    {
+      // XXX, in some cases ffmpeg returns a negative packet size
+      if(m_pFormatContext->pb && !m_pFormatContext->pb->eof_reached)
       {
-        // timeout, probably no real error, return empty packet
+        CLog::Log(LOGERROR, "CDVDDemuxFFmpeg::Read() no valid packet");
         bReturnEmpty = true;
-      }
-      else if (result < 0)
-      {
         Flush();
       }
-      else if (pkt.size < 0 || pkt.stream_index >= MAX_STREAMS)
-      {
-        // XXX, in some cases ffmpeg returns a negative packet size
-        if(m_pFormatContext->pb && !m_pFormatContext->pb->eof_reached)
-        {
-          CLog::Log(LOGERROR, "CDVDDemuxFFmpeg::Read() no valid packet");
-          bReturnEmpty = true;
-          Flush();
-        }
-        else
-          CLog::Log(LOGERROR, "CDVDDemuxFFmpeg::Read() returned invalid packet and eof reached");
-
-        m_dllAvCodec.av_free_packet(&pkt);
-      }
       else
-      {
-        AVStream *stream = m_pFormatContext->streams[pkt.stream_index];
+        CLog::Log(LOGERROR, "CDVDDemuxFFmpeg::Read() returned invalid packet and eof reached");
 
-        if (m_program != UINT_MAX)
+      m_dllAvCodec.av_free_packet(&pkt);
+    }
+    else
+    {
+      AVStream *stream = m_pFormatContext->streams[pkt.stream_index];
+
+      if (m_program != UINT_MAX)
+      {
+        /* check so packet belongs to selected program */
+        for (unsigned int i = 0; i < m_pFormatContext->programs[m_program]->nb_stream_indexes; i++)
         {
-          /* check so packet belongs to selected program */
-          for (unsigned int i = 0; i < m_pFormatContext->programs[m_program]->nb_stream_indexes; i++)
+          if(pkt.stream_index == (int)m_pFormatContext->programs[m_program]->stream_index[i])
           {
-            if(pkt.stream_index == (int)m_pFormatContext->programs[m_program]->stream_index[i])
-            {
-              pPacket = CDVDDemuxUtils::AllocateDemuxPacket(pkt.size);
-              break;
-            }
+            pPacket = CDVDDemuxUtils::AllocateDemuxPacket(pkt.size);
+            break;
           }
-
-          if (!pPacket)
-            bReturnEmpty = true;
         }
-        else
-          pPacket = CDVDDemuxUtils::AllocateDemuxPacket(pkt.size);
 
-        if (pPacket)
-        {
-          // lavf sometimes bugs out and gives 0 dts/pts instead of no dts/pts
-          // since this could only happens on initial frame under normal
-          // circomstances, let's assume it is wrong all the time
-          if(pkt.dts == 0)
+        if (!pPacket)
+          bReturnEmpty = true;
+      }
+      else
+        pPacket = CDVDDemuxUtils::AllocateDemuxPacket(pkt.size);
+
+      if (pPacket)
+      {
+        // lavf sometimes bugs out and gives 0 dts/pts instead of no dts/pts
+        // since this could only happens on initial frame under normal
+        // circomstances, let's assume it is wrong all the time
+        if(pkt.dts == 0)
+          pkt.dts = AV_NOPTS_VALUE;
+        if(pkt.pts == 0)
+          pkt.pts = AV_NOPTS_VALUE;
+
+        if(m_bMatroska && stream->codec && stream->codec->codec_type == AVMEDIA_TYPE_VIDEO)
+        { // matroska can store different timestamps
+          // for different formats, for native stored
+          // stuff it is pts, but for ms compatibility
+          // tracks, it is really dts. sadly ffmpeg
+          // sets these two timestamps equal all the
+          // time, so we select it here instead
+          if(stream->codec->codec_tag == 0)
             pkt.dts = AV_NOPTS_VALUE;
-          if(pkt.pts == 0)
+          else
             pkt.pts = AV_NOPTS_VALUE;
+        }
 
-          if(m_bMatroska && stream->codec && stream->codec->codec_type == AVMEDIA_TYPE_VIDEO)
-          { // matroska can store different timestamps
-            // for different formats, for native stored
-            // stuff it is pts, but for ms compatibility
-            // tracks, it is really dts. sadly ffmpeg
-            // sets these two timestamps equal all the
-            // time, so we select it here instead
-            if(stream->codec->codec_tag == 0)
-              pkt.dts = AV_NOPTS_VALUE;
-            else
-              pkt.pts = AV_NOPTS_VALUE;
-          }
-
-          // we need to get duration slightly different for matroska embedded text subtitels
-          if(m_bMatroska && stream->codec->codec_id == CODEC_ID_TEXT && pkt.convergence_duration != 0)
-            pkt.duration = pkt.convergence_duration;
-
-          if(m_bAVI && stream->codec && stream->codec->codec_type == AVMEDIA_TYPE_VIDEO)
-          {
-            // AVI's always have borked pts, specially if m_pFormatContext->flags includes
-            // AVFMT_FLAG_GENPTS so always use dts
-            pkt.pts = AV_NOPTS_VALUE;
-          }
+        // we need to get duration slightly different for matroska embedded text subtitels
+        if(m_bMatroska && stream->codec->codec_id == CODEC_ID_TEXT && pkt.convergence_duration != 0)
+          pkt.duration = pkt.convergence_duration;
 
-          // copy contents into our own packet
-          pPacket->iSize = pkt.size;
+        if(m_bAVI && stream->codec && stream->codec->codec_type == AVMEDIA_TYPE_VIDEO)
+        {
+          // AVI's always have borked pts, specially if m_pFormatContext->flags includes
+          // AVFMT_FLAG_GENPTS so always use dts
+          pkt.pts = AV_NOPTS_VALUE;
+        }
 
-          // maybe we can avoid a memcpy here by detecting where pkt.destruct is pointing too?
-          if (pkt.data)
-            memcpy(pPacket->pData, pkt.data, pPacket->iSize);
+        // copy contents into our own packet
+        pPacket->iSize = pkt.size;
 
-          pPacket->pts = ConvertTimestamp(pkt.pts, stream->time_base.den, stream->time_base.num);
-          pPacket->dts = ConvertTimestamp(pkt.dts, stream->time_base.den, stream->time_base.num);
-          pPacket->duration =  DVD_SEC_TO_TIME((double)pkt.duration * stream->time_base.num / stream->time_base.den);
+        // maybe we can avoid a memcpy here by detecting where pkt.destruct is pointing too?
+        if (pkt.data)
+          memcpy(pPacket->pData, pkt.data, pPacket->iSize);
 
-          // used to guess streamlength
-          if (pPacket->dts != DVD_NOPTS_VALUE && (pPacket->dts > m_iCurrentPts || m_iCurrentPts == DVD_NOPTS_VALUE))
-            m_iCurrentPts = pPacket->dts;
+        pPacket->pts = ConvertTimestamp(pkt.pts, stream->time_base.den, stream->time_base.num);
+        pPacket->dts = ConvertTimestamp(pkt.dts, stream->time_base.den, stream->time_base.num);
+        pPacket->duration =  DVD_SEC_TO_TIME((double)pkt.duration * stream->time_base.num / stream->time_base.den);
 
+        // used to guess streamlength
+        if (pPacket->dts != DVD_NOPTS_VALUE && (pPacket->dts > m_iCurrentPts || m_iCurrentPts == DVD_NOPTS_VALUE))
+          m_iCurrentPts = pPacket->dts;
 
-          // check if stream has passed full duration, needed for live streams
-          if(pkt.dts != (int64_t)AV_NOPTS_VALUE)
-          {
-            int64_t duration;
-            duration = pkt.dts;
-            if(stream->start_time != (int64_t)AV_NOPTS_VALUE)
-              duration -= stream->start_time;
 
-            if(duration > stream->duration)
-            {
-              stream->duration = duration;
-              duration = m_dllAvUtil.av_rescale_rnd(stream->duration, stream->time_base.num * AV_TIME_BASE, stream->time_base.den, AV_ROUND_NEAR_INF);
-              if ((m_pFormatContext->duration == (int64_t)AV_NOPTS_VALUE && m_pFormatContext->file_size > 0)
-                  ||  (m_pFormatContext->duration != (int64_t)AV_NOPTS_VALUE && duration > m_pFormatContext->duration))
-                m_pFormatContext->duration = duration;
-            }
-          }
+        // check if stream has passed full duration, needed for live streams
+        if(pkt.dts != (int64_t)AV_NOPTS_VALUE)
+        {
+          int64_t duration;
+          duration = pkt.dts;
+          if(stream->start_time != (int64_t)AV_NOPTS_VALUE)
+            duration -= stream->start_time;
 
-          // check if stream seem to have grown since start
-          if(m_pFormatContext->file_size > 0 && m_pFormatContext->pb)
+          if(duration > stream->duration)
           {
-            if(m_pFormatContext->pb->pos > m_pFormatContext->file_size)
-              m_pFormatContext->file_size = m_pFormatContext->pb->pos;
+            stream->duration = duration;
+            duration = m_dllAvUtil.av_rescale_rnd(stream->duration, stream->time_base.num * AV_TIME_BASE, stream->time_base.den, AV_ROUND_NEAR_INF);
+            if ((m_pFormatContext->duration == (int64_t)AV_NOPTS_VALUE && m_pFormatContext->file_size > 0)
+                ||  (m_pFormatContext->duration != (int64_t)AV_NOPTS_VALUE && duration > m_pFormatContext->duration))
+              m_pFormatContext->duration = duration;
           }
+        }
 
-          pPacket->iStreamId = pkt.stream_index; // XXX just for now
+        // check if stream seem to have grown since start
+        if(m_pFormatContext->file_size > 0 && m_pFormatContext->pb)
+        {
+          if(m_pFormatContext->pb->pos > m_pFormatContext->file_size)
+            m_pFormatContext->file_size = m_pFormatContext->pb->pos;
         }
-        m_dllAvCodec.av_free_packet(&pkt);
+
+        pPacket->iStreamId = pkt.stream_index; // XXX just for now
       }
+      m_dllAvCodec.av_free_packet(&pkt);
     }
   }
+  } // end of lock scope
   if (bReturnEmpty && !pPacket)
     pPacket = CDVDDemuxUtils::AllocateDemuxPacket(0);
 

diff --git a/xbmc/cores/dvdplayer/DVDInputStreams/DVDInputStreamNavigator.cpp b/xbmc/cores/dvdplayer/DVDInputStreams/DVDInputStreamNavigator.cpp
index 35d0c95..c9c8f40 100644 (file)
--- a/xbmc/cores/dvdplayer/DVDInputStreams/DVDInputStreamNavigator.cpp
+++ b/xbmc/cores/dvdplayer/DVDInputStreams/DVDInputStreamNavigator.cpp
@@ -267,30 +267,14 @@ int CDVDInputStreamNavigator::ProcessBlock(BYTE* dest_buffer, int* read)
   if(m_holdmode == HOLDMODE_HELD)
     return NAVRESULT_HOLD;
 
-  try
-  {
-    // the main reading function
-    if(m_holdmode == HOLDMODE_SKIP)
-    { /* we where holding data, return the data held */
-      m_holdmode = HOLDMODE_DATA;
-      result = DVDNAV_STATUS_OK;
-    }
-    else
-      result = m_dll.dvdnav_get_next_cache_block(m_dvdnav, &buf, &m_lastevent, &len);
-
-  }
-  // this appears to be impossible as nothing in the try ever throws
-  // TODO: validate the above
-  catch (...)
-  {
-    CLog::Log(LOGERROR, "CDVDInputStreamNavigator::ProcessBlock - exception thrown in dvdnav_get_next_cache_block.");
-
-    // okey, we are probably holding a vm_lock here so leave it.. this could potentialy cause problems if we aren't holding it
-    // but it's more likely that we do
-//    LeaveCriticalSection((LPCRITICAL_SECTION)&(m_dvdnav->vm_lock));
-    m_bEOF = true;
-    return NAVRESULT_ERROR;
+  // the main reading function
+  if(m_holdmode == HOLDMODE_SKIP)
+  { /* we where holding data, return the data held */
+    m_holdmode = HOLDMODE_DATA;
+    result = DVDNAV_STATUS_OK;
   }
+  else
+    result = m_dll.dvdnav_get_next_cache_block(m_dvdnav, &buf, &m_lastevent, &len);
 
   if (result == DVDNAV_STATUS_ERR)
   {

diff --git a/xbmc/threads/CriticalSection.h b/xbmc/threads/CriticalSection.h
index da7c886..f24edf8 100644 (file)
--- a/xbmc/threads/CriticalSection.h
+++ b/xbmc/threads/CriticalSection.h
@@ -52,16 +52,16 @@
 template<class L> class CountingLockable
 {
 protected:
-  L m;
+  L mutex;
   unsigned int count;
 
 public:
   inline CountingLockable() : count(0) {}
 
   // boost::thread Lockable concept
-  inline void lock() { m.lock(); count++; }
-  inline bool try_lock() { return m.try_lock() ? count++, true : false; }
-  inline void unlock() { count--; m.unlock(); }
+  inline void lock() { mutex.lock(); count++; }
+  inline bool try_lock() { return mutex.try_lock() ? count++, true : false; }
+  inline void unlock() { count--; mutex.unlock(); }
 
   /**
    * This implements the "exitable" behavior mentioned above.
@@ -70,15 +70,14 @@ public:
   { 
     // it's possibe we don't actually own the lock
     // so we will try it.
-    unsigned int ret = count; 
+    unsigned int ret = 0;
     if (try_lock())
     {
-      unlock(); // unlock the try_lock
-      for (unsigned int i = 0; i < ret; i++) 
+      ret = count - 1;  // The -1 is because we don't want 
+                        //  to count the try_lock increment.
+      while (count > 0) // This will also unlock the try_lock.
         unlock();
     }
-    else
-      ret = 0;
 
     return ret; 
   }

diff --git a/xbmc/threads/Interruptible.cpp b/xbmc/threads/Interruptible.cpp
index f0a9f88..ec82bc2 100644 (file)
--- a/xbmc/threads/Interruptible.cpp
+++ b/xbmc/threads/Interruptible.cpp
@@ -42,8 +42,7 @@ namespace XbmcThreads
 
       {
         CSingleLock lock(staticMutexForLockingTheListOfIInterruptiblesForCallingInterrupt);
-        for (iter=interruptibles->begin(); iter != interruptibles->end(); iter++)
-          list.push_back(*(iter));
+        list = *interruptibles;
       }
 
       for (iter=list.begin(); iter != list.end(); iter++)

diff --git a/xbmc/threads/SharedSection.h b/xbmc/threads/SharedSection.h
index ef7e44e..e482758 100644 (file)
--- a/xbmc/threads/SharedSection.h
+++ b/xbmc/threads/SharedSection.h
@@ -39,9 +39,9 @@ class CSharedSection : public CountingLockable<boost::shared_mutex>
 {
 public:
 
-  inline void lock_shared() { m.lock_shared(); }
-  inline bool try_lock_shared() { return m.try_lock_shared(); }
-  inline void unlock_shared() { return m.unlock_shared(); }
+  inline void lock_shared() { mutex.lock_shared(); }
+  inline bool try_lock_shared() { return mutex.try_lock_shared(); }
+  inline void unlock_shared() { return mutex.unlock_shared(); }
 };
 
 class CSharedLock : public boost::shared_lock<CSharedSection>

diff --git a/xbmc/utils/DownloadQueue.cpp b/xbmc/utils/DownloadQueue.cpp
index be65eb6..4101090 100644 (file)
--- a/xbmc/utils/DownloadQueue.cpp
+++ b/xbmc/utils/DownloadQueue.cpp
@@ -153,39 +153,38 @@ void CDownloadQueue::Process()
 
       // now re-grab the item as we may have cancelled our download
       // while we were working
-      {
-        CSingleLock lock2(m_critical);
+      { CSingleLock lock2(m_critical); // open lock2 scope
 
-        request = m_queue.front();
-        m_queue.pop();
+      request = m_queue.front();
+      m_queue.pop();
 
-        // if the request has been cancelled our observer will be NULL
-        if (NULL != request.observer)
+      // if the request has been cancelled our observer will be NULL
+      if (NULL != request.observer)
+      {
+        try
         {
-          try
+          if (bFileRequest)
           {
-            if (bFileRequest)
-            {
-              request.observer->OnFileComplete(request.ticket, request.content, dwSize,
-                                               bSuccess ? IDownloadQueueObserver::Succeeded : IDownloadQueueObserver::Failed );
-            }
-            else
-            {
-              request.observer->OnContentComplete(request.ticket, request.content,
-                                                  bSuccess ? IDownloadQueueObserver::Succeeded : IDownloadQueueObserver::Failed );
-            }
+            request.observer->OnFileComplete(request.ticket, request.content, dwSize,
+                                             bSuccess ? IDownloadQueueObserver::Succeeded : IDownloadQueueObserver::Failed );
           }
-          catch (...)
+          else
           {
-            CLog::Log(LOGERROR, "exception while updating download observer.");
+            request.observer->OnContentComplete(request.ticket, request.content,
+                                                bSuccess ? IDownloadQueueObserver::Succeeded : IDownloadQueueObserver::Failed );
+          }
+        }
+        catch (...)
+        {
+          CLog::Log(LOGERROR, "exception while updating download observer.");
 
-            if (bFileRequest)
-            {
-              CFile::Delete(request.content);
-            }
+          if (bFileRequest)
+          {
+            CFile::Delete(request.content);
           }
         }
       }
+      } // close lock2 scope
     }
 
     Sleep(500);