Make ToxAV stateless #130

GrayHatter · 2016-09-11T23:28:16Z

See #40.

This change is

iphydf · 2016-09-12T11:40:16Z

Review status: 0 of 20 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

other/apidsl/toxav.in.h, line 34 at r1 (raw file):

#endif

#define TOXAV_PAIR(TYPE1__, TYPE2__) struct { TYPE1__ first; TYPE2__ second; }

Why is this in the public API?

other/apidsl/toxav.in.h, line 221 at r1 (raw file):

 * thread from tox_iterate.
 */
void iterate(void *userdata);

any userdata like in tox.in.h.

toxav/audio.c, line 99 at r1 (raw file):

    ac->av = av;
    ac->friend_number = friend_number;
    ac->toxav_audio_frame_fxn = cb;

I very much dislike "fxn" as abbreviation. Also, none of the other callbacks have this suffix. None of the callbacks have the namespace prefixed, either, and that doesn't really make sense inside structs (since they have their own scope). How about naming this on_audio_frame and renaming (not now) all other callbacks to on_$event? There is really no reason to do this rename now though. Feel free to revert to acb, since not all of them have been renamed anyway (e.g. BWController::mcb is still a TLA).

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

    toxav_call_cb *toxav_call_fxn;
    toxav_call_state_cb *toxav_call_state_fxn;
    toxav_bit_rate_status_cb *toxav_bitrate_status_fxn;

Why was this moved?

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

    toxav_bit_rate_status_cb *toxav_bitrate_status_fxn;

    toxav_audio_receive_frame_cb *toxav_audio_frame_fxn; /* Audio frame receive callback */

(Awesome comment, I wouldn't have figured out that audio receive frame cb means audio frame receive callback). Perhaps remove the comment?

toxcore/Messenger.c, line 1714 at r1 (raw file):

 */
void m_callback_msi_packet(Messenger *m, void (*function)(Messenger *m, uint32_t, const uint8_t *, uint16_t, void *,
                           void *), void *toxav_session)

Rename to object so it's like all the other callbacks that take an extra object. We'll improve that later, but for now it's good to be consistent so you can grep for object.

Comments from Reviewable

JFreegman · 2016-09-12T15:42:36Z

other/apidsl/tox.in.h, line 1094 at r1 (raw file):

   * WARNING, calling ${friend.delete} while there's an active ToxAV call will
   * result in undefined behavior. It's the client's responsibility to end all
   * ToxAV calls before deleting a friend.

Why? The correct behaviour would be to have the core automatically end the call before deleting the friend.

Comments from Reviewable

JFreegman · 2016-09-12T15:58:18Z

other/apidsl/tox.in.h, line 1094 at r1 (raw file):

Previously, JFreegman wrote…

Why? The correct behaviour would be to have the core automatically end the call before deleting the friend.

Even an error would be better than undefined behaviour.

Comments from Reviewable

GrayHatter · 2016-09-13T18:17:16Z

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

other/apidsl/tox.in.h, line 1094 at r1 (raw file):

Previously, JFreegman wrote…

Even an error would be better than undefined behaviour.

We talked about this in IRC, toxcore could error, and not delete the friend. But that would be a change that only exists in this one case, and no one felt good about that.

ToxAV will delete the session/end the call. But in doing so it'll generate a callback (of which there's no clean way of avoiding) In this case, the callback won't have access to userdata, so userdata will just be NULL (and that's the reason for the UB warning).

Comments from Reviewable

GrayHatter · 2016-09-16T04:52:34Z

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

other/apidsl/toxav.in.h, line 34 at r1 (raw file):

Previously, iphydf wrote…

Why is this in the public API?

IMO macros should go into the header. But in this case, I'd forgotten that it's a public API header.

Because it's only used in toxav.c. I've fixed this.

other/apidsl/toxav.in.h, line 221 at r1 (raw file):

Previously, iphydf wrote…

any userdata like in tox.in.h.

Done.

toxav/audio.c, line 99 at r1 (raw file):

Previously, iphydf wrote…

I very much dislike "fxn" as abbreviation. Also, none of the other callbacks have this suffix. None of the callbacks have the namespace prefixed, either, and that doesn't really make sense inside structs (since they have their own scope). How about naming this on_audio_frame and renaming (not now) all other callbacks to on_$event? There is really no reason to do this rename now though. Feel free to revert to acb, since not all of them have been renamed anyway (e.g. BWController::mcb is still a TLA).

I'd hurt me to much to use a TLA here. changed to the former.

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

Previously, iphydf wrote…

Why was this moved?

not moved, renamed.

Because I dropped the macro wrapper that also created the userdata (in a struct var called second)

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

Previously, iphydf wrote…

