Month 5 - Week 2 - Final Touches

 Last week, we discussed finishing the WAV file reader and getting the overall status for it marked as completed. There were some final changes. Moving over to GFile for UWP was a big step, but there was still some branching in the code, primarily due to Linux and Windows differences. I decided to focus my energy on getting these rounded out and fixed. This will be a light post, and step through everything fairly quickly.

The thing to really focus on and correct is how Linux and Windows structure the WAV header. Internally, Linux structures a WAV fairly simple, and this is due to it not requiring the extra data that was later attached when Microsoft extended the WAV format. Windows contains many things, however. It contains the primary information which is the same as Linux, but a union that holds additional information. This is also paired with a GUID. The purpose of the union and GUID is unknown to me, but to bridge compatibility with platforms, I had to move away from a platform-specific definition and move to an internal definition, which is exactly what I did.

If you have a keen eye with the Windows API, you will notice immediately that this new structure setup for WavReader is the exact same as Windows's definition. I did this for a simple reason: to support Windows and allow support for Linux. If I used the Windows definition, it wouldn't be able to compile for Linux due to obvious reasons (Windows API not being available for other OSes). After moving to the new structure, it was now the process of hooking everything up and removing old branching.

After making these changes, now WavReader no longer branches and works on every platform universally. Additionally, it acts as a wonderful introduction to what is possible with the library, as all of the heavy lifting for data handling is done by library code (GFile). So a lot of beginners just getting into Gateware could learn a fair bit by reading through the source code of WavReader.

This marks this merge request as complete. I marked my name in the list of contributors, and let the merge get reviewed again. It was merged. The last thing I did for audio was make two merge requests, one focusing on some simple Windows refactors, and the other focusing on UWP refactors. Effectively syncing both to the same modern standard as Linux. In total, everything is now completed.

What now? Now I find a new issue I can tackle, a bug or possible feature that will take only two weeks to implement. With my time almost finished with Gateware, I'm no longer able to tackle such large issues as I have in the past, so that will be left up to others. However, I will try to provide supporting changes here and there, and see what I can do with the time I have left for finals.

Month 5 - Week 1 - Finishing the WAV File Reader

 In the previous week, we discussed and showed the introduction to the WAV file reader I'm implementing. The goal of this reader is to allow WAV files to be universal in logic while retaining the cross-platform nature of supporting not just Linux or Windows, but UWP as well. I'm excluding Mac from this as it already has a native WAV reader provided, so we don't have to implement this, thankfully. Now, I showed the very early stages of this file reader, specifically its Linux and Windows implementation, and what was needed for that. Well, over this week, we added UWP support and a lot of work had to go into that. There were also changes made to the unit tests to support the new UWP usage, among many other things. There's a lot, but I will go over them as quick as I can while retaining the main bits of information.

Firstly, UWP's management for file reading was much different from Linux and Windows. Linux and Windows used GFile for most of the file handling and file communication, however, UWP used an internal Windows library that is within the File API. This API had many quirks that I didn't like, but at the time felt it had to be used, resulting in my reader class getting very cluttered upon initial implementation.

As you can see, there was a lot of code added, and this code specifically was just for opening the file. Some interesting things to note specifically with UWP's file handling:

1. All files are stored as a handle that we pass to functions to perform actions we want. This isn't too unknown and is regular, but is normally hidden, meaning the library is even more low-level than regular filesystem code in C++.
2. `SetFilePointer` on the surface looks to be like a form of seeking inside of a file, but is actually lower-level than that, it is a function that is used to make the seek function.
3. All file paths have to be wide char paths, if they are not, it will fail to open outright.

In total, the File API worked with is extremely low-level and lightweight, but also more bare compared to traditional options. this meant more work had to be put in just to get the state of the UWP support 1:1 with the other platforms. Extra frustrating is that UWP didn't like working with GFile, so I had to take note and continue implementing using this interface. 

By the end of the implementation, it bumped the full source file from around 500 lines of code to over 700 lines. Most of the additions were all focused on preprocessor branching, platform checking, and platform-specific code. Not good since GFile is meant to prevent a lot of the IO branching I had to do. During the meeting, the code was shown in a working state on all platforms and was shown to do the job intended, but I brought this issue up, and we discussed fixes. By the end of the meeting, I was assigned to figure out why GFile was failing and fix it. That's exactly what I did.

I spent a long time simply debugging and understanding the issue. I removed all traces of the File API used and used GFile instead. Here are something interesting I found.

So, come to find out, this was a pathing issue. Something that was happening because GFile wants relative pathing to work with, not absolute pathing. What this means is the unit tests have to change since they give absolute paths to the audio files. We don't want that, we want it to work with relative paths. So, we have to update the unit tests.

So after making all the edits, updating some of the code to reduce branching, and removing all the File API code, the tests now work.

There's still some cleaning I have to do to the code itself before I commit, but once that is done, I can rest easy knowing that my file reader is now using GFile completely, and executing its functionality 1:1 with every platform. The last change needed is replacing the Windows structures used currently for the WAV header info with actual full and proper internal structures. This will remove more branching, and separate any dependencies for this WAV reading functionality. Luckily, this shouldn't take me long and should be a relatively smooth process in total. It will just take some time to fully implement. Overall, this is everything I worked on this week.

Month 4 - Week 4 - WAV File Reader

Last week I went over all the changes with the audio that were done. There was a lot that happened, and a lot of different alterations were made, albeit small just to add finalizing touches. With quite some relief, the changes were merged and are now a part of the official release. However, some more modifications can be done, one, in particular, is the implementation of the WAV file reader. In its current state, there are over six individual implementations. Two for each platform (Windows, UWP, and Linux). I want to implement a universal form of this WAV reader and get it synced, so there is only one implementation that handles everything needed. That way, if we support more WAV features, it will sync across all platforms without us having to fight over all 8 implementations just to get it working as intended. We want easy usability for audio, the less complex, the better. Especially since we want to focus on the APIs we use to make the audio happen rather than the small stuff like WAV file reading implementation.

Linux was rather easy since I had just been working on it for the last month and a half, leading on to two months now. There was a lot that I focused on since Linux was probably the most low-level implementation of this WAV file reader. It had to introduce many particular structures to work the same way as Windows. The total implementation once Linux was finished was ~600 lines of code. This was the main chunk of code needed to get the base implementation down.

This is a very small glimpse into the WavReader class, but it shows perfectly what this class aims to do. You can how a lot of the code is condensed and built around making the WAV file reading as simple as possible. The code itself is fast and straightforward, and while I would enjoy simply using `constexpr` for a lot of what this class does, I sadly don't have access to that feature quite yet (however, when we update to C++17, I would like to make this class more optimized). So far, everything is looking good and the implementation across GSound and GMusic both show massive improvements in file size.

Now, we move to Windows. Windows has a lot of architecture built into it by default, so there are small improvements we can make if we are compiling for it. In particular, we can replace some of the data types we have to manually make in Linux.

As you can see, most of the code changes to WavReader from here on will simply modify basic data representation, with most of the functionality staying exactly the same. This allows us to focus more on getting WavReader properly hooked into all GSound and GMusic implementations with little confusion or frustration on how to get it done.

Same as before as well, we see a massive deduction in code once WavReader is implemented for both GSound and GMusic on Windows.

Lastly, UWP. I have just started trying to get its implementation working correctly. I had hoped that GFile and the previous data representation would work for it, however, GFile fails to work correctly for this type of use case. What has to be done now is for me to implement the low-level file handling that WinRT provides so I can read the WAV file data correctly. The reason for these issues, I suspect, is due to UWP using UTF-8/UTF-16 to read file data, making the generic GFile fail when attempting to read the files we need. I'm not sure if this is intended behavior for GFile, but I will be discussing it during the next meeting on Wednesday.

Overall, the UWP changes will be more extensive than the others but should be able to pass once I'm finished updating WavReader to support it. After that, the rest should be pretty straightforward. And with hope, this should be merged by Wednesday. For now, this is all that has been done.

Month 4 - Week 3 - Finalization of the Audio

 Last week we had discussed closely about the audio and the eventual finalization process that would take place to merge the large amount of changes that were done. This week, these changes have been made. The theory I mentioned last week was tested and sadly bared no fruit towards fixing the popping issue. So, instead of focusing more on attempting to fix the problem, I chose to move forward with finalizing the changes and merging those as it still provides fixes to over three issues on the board, each of which was much anticipated. We will quickly go over all three issues that were fixed.

