Open nurupo opened 1 month ago
@iphydf cimple doesn't recognize OPUS_SET_VBR macro
c-toxcore/toxav/audio.c:388: definition of
create_audio_encoder
references undefined global function/constantOPUS_SET_VBR
[-Wcallgraph]
It's defined in opus_defines.h, which is included by opus.h, included by audio.h, included by audio.c.
is this fully working?
#define OPUS_SET_VBR
Warning:
Only the MDCT mode of Opus can provide hard CBR behavior.
#define OPUS_SET_VBR_CONSTRAINT
Warning:
Only the MDCT mode of Opus currently heeds the constraint. Speech mode ignores it completely, hybrid mode may fail to obey it if the LPC layer uses more bitrate than the constraint would have permitted.
@zoff99 Are you saying that it's not fully working for you? How so?
Also, what are quoting? Can you link to the source of what are you quoting? Is that an output you get when compiling toxav? Perhaps an output of one of our CIs? Or are you just quoting the documentation?
define OPUS_SET_VBR
Warning: Only the MDCT mode of Opus can provide hard CBR behavior.
I saw this warning when reading earlier versions of the Opus documentation, e.g. in Opus 1.0.3 released on Jul 11, 2013. But later versions removed that warning, e.g. it seems to be gone in Opus 1.1.3 released on Jul 15, 2016, and the later versions. So it appears that this was resolved in Opus long time ago and that warning no longer applies.
Am I wrong? Do you think we need to enable the MDCT mode?
define OPUS_SET_VBR_CONSTRAINT
Warning: Only the MDCT mode of Opus currently heeds the constraint. Speech mode ignores it completely, hybrid mode may fail to obey it if the LPC layer uses more bitrate than the constraint would have permitted.
Could you clarify on how is OPUS_SET_VBR_CONSTRAINT
relevant here?
The documentation says:
so it appears that this setting does nothing when using the CBR mode, which is what this PR makes toxav use.
Am I missing something?
i activated CBR mode for opus some years ago in my fork, and then switched back to VBR again. can't remember the reason right now.
if we are switching to CBR mode i would want that $someone tests with the 2 (3) still maintained clients in some real world audio calls. audio calls in UDP mode, audio calls in TCP mode, audio calls in TCP mode over TOR. if that works without any issues (compared the what we have now) we can merge and i will also do that in my fork.
I'm unable to test it at the moment. Did you get any chance to test this, @zoff99?
There is a person on IRC claiming that it works for them using toxic and qtox clients, over localhost udp, tcp and tcp over tor. Tough given they are the same parson who authored the PR diff, I'd prefer if someone else tested it too.
i will hopefully get the chance to test it in the next weeks
first test completed: tox_videoplayer -> toxblinkenwall (audio quality test with https://yewtu.be/watch?v=o8dEgTMZ81g) no issues in quality with CBR mode on LAN
2nd test completed: making sure it actually has the wanted effect in the context and setup of toxav (caveat: using my fork) (tested with tox_videplayer and speech)
with VBR:
===> opus bytes: 8
===> opus bytes: 8
===> opus bytes: 8
===> opus bytes: 8
===> opus bytes: 8
===> opus bytes: 8
===> opus bytes: 8
===> opus bytes: 8
===> opus bytes: 8
===> opus bytes: 629
===> opus bytes: 985
===> opus bytes: 962
===> opus bytes: 962
===> opus bytes: 962
===> opus bytes: 962
===> opus bytes: 956
===> opus bytes: 876
===> opus bytes: 874
===> opus bytes: 854
===> opus bytes: 950
===> opus bytes: 962
===> opus bytes: 962
with CBR:
===> opus bytes: 960
===> opus bytes: 960
===> opus bytes: 960
===> opus bytes: 960
===> opus bytes: 960
===> opus bytes: 960
===> opus bytes: 960
===> opus bytes: 960
===> opus bytes: 960
===> opus bytes: 960
===> opus bytes: 960
===> opus bytes: 960
===> opus bytes: 960
===> opus bytes: 960
===> opus bytes: 960
===> opus bytes: 960
===> opus bytes: 960
===> opus bytes: 960
===> opus bytes: 960
===> opus bytes: 960
===> opus bytes: 960
===> opus bytes: 960
===> opus bytes: 960
===> opus bytes: 960
===> opus bytes: 960
===> opus bytes: 960
===> opus bytes: 960
===> opus bytes: 960
===> opus bytes: 960
seems it does work properly. but it does waste a lot of bandwidth especially if only 1 person is talking at a time in a call.
@JFreegman did anybody test this with toxic audio and video calls?
VBR is susceptible to a transcription attack, where words can be deducted from bandwidth fluctuations, even despite the audio being encrypted. Toxcore does add padding, but it's just 0-7 bytes, to pad to a 8 byte boundary, which might not be enough. CBR is safe from this attack, it is the industry recommendation to use CBR: "Applications conveying highly sensitive unstructured information SHOULD NOT use codecs in VBR mode."[1], and is what other secure messengers use too, e.g. Signal.
Here are some papers on this topic:
Reconstruction of Encrypted VoIP Conversations: Hookt on Fon-iks," 2011 IEEE Symposium on Security and Privacy, Oakland, CA, USA, 2011, pp. 3-18, doi: 10.1109/SP.2011.34.
Thanks to an IRC user who asked to remain anonymous for sending the diff.
[1] https://datatracker.ietf.org/doc/html/rfc6562#section-3
This change is