(Awesome comment, I wouldn't have figured out that audio receive frame cb means audio frame receive callback). Perhaps remove the comment?

Done.

toxcore/Messenger.c, line 1714 at r1 (raw file):

Previously, iphydf wrote…

Rename to object so it's like all the other callbacks that take an extra object. We'll improve that later, but for now it's good to be consistent so you can grep for object.

Done.

But I don't like it...

Comments from Reviewable

iphydf · 2016-09-16T10:25:08Z

Review status: 0 of 20 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

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

    pthread_mutex_t mutex[1];

    toxav_call_cb *toxav_call_fxn;

Can you rename these things to on_something as well?

Comments from Reviewable

JFreegman · 2016-09-16T19:18:06Z

Review status: 0 of 20 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

Comments from Reviewable

JFreegman · 2016-09-16T19:18:14Z

Reviewed 12 of 20 files at r1, 8 of 8 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

Comments from Reviewable

GrayHatter · 2016-09-17T02:27:39Z

Review status: 1 of 20 files reviewed at latest revision, 2 unresolved discussions.

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

Previously, iphydf wrote…

Can you rename these things to on_something as well?

Done.

Comments from Reviewable

JFreegman · 2016-09-17T02:35:59Z

Reviewed 18 of 19 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

Comments from Reviewable

nurupo · 2016-09-17T04:38:07Z

Reviewed 1 of 20 files at r1, 17 of 19 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

Comments from Reviewable

nurupo · 2016-09-17T04:38:50Z

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

Comments from Reviewable

iphydf · 2016-09-17T08:16:54Z

Review status: all files reviewed at latest revision, 3 unresolved discussions.

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

    toxav_call_cb *on_call;
    toxav_call_state_cb *on_call_state;
    toxav_bit_rate_status_cb *on_bitrate_change;

This is incorrect. Bit rate status is not bit rate change. It is the "advice" callback.

Comments from Reviewable

GrayHatter · 2016-09-17T08:21:30Z

Review status: all files reviewed at latest revision, 2 unresolved discussions.

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

Previously, iphydf wrote…

This is incorrect. Bit rate status is not bit rate change. It is the "advice" callback.

I had the same thought. But regardless of what the client does, toxav assumes you'll accept it's bitrate. So the naming here is still consistent. When toxav changes what it want's the bitrate to be. You get this callback, just because the client ignores it, doesn't' change it's name.

I.e. ignored or not, the next time you get this callback, it'll be lower then the last time. It won't repeat the previous suggestion if ignored.

Comments from Reviewable

nurupo · 2016-09-17T08:40:05Z

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

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

Previously, GrayHatter (Gregory Mullen) wrote…

I had the same thought. But regardless of what the client does, toxav assumes you'll accept it's bitrate. So the naming here is still consistent. When toxav changes what it want's the bitrate to be. You get this callback, just because the client ignores it, doesn't' change it's name.

I.e. ignored or not, the next time you get this callback, it'll be lower then the last time. It won't repeat the previous suggestion if ignored.

I think it's not about whether toxcore will repeat previous suggestion or not, but about the actual bitrate of the video/audio going through the wire, which toxcore doesn't change on its own.

Comments from Reviewable

GrayHatter · 2016-09-17T08:50:02Z

Review status: 19 of 21 files reviewed at latest revision, 2 unresolved discussions.

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

Previously, nurupo wrote…

I think it's not about whether toxcore will repeat previous suggestion or not, but about the actual bitrate of the video/audio going through the wire, which toxcore doesn't change on its own.

Done.

Comments from Reviewable

mannol

I hate reviewable... Anyways I failed to see the point of this change.

mannol · 2016-09-17T09:45:51Z

toxav/toxav.c

@@ -36,16 +36,18 @@
 #include <stdlib.h>
 #include <string.h>

+#define TOXAV_PAIR(TYPE1__, TYPE2__) struct { TYPE1__ first; TYPE2__ second; }


why is this prefixed with TOXAV? It's not a part of the public API

mannol · 2016-09-17T09:48:18Z

And by change I mean this PR.

iphydf · 2016-09-17T13:10:30Z

@mannol: See #40 for the reason for this PR.

Review status: 19 of 21 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

Comments from Reviewable

mannol · 2016-09-17T13:15:48Z

This way you loose the ability to provide different user data pointers per callback; I can see that being useful. Anyways, it mostly effects upper levels so this (almost) LGTM.

iphydf · 2016-09-17T13:27:45Z

Yes, that is an intentional tradeoff. This way the API is simplified for the common case (most clients either don't use userdata or use a single userdata for all callbacks) and much easier to write FFI (foreign language bindings) for. It also makes it easier to reason about behaviour: currently, any function could potentially invoke a callback. Turns out, indeed some group functions directly invoke callbacks, so not every callback comes from tox_iterate. With stateless callbacks, we can be sure that only the iterate functions can ever invoke callbacks.

Review status: 19 of 21 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

Comments from Reviewable

mannol · 2016-09-17T13:34:55Z

Right. Once the TOXAV_PAIR is renamed I'll lgtm this.

mannol · 2016-09-17T18:21:45Z

Comments from Reviewable

mannol · 2016-09-17T18:21:54Z

Reviewed 1 of 20 files at r1, 18 of 19 files at r3, 1 of 2 files at r5, 1 of 1 files at r6.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

Comments from Reviewable

iphydf · 2016-09-17T20:58:06Z

Reviewed 1 of 20 files at r1, 18 of 19 files at r3, 1 of 2 files at r5, 1 of 1 files at r6.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

Comments from Reviewable

GrayHatter · 2016-09-17T21:09:03Z

unassigned @tux3 and @cleverca22 for being idle. 4/6 approved at time of merge.

GrayHatter force-pushed the toxav_stateless branch from 27efe0e to 91ec4fb Compare September 11, 2016 23:29

GrayHatter assigned cleverca22, JFreegman, nurupo, tux3, iphydf and mannol Sep 11, 2016

GrayHatter force-pushed the toxav_stateless branch 4 times, most recently from 23199d3 to c98a168 Compare September 11, 2016 23:52

GrayHatter force-pushed the toxav_stateless branch from 32e7e6b to 68b8d66 Compare September 16, 2016 20:44

GrayHatter force-pushed the toxav_stateless branch from 9e134ad to 4615275 Compare September 17, 2016 08:49

GrayHatter force-pushed the toxav_stateless branch from 4615275 to ebaa768 Compare September 17, 2016 08:53

mannol reviewed Sep 17, 2016

View reviewed changes

GrayHatter force-pushed the toxav_stateless branch from ebaa768 to 04066b7 Compare September 17, 2016 18:05

Make ToxAV stateless

21f8db1

GrayHatter force-pushed the toxav_stateless branch from 8db8c25 to 21f8db1 Compare September 17, 2016 21:06

GrayHatter unassigned cleverca22 and tux3 Sep 17, 2016

iphydf merged commit 21f8db1 into TokTok:master Sep 17, 2016

iphydf modified the milestone: v0.0.1 Nov 5, 2016

GrayHatter deleted the toxav_stateless branch November 19, 2016 07:18

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make ToxAV stateless #130

Make ToxAV stateless #130

GrayHatter commented Sep 11, 2016 •

edited by iphydf

Loading

iphydf commented Sep 12, 2016

JFreegman commented Sep 12, 2016

JFreegman commented Sep 12, 2016

GrayHatter commented Sep 13, 2016

GrayHatter commented Sep 16, 2016

iphydf commented Sep 16, 2016

JFreegman commented Sep 16, 2016

JFreegman commented Sep 16, 2016

GrayHatter commented Sep 17, 2016

JFreegman commented Sep 17, 2016

nurupo commented Sep 17, 2016

nurupo commented Sep 17, 2016

iphydf commented Sep 17, 2016

GrayHatter commented Sep 17, 2016

nurupo commented Sep 17, 2016

GrayHatter commented Sep 17, 2016

mannol left a comment

mannol Sep 17, 2016

mannol commented Sep 17, 2016

iphydf commented Sep 17, 2016

mannol commented Sep 17, 2016

iphydf commented Sep 17, 2016

mannol commented Sep 17, 2016

mannol commented Sep 17, 2016

mannol commented Sep 17, 2016

iphydf commented Sep 17, 2016

GrayHatter commented Sep 17, 2016

Make ToxAV stateless #130

Make ToxAV stateless #130

Conversation

GrayHatter commented Sep 11, 2016 • edited by iphydf Loading

iphydf commented Sep 12, 2016

JFreegman commented Sep 12, 2016

JFreegman commented Sep 12, 2016

GrayHatter commented Sep 13, 2016

GrayHatter commented Sep 16, 2016

iphydf commented Sep 16, 2016

JFreegman commented Sep 16, 2016

JFreegman commented Sep 16, 2016

GrayHatter commented Sep 17, 2016

JFreegman commented Sep 17, 2016

nurupo commented Sep 17, 2016

nurupo commented Sep 17, 2016

iphydf commented Sep 17, 2016

GrayHatter commented Sep 17, 2016

nurupo commented Sep 17, 2016

GrayHatter commented Sep 17, 2016

mannol left a comment

Choose a reason for hiding this comment

mannol Sep 17, 2016

Choose a reason for hiding this comment

mannol commented Sep 17, 2016

iphydf commented Sep 17, 2016

mannol commented Sep 17, 2016

iphydf commented Sep 17, 2016

mannol commented Sep 17, 2016

mannol commented Sep 17, 2016

mannol commented Sep 17, 2016

iphydf commented Sep 17, 2016

GrayHatter commented Sep 17, 2016

GrayHatter commented Sep 11, 2016 •

edited by iphydf

Loading