Newer PulseAudio versions results in GMusic3D crashing (#255)

This issue was a tricky one. On the surface it looked to be a change to the library that altered the original functionality, however, this issue was much easier to fix than expected. Instead of internal changes, I inspected the error message that had been printing.

This assertion was hard to break down initially, but it made much more sense over time. The assertion was telling the developer that the thread's pool stat exceeded the allotted size of the stream's buffer. To make it even more simple to state, the stream's buffer reached its maximum allowed limit, and we were attempting to write more than what it could handle on the server side. Because of this deduction of the assertion, I inspected the code deeper and found we weren't polling at all for the server on how much data we were allowed to write. Because of this, I added the functionality, which was much easier than expected.

This little change fixed the issue entirely, removing any need to worry about GMusic failing. This also fixed PipeWire's distortion of music as well. This is primarily because PipeWire, instead of asserting for this situation, it will just overwrite data to fit the new data, which results in the audio sounding distorted or broken to the end user. If we poll like this, the audio sounds completely normal as we expect it to.

Pulseaudio holds onto memory after cleanup (Linux) (#17)

This is an old issue but related heavily to memory leaks that were present in the audio before I started my fixes. This issue was spotted by using Valgrind (a popular memory leak detection tool). It detected several memory leaks in the audio, which resulted in me stepping in with a keen interest in what could be causing it.

I fancy myself as a memory developer. I enjoy these kinds of issues because memory leaks are fun to tackle and understand. They are tricky, but that's why I like them in the end. Due to how sneaky they can be sometimes, I enjoy breaking them into sections and really approach how to fix them. However, this change was a little intense to fix the problem. A lot of the code internally was C++98, because of this, there was a lot of code that was old and not set to modern C++ standards. I decided the fix for this would be replacing the manual `new` and `delete` allocations with the more modern `unique_ptr` usage. This will handle the actual allocation while also managing the deallocation without us worrying about anything slipping through the cracks. The implementation was rather extensive but improved the overall code massively.

There are of course many more changes to the code, but this is just a brief glimpse into the changes made. Overall, I was happy with the results, mainly because of this; if the memory leak is still present, then it is internal to PulseAudio and not with the implementation code.

Pipewire support for PulseAudio is skipping, distorted, and sometimes fails working (#256)

Lastly, we have this behemoth.

There is not one change that fixed this issue, instead, it was a collection of different changes made to PulseAudio that improved the usage which eventually fixed this problem. The implementation in its current state is a 1:1 ratio, meaning that PulseAudio and PipeWire are acting the same. This is good because that means any outside issues are now fixed across both subsystems. Besides the popping on both subsystems, everything is working as expected now.

Final Notes

With everything done, there were over 1,600 lines of code changed. About 780 lines were added with 820 removed, that's a lot of changes. The review took a minute, there were over 103 commits to comb through and lots of changes to go through, but in the end, it was merged successfully. So, what now?

Well, there is one last change I'm doing for both Windows and Linux, a universal WAV file reader. This will lessen the amount of implementations that exist for reading WAV files. I'm mostly done with it now, just have to get the GMusic implementation to work correctly, and then we'll be golden to replace full implementations. I would provide screenshots for this, but I forgot to actually push these changes to GitLab, so they're still on my local Linux sadly, and I'm writing this blog post on my Windows. I will be providing the solution I have written up next week however once everything is fully completed, which should be by tomorrow (I would've had this done sooner, but personal life took away my ability to write it up quick enough).

Beyond that, the next issue will be the ghost window issue for Mac which will more than likely take all my focus for the rest of this month and next month. I'm hoping learning Objective-C won't be too painful, but I know XCode will have its quirks that I'll have to get used to.

Overall, this was everything done over the last month and a half, and I'm looking forward to tackling some new issues that are unrelated to audio, as, if I'm being honest, audio has been tiring. Doing a different issue would be like tasting a different ice cream flavor, and I'm looking forward to that, but I have a job to do, and I plan to at least do it right. With that, this is all I have to say for now. Till next week.

Month 4 - Week 2 - Health Issues and Finalizing the Audio Fix

 This merge request has been a beast, and sadly, the ending is not the one I would've preferred. However, a lot of time was spent on this, and a lot of effort was poured into it, so even though I couldn't get everything I wanted, the outcome is still substantially better than before.

Let's provide a little preface before we continue. My blog post this week will be very brief compared to my traditional blog posts. I've been dealing with some pesky health issues this week that have affected a lot of things for me, from food consumption to digestion issues to general body weakness, it's been a lot. To add onto this, it has also spurred a rather severe form of insomnia so my ability to work effectively has taken a massive toll as compared to normal. You can see this by the timestamps on my commits.

Safe to say, I'm in a bit of a rut right now, but with hope, I'll be back to normal soon and will be able to pour my best effort into continuing to fix bugs and get some of the issues present on the board taken down.

Now, onto the changes with this merge request, as the outcome of it has changed. Sadly, two issues will still be present after this issue closes. Over the last couple of weeks, I've been pouring in constant effort to attempt to fix this popping audio bug, however, any fix I provide doesn't get it. I've done many different solutions, from new drain flags to partitioning how much data is sent to the server, all the way to even corking the stream before flushing the buffer in an attempt to rid it of popping when the audio stops. Even with all of these changes, nothing seems to work, and the issue still persists. Even more frustrating is that the PipeWire support is affected by some of these edits, particularly the drain flag and corking. Both "fixes" result in the audio cutting off early or sounding more distorted for PipeWire, which leads me to believe that those solutions aren't correct and won't help the overall PipeWire/PulseAudio compatibility I'm seeking for the library.

So, with that, those changes have been reverted.

The last attempt I could throw is writing over what the polling write size gives for the server side of PulseAudio. This could fix it because this is exactly what GMusic does. Since GMusic relies on the streaming buffer size, PulseAudio has to support this buffer size on the server side. Something else to note is when I was making PulseAudio print out the write size it allowed initially, it supported only up to the sample rate. If you need to get more familiar with audio, the sample rate is how many samples make up a single second in audio. So PulseAudio was only allowing 1 second of audio to be written. This is a little problematic as writing just a second could cause skipping or more issues since the code to write to the stream itself could take longer than a second. GMusic doesn't suffer from this, however, since it writes the streaming buffer size, which is around ~65,000 bytes. This gives GMusic more than enough time to process and calculate what it needs to before it needs to write to the stream again. Pretty useful.

Possibly, as a theory, this is what GSound is suffering from. However, I won't be sure until I test it.

That will probably be my last attempt to fix this issue. By Monday, I will be finalizing the refactoring I've done, and the fixes I have made, before merging it completely. Even though I could fix the issue initially targeted, I was able to greatly modernize the code and bring it up to speed with C++11. I also improved much of the memory safety with the usages of `unique_ptr`, though this safety to performance won't be felt until C++17 for library developers, since `constexpr` is not introduced for `unique_ptr`s until C++14/17. Thankfully, those using the library can enjoy high performance and memory safety since this is a single header library, thus it will only compile when the user builds their application (which tends to default to C++14/17 now).

In total, there were over 93 commits, and almost 1,000 lines of code changed.

This took a lot of effort and focus to get all the changes implemented, and overall, I'm happy with the results. Though, as I mentioned, I would've liked to have seen this popping issue fixed. I have that last theory I can test, but if it doesn't fix the issue, it will have to sit unsolved for a little while longer.

While this blog post doesn't go over any problems I faced and acts more as an overview of the last month and a half of work I've done on this single interface, I feel it is worth writing all of this. Not just for me, but for others who may have to approach my code in the future. This way they know what I've tried and tested. It will help check off a lot of things from the list, and help them with diagnosing and coming up with solutions. I'm fine with that.

So what am I doing after this merge request? Well, I'm going to be focusing on implementing a universal WAV file reader, that incorporates a more C++-compliant structure to everything, as well as providing proper safety checks for everything. There were several safety checks that the old system didn't have.

There are also a lot of odd choices in terms of how the file is read for each implementation, particularly with GSound and GMusic. One will read all the data, and the other will read the file twice (if I'm not mistaken). Once for the header information, and another time to find the stream data. I find this odd and feel there could be a class for this, something that you can control to simply load all the data for you, or get a nice index/pointer to the data.

Besides that, the next issue to focus on after the quick implementation of the WAV file reader will be the long-awaited ghost windows that are plaguing Mac. I'm not sure how well I'll do tackling it, as I have no experience in Objective-C, but I should get the hang of everything relatively quick. At the end of the day, any C language compiles to the same assembly, so I should be okay.

With that, this is all I have to give for the blog post this week. I wish I could've done more, but my health has been in constant waves this entire week, so I'm having to balance everything lightly. I hope this provides some good insight into everything that has been done.

Month 4 - Week 1 - Diagnosing The Popping Sound

I will take the time to preface this blog post by saying there will not be a lot of information within it. This is mainly due to some personal health issues that suddenly sprang up earlier in the week, so I was a little tight on time in terms of what kind of work I could do. However, even with this limitation, I still want to be able to do some work and try to figure out some of the issues that seem to be occurring with PulseAudio.

The first thing I will do is a brief breakdown of the issue. This is pretty easy. When the audio is playing, there are occasional popping sounds that can be heard when the audio ends. On PipeWire, the audio sounds a little worse as a static sound can form within the audio occasionally. PipeWire's PulseAudio support is pretty great and isn't known for having issues like these, so this points to our implementation being error-prone or heavily misused from how PulseAudio should be used.

Unlike my other posts, I don't have a fix for this currently, so instead I will be listing the fixes that I have tried thus far and show what has been done in an attempt to get this issue solved.

Volume Misuse

One theory I had was possible volume issues as some of the tests that caused popping manipulated the volume, this includes some of the surround sound tests and all of the 3D audio tests. However, I can't be certain of this because all of the math involved with it looks to be correct. Though, there could be a misuse of functions inside the volume controls. I may look into this further.

Multiple Drain Calls

Another possible issue is that `pa_stream_drain()` is called repeatedly whenever GSound reaches the end of the buffer. This function is inherently supposed to only be called once. So, maybe the repeated calls to it are caused by the popping sound. I made a simple fix for this, but the results showed no difference. However, I may still implement the change just to have cleaner interfacing with PulseAudio. The fix simply incorporates an atomic flag for draining to prevent it from being called repeatedly.

Not Corking Stream Before Flushing

This is more of a critique of my own fix. As we discussed last week, there were some changes I made to fix some issues I found in the code. One of those is the erroneous use of `pa_stream_cancel_write()`. The fix for this was using `pa_stream_flush()` instead, but this could also cause a possible popping sound. This is because when you call the function, it flushes the buffer and empties it entirely. What this means is that the buffer will effectively lose all the data it has. So, if by some means, it was halfway through a sample when it was flushed, it could cause a popping sound. The fix for this would be corking the stream before flushing it. I tried this fix and did notice a difference, but found the result to sound a little weird. Some samples would end a little earlier than expected, and the popping, most of all, was still present.

Incomplete Samples Written to the Buffer

I won't be spending a lot of time explaining this. The basic idea is that when we write to the stream's buffer, we may be writing an incomplete sample due to not sizing based on how large the samples are. This, however, is not how the buffer works. The stream's buffer should never reach the end where incomplete samples are, solely because the buffer is treated like an hourglass. Imagine the buffer, with all of the data, as an hourglass. You have the sand at the top, our data, and the middle where the glass is pinched is what's playing, with the bottom being the already played data. The sand is going to slowly drain into the bottom at the pace of the sample rate. So when the sand at the top starts getting low, we can just add more sand to the top so it can continue playing. This might be a bit of an oversimplification, but this is generally how most audio libraries handle stream data. Because of this, this "issue" isn't really a problem. As long as we are adding sand at the correct pace, there shouldn't be any popping or skipping of the audio.

Conclusion

I wish I could have included more detail or even a fix this week, but this problem has proved to be a rather large headache that is difficult to locate. I'm starting to believe that all of these different issues that I've spotted may be connected to each other thus making the popping sound happen. So possibly fixing all of them could fix the popping issue itself. I will be spending next week making major changes to the code to get it more structured and grounded for PulseAudio to effectively work with it. There is also the topic of making a universal WAV file reader implementation that I still have to do, however, I have been a little distracted trying to get this problem sorted out. This is primarily because if I fix this issue, then PulseAudio popping is fixed and PipeWire support is fully functional, meaning two issues are handled in one. The problem is just trying to get it fixed.

With that, these are all the changes I have made so far, I will hopefully be providing a larger update next week showing what solution I came up with. And that will also include fewer health issues I hope.

Month 3 - Week 4 - Fixes to Many Bugs in Audio

Last week we touched upon the audio system. There was a lot that needed to be focused on, and a lot that happened after the unique pointer change. One of the main adjustments was further refactoring. I wanted to set a key point in trying to modernize the code somewhat, as a lot of it was written around the time the library used C++98. For this blog post, however, I'm not going to be focusing on all of the changes I did. I'm instead going to be a little bit more targeted and focus specifically on certain issues that I was working on. Some of these involved library code changes, other bits involved fixing issues that were found present in the code, and a lot of it involved research into understanding what some of these functions were doing in more detail. So let's start with the easiest.

Fixing Erroneous Function Calls with PulseAudio

One of the first things I really started focusing on was researching in more detail what these function calls were actually doing. I had an idea of what the underlying PulseAudio system was like, that the library itself communicated with the PulseAudio server that ran on the Linux operating system. This is rather easy to understand about Linux if you've worked in the space long enough. It's not as simple as how Windows works, where you're more or less interfacing with a driver, this is a little bit more direct in that it doesn't communicate with the drivers but more or less handles how the audio will be outputted to the drivers. With this distinction in mind, we have to imagine that we are a client that is communicating with the server whenever we are handling audio. Thus all of these function calls have to send a packet to the server, and then receive that packet with additional information on what's happening. I wanted to understand how some functions worked, particularly, a function that I had a curiosity about. This function was `pa_stream_cancel_write`.

Whenever you call to write data to the stream on the server, you have a lot of control over how this data can be written, transferred, and even used. Now what this function is supposed to do is that it is supposed to cancel the write operation. However, from reading the documentation, this function has to be called before the write is fully called. This function is only used with a particular function, `pa_stream_begin_write`. There is a problem with this, we never call that function. So what does the cancel operation do? Well, it does nothing. So when we call to stop the audio, we actually don't stop it at all, we just call to a function that doesn't do anything.

I looked deeper into this, and then started sourcing alternatives, eventually landing on the function `pa_stream_flush` which will stop the playback and empty the buffer fully. This might be what stop is expected to do, however, if not, we can use the cork function to instead pause the playback. The change for this was rather important and definitely made calls to stop much clearer.

Fixing Looping Failure Inside GMusic

This issue was a little bit more involved. There are normally two models you can roll with when doing write operations to PulseAudio. The first is asynchronous, and the second is polling. There was a problem with GMusic, it didn't use either of these models. So what would happen to cause the looping failure was that GMusic was constantly writing to the PulseAudio stream buffer, on the server side. Eventually, due to the sample song being large enough, it crossed the maximum allocation limit allowed for the stream, and the server would assert a failure that the client would receive. This was the failure that was being reported out of `pulsecore` which I mentioned last week. The fix is pretty clear, we just need to adapt one of these models for GMusic. The easiest, since we are already threading, would be polling. All we need is to know the size that's available to write to in the PulseAudio server. We can then wait until the server has a size available, and then write data to it. It doesn't have to be exact, it just needs enough room that it can write the data effectively.

This fix was surprisingly very easy.

Just with these small additions, now GMusic can play audio without overloading the buffer. This also had another benefit. Due to this change, PipeWire can now play the audio perfectly fine. There is still static and popping, but that is a result of another issue that has to be worked on. What this means, however, is that PipeWire support may be entirely possible here soon. However, I'm going to need more time to implement the changes needed for this.

Furthermore, due to both of these fixes, I've also included in the merge request that both of the new issues made around the audio will be fixed upon merge. This means that this merge request will now close a total of 4 issues in one merge, which is something I'm rather looking forward to.

Beyond these two major fixes, there was a plethora of refactoring changes that were done. Most of these work to make the code more readable and understandable. An example of this is the picture below.

Changes like these will not only make the code easier to get into but also make reading it more pleasant. This change was larger than my previous refactors, but it sought to fix the amount of nesting and confusing branch control calls (`break`, `continue`, etc.) that were being done.

Most of this week was devoted entirely to researching and understanding the PulseAudio library. There was a key focus on understanding this before I made any major functional changes. I wanted to ensure that my fixes wouldn't only stand the test of time afterwards, but also be reliable overall for users that are working on the Linux side of code.

With that, this is what I've done this week. It wasn't as intense as the previous weeks, but I found the progress to be more than fruitful for the overall stability of the audio system on Linux.

Month 3 - Week 3 - Fixing the Event System and Continuing with Audio Fix

Last week we went into the discussion of all the different things that were being pushed for merging, different issues encountered when refactoring the audio code for the bug fix, and then ending it with the return of the event system issue that was starting to plague throughout the Debian build of the library. This week, we will discuss the changes to the event reporting system and what I did to fix this bug.

This was a rather frustrating issue to tackle as I ended it on the complicated matter of figuring out how to locate a way to implement the fix. I was originally working on the idea that since it was X11 causing the problems, I could check which version of X11 was running and then build a solution for that version and the ones occurring after it. However, this solution wasn’t a clean one. What if the check fails at some point? What if a later version fixes the problem that was being encountered? Lastly, is my solution a topic of over-engineering?

I started to do some minor research into the topic, and the code itself. I looked into the code that specifically gave us the property return value. I figured—maybe—it could shine a light on the matter and help me understand in better detail what could be happening. Looking into some of the scarce documentation on `XGetWindowProperty`, I found something rather interesting of note. The property return isn’t exactly a single value. It can contain a single value—but is capable of sending more than just one return code. So, the property return is treated more like an array rather than a singular value. I had already known this due to the code below, however, I did not consider that there may be something more to this.

Furthermore, it is noted by other developers that the function that gives us the property return values is known to have—in the only words I can describe—undefined behavior. The return can move, reorder, position, and even be absent of values in events. It’s not entirely known why it does this, but depending on the event you are reading, they are things to take careful consideration of.

I started working on this by creating a little logging of the property event codes, this was put inside of a while loop that would cycle through all the codes in the event being fired. Whenever I would maximize the window, or minimize the window, I took note of all the events it would fire. The thing I found interesting about the maximize event is it would report a vertical and horizontal maximize property return.  However, with the minimize, I found that it would report the same two property returns, along with a hidden property return. And much like what was mentioned previously, on Arch, the order of events was different compared to Debian, thus explaining why my initial fix wasn’t working.

I decided that instead of using bitmasks to try and fix the event reporting problem as I did initially, I would instead use these multiple return values to properly determine if a maximize or a minimize event was happening. The first step was the logic of the loop I made for logging. I rearranged the code and proceeded to check for the specific conditions to be met. If the property return value was vertical max, toggle the vertical maximize flag, if it was horizontal, toggle the horizontal flag, and so on. The exception to this was the hidden flag. If the hidden flag was present, we want to toggle the other two off, this is because if the other two are active, the minimize event could be mistaken as a maximize since the maximize only reports the vertical and horizontal property return values.

The next thing needed was to simply form the condition of this check. We can determine a maximize event is occurring fairly quickly, we just need to confirm that 1) the window is the same, 2) the vertical and horizontal flags are true, and 3) the previous event is not a maximize (this will prevent spamming of the event). If we meet all these requirements, then a maximize event has occurred. The minimize event is much the same, but it is looking for the hidden flag being raised over the vertical and horizontal flags.

After we’ve gotten these conditions settled, the replacement of the bitmask was very straightforward. With this replacement, we now report the maximize and minimize of the window without any extra hassle. Furthermore, this also fixes the other bug that was occurring last week where the event test case in the unit test was failing due to a bitmask overflow occurring which resulted in the events inversing, showing minimize on maximize and maximize on minimize.

Lastly, the final change I made was removing this code. I chose to do this as the if chain it was connected to has an else block that will return a maximize immediately, I figured that removing this would improve the performance of the code by minimizing some of the conditional checking it has to do. Something else to add to this is that with the addition of my code, there is a chance this code will no longer run since my code can pick up maximize events very quickly just by reading the multiple return values. So, I removed this and tested and found all the tests worked as expected, and my manual testing also passed, on both Debian and Arch. So, with that, this code was now effectively fixed.

Of course, there was more work this week. Since this bug was fixed rather early in the week, and was considered a high-priority issue, I found myself open to working on the audio issue once more.

I found myself mostly refactoring a lot of the audio code. It was written back when C++98 was used for the library—at least that’s what I’ve been told—so a majority of the code reflected that. Usages of new and delete, the use of NULL over nullptr, and general low-level management that doesn’t reflect the more modern usages seen in C++11. A lot of my changes focused on trying to modernize the code and get it into a more functional state while also removing the rather tricky memory leaks that were spread throughout it. Almost all of them looked to be small oversights that were not noticed when initially written. The way I fixed this problem was by using `unique_ptr`. The fix wasn’t nearly as easy though. Some of the changes required making custom delete functions for the pointers since the pointers were managed by PulseAudio. These had to be handled in a very particular way since the library reference counts these allocations. So, neglecting to delete them properly results in the leakage of memory or worse, runtime crashes.

