refactor: Make ToxAV independent of toxcore internals.

[zoff99]

This change is 

[ghost]

Congratulations 🎉. DeepCode analyzed your code in 2.973 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

[codecov]

Codecov Report

Attention: 188 lines in your changes are missing coverage. Please review.

Comparison is base (d55fc85) 73.61% compared to head (5e798c0) 73.90%.

Files	Patch %	Lines
toxav/msi.c	55.40%	66 Missing ⚠️
toxav/rtp.c	57.14%	54 Missing ⚠️
toxav/toxav.c	71.90%	34 Missing ⚠️
toxav/bwcontroller.c	15.15%	28 Missing ⚠️
toxcore/tox.c	93.02%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1431      +/-   ##
==========================================
+ Coverage   73.61%   73.90%   +0.28%     
==========================================
  Files         148      148              
  Lines       30404    30429      +25     
==========================================
+ Hits        22383    22489     +106     
+ Misses       8021     7940      -81     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[iphydf]

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @zoff99 and @zugz)

---

toxav/bwcontroller.c, line 190 at r9 (raw file):

Previously, iphydf wrote…

Also log an error here.

Still log.

---

toxav/bwcontroller.c, line 200 at r9 (raw file):

Previously, zoff99 (Zoff) wrote…

i dont know. but changed now

Is it? I don't see the change here.

---

toxav/msi.c, line 890 at r1 (raw file):

Previously, zoff99 (Zoff) wrote…

not sure if i can do that. i tried and got into a circular problem

You can do this. You just need a forward declaration.

[iphydf]

Reviewed 5 of 22 files at r1, 1 of 7 files at r2, 4 of 9 files at r8, 3 of 11 files at r13, 3 of 8 files at r14, 1 of 3 files at r15, 6 of 10 files at r16.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @zoff99 and @zugz)

---

toxav/bwcontroller.h, line 11 at r16 (raw file):

#include "../toxcore/tox.h"
#include "../toxcore/mono_time.h"

Sort.

---

toxav/bwcontroller.c, line 57 at r5 (raw file):

Previously, iphydf wrote…

What does this do? Can you add a comment to say what it means for this to be true or false?

Ping.

---

toxav/bwcontroller.c, line 82 at r16 (raw file):

    retu->cycle.last_refresh_timestamp = now;
    retu->tox = tox;
    retu->bwc_receive_active = true; /* default: true */

What does the comment here mean? I can see in the code that it's set to true. Is the value of the comment that it's saying that this is a default and can be overridden later?

---

toxav/bwcontroller.c, line 194 at r16 (raw file):

    /* get BWController object from Tox and friend number */
    ToxAV *toxav = (ToxAV *)tox_get_av_object(tox);

Make the ToxAV object non-void-pointer and remove the cast here.

---

toxav/msi.c, line 175 at r16 (raw file):

    }

    if (!session) {

I'd collapse these 2 ifs into a single one.

---

toxav/msi.c, line 179 at r16 (raw file):

    }

    Tox_Err_Friend_Query f_con_query_error;

f_conn or f_con?

---

toxav/msi.c, line 480 at r16 (raw file):

    uint8_t *data_new = (uint8_t *)calloc(1, length_new);

    if (!data_new) {

data_new == nullptr

---

toxav/msi.c, line 495 at r16 (raw file):

    tox_friend_send_lossless_packet(tox, friendnumber, data_new, length_new, &error);

    // TODO(Zoff): make this better later! -------------------

Can you make a single comment at the top explaining what's bad here that needs to be made better later, instead of repeating the same comment 4 times?

---

toxav/msi.c, line 874 at r16 (raw file):

{
    if (length2 < 2) {
        // we need more than the ID byte for MSI messages

This would be an error, right? We should log that.

---

toxav/msi.c, line 888 at r16 (raw file):

    ToxAV *toxav = (ToxAV *)tox_get_av_object(tox);

    if (!toxav) {

toxav == nullptr

---

toxav/msi.c, line 894 at r16 (raw file):

    MSISession *session = tox_av_msi_get(toxav);

    if (!session) {

== nullptr

---

toxav/rtp.c, line 352 at r1 (raw file):

Previously, iphydf wrote…

We could store a Mono_Time pointer in RTPSession, right?

Can't we?

---

toxav/rtp.c, line 53 at r16 (raw file):

static struct RTPMessage *new_message(Tox *tox, const struct RTPHeader *header, size_t allocate_len,
                                      const uint8_t *data,
                                      uint16_t data_length)

Remove extra line break.

---

toxav/rtp.c, line 59 at r16 (raw file):

    if (msg == nullptr) {
        LOGGER_API_DEBUG(tox, "%s:%d:%s:msg=calloc(%d):NULL", __FILE__, __LINE__, __func__,

Logger already puts __FILE__ etc into the callback.

---

toxav/rtp.c, line 348 at r16 (raw file):

    }

    if (!session->toxav) {

I'd collapse these 2 ifs as well.

---

toxav/rtp.c, line 472 at r16 (raw file):

    }

    ToxAV *toxav = (ToxAV *)tox_get_av_object(tox);

Avoid cast by changing the type of the function.

---

toxav/rtp.c, line 474 at r16 (raw file):

    ToxAV *toxav = (ToxAV *)tox_get_av_object(tox);

    if (!toxav) {

== nullptr

---

toxav/rtp.c, line 480 at r16 (raw file):

    ToxAVCall *call = (ToxAVCall *)call_get(toxav, friendnumber);

    if (!call) {

== nullptr

---

toxav/rtp.c, line 486 at r16 (raw file):

    RTPSession *session = rtp_session_get(call, data[0]);

    if (!session) {

== nullptr (I'm going to stop now)

---

toxav/rtp.c, line 560 at r16 (raw file):

        session->mcb(rtp_get_mono_time_from_rtpsession(session), session->cs, session->mp);
        session->mp = nullptr;
        return;

Unused return.

---

toxav/rtp.c, line 718 at r16 (raw file):

    session->ssrc = payload_type == RTP_TYPE_VIDEO ? 0 : random_u32(); // Zoff: what is this??
    session->payload_type = payload_type;
    // session->m = m;

Remove?

---

toxav/rtp.c, line 722 at r16 (raw file):

    session->toxav = toxav;
    session->friend_number = friendnumber;
    session->rtp_receive_active = true; /* default: true */

Same here about the "default" comment: why do we write that here?

---

toxav/rtp.c, line 783 at r16 (raw file):

 */
int rtp_send_data(RTPSession *session, const uint8_t *data, uint32_t length,
                  bool is_keyframe, const Logger *log)

log is now an unused parameter, I think, so we can delete it.

---

toxav/toxav.c, line 76 at r16 (raw file):

};

#include "toxav_hacks.h"

Can we put this together with the other includes at the top?

---

toxav/toxav.c, line 169 at r16 (raw file):

    }

    return nullptr;

Dead code.

---

toxav/toxav.c, line 223 at r16 (raw file):

    // save Tox object into toxcore
    tox_set_av_object(av->tox, (void *)av);

Cast-to-voidptr is implicit.

---

toxav/toxav.c, line 417 at r16 (raw file):

}

static int toxav_friend_exists(const Tox *tox, int32_t friendnumber)

What is this function's purpose. Is its purpose to turn a bool into an int? The function name is a bit misleading, because toxav_ functions take a ToxAV*, while this one takes a Tox*.

---

toxav/toxav.c, line 440 at r16 (raw file):

    ToxAVCall *call;

    if (toxav_friend_exists(av->tox, friend_number) == 0) {

if (!tox_friend_exists(av->tox, friend_number)) {

---

toxav/toxav.c, line 1251 at r16 (raw file):

    ToxAVCall *call = nullptr;

    Tox_Err_Friend_Query f_con_query_error;

f_conn?

[Green-Sky]

should we adjust the milestone on this one?

[Green-Sky]

*ToxAV object