Apply multiple stability fixes and release a new qTox version #1
Conversation
|
If you are interested in this package. Feel free to create an account and help co-maintain it. |
Sure, I have followed the instructions on the page. @kwizart what should be the next steps? |
|
@nickolay168 Usually it should be the current maintainer to ack. Unless @eclipseo can answer here ? |
|
Thank you! |
|
Hi, sorry for delay, since you already did https://github.com/nickolay168/qTox/releases/tag/v1.17.7 , I prefer use the tag , |
Sure, Thank you! I have updated the PR. I have also used a new tag for version, where I have removed unsupported unfinished code. |
|
I fixed two minor details and squash commits , push and built for Fedora 41 and rawhide . |
|
I ain't sure about this commit for ffmpeg 7.x Could you explain why you choose to remove avformat_free_context() ? Could you fix this alignment: qTox/qTox@c64d79f#diff-e8b9f765aa2e392d5753e626bcb07aa1a8e78346da63be1ccf361ad402e280bdR316 I know it's not Python, but still 😄 Could you check why you have absolute paths in translations.qrc: qTox/qTox@3f3a7e6#diff-4233401a9e85a25235756d3625491988bf9b6294605080284bb435698dc6c542R1 |
|
Also, ok this is an unofficial fork, I've checked your changes, and there are no glaring issues. But there are also unofficial forks like : https://github.com/TokTok/qTox and they have @iphydf on board who is one of the main c-toxcore dev. Their fork is very active, with a lot more changes, and with more "stars". I am more inclined to use their fork as the new basis rather than yours, because you seem to work solo. They have applied a similar solution for security by also removing the Extension message support TokTok/qTox@f9d229f Considering what happen last year with xz utils, considering the sensibility of this project, I'd rather be cautious and use the fork made by people working also on c-toxcore. Would you have a problem contributing your change to their repo and working with them? cc @sergiomb2 @kwizart for opinions. |
qTox-1.17.7.patch
Outdated
| std::atomic<IAudioControl*> audio; | ||
| + // atomic flag showing that we do not need to accept frames as the cancel | ||
| + // call request was sent. | ||
| + std::atomic<bool> isCancelling; |
There was a problem hiding this comment.
I think this is a fix for a crash/race condition in closing qTox during a call. We've made a few changes (e.g. TokTok/qTox#274 and TokTok/qTox#192) towards that. Can you check whether this change is still necessary?
There was a problem hiding this comment.
It seems like it is still needed, because it solves different issue. It is fixing rare situation, when user tryes to cancel the call AND simultaneously the Video/Audio frame comes in. If I am remembering it correctly that is what happens
- User hits hang up and the
QWriteLocker locker{&callsLock};will lock the frame frocessing - Now in very edge case, the frame comes between the (1) and the call
toxav_call_control(toxav.get(), friendNum, TOXAV_CALL_CONTROL_CANCEL, &err);which will never complete as it will hit the mutex inside toxcore (ToxAv->mutex). This mutex will wait for frame to be processed , which will never happen because of (1)
#274 Fixed the lambda expression, sending frames, when the call is being terminated.
#192 Makes sure that the calls are being cleaned up when CoreAV is being destroyed.
So it seems that is a separate issue, which is an edge case, but still can happen.
qTox-1.17.7.patch
Outdated
| +#ifdef SPELL_CHECKING | ||
| + if (decorator != nullptr) { | ||
| + delete decorator; | ||
| + decorator = nullptr; |
qTox-1.17.7.patch
Outdated
| } | ||
|
|
||
| - for (auto form : chatForms) { | ||
| - delete form; |
There was a problem hiding this comment.
This change makes sense (given QObject child deletion), but what issue does it fix? Does deleting these prematurely cause a problem?
There was a problem hiding this comment.
This code does not causes issues by itself, in this change I just wanted to unify handling of chatForm and groupChatForm.
|
https://github.com/nickolay168/qtox tag v1.17.8 is what build on RPMFusion . |
|
@nickolay168 648BF2EEE794E94444B848F8FC6AD3BA029C9BC2649BA761EF556DA17F549022A8D7596E7DBA is a groupbot that will invite you to the toktok-dev chat. I think there are at least a few fixes in your repo that we didn't address yet (e.g. bugs when history is off, because I've never tested that). Let's get them merged :). |
|
I have also a PR to port qTox to KDE6, but I was testing it. Let me look
tomorrow, how we can work it out.
Thank you!
Nikolay Rovinskiy
… @nickolay168 648BF2EEE794E94444B848F8FC6AD3BA029C9BC2649BA761EF556DA17F549022A8D7596E7DBA is a groupbot that will invite you to the toktok-dev chat. I think there are at least a few fixes in your repo that we didn't address yet (e.g. bugs when history is off, because I've never tested that). Let's get them merged :).
|
|
That's also done. We're on Qt6 now. |
The official qTox version (https://github.com/qTox/qTox) used to take the list of devices by manually creating the AVFormatContext, which in turn involved copying the structure of AVInputFormat's AVClass that required allocation of private data for the specific AVClass, contained in the iformat variable. The manual allocation required the size of this class, however, the priv_data_size field of AVInputFormat was long deprecated and finally removed in ffmpeg v. 7. Here is the commit in ffmpeg repository, which has broken qTox: |
Yes, probably it is the best way to go forward. |
so also approve start using https://github.com/TokTok/qTox ? |
I Think, generally yes, as it seems to be the next official qTox repo. |
|
TokTok/qTox is built against ffmpeg 7.1: https://github.com/TokTok/dockerfiles/blob/master/qtox/download/download_ffmpeg.sh#L10 We also build against some older versions on CI. Whichever is in Ubuntu 22.04 or debian stable. |
@sergiomb2 Then it is totally reasonable to move to the new branch. It would be also great to use toxcore 0.2.20. It looks like it contains some great fixes. |
I have applied some stability fixes, here is the list from my change log:
I have decided also to bump the version, to avoid issues with the same versions with different code.