Skip to content

Commit

Permalink
Revert "Potential memory corruption problems in audiooutputbase"
Browse files Browse the repository at this point in the history
This reverts commit 81aeb49.

Rationale:
This commit creates problems with recognition of 5.1 audio devices, see the thread
in mythtv-users "Digital audio Capability grayed out" from James Abernathy.

The problem is that the field "Digital Audio Capabilities" is always disabled
(shown as grey and not accessible) independent of the selected audio device.
Correct behavior is that when a digital output such as HDMI is selected that
it is then possible to select the capabilities of that digital output.
This is not possible when the field is disabled.

I see the following two changes in this commit:
Around line 198 the "bugfix: don't allocate".
The original code creates a copy of the complete struct pointed to by m_outputSettingsDigitalRaw; then
the Users are added and that is then saved in m_outputSettingsDigital.
This means the m_outputSettingsDigitalRaw and m_outputSettingsDigital are always different addresses.

The "bugfix: don't allocate" code changes this by copying the pointer value from m_outputSettingsDigitalRaw, then
the Users are added and then it is saved in m_outputSettingsDigital.

This does change two things:
- The m_outputSettingsDigitalRaw and m_outputSettingsDigital now point to the same struct, so the pointer values are the same.
- The m_outputSettingsDigitalRaw has been modified by adding the users, so it is not the Raw data anymore.
Note that if the idea is that m_outputSettingsDigital and m_outputSettingsDigitalRaw point to the same data then one pointer would be enough.
My conclusion is that this must be the change that causes the device recognition problems.

The "bugfix: don't allocate" code then causes problems in the destructor where now m_outputSettingsDigitalRaw
and m_outputSettingsDigital point to the same object and have the same value.
To protect against twice deleting the same memory the code starting at line 93
"// These all seem to be the same pointer, avoid freeing multiple times"
has been added.

To summarize my analysis:
- The "bugfix: don't allocate" code does create the problem in device recognition by copying a pointer instead of copying a struct.
- The fix in the destructor is only needed as a consequence of this.

Reverting the commit and then checking the behavior of the code with valgrind does not show memory corruption problems.
  • Loading branch information
kmdewaal committed Aug 13, 2022
1 parent 26b2e76 commit c370f51
Showing 1 changed file with 7 additions and 12 deletions.
19 changes: 7 additions & 12 deletions mythtv/libs/libmyth/audio/audiooutputbase.cpp
Expand Up @@ -92,18 +92,13 @@ AudioOutputBase::~AudioOutputBase()
"~AudioOutputBase called, but KillAudio has not been called!");

// We got this from a subclass, delete it
// These all seem to be the same pointer, avoid freeing multiple times
VBAUDIO(QString("m_outputSettings != m_outputSettingsDigital : %1").arg(m_outputSettings != m_outputSettingsDigital));
if (m_outputSettingsDigital != m_outputSettings)
delete m_outputSettings;
delete m_outputSettingsRaw;
if (m_outputSettings != m_outputSettingsDigital)
{
if (m_outputSettingsDigitalRaw != m_outputSettingsDigital && m_outputSettingsDigitalRaw != m_outputSettings)
delete m_outputSettingsDigitalRaw;
delete m_outputSettingsDigital;
delete m_outputSettingsDigitalRaw;
}
VBAUDIO(QString("m_outputSettingsRaw != m_outputSettings %1").arg(m_outputSettingsRaw != m_outputSettings));
if (m_outputSettingsRaw != m_outputSettings)
delete m_outputSettingsRaw;
delete m_outputSettings;

if (m_kAudioSRCOutputSize > 0)
delete[] m_srcOut;
Expand Down Expand Up @@ -197,9 +192,9 @@ AudioOutputSettings* AudioOutputBase::GetOutputSettingsUsers(bool digital)
else if (m_outputSettingsDigital)
return m_outputSettingsDigital;

//bugfix: don't allocate as GetOutputSettings will do it
//auto* aosettings = new AudioOutputSettings;
AudioOutputSettings* aosettings = GetOutputSettingsCleaned(digital);
auto* aosettings = new AudioOutputSettings;

*aosettings = *GetOutputSettingsCleaned(digital);
aosettings->GetUsers();

if (digital)
Expand Down

0 comments on commit c370f51

Please sign in to comment.