Just by a surface look at the code, it is clear the kind of work that has to be done to effectively ensure these pointers are managed. There may have been more elegant solutions for this, but this is the one I found to be most readable and straightforward from an outsider's perspective. You may be wondering how these functions get passed into the unique pointer. That, in the end, is handled on the assignment of the pointer.

Almost all of the pointers that need these delete functions are handled like this. This code, in my complete opinion, is messy, however, I can’t use the modern luxuries of `std::make_unique` since it wasn’t introduced until C++17, even then, `std::make_unique` doesn’t support setting the custom delete function we need, so this is our best option. Throughout all the code, we replace the initialization and usages of these variables with the unique pointer equivalent, while also fixing instances of new and delete with other local variables.

This will ensure that no possible memory leaks can occur. While this example is rather clear that no memory leak would happen, what about this one?

We can’t be sure, but I know that `CreateFilePath` performs a memory allocation, and I know that it isn’t freed before that reallocation occurs, so in this instance, a memory leak occurred.

Now in these changes, we know for certain that the memory is no longer leaking. We are returning a unique pointer and instead letting it maintain the memory being used and letting it deallocate using RAII over user control. These little changes just save the confusion and make it clear for new developers what to use without them having to mentally track every allocation and deallocation they make. While I can do it, and many others can, others may have a harder time with that. Especially with classes like this one which do a lot.

