Use C++ <random> instead of Qt or POSIX #460
Conversation
|
This pull request does not only change the random function but it has also changes in the range of the random numbers in some places. |
Another alternative, which would appear more consistent with the MythTV coding standards documentation, would be that after the (imminent) release of v32 to advocate for bumping the minimum QT level(*) and cleaning up the ifdef'ery if the core devs are too annoyed with such. But that would be something for master-next (v33-Pre) after checking what the various target OS's now have as their shipping version (I suspect 5.12 could be the new minimum, but I have not checked all the various target platforms). (*) As I recall, with the current master, the reasoning for Qt 5.9 as a minimum was that at the time a few of the targeted OS's were at 5.9. I think all of those OS's have either aged out of the targeted platform list, or are otherwise no longer supported for master (due to other requirements). |
|
https://pkgs.org/search/?q=Qtbase Ubuntu 20.04 LTS is 5.12. Ubuntu 22.04 LTS should be 5.15 like everything else. Of note, Qt 5.12 is EOL as of December. |
Centos 7 has already been dropped from master. |
|
The POSIX random() function has now been replaced with MythRandom. |
|
PacketBuffer::FreePacket no longer has any (reasonable) risk of errors from overflowing. |
It was only used for a private variable, so hide it in the constructor in the .cpp file. This drastically reduces the recompilations needed when changing mythrandom.h.
mingw32 complains about it.
This forces the use of the unsigned overload, which technically may be a change. However, we never actually request any negative numbers and most calls are of the form (0, x), so this is functionally equivalent.
The file uses mythrandom.h instead. Add include <cstdint>, which is also transitively included by "mythrandom.h".
m_next_empty_packet_key is a random number that is incremented. In FreePacket(), the upper 32 bits are tested for equivalence. However, the incrementing of m_next_empty_packet_key could overflow the lower 32 bits, changing the upper 32. This would cause the if statement to be false and the packet to not be marked as empty. Starting with m_next_empty_packet_key with the lower 32 bits cleared, prevents this error by allowing 2^32 packets before overflowing, far more than can be reasonably expected.
ulmus-scott
changed the title
|
These changes look fine to me. I keep going back to look at the packetbuffer.cpp change because it does produce a different result than the original, but if the comment in packetbuffer.h is correct then this appears to be a better solution. The old solution filled the bottom 56 bits of It does seems to me though that |
I think the expected value of the initial packet buffer size would be 2^31 on average before overflowing and increasing in size to 2^32 after overflowing.
On average it never happened before either (2^31).
If you look at udppacket.h and mythtv/mythtv/libs/libmythtv/recorders/rtp/packetbuffer.h Lines 35 to 41 in d479280 UDPPacket is a reference counted QByteArray, so the memory would be freed when all references have fallen out of scope. It is not quite a memory leak because the packets can still be used in the future instead of creating new ones. If the UDPPacket were a fixed size, I think it would make more sense. A fixed size ring buffer might be a better choice instead of this. Although, once the PacketBuffer goes out of scope, all of the memory should be freed. So it is a change, but I don't think it's a significant one. We could also leave that for later and just use . |
|
@linuxdude42 Some additional probabilities for overflowing: |
This also removes the conditional compilation due to the Qt random number generator functions added in 5.10.
Checklist