Removed raw array pointer types for std::vector by tcoyvwac · Pull Request #4341 · mixxxdj/mixxx

tcoyvwac · 2021-09-30T11:18:14Z

The changeset's style tries to match like-for-like the current code(base).

Note: m_pFifoChunkBuffer was actually an array pointer; was not deleted safely. Refactor fixes memory leak.

An exception was made for pBuffer in util/sample.cpp. Refactoring will be done later.

uklotzde

Thanks for starting this cleanup.

uklotzde · 2021-09-30T11:54:49Z

-        delete[] m_pRightTempBuffer;
-        m_pLeftTempBuffer = new CSAMPLE[halfLength];
-        m_pRightTempBuffer = new CSAMPLE[halfLength];
+        m_pLeftTempBuffer = std::vector<CSAMPLE>(halfLength);


Using resize() is sufficient here and better reveals the intention.

uklotzde · 2021-09-30T11:55:47Z

-    m_outputL = new float[MAX_BUFFER_LEN];
-    m_outputR = new float[MAX_BUFFER_LEN];
-    m_params = new float[pManifest->parameters().size()];
+    m_inputL = std::vector<CSAMPLE>(MAX_BUFFER_LEN);


These assignments should be replaced by direct initialization.

I think I understand your suggestion but could you help me understand your term "replaced by direct initialization"?

Do you mean to express, "replaced by using the constructor's member initialisation lists (directly)"? Currently, the objects are being assigned via "direct initialization" using your term in this context.

Add m_inputL{MAX_BUFFER_LEN} to the initializer list instead of default constructing and then re-assigning it.

uklotzde · 2021-09-30T11:56:40Z

-    m_default = new float[numPorts];
-    lilv_plugin_get_port_ranges_float(m_pLV2plugin, m_minimum, m_maximum,
-                                      m_default);
+    m_minimum = std::vector<float>(numPorts);


Also, direct initialization?

I think I understand the suggestion, however could you confirm please?

uklotzde · 2021-09-30T11:57:26Z