After performing these changes and testing them, I continued to do minor refactoring while also including another bug to close the merge request. This other bug simply remarked fixing memory leaks found with the audio class. Since I just made this rather large change to the code, I added it as it will more than likely be fixed along with the initial bug.

Now, to close the topic for this week, I have created two new issues that will have to be tracked. Firstly, recent PulseAudio versions have started crashing during 3D tests. The crash is happening from an assertion failure within the internal code for PulseAudio, within the module `pulsecore`. This is something that will have to be worked on just to ensure that newer versions are supported, as my unit testing is currently broken right now because of it. Lastly, the other issue is that Linux users using PipeWire will suffer from distorted, fast-forwarded, skipping, and corrupted audio. I was using PipeWire initially without realizing it and found out that PipeWire with `pipewire-pulse` does not like or play with the GSound and GMusic code all that well. The workaround for this is using PulseAudio directly, but that introduces the other issue with the new versions not liking GMusic3D. So, in the end, both of these issues are related, and both will need to be fixed and closed at some point soon. I may work towards trying to fix this issue before tackling the ghost window that Lari wants me to work on. Leaving GAudio in the state it is in is not sitting right with me, and the more I tinker with it and use it, the more I find pockets of issues that need to be sorted out. I’d like to work on these and fix them before I feel comfortable enough to move on to the next issue, which would be ghost windows on macOS.

