From 2f0f88a0ee7a862c2810b7e720b26c2f4cf168fc Mon Sep 17 00:00:00 2001 From: Chris Tallon Date: Mon, 24 Feb 2020 16:04:53 +0000 Subject: [PATCH] Remove Signal class, use std:: condvar etc natively --- boxstack.h | 1 - osdopenvg.cc | 41 ++++++++++++++++++++------ osdopenvg.h | 7 +++-- signal.cc | 83 ---------------------------------------------------- signal.h | 53 --------------------------------- videoomx.cc | 71 ++++++++++++++++++++++++++++++++------------ videoomx.h | 12 ++++---- 7 files changed, 96 insertions(+), 172 deletions(-) delete mode 100644 signal.cc delete mode 100644 signal.h diff --git a/boxstack.h b/boxstack.h index 41232f3..188f7de 100644 --- a/boxstack.h +++ b/boxstack.h @@ -22,7 +22,6 @@ #include #include -#include #include #include #include diff --git a/osdopenvg.cc b/osdopenvg.cc index 6441788..4a2f15f 100644 --- a/osdopenvg.cc +++ b/osdopenvg.cc @@ -17,21 +17,20 @@ along with VOMP. If not, see . */ +#include +#include +#include +#include +#include +#include -#include "osdopenvg.h" #include "videoomx.h" #include "surface.h" - #include "message.h" #include "command.h" #include "teletxt/txtfont.h" -#include -#include -#include -#include -#include - +#include "osdopenvg.h" #define EXTERNALPICTURE(name, fname, fileextension) extern uint8_t name ## _data[] asm("_binary_other_"#fname"_"#fileextension"_start"); \ @@ -1362,7 +1361,16 @@ bool OsdOpenVG::processTasks() taskmutex.unlock(); vgmutex.unlock(); //threadCheckExit(); - vgtaskSignal.signal(); + + /* Getting rid of Signal class. As with VideoOMX, just replicate what Signal did here + * and figure out if any of this can be simplified later. e.g. taskmutex sounds + * like it should be the mutex being used. 3 mutexes here??? + */ + + vgTaskSignalMutex.lock(); + vgTaskSignal.notify_one(); + vgTaskSignalMutex.unlock(); + taskmutex.lock(); vgmutex.lock(); worked=true; @@ -1409,6 +1417,18 @@ unsigned int OsdOpenVG::putOpenVGCommand(OpenVGCommand& comm,bool wait) while (wait) { unsigned int resp; if (!haveOpenVGResponse(comm.id,&resp)) { + +// Log::getInstance()->log("OsdOpenVG", Log::DEBUG, "putOpenVGCommand wait_for"); + std::unique_lock ul(vgTaskSignalMutex); + auto a = vgTaskSignal.wait_for(ul, std::chrono::milliseconds(100)); + ul.unlock(); +/* + if (a == std::cv_status::timeout) + Log::getInstance()->log("OsdOpenVG", Log::DEBUG, "putOpenVGCommand timeout"); + else + Log::getInstance()->log("OsdOpenVG", Log::DEBUG, "putOpenVGCommand no timeout"); +*/ + /* struct timespec target_time; clock_gettime(CLOCK_REALTIME,&target_time); target_time.tv_nsec+=1000000LL*100; @@ -1417,6 +1437,9 @@ unsigned int OsdOpenVG::putOpenVGCommand(OpenVGCommand& comm,bool wait) target_time.tv_sec+=1; } vgtaskSignal.waitForSignalTimed(&target_time); + */ + + } else { return resp; } diff --git a/osdopenvg.h b/osdopenvg.h index 0742821..49e81ac 100644 --- a/osdopenvg.h +++ b/osdopenvg.h @@ -22,6 +22,7 @@ #include #include +#include #include #include @@ -34,7 +35,6 @@ #include "defines.h" #include "log.h" #include "threadp.h" -#include "signal.h" #include "videoomx.h" #include "staticartwork.h" #ifdef PICTURE_DECODER_OMX @@ -134,7 +134,10 @@ protected: std::mutex vgmutex; std::mutex taskmutex; - Signal vgtaskSignal; + + std::mutex vgTaskSignalMutex; + std::condition_variable vgTaskSignal; + std::deque vgcommands; std::deque vgresponses; bool processTasks(); diff --git a/signal.cc b/signal.cc deleted file mode 100644 index bc4eb72..0000000 --- a/signal.cc +++ /dev/null @@ -1,83 +0,0 @@ -/* - Copyright 2004-2005 Chris Tallon - - This file is part of VOMP. - - VOMP is free software; you can redistribute it and/or modify - it under the terms of the GNU General Public License as published by - the Free Software Foundation; either version 2 of the License, or - (at your option) any later version. - - VOMP is distributed in the hope that it will be useful, - but WITHOUT ANY WARRANTY; without even the implied warranty of - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - GNU General Public License for more details. - - You should have received a copy of the GNU General Public License - along with VOMP; if not, write to the Free Software - Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. -*/ -#include "signal.h" - -// FIXME -// well, I say fix... Delete me. - - -Signal::Signal() -{ - pthread_cond_init(&cond, NULL); - pthread_mutex_init(&condMutex, NULL); -} - -Signal::~Signal() -{ - pthread_cond_destroy(&cond); - pthread_mutex_destroy(&condMutex); -} - - - -void Signal::signal() -{ -#ifndef WIN32 - pthread_mutex_lock(&condMutex); - pthread_cond_signal(&cond); - pthread_mutex_unlock(&condMutex); -#else -#error "Not ported yet" -#endif -} - -void Signal::waitForSignal() -{ -#ifndef WIN32 - pthread_mutex_lock(&condMutex); - pthread_cond_wait(&cond, &condMutex); - pthread_mutex_unlock(&condMutex); -#else -#error "Not ported yet" -#endif -} - -void Signal::waitForSignalTimed(struct timespec* ts) -{ -#ifndef WIN32 - pthread_mutex_lock(&condMutex); - pthread_cond_timedwait(&cond, &condMutex, ts); - pthread_mutex_unlock(&condMutex); -#else -#error "Not ported yet" -#endif -} - -void Signal::waitForSignalTimed(unsigned int ts) -{ - struct timespec target_time; - clock_gettime(CLOCK_REALTIME,&target_time); - target_time.tv_nsec+=1000000LL*ts; - if (target_time.tv_nsec>999999999) { - target_time.tv_nsec-=1000000000L; - target_time.tv_sec+=1; - } - waitForSignalTimed(&target_time); -} diff --git a/signal.h b/signal.h deleted file mode 100644 index dd98d93..0000000 --- a/signal.h +++ /dev/null @@ -1,53 +0,0 @@ -/* - Copyright 2004-2005 Chris Tallon - - This file is part of VOMP. - - VOMP is free software; you can redistribute it and/or modify - it under the terms of the GNU General Public License as published by - the Free Software Foundation; either version 2 of the License, or - (at your option) any later version. - - VOMP is distributed in the hope that it will be useful, - but WITHOUT ANY WARRANTY; without even the implied warranty of - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - GNU General Public License for more details. - - You should have received a copy of the GNU General Public License - along with VOMP; if not, write to the Free Software - Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. -*/ - -#ifndef SIGNAL_H -#define SIGNAL_H - -#ifndef WIN32 -#include -#include -#else -#include -#include -#endif - - -class Signal { -public: - Signal(); - ~Signal(); - void signal(); // releases a thread that has called threadWaitForSignal - void waitForSignal(); // pauses thread until dignal() is called - void waitForSignalTimed(struct timespec*); // pauses thread until signal() is called or timer expires - void waitForSignalTimed(unsigned int ts); - -protected: -#ifndef WIN32 - pthread_cond_t cond; - pthread_mutex_t condMutex; -#else -#error "Not ported yet" -#endif - -}; - - -#endif diff --git a/videoomx.cc b/videoomx.cc index ed52061..2f207c9 100644 --- a/videoomx.cc +++ b/videoomx.cc @@ -17,7 +17,12 @@ along with VOMP. If not, see . */ -#include "videoomx.h" +#include +#include + +#include + +#include "log.h" #include "audioomx.h" #include "demuxer.h" #include "vdr.h" @@ -26,12 +31,7 @@ #include "boxstack.h" #include "inputman.h" -#include - -#include - -// temp -#include "log.h" +#include "videoomx.h" //A lot of parts of this file are heavily inspired by xbmc omx implementations @@ -167,14 +167,41 @@ OMX_ERRORTYPE VideoOMX::EventHandler_OMX(OMX_IN OMX_HANDLETYPE handle,OMX_IN OMX } +void VideoOMX::signalOmx() +{ + /* + * Getting rid of Signal class. It looks like VideoOMX uses a wait-on-condition-variable in WaitForEvent() + * and CommandFinished(). These methods both use timed waits and don't use exact thread synchronisation - + * i.e. a caught signal will end the wait early but a missed signal doesn't matter. So, I'm just copying + * in what the Signal class used to do here and I'll sort it out later. + * Q: Are the found places the only synchronisation points? Would it be possible to change this to use + * exact sychronisation and remove the wait spin loops? Unknown. + * + * This omx_event_mutex - is this exactly locking the very thing the condition variable is being used + * for? i.e. is omx_event_mutex really the mutex that should be being used with the cond var? + * + * Callers of signalOmx: + * + * VideoOMX::AddOmxEvent, VideoOMX::ReturnEmptyOMXBuffer + * ImageOMX::ReturnEmptyOMXBuffer, ImageOMX::ReturnFillOMXBuffer + * AudioOMX::ReturnEmptyOMXBuffer, AudioOMX::FillBufferDone_OMX + * + * Surprise: WaitForEvent isn't a long running loop while video is playing. + */ + + omx_event_ready_signal_mutex.lock(); + omx_event_ready_signal.notify_one(); // Signal called pthread_cond_signal - unblock one + omx_event_ready_signal_mutex.unlock(); +}; + void VideoOMX::AddOmxEvent(VPE_OMX_EVENT new_event) { - omx_event_mutex.lock(); - omx_events.push_back(new_event); - omx_event_mutex.unlock(); - omx_event_ready_signal.signal(); -} + omx_event_mutex.lock(); + omx_events.push_back(new_event); + omx_event_mutex.unlock(); + signalOmx(); +} OMX_ERRORTYPE VideoOMX::EmptyBufferDone_OMX(OMX_IN OMX_HANDLETYPE hcomp,OMX_IN OMX_PTR appdata,OMX_IN OMX_BUFFERHEADERTYPE* buffer){ @@ -194,7 +221,8 @@ void VideoOMX::ReturnEmptyOMXBuffer(OMX_BUFFERHEADERTYPE* buffer){ input_bufs_omx_free.push_back(buffer); //Log::getInstance()->log("Video", Log::NOTICE, "ReturnEmptyOMXBuffer %d",input_bufs_omx_free.size()); input_bufs_omx_mutex.unlock(); - omx_event_ready_signal.signal(); + + signalOmx(); } OMX_ERRORTYPE VideoOMX::FillBufferDone_OMX(OMX_IN OMX_HANDLETYPE hcomp, OMX_IN OMX_PTR appdata,OMX_IN OMX_BUFFERHEADERTYPE* buffer) { @@ -1793,9 +1821,13 @@ int VideoOMX::WaitForEvent(OMX_HANDLETYPE handle,OMX_U32 event, int wait) //need } omx_event_mutex.unlock(); - omx_event_ready_signal.waitForSignalTimed(10); - //MILLISLEEP(2); - i++; + + //Log::getInstance()->log("Video", Log::DEBUG, "WaitForEvent"); + std::unique_lock ul(omx_event_ready_signal_mutex); + omx_event_ready_signal.wait_for(ul, std::chrono::milliseconds(10)); + ul.unlock(); + + i++; } Log::getInstance()->log("Video", Log::DEBUG, "WaitForEvent waited too long %x %x",handle,event); @@ -1900,8 +1932,11 @@ int VideoOMX::CommandFinished(OMX_HANDLETYPE handle,OMX_U32 command,OMX_U32 data } omx_event_mutex.unlock(); - omx_event_ready_signal.waitForSignalTimed(10); - //MILLISLEEP(2); + + std::unique_lock ul(omx_event_ready_signal_mutex); + omx_event_ready_signal.wait_for(ul, std::chrono::milliseconds(10)); + ul.unlock(); + i++; } diff --git a/videoomx.h b/videoomx.h index b3819b7..1143349 100644 --- a/videoomx.h +++ b/videoomx.h @@ -30,13 +30,10 @@ #include #include #include - -#include "signal.h" +#include #include "defines.h" #include "video.h" -#include "threadsystem.h" - #define OMX_SKIP64BIT @@ -224,7 +221,11 @@ class VideoOMX : public Video void clockUnpause(); bool isClockPaused() {return clockpaused;}; - void signalOmx() {omx_event_ready_signal.signal();}; + void signalOmx(); + std::condition_variable omx_event_ready_signal; + std::mutex omx_event_ready_signal_mutex; + + void interlaceSwitch4Demux(); @@ -277,7 +278,6 @@ class VideoOMX : public Video bool omx_first_frame; std::mutex omx_event_mutex; - Signal omx_event_ready_signal; std::list omx_events; -- 2.39.5