@@ -153,9 +153,6 @@ LV2Manifest::LV2Manifest(const LilvPlugin* plug,
 }

 LV2Manifest::~LV2Manifest() {


Please either delete empty destructors or replace them with ...() override = default; in derived classes. If needed move the = default; part into the .cpp file.

Rule of Zero is preferred as it is less code to maintain. Happy to remove all empty destructors as first choice. 😸
If the = default is inline, shouldn't be rather in the header declaration? (Remember, = default nearly always adds a hidden noexcept to its declaration.)

If inlining the destructors is possible than yes.

Destructors should never throw in the first place. Fallible shutdown procedures have to be implemented outside of destructors as separate functions.

uklotzde · 2021-09-30T11:58:57Z

    // Actual encoder init is done below.
    aacEncOpen(&m_aacEnc, 0, m_channels);
-    m_pAacDataBuffer = new unsigned char[kOutBufferBits * m_channels]();
+    m_pAacDataBuffer = std::vector<unsigned char>(kOutBufferBits * m_channels);


Use resize() to avoid repeating the type?

uklotzde · 2021-09-30T12:08:44Z

    CircularBuffer(unsigned int iLength)
            : m_iLength(iLength),
-              m_pBuffer(new T[m_iLength]),
+              m_pBuffer(std::vector<T>(m_iLength)),


m_pBuffer{m_iLength}

The "Uniform Initialization Syntax" is not entirely "uniform"...

Both m_pBuffer(m_iLength) and m_pBuffer{m_iLength} work for me. Did you try it? Please be more specific.

Comment also applies to all other occurrences.

uklotzde · 2021-09-30T12:09:14Z

  public:
    explicit FIFO(int size)
-            : m_data(NULL) {
+            : m_data{std::vector<DataType>(size)} {


m_data{size}

😿 sadly...

uklotzde · 2021-09-30T12:11:08Z

        // now.
    }

    virtual ~CircularBuffer() {


virtual ~CircularBuffer() = default;

uklotzde · 2021-09-30T12:11:31Z

-    m_pFilter = new double[m_iFilterLength];
+        : m_iFilterLength(kiRotaryFilterMaxLen),
+          m_iFilterPos(0),
+          m_pFilter(std::vector<double>(m_iFilterLength)),


m_pFilter{m_iFilterLength}

uklotzde · 2021-09-30T12:11:42Z

    }
 }

 Rotary::~Rotary() {


delete the obsolete destructor

Swiftb0y

Changing from bare arrays to std::vector comes with a penalty on memory consumption, because vector allocates more elements than required so it doesn't need to reallocate immediately on insertion.

Swiftb0y · 2021-09-30T11:37:00Z

+    m_inputL = std::vector<CSAMPLE>(MAX_BUFFER_LEN);
+    m_inputR = std::vector<CSAMPLE>(MAX_BUFFER_LEN);
+    m_outputL = std::vector<CSAMPLE>(MAX_BUFFER_LEN);
+    m_outputR = std::vector<CSAMPLE>(MAX_BUFFER_LEN);
+    m_params = std::vector<float>(pManifest->parameters().size());


std::array is probably the better choice here.

I don't really know actually. If we rely on the fact that we don't reallocate later, we should use std::array. If we rather want to save some memory, std::vector might be the better choice.

If the size is known at compile time then we could use std::array.

Well, in this particular case, its known because its a #define. However, I don't know if thats because we actually know that there is only ever going to be exactly MAX_BUFFER_LEN elements, or if we just allocate this many elements statically to make sure we don't have to reallocate in RT code.

MAX_BUFFER_LEN is an ugly quick hack. The buffers should be allocated based on the buffer size configured in the preferences and reallocated when that is changed. That is outside of the scope of this PR, but for this PR we should not rely on the buffer size being known at compile time.

Well its standard practice in microcontroller-realm where there is no heap-allocation. I assume this code was written by daniel which is why he has less issues with this method than we have.

Swiftb0y · 2021-09-30T11:52:13Z

-        outputBuf.bufs = (void**)&m_pAacDataBuffer;
+        outputBuf.bufs = reinterpret_cast<void**>(m_pAacDataBuffer.data());


I'm unsure whether this is legal, but I'd need someone else to have a look at it. Would static_cast also work?

Good catch! That's why reinterpret_cast is dangerous. The & got lost!

outputBuf.bufs = &static_cast<void*>(m_pAacDataBuffer.data());

So note, .bufs datatype is void**. This means one is already in the "danger zone" 👀 !

static_cast<T>, (&static_cast<T>) only works for datatypes it can "safely" convert to.
With void** it is not possible to assume "safety". So static_cast<T> cannot work in this realm. You need to use reinterpret_cast<void**> to make "the magic" happen. ✨ 🔥

Nevertheless, m_pAacDataBuffer.data() is the wrong pointer. The API expects an array of pointers to multiple buffers and not just a pointer to a single buffer, i.e. double indirection!

I agree with you. So unsigned char* m_pAacDataBuffer; and .bufs = (void**)&m_pAacDataBuffer; was how it was declared and assigned previously.
In your opinion which one would be preferable, could you help me decide?

// [1] .bufs = reinterpret_cast<void**>(std::addressof(*std::begin(m_pAacDataBuffer))); // [2] .bufs = reinterpret_cast<void**>(std::addressof(*m_pAacDataBuffer.data())); // [3] .bufs = reinterpret_cast<void**>(&(*m_pAacDataBuffer.data())); // [4] .bufs = reinterpret_cast<void**>(std::addressof(std::begin(m_pAacDataBuffer)[0]));

How about this safe variant that does not require any explicit casts:

void* outputBufs[] = {m_pAacDataBuffer.data()}; outputBuf.bufs = outputBufs;

Ya, a double implicit way seems more elegant to solve this one. Will go with this. 😺

The version with the temporary array also reveals the actual semantics.

Swiftb0y · 2021-09-30T12:03:46Z

+            : m_data{std::vector<DataType>(size)} {
        size = roundUpToPowerOf2(size);
        // If we can't represent the next higher power of 2 then bail.
        if (size < 0) {
            return;
        }
-        m_data = new DataType[size];
        PaUtil_InitializeRingBuffer(
-                &m_ringBuffer, sizeof(DataType), size, m_data);
+                &m_ringBuffer, sizeof(DataType), size, m_data.data());


the PA_RingBuffer and the vector now have differing sizes which some other code might rely upon.

Swiftb0y · 2021-09-30T12:05:53Z

+          m_pFilter(std::vector<double>(m_iFilterLength)),
+          m_dCalibration(1.0),
+          m_dLastValue(0.0),
+          m_iCalibrationCount(0) {
    for (int i = 0; i < m_iFilterLength; ++i) {
        m_pFilter[i] = 0.;
    }


Suggested change

m_pFilter(std::vector<double>(m_iFilterLength)),

m_dCalibration(1.0),

m_dLastValue(0.0),

m_iCalibrationCount(0) {

for (int i = 0; i < m_iFilterLength; ++i) {

m_pFilter[i] = 0.;

}

m_pFilter(std::vector<double>(m_iFilterLength, 0.0)),

m_dCalibration(1.0),

m_dLastValue(0.0),

m_iCalibrationCount(0) {

we can get rid of the manual initialization loop if we rely on the constructor to do this.

m_pFilter{m_iFilterLength, 0.0}

"uniform" != uniform in c++ world of fun...

Swiftb0y · 2021-09-30T12:08:40Z

-    m_imageData = new uchar[size * size * 4];
-    m_qImage = QImage(m_imageData, size, size, 0, QImage::Format_ARGB32);
+    m_imageData = std::vector<unsigned char>(size * size * 4);


uchar is a Qt typedef which is also used by their API. While its technically just an unsigned char, they could decide to change this in the future which would then break our code. Therefore I'd stick to using uchar.

Swiftb0y · 2021-09-30T12:12:27Z

-    if (m_imageData != nullptr) {
-        memset(m_imageData, 0, sizeof(uchar) * m_iSize * m_iSize * 4);
-    }
+    m_imageData = std::vector<unsigned char>(m_iSize * m_iSize * 4);


Suggested change

m_imageData = std::vector<unsigned char>(m_iSize * m_iSize * 4);

m_imageData.clear()

just empty the vector, doesn't reallocate but leaves the capacity unchanged.

That's wrong. You are only allowed to access the elements up to the (initialized) size of the vector. The elements between size and capacity must not be used. Clearing the vector would set its size to 0.

good catch, then this should be valid:

Suggested change

m_imageData = std::vector<unsigned char>(m_iSize * m_iSize * 4);

m_imageData.clear()

m_imageData.resize(m_iSize * m_iSize * 4);

.clear() sets the size to 0 and then .resize defaults constructs all the elements. heap-allocation and deallocation is still avoided.

clear() is redundant, just resize() with initialization

If I'm reading cppreference correctly, when the new size is smaller than the current size, the vector just gets truncated, if its larger, only then will the space be initialized with the elements. So when the new size is equal to the old size, the resize operation is a no-op and will leave the old items in the container.

Oh, you are right! If re-initialization of all elements is required then resize() alone doesn't work as expected.

But then we should better wrap consecutive clear() + resize() calls into a helper function and document it properly. If it is only used once an inline comment is sufficient.

tcoyvwac · 2021-09-30T12:49:55Z

Thanks for all the feedback / comments, and joint agreement to handle raw dynamic elements with more automatic datatypes. Will be adding your suggestion very soon.

Some notes:

I have my preferences on the code / PR changeset, but all initial PR changesets submitted will try to "match" the previous codebase's "style" as much as possible on first submission.
Some out-of-place changeset edits are because of pre-commit. They are stylised changes just committed to pass the checks.
...And so, if there is something about the operational structure of the code spotted in those stylised changesets, they will not be worked on in this Pull Request unfortunately (to keep PR topic-scope small).
But please leave a comment on the operational code you could see improved! 😸 It is a welcomed "TODO" note for all.

Be-ing · 2021-09-30T13:03:50Z

Thank you. Plain C arrays are a dangerous, confusing mess.

Be-ing · 2021-09-30T13:29:38Z

@tcoyvwac are you on Zulip? Since you seem to like doing code cleanup, it would be a really big help if you work on fixing compile errors with Qt6. You can work on that by configuring CMake with -D QT6=ON. I recommend to make a separate CMake build directory for Qt6 so you don't need to reconfigure switching between Qt6 and Qt5.

tcoyvwac · 2021-10-03T12:45:35Z

When all the PR comments are basically answered / resolved, will rebase all the commits into one so it can be ready to be merged. For now, just making a separate commit for edits so it is easier to read the PR.

uklotzde · 2021-10-05T22:59:26Z


 void EncoderFdkAac::processFIFO() {
-    if (!m_pInputFifo || !m_pFifoChunkBuffer) {
+    if (!m_pInputFifo || !m_pFifoChunkBuffer.data()) {


m_pFifoChunkBuffer.empty()?

uklotzde · 2021-10-05T23:00:48Z

        AACENC_BufDesc outputBuf;
        outputBuf.numBufs = 1;
-        outputBuf.bufs = (void**)&m_pAacDataBuffer;
+        void* dataBufs[] = {m_pAacDataBuffer.data()};


chunkBuffer vs. dataBufs? Please try to keep the naming consistent.

uklotzde · 2021-10-05T23:01:48Z

            : aw(config(), QSqlDatabase()),
-              bigbuf(nullptr),
-              canaryBigBuf(nullptr) {
+              bigbuf(),


No explicit default constructor calls required.

uklotzde · 2021-10-06T13:02:13Z

    CircularBuffer(unsigned int iLength)
            : m_iLength(iLength),
-              m_pBuffer(new T[m_iLength]),
+              m_pBuffer(std::vector<T>(m_iLength)),


Both m_pBuffer(m_iLength) and m_pBuffer{m_iLength} work for me. Did you try it? Please be more specific.

Comment also applies to all other occurrences.

uklotzde · 2021-10-12T22:51:28Z

  public:
    explicit FIFO(int size)
-            : m_data(NULL) {
+            : m_data(size) {


The size of m_data now differs from size. size has to be adjusted before allocating the buffer.

Just noticed that your comment is the same as @Swiftb0y's comment. Sorry Swiftb0y for not resolving it!

💭 One option is a .resize() where m_data is assigned to instead.

💭 Another option would be to write : m_data(roundUpToPowerOf2(size)) to keep constructor initialisation.

Initialize it with the correct size and then use m_data.size() instead of size

uklotzde · 2021-10-12T22:53:33Z

-    if (m_imageData != nullptr) {
-        memset(m_imageData, 0, sizeof(uchar) * m_iSize * m_iSize * 4);
-    }
+    m_imageData.clear();


Please add a comment why clear() is needed before resize()

It isn't needed. Not sure why I added it! Will be removed.

It is required, see #4341 (comment)

As this is inside VinylControlSignalWidget::resetWidget(), would be happy to replace the code in the function with m_imageData = decltype(m_imageData)(m_iSize * m_iSize * 4); (or explicit datatype form: std::vector<unsigned char>(m_iSize * m_iSize * 4);)

Just noticed that the std::vector's internal datatype should be uchar as @Swiftb0y wrote in another comment. Looking at the git-blame for vinylcontrolsignalwidget.h, the previous array was declared as unsigned char. But agreeing with SwiftB0y, will update the std::vector's internal datatype to uchar.

The decltype form is nice because there is need to worry about these stale (datatype) declarations...

Is the resize really needed? According to the legacy code std::fill() is sufficient here. We don't even need to repeat the size calculation which is a welcome side-effect because it reduces redundancy.

Resonate with your questioning around the code @uklotzde. I agree that std::fill would be a good candidate.

This changeset / The PR is just to swiftly wrap raw array pointer objects / types around more "automatic" datatypes (and avoid the purpose of one's datatype's code).
After the objects are more "secure", the "implementation" these objects / types exist in, become more visible to additional changes, for example: std::fill.

m_imageData = decltype(m_imageData)(m_iSize * m_iSize * 4); (or .clear() & .resize(m_iSize * m_iSize * 4)) is normally~ (reasonable confidence) a signature for a std::fill replacement.

I don't understand why we should defer this change until later? memset produces the same outcome as std::fill. You could even preserve the invocation of memset, but the code would be less readable than with std::fill.

Sure, no objections to adding this. 😸
Until explicit permission has been granted to expand the PR to cover things like std::fill, I am trying to make the changeset very conservatively match the current codebase as close and as small of an edited footprint as possible, because it is polite PR etiquette to the Mixxx project.

Happy to add the std::fill changes if you like of course. I would like to merge the PR sooner.

std::fill has now been added to the changeset / PR. ✔️

Be-ing · 2021-10-13T03:11:15Z

Please fix the merge conflict from #4386.

Be-ing · 2021-10-13T13:10:19Z

+            pIn,
+            halfLength);
+    SampleUtil::applyGain(m_pLeftTempBuffer.data(), 32767, halfLength);
+    SampleUtil::applyGain(m_pRightTempBuffer.data(), 32767, halfLength);


After this PR, it would be great to change the SampleUtil functions to not take plain C arrays as parameters.

tcoyvwac · 2021-10-15T14:41:01Z

Are there any more changes needed / requested?

uklotzde · 2021-10-18T11:08:24Z

-    if (m_imageData != nullptr) {
-        memset(m_imageData, 0, sizeof(uchar) * m_iSize * m_iSize * 4);
-    }
+    m_imageData = decltype(m_imageData)(m_iSize * m_iSize * 4);


clear() followed by resize() would not reallocate if not necessary. Did you consider this?

(VinylControlSignalWidget::setSize() exists, so the function resetWidget() is confusing). A std::fill was the first choice because a "resize" is not being done in the function, just a setting of the value 0.
Because maybe adding std::fill to this changeset could be out of scope, your suggestion clears things up ✔️

See my comment below. Let's don't change the behavior of the code by simply using std::fill().

I didn't know std::fill was a thing, seems like the perfect tool here.

uklotzde · 2021-10-18T17:59:03Z

-    if (m_imageData != nullptr) {
-        memset(m_imageData, 0, sizeof(uchar) * m_iSize * m_iSize * 4);
-    }
+    m_imageData.clear();


Is the resize really needed? According to the legacy code std::fill() is sufficient here. We don't even need to repeat the size calculation which is a welcome side-effect because it reduces redundancy.

Notes: * m_pFifoChunkBuffer was actualy an array pointer; was not deleted safely. Refactor fixes memory leak. * Used void*[] implicit conversions for protobuf. * Used "auto _ = std::vector{}" style for some assignments. * Default'ed empty virtual destructors and fully removed other empty destructors from codebase. * Used std::vector<T>.resize() in place of raw-new.

uklotzde

Thank your for all the efforts and discussions! I guess this is ready now. LGTM

If anyone else wants to take a final look please do and then merge. Otherwise I will merge it later.

tcoyvwac · 2021-10-20T00:08:28Z

Thank you very much for your understanding and helpful advice everyone!
Thank you for accepting the PR & merging! 😸 🎉

Will also be fixed by mixxxdj#4341.

github-actions Bot added the code quality label Sep 30, 2021

uklotzde suggested changes Sep 30, 2021

View reviewed changes

Swiftb0y reviewed Sep 30, 2021

View reviewed changes

daschuer mentioned this pull request Sep 30, 2021

Vectorize two loops in the LV2 process function #4343

Merged

tcoyvwac force-pushed the fix/remove-raw-pointer-types branch from 7964ada to 0da929d Compare October 3, 2021 12:41

tcoyvwac force-pushed the fix/remove-raw-pointer-types branch from 0da929d to dcb28bd Compare October 3, 2021 12:48

uklotzde suggested changes Oct 6, 2021

View reviewed changes

tcoyvwac force-pushed the fix/remove-raw-pointer-types branch 5 times, most recently from 62c0fe6 to 8f89ba9 Compare October 10, 2021 18:50

uklotzde mentioned this pull request Oct 12, 2021

EncoderFdkAac: Fix memory leak #4386

Merged

uklotzde suggested changes Oct 12, 2021

View reviewed changes

tcoyvwac force-pushed the fix/remove-raw-pointer-types branch from 8f89ba9 to e0a123b Compare October 13, 2021 12:43

Be-ing reviewed Oct 13, 2021

View reviewed changes

tcoyvwac force-pushed the fix/remove-raw-pointer-types branch 3 times, most recently from 21748f3 to 3dfdf29 Compare October 14, 2021 09:47

uklotzde reviewed Oct 18, 2021

View reviewed changes

tcoyvwac force-pushed the fix/remove-raw-pointer-types branch from 3dfdf29 to 9468ee5 Compare October 18, 2021 13:17

uklotzde suggested changes Oct 18, 2021

View reviewed changes

tcoyvwac force-pushed the fix/remove-raw-pointer-types branch from 9468ee5 to 242e631 Compare October 19, 2021 09:39

uklotzde approved these changes Oct 19, 2021

View reviewed changes

uklotzde merged commit 070a475 into mixxxdj:main Oct 19, 2021

tcoyvwac deleted the fix/remove-raw-pointer-types branch October 20, 2021 00:08

uklotzde mentioned this pull request Oct 21, 2021

Improve description why we don't want to rebased PRs #4464

Merged

JaviVilarroig pushed a commit to JaviVilarroig/mixxx that referenced this pull request Oct 26, 2021

EncoderFdkAac: Fix memory leak in destructor

284819b

Will also be fixed by mixxxdj#4341.

		@@ -153,9 +153,6 @@ LV2Manifest::LV2Manifest(const LilvPlugin* plug,
		}

		LV2Manifest::~LV2Manifest() {

		outputBuf.bufs = (void**)&m_pAacDataBuffer;
		outputBuf.bufs = reinterpret_cast<void**>(m_pAacDataBuffer.data());

	m_imageData = std::vector<unsigned char>(m_iSize * m_iSize * 4);
	m_imageData.clear()

	m_imageData = std::vector<unsigned char>(m_iSize * m_iSize * 4);
	m_imageData.clear()
	m_imageData.resize(m_iSize * m_iSize * 4);

Uh oh!

Conversation

tcoyvwac commented Sep 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

uklotzde left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcoyvwac Oct 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcoyvwac Oct 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Swiftb0y left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Swiftb0y Sep 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uklotzde Sep 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcoyvwac Oct 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

tcoyvwac commented Sep 30, 2021 •

edited

Loading

tcoyvwac Oct 1, 2021 •

edited

Loading

tcoyvwac Oct 1, 2021 •

edited

Loading

Swiftb0y Sep 30, 2021 •

edited

Loading

uklotzde Sep 30, 2021 •

edited

Loading

tcoyvwac Oct 1, 2021 •

edited

Loading