Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix some outstanding issues #6

Merged
merged 6 commits into from
Dec 4, 2022
Merged

Fix some outstanding issues #6

merged 6 commits into from
Dec 4, 2022

Conversation

dr-m
Copy link
Contributor

@dr-m dr-m commented Nov 19, 2022

I had already submitted the fixes to #1 and #4 as comments in the issues. Here they are in pull request form.

I was also able to fix the VDR shutdown problems on my system (#5).

All of these issues are a showstopper for me, and #1 seems to be a showstopper for many users. There exist fixes for it in the wild; see for example vdr-projects/vdr-plugin-rpihddevice@b527fe5 (which also fixes #2).

cRpiDisplay::GetModeFormat(): If modeY is not recognized, pixelHeight
can be 0. Avoid dividing by it.

Closes #4
dr-m referenced this pull request in vdr-projects/vdr-plugin-rpihddevice Nov 19, 2022
audio.c Outdated Show resolved Hide resolved
omx.c Outdated Show resolved Hide resolved
The elements stored in cRpiAudioDecoder::m_ptsQueue could be leaked.
Let us simply store Pts objects directly instead of storing pointers.
To elide some copying, use C++11 emplace() when it is available.

Furthermore, we remove unnecessary pointer indirection and separate
memory heap allocation of some cMutex and cCondWait objects.

cOvgPixmap::DrawText(): Only allocate a buffer when it is going to
be used.
For vgDrawGlyphs(), the adjustments arrays must be of the same size
as the glyph arrays. Let us append a dummy element to the end.
This fixes a heap-buffer-overflow reported by AddressSanitizer.
No visible difference was observed in the output.
cOmxDevice::DeInit(): Simply invoke SetPlayMode(pmNone) to invoke
ilclient_disable_tunnel(), cRpiAudioDecoder::DeInit() and
cOmx::DeInit() in a clean way. Change the return type to void
and remove the unnecessary virtual member function declaration.

cRpiAudioDecoder::Reset(): Check for Active(), in case the decoder
thread had already terminated.

cOmxEvents: Remove unnecessary pointer indirection and an unused data
member cOmxEvents::m_signal.

cOmxEvents::Get(): Fix a race condition that could cause
m_portEvents->Add(0) from cOmx::DeInit() to go unnoticed.

cOmx::DeInit(): Wait for !Active(). Do not call
ilclient_disable_tunnel(), because it was already called.

Closes #5
@dr-m
Copy link
Contributor Author

dr-m commented Dec 3, 2022

I tested 05bf186 on VDR 2.6.2 with the following patch:

diff --git a/Makefile b/Makefile
index a251f903..e9f0c6f2 100644
--- a/Makefile
+++ b/Makefile
@@ -16,7 +16,7 @@ CC       ?= gcc
 CFLAGS   ?= -g -O3 -Wall
 
 CXX      ?= g++
-CXXFLAGS ?= -g -O3 -Wall -Werror=overloaded-virtual -Wno-parentheses
+CXXFLAGS ?= -g -O3 -Wall -Werror=overloaded-virtual -Wno-parentheses -fsanitize=address
 CXXFLAGS += $(CPPFLAGS)
 
 CDEFINES  = -D_GNU_SOURCE
diff --git a/device.c b/device.c
index 4b9c9cc7..c33e3297 100644
--- a/device.c
+++ b/device.c
@@ -125,7 +125,6 @@ cDevice::~cDevice()
   delete dvbSubtitleConverter;
   if (this == primaryDevice)
      primaryDevice = NULL;
-  Cancel(3);
 }
 
 bool cDevice::WaitForAllDevicesReady(int Timeout)
@@ -457,7 +456,10 @@ void cDevice::SetCamSlot(cCamSlot *CamSlot)
 void cDevice::Shutdown(void)
 {
   deviceHooks.Clear();
+  for (int i = 0; i < numDevices; i++)
+      device[i]->Cancel(-1);
   for (int i = 0; i < numDevices; i++) {
+      device[i]->Cancel(30);
       delete device[i];
       device[i] = NULL;
       }
diff --git a/dvbdevice.c b/dvbdevice.c
index 51331485..1caf1218 100644
--- a/dvbdevice.c
+++ b/dvbdevice.c
@@ -573,6 +573,7 @@ private:
   virtual void Action(void);
 public:
   cDvbTuner(const cDvbDevice *Device, int Adapter, int Frontend);
+  void Shutdown();
   virtual ~cDvbTuner();
   bool ProvidesDeliverySystem(int DeliverySystem) const;
   bool ProvidesModulation(int System, int StreamId, int Modulation) const;
@@ -648,12 +649,17 @@ cDvbTuner::cDvbTuner(const cDvbDevice *Device, int Adapter, int Frontend)
   Start();
 }
 
-cDvbTuner::~cDvbTuner()
+void cDvbTuner::Shutdown()
 {
   tunerStatus = tsIdle;
   newSet.Broadcast();
   locked.Broadcast();
   Cancel(3);
+}
+
+cDvbTuner::~cDvbTuner()
+{
+  Shutdown();
   UnBond();
   /* looks like this irritates the SCR switch, so let's leave it out for now
   if (lastDiseqc && lastDiseqc->IsScr()) {
@@ -1865,6 +1871,8 @@ cDvbDevice::cDvbDevice(int Adapter, int Frontend)
 
 cDvbDevice::~cDvbDevice()
 {
+  if (dvbTuner)
+     dvbTuner->Shutdown();
   StopSectionHandler();
   delete dvbTuner;
   delete ciAdapter;

Copy link
Owner

@reufer reufer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me - thanks a lot for your efforts!

@reufer reufer merged commit 0c2c6af into reufer:master Dec 4, 2022
@dr-m dr-m mentioned this pull request Dec 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

README contains dead or stale links to vdr-developer.de
2 participants