Month 3 - Week 2 - Into the Storm

 The title of this blog post for this week might be a bit farther out from my normal naming, but by no means am I exaggerating it. This week was chaotic and swamped with different issues that formed into twists and turns, turned into other issues, and outright formed into chaos towards the end. So, strap in, because there is a lot to cover.

In my last article, I discussed the work I was doing to get the Gateware foundation up-to-spec with its CMake. This was primarily focused on Linux as I felt it needed to be the most capable. This is primarily because CMake is built for the Linux family. It has tight integration and support for many of its architectures and strives to support a wide range of purely and wholly Linux-specific options. Because of this, I wanted most, if not all, of the CMake code for Linux to be tuned and optimized for what Linux is all about, options.

I went into further detail about this last week, so I won't linger on the likely tiring topic. However, I will cover some of the changes I made to this.

It was with happy notes that two major branches got merged of the many I made last week. The first one which I'm quite happy with is the general tags. This little change was one I was looking very much forward to and was happy to see it incorporated.

Another merge request, and really, the primary one, was the overhaul to the CMake for Linux.

All of the changes for it are listed concisely on the post as it covers a lot of different changes that were made which I found to be a fair bit useful. Something rather enjoyable of note was the different merge requests that were created from this one.

When this merge request was originally made, it involved mainly bringing the support listed as well as a plethora of refactoring aimed to make the CMake functionally more readable and concise, but it also spawned many new things as well. One in particular was Clang support.

Back in the early days of Gateware, it was primarily developed using Clang, specifically Clang 6 from what the history of commits tells me. At some point during development, around the time the project was facing a problem involving auto-linking, Clang was dropped in turn to support GCC instead. This put the project in a stasis on Linux where you have to use GCC only. This was something I was particularly questionable of as I like Linux's philosophy of giving the user options. So, I questioned if Clang could be brought back. The current version of Clang today is Clang 16, quite a bit further ahead than it was back then, so I figured it would support much of the features Gateware was originally waiting on. So, I did what developers do best, and started prototyping.

The first real edit I did was modifying the flags passed to the compiler. This was really the only major breaking change preventing Clang from being used. Once this change was done, it was off to compile. There were some immediate issues, one in particular was rather simple.

Some of the code inside of GAudio was using `nullptr_t` without the `std::` namespace added. In GCC, this wouldn't cause any problems, however, Clang is a little more picky about usages like this and makes it clear from the start that this should be added. Those changes were made, and then we encountered our first real issue, one that slipped passed even the seasoned devs.

This little piece of code was rather interesting. At first, I questioned how GCC was even compiling it, to begin with, but that led to a deeper rabbit hole. So, in case you happen to not understand what's happening here, let me explain. This code initializes a stack array of a certain size, stashing the width and height in the first two indexes before storing the rest of the space with pixel information. In total, this code takes pixel information stored in a certain way and then repacks it in another way. A great example is taking a BGRA image and then turning it into an ARGB image. This is effectively what it's doing. Now here is the problem. When defining a stack array like this, the size should be constant so that way we know that the array will not be stored dynamically to the stack. However, in the image, we don't see that.

We can see the size of the array is `convertedLength`, and we see that the variable is constant which is good, but what about `_width` or `_height`? Well, those are non-constant parameters. So, by our understanding, this is a non-standard array, furthermore, this code shouldn't be compiling right now, even with GCC. Yet, somehow, GCC is letting this build and pass perfectly fine.

That's when Clang came in. It immediately called out this issue, as loud as it could, making it loud and clear that this code was not only illegal but also an error. So with that, I immediately went to correct it.

This simple little change is not only clear but gets the point across evidently how the memory is handled. We aren't going to manually handle the array, we have no reason to. Instead, we will let the unique pointer type manage that, and it will give us a window into the array so we can modify and alter everything how we need to. After making this edit and doing a couple other bits of adjustments to the code, Clang support was now officially added. Wonderful!

Lastly, for our little string issue we had last week, I was able to make the last changes to it, adding the flags for both Clang and GCC and marking it done as well.

Now, what if I told you, that even with all of this progress, we still aren't done, not even halfway? Surprising right? I'm on a roll, and yet, we haven't even gotten to the good part. That is correct, it was after these changes that I started spiraling into a massive problem.

There was a merge request that I made recently to correct some issues with the GAudio tests. I wanted to do this as a way to help lessen some of the Clang warnings. It was a simple syntax issue that was overlooked and nothing I would consider too serious.

After making these changes, I began working on the primary bug that had been marked since the first week, the audio bug. A lot of my work was just on refactoring alone as I tried diagnosing and figuring out the issue at play as it was bewildering. Something internally wasn't working right, and things were breaking quite a bit. I did a lot of debugging, testing, and runs to try and figure out the problem, refactoring the code along the way to help read it easier as I figured this would be my home for the next several days. While doing this, I also decided to start diagnosing a rather annoying window event reporting failure in the unit test. This had been happening for a little while now, and I took note of it but didn't pay it any mind as I figured it may have been configuration-specific. I never had the issue show up until after a recent update to Xorg and a modification to my desktop. So I thought it was just a random one-off issue that wasn't general, as the runners were working and passing fine. However, I felt it would be in my best interest to look into it and see what could've possibly been happening. Upon inspecting the code, I came to find out it was occurring when the `Maximize` function would be called for GWindow. This event to maximize the window would do as expected, but instead of just reporting a singular maximize event, it would report a maximize event, and then do an event burst that was identical to maximize/minimize events after the initial maximize. This was troublesome as it meant a bug slipped in, one that I had introduced.

With this, I started going to work on fixing it. I figured with this being an edge case, the fix would be somewhat targeted but would work.

This code here was the fix. With this little edit, the events started working as expected again. There was a problem, however, if this issue happens to be isolated to just the latest versions of Xorg, then what does that mean for older versions? The problem would arise where the maximize would report fine, but now the minimize would be unreported due to the toggle I introduced for the manual call. This isn't good. I needed to test this. Specifically, I needed to update the Linux Mint runner to the latest version of Xorg, and then test the code before and after the change to verify it was Xorg. This had its own problems.

The Linux Mint runner that was currently being used was 20.3 (I may be slightly off, but it was the major version 20). When I got passed some initial issues updating, I found that this release was already up-to-date with the latest packages possible for that Mint version, which were all out of date by a fair bit. That's not good, I have to be able to test for this issue as best as I can, so I chose the next best option. I made a VM of Linux Mint and then cloned the GSource repo, building and running it on 21.3 of Linux Mint, which was running a much newer version of Xorg. This would give me a good idea if it was Xorg causing the issue or not.

I ran the test and waited... then waited... and waited more. Eventually, the tests were done, and to my surprise, no issue. That's confusing. Is it Xorg causing the issue then? I wanted to do a manual test just to be sure. I toggled the manual tests and then started testing them. I maximized the window and got the maximized event. I minimized it... nothing. Maximized... nothing again. Oh no. Nothing was reported. This isn't good. For some reason, the fix I had developed for this event reporting issue wasn't working at all on Linux Mint, not just that, but Debian. It was only Arch that was working as expected. That's not good at all. It means the most popular branch of Linux still has a bug I thought I fixed, furthermore, it means I have Arch-specific code inside of the Debian build that's doing nothing.

Sadly, after careful consideration, I had to reopen the issue.

Because of this issue resurfacing, I discussed with Lari the course of action and came to the conclusion that the audio bug would have to be put on the back burner while I focus on fixing these event reporting issues. Hopefully, by next week, I will be able to put an end to this issue and have it fixed permanently, or at least until X11 updates with more breaking changes.

For now, this is where I'm at. Next week will be focused heavily on fixing this bug, as well as the child to it. By next week, I hope that this issue will be fixed so that I may move forward with the audio bug and continue fixing it for week four.