New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve sending of large video frames in toxav. #718

Merged
merged 1 commit into from Feb 12, 2018

Conversation

Projects
8 participants
@iphydf
Member

iphydf commented Jan 21, 2018

This change does not include the addition of VP9. We do that in a
separate pull request.


This change is Reviewable

@iphydf iphydf added this to the v0.2.0 milestone Jan 21, 2018

@CLAassistant

This comment has been minimized.

CLAassistant commented Jan 21, 2018

CLA assistant check
All committers have signed the CLA.

@endoffile78

This comment has been minimized.

Member

endoffile78 commented Jan 21, 2018

Reviewed 8 of 8 files at r1.
Review status: all files reviewed at latest revision, 13 unresolved discussions.


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

        // if length(31 bits) > 1FFFFFFF then use all bits for length
        // and assume its a keyframe (most likely is anyway)
        if (LOWER_31_BITS(length_v3) > 0x1FFFFFFF) {

Create a constant for 0x1FFFFFFF


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

{
    assert(allocate_len >= data_length);
    struct RTPMessage *msg = (struct RTPMessage *)calloc(sizeof(struct RTPMessage) +

Make sure memory is allocated.


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

        uint32_t offset, uint32_t full_data_length, uint8_t is_keyframe)
{
    struct RTPMessage *msg = (struct RTPMessage *)calloc(1, sizeof(struct RTPMessage) + allocate_len);

Make sure memory is allocated.


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

static void move_slot(struct RTPWorkBufferList *wkbl, int8_t dst_slot, int8_t src_slot)
{
    memcpy(&(wkbl->work_buffer[dst_slot]), &(wkbl->work_buffer[src_slot]), sizeof(struct RTPWorkBuffer));

Add a check to make sure dst_slot isn't larger than USED_RTP_WORKBUFFER_COUNT


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

    if (offset_v3 > length_v3) {
        LOGGER_ERROR(log, "memory size too small!");

Shouldn't there be a return here or at least changing the offset?


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

    if (session_v3->work_buffer_list == NULL) {
        session_v3->work_buffer_list = (struct RTPWorkBufferList *)calloc(1, sizeof(struct RTPWorkBufferList));

Make sure memory is allocated.


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

        }

        if (slot == -1) {

Use else if


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

            if (slot == -2) {
                free(m_new);

Possible double free or freeing a NULL pointer.


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

        }

        if (slot == -1) {

Use else if


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

            if (slot == -2) {
                free(m_new);

Possible double free or freeing a NULL pointer.


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

        }

        if (slot > -1) {

Use else if


toxav/video.c, line 371 at r1 (raw file):

            (uint8_t)header_v3->pt == (rtp_TypeVideo % 128)) {
        LOGGER_DEBUG(vc->log, "rb_write msg->len=%d b0=%d b1=%d", (int)msg->len, (int)msg->data[0], (int)msg->data[1]);
        free(rb_write((RingBuffer *)vc->vbuf_raw, msg));

rb_write can return NULL.


toxav/video.c, line 373 at r1 (raw file):

        free(rb_write((RingBuffer *)vc->vbuf_raw, msg));
    } else {
        free(rb_write((RingBuffer *)vc->vbuf_raw, msg));

rb_write can return NULL.


Comments from Reviewable

@zoff99

This comment has been minimized.

zoff99 commented Jan 21, 2018

changes:

  • fix the video bug (video frames larger than 65KBytes) by sending full frame length in alternate header field
  • improve video frame reconstruction logic with slots
  • configure video encoder and decoder to be multihtreaded
  • set error resilience flags on video codec
  • change encoder and decoder softdeadline
@iphydf

This comment has been minimized.

Member

iphydf commented Jan 21, 2018

Review status: 7 of 8 files reviewed at latest revision, 13 unresolved discussions, some commit checks failed.


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

Previously, endoffile78 (Endoffile) wrote…

Create a constant for 0x1FFFFFFF

@zoff99 what should we call it?


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

Previously, endoffile78 (Endoffile) wrote…

Make sure memory is allocated.

Done.


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

Previously, endoffile78 (Endoffile) wrote…

Make sure memory is allocated.

Done.


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

Previously, endoffile78 (Endoffile) wrote…

Add a check to make sure dst_slot isn't larger than USED_RTP_WORKBUFFER_COUNT

Done.


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

Previously, endoffile78 (Endoffile) wrote…

Shouldn't there be a return here or at least changing the offset?

@zoff99 what should we do when this happens?


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

Previously, endoffile78 (Endoffile) wrote…

Make sure memory is allocated.

Done.


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

Previously, endoffile78 (Endoffile) wrote…

Use else if

No need, there is a return in the previous one.


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

Previously, endoffile78 (Endoffile) wrote…

Possible double free or freeing a NULL pointer.

free(NULL) is fine. How does a double free happen?


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

Previously, endoffile78 (Endoffile) wrote…

Use else if

See above.


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

Previously, endoffile78 (Endoffile) wrote…

Possible double free or freeing a NULL pointer.

See above.


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

Previously, endoffile78 (Endoffile) wrote…

Use else if

That would change the semantics.


toxav/video.c, line 371 at r1 (raw file):

Previously, endoffile78 (Endoffile) wrote…

rb_write can return NULL.

free(NULL) is safe.


toxav/video.c, line 373 at r1 (raw file):

Previously, endoffile78 (Endoffile) wrote…

rb_write can return NULL.

free(NULL) is safe.


Comments from Reviewable

@endoffile78

This comment has been minimized.

Member

endoffile78 commented Jan 21, 2018

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


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

Previously, iphydf wrote…

free(NULL) is fine. How does a double free happen?

I'm not sure how likely it is and maybe its not but if m_new is not NULL and session->mcb is m_new is freed. Then slot is updated and if its -2 m_new is freed.


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

Previously, iphydf wrote…

See above.

same as above


Comments from Reviewable

@iphydf

This comment has been minimized.

Member

iphydf commented Jan 24, 2018

Review status: 7 of 8 files reviewed at latest revision, 4 unresolved discussions.


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

Previously, endoffile78 (Endoffile) wrote…

I'm not sure how likely it is and maybe its not but if m_new is not NULL and session->mcb is m_new is freed. Then slot is updated and if its -2 m_new is freed.

That doesn't seem correct. But more interestingly, m_new should always be null here:

  • If m_new is null and slot is -2, it's free(null).
  • If m_new is not null, then regardless of session->mcb, it is set to null, so this is free(null).

It's always free(null). @zoff99 can you look into this?


Comments from Reviewable

@iphydf

This comment has been minimized.

Member

iphydf commented Jan 24, 2018

Review status: 7 of 8 files reviewed at latest revision, 4 unresolved discussions.


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

Previously, endoffile78 (Endoffile) wrote…

same as above

Same.


Comments from Reviewable

@iphydf iphydf changed the title from Improve video key frame sending. to WIP #2: Improve video key frame sending. Jan 25, 2018

@iphydf iphydf modified the milestones: v0.2.0, v0.2.x Jan 25, 2018

@iphydf iphydf added this to In Progress in Video Jan 25, 2018

@sudden6

This comment has been minimized.

sudden6 commented Jan 27, 2018

Reviewed 1 of 8 files at r1, 1 of 3 files at r3.
Review status: 6 of 8 files reviewed at latest revision, 9 unresolved discussions.


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

    }

    if (wkbl->work_buffer[0].frame_type == video_frame_type_KEYFRAME) {

chain with && instead of nested ifs


toxav/rtp.c, line 374 at r3 (raw file):

static struct RTPMessage *process_oldest_frame(Logger *log, struct RTPWorkBufferList *wkbl)
{
    if (wkbl->next_free_entry > 0) {

reverse condition and use early return


toxav/rtp.c, line 413 at r3 (raw file):

    if (slot == (wkbl->next_free_entry - 1)) {
        memset(&(wkbl->work_buffer[slot]), 0, sizeof(struct RTPWorkBuffer));
        wkbl->next_free_entry--;

this line is the the same in both branches, move outside conditional


toxav/toxav.c, line 885 at r3 (raw file):

                uint32_t frame_length_in_bytes = pkt->data.frame.sz;

                if (LOWER_31_BITS(frame_length_in_bytes) > 0x1FFFFFFF) {

invert condition so there isn't an empty if path


toxav/video.c, line 371 at r3 (raw file):

            (uint8_t)header_v3->pt == (rtp_TypeVideo % 128)) {
        LOGGER_DEBUG(vc->log, "rb_write msg->len=%d b0=%d b1=%d", (int)msg->len, (int)msg->data[0], (int)msg->data[1]);
        free(rb_write((RingBuffer *)vc->vbuf_raw, msg));

same line in both branches, move out of conditional


Comments from Reviewable

@iphydf

This comment has been minimized.

Member

iphydf commented Jan 30, 2018

Review status: 5 of 8 files reviewed at latest revision, 9 unresolved discussions.


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

Previously, sudden6 wrote…

chain with && instead of nested ifs

Done.


toxav/rtp.c, line 374 at r3 (raw file):

Previously, sudden6 wrote…

reverse condition and use early return

Done.


toxav/rtp.c, line 413 at r3 (raw file):

Previously, sudden6 wrote…

this line is the the same in both branches, move outside conditional

Done.


toxav/toxav.c, line 885 at r3 (raw file):

Previously, sudden6 wrote…

invert condition so there isn't an empty if path

Done.


toxav/video.c, line 371 at r3 (raw file):

Previously, sudden6 wrote…

same line in both branches, move out of conditional

Done.


Comments from Reviewable

@sudden6

This comment has been minimized.

sudden6 commented Feb 2, 2018

Reviewed 1 of 8 files at r1, 2 of 5 files at r4.
Review status: 3 of 8 files reviewed at latest revision, 6 unresolved discussions.


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

{
    if (bwc->packet_loss_counted_cycles > BWC_AVG_LOSS_OVER_CYCLES_COUNT) {
        if (current_time_monotonic() - bwc->cycle.last_sent_timestamp > BWC_SEND_INTERVAL_MS) {

I think these two if's can be joined by && instead of nesting


toxav/rtp.c, line 308 at r5 (raw file):

    }

    return false;

early return?


Comments from Reviewable

@iphydf

This comment has been minimized.

Member

iphydf commented Feb 2, 2018

Review status: 0 of 12 files reviewed at latest revision, 6 unresolved discussions.


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

Previously, sudden6 wrote…

I think these two if's can be joined by && instead of nesting

Done.


toxav/rtp.c, line 308 at r5 (raw file):

Previously, sudden6 wrote…

early return?

Done.


Comments from Reviewable

@iphydf iphydf changed the title from WIP #2: Improve video key frame sending. to Improve video key frame sending. Feb 2, 2018

@iphydf iphydf changed the title from Improve video key frame sending. to Improve sending of large video frames in toxav. Feb 2, 2018

@iphydf iphydf changed the title from Improve sending of large video frames in toxav. to #2: Improve sending of large video frames in toxav. Feb 2, 2018

@sudden6

This comment has been minimized.

sudden6 commented Feb 2, 2018

Reviewed 4 of 8 files at r7.
Review status: 4 of 13 files reviewed at latest revision, 9 unresolved discussions.


toxav/rtp.c, line 612 at r7 (raw file):

            }

            m_new = nullptr;

could there be a ressource leak when m_new == true and session->mcb == true? because then m_new is set to nullptr in this line. Or are there other references to m_new that are then freed?


toxav/rtp.c, line 630 at r7 (raw file):

                                 offset_v3, data, length);

        //if ((frame_complete == 1) && is_keyframe)

dead comments?


toxav/rtp.c, line 645 at r7 (raw file):

                }

                m_new = nullptr;

same possible resource leak?


toxav/video.c, line 301 at r7 (raw file):

        const struct RTPHeader *header_v3 = &p->header;

        if ((uint8_t)((header_v3->flags & RTP_LARGE_FRAME) != 0)) {

!= 0 isn't really needed IMO, but more important the cast is not needed and possibly confusing.


toxav/video.c, line 367 at r7 (raw file):

    pthread_mutex_lock(vc->queue_mutex);

    if ((uint8_t)((header_v3->flags & RTP_LARGE_FRAME) != 0) &&

same


Comments from Reviewable

@iphydf

This comment has been minimized.

Member

iphydf commented Feb 2, 2018

Review status: 4 of 13 files reviewed at latest revision, 9 unresolved discussions.


toxav/rtp.c, line 612 at r7 (raw file):

Previously, sudden6 wrote…

could there be a ressource leak when m_new == true and session->mcb == true? because then m_new is set to nullptr in this line. Or are there other references to m_new that are then freed?

It's either freed or passed to the callback mcb. The mcb callback becomes either a video or audio queue function, which either frees it or puts it into a jitter buffer (omg str8c, no unique_ptrs :().


toxav/rtp.c, line 630 at r7 (raw file):

Previously, sudden6 wrote…

dead comments?

Done.


toxav/rtp.c, line 645 at r7 (raw file):

Previously, sudden6 wrote…

same possible resource leak?

Same. I've only looked for 3 minutes, but it seems ok.


toxav/video.c, line 301 at r7 (raw file):

Previously, sudden6 wrote…

!= 0 isn't really needed IMO, but more important the cast is not needed and possibly confusing.

Done.


toxav/video.c, line 367 at r7 (raw file):

Previously, sudden6 wrote…

same

Done.


Comments from Reviewable

@iphydf

This comment has been minimized.

Member

iphydf commented Feb 6, 2018

Review status: 0 of 18 files reviewed at latest revision, 9 unresolved discussions.


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

Previously, iphydf wrote…

@zoff99 what should we call it?

Done.


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

Previously, iphydf wrote…

@zoff99 what should we do when this happens?

Done.


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

Previously, iphydf wrote…

That doesn't seem correct. But more interestingly, m_new should always be null here:

  • If m_new is null and slot is -2, it's free(null).
  • If m_new is not null, then regardless of session->mcb, it is set to null, so this is free(null).

It's always free(null). @zoff99 can you look into this?

Done.


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

Previously, iphydf wrote…

Same.

Done.


toxav/rtp.c, line 612 at r7 (raw file):

Previously, iphydf wrote…

It's either freed or passed to the callback mcb. The mcb callback becomes either a video or audio queue function, which either frees it or puts it into a jitter buffer (omg str8c, no unique_ptrs :().

Done.


toxav/rtp.c, line 630 at r7 (raw file):

Previously, iphydf wrote…

Done.

Done.


toxav/rtp.c, line 645 at r7 (raw file):

Previously, iphydf wrote…

Same. I've only looked for 3 minutes, but it seems ok.

Done.


Comments from Reviewable

@sudden6

This comment has been minimized.

sudden6 commented Feb 6, 2018

:lgtm_strong:


Reviewed 1 of 13 files at r6, 5 of 19 files at r10, 4 of 14 files at r12, 8 of 8 files at r13.
Review status: all files reviewed at latest revision, 4 unresolved discussions.


Comments from Reviewable

@iphydf iphydf unassigned zugz Feb 8, 2018

@iphydf

This comment has been minimized.

Member

iphydf commented Feb 8, 2018

@robinlinden, @endoffile78: this is ready for another round of review.

@endoffile78

This comment has been minimized.

Member

endoffile78 commented Feb 11, 2018

:lgtm_strong:


Reviewed 8 of 8 files at r13.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@iphydf iphydf changed the title from #2: Improve sending of large video frames in toxav. to Improve sending of large video frames in toxav. Feb 11, 2018

@robinlinden

This comment has been minimized.

Member

robinlinden commented Feb 11, 2018

:lgtm_strong:


Reviewed 4 of 13 files at r6, 1 of 9 files at r9, 4 of 19 files at r10, 1 of 9 files at r11, 7 of 8 files at r13, 1 of 2 files at r15, 1 of 3 files at r16, 2 of 2 files at r17.
Review status: all files reviewed at latest revision, all discussions resolved.


toxav/rtp.c, line 49 at r17 (raw file):

{
    assert(allocate_len >= data_length);
    struct RTPMessage *msg = (struct RTPMessage *)calloc(sizeof(struct RTPMessage) + allocate_len, 1);

calloc generally takes num, size as arguments. This looks weird even if it does the same allocation.


toxav/rtp.c, line 300 at r17 (raw file):

    // frame data array.
    memcpy(
        (slot->buf->data + header->offset_full),

Unnecessary parentheses.


toxav/rtp.c, line 311 at r17 (raw file):

    slot->buf->header.received_length_full = slot->received_len;

    return (slot->received_len == header->data_length_full);

Unnecessary parentheses.


toxav/rtp.c, line 367 at r17 (raw file):

    LOGGER_DEBUG(log, "wkbl->next_free_entry:003=%d", session->work_buffer_list->next_free_entry);

    const bool is_multipart = (full_frame_length != incoming_data_length);

Unnecessary parentheses.


Comments from Reviewable

Improve video key frame sending.
This change does not include the addition of VP9. We do that in a
separate pull request.

Changes:

* fix the video bug (video frames larger than 65KBytes) by sending full
  frame length in alternate header field
* improve video frame reconstruction logic with slots
* configure video encoder and decoder to be multihtreaded
* set error resilience flags on video codec
* change encoder and decoder softdeadline
@iphydf

This comment has been minimized.

Member

iphydf commented Feb 11, 2018

Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks pending.


toxav/rtp.c, line 49 at r17 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

calloc generally takes num, size as arguments. This looks weird even if it does the same allocation.

Done.


toxav/rtp.c, line 300 at r17 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

Unnecessary parentheses.

Done.


toxav/rtp.c, line 311 at r17 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

Unnecessary parentheses.

Done.


toxav/rtp.c, line 367 at r17 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

Unnecessary parentheses.

Done.


Comments from Reviewable

@zoff99

This comment has been minimized.

zoff99 commented Feb 12, 2018

@robinlinden @iphydf
Unnecessary parentheses
i wished for those, for easier reading.

@iphydf iphydf merged commit 7213582 into TokTok:master Feb 12, 2018

7 checks passed

GPG All commits have a verified GPG signature
WIP ready for review
Details
ci/circleci Your tests passed on CircleCI!
Details
code-review/reviewable 3/3 LGTMs
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

Video automation moved this from In Progress to Done Feb 12, 2018

@iphydf iphydf deleted the iphydf:video-fix branch Feb 12, 2018

@iphydf iphydf modified the milestones: v0.2.x, v0.2.0 Feb 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment