TokTok / c-toxcore

The future of online communications.
https://tox.chat
GNU General Public License v3.0
2.21k stars 280 forks source link

docs(toxav): fix docs of toxav.h #2754

Open Green-Sky opened 2 months ago

Green-Sky commented 2 months ago

This change is Reviewable

zoff99 commented 2 months ago

acutally "b" (lowercase b) always means bits. so Kb is always Kilobit(s). versus KB and KiB

zoff99 commented 2 months ago

and video bitrate is correct as Kb/s (kilo bits per second). its multiplied by 1000 later because vpx codec needs the value as b/s (bits per second)

well i am still checking this. its been a long time. hold on a bit ...

nurupo commented 2 months ago

zoff is right, Kb and kbit are the same unit, so that particular unit is not really being fixed, just reworded. I do support the change though, being more explicit/clear in the documentation and spelling it out is probably a good thing.

Green-Sky commented 2 months ago

@zoff99 yea we multiply both audio and video rates by 1000, BUT vpx_encoder.h:

  /*!\brief Target data rate
   *
   * Target bitrate to use for this stream, in kilobits per second.
   */
  unsigned int rc_target_bitrate;
zoff99 commented 2 months ago

@zoff99 yea we multiply both audio and video rates by 1000, BUT vpx_encoder.h:

  /*!\brief Target data rate
   *
   * Target bitrate to use for this stream, in kilobits per second.
   */
  unsigned int rc_target_bitrate;

yeah thats why i say im still checking. in my forks i think its correct but let me actually check ...

zoff99 commented 2 months ago

yeah its wrong in toktok:

https://github.com/TokTok/c-toxcore/blob/5344d7f84d060311f2147b343cdd3fd4c433d7dd/toxav/video.c#L420

but fixed in mine:

https://github.com/zoff99/c-toxcore/blob/3511870af9928c7d09a47f0196ccb0ed3f0e0e9c/toxav/codecs/vpx/codec.c#L490

cfg2.rc_target_bitrate = (bit_rate / 1000);

Green-Sky commented 2 months ago

sidenote: opus changed, and the new meaningful lower bound for the bitrate is 0.5 ...

zoff99 commented 2 months ago

but in any case the toxav PR needs to be merged first. fixes of old bugs come only afterwards. we made a roadmap and agreed on it to my recollection

Green-Sky commented 2 months ago

@zoff99 do we preserve behavoir and adjust the comments (this pr as-is) or do we fix it and change behavior?

Green-Sky commented 2 months ago

but in any case the toxav PR needs to be merged first. fixes of old bugs come only afterwards. we made a roadmap and agreed on it to my recollection

This pr has no conflicts with your pr, so that does not really matter, only if we change the code.

Also, someone needs to fix circle ci first ...

zoff99 commented 2 months ago

i would imagine we rebase and tweak this PR after toxav pr is merged. what are the blockers now that the toxav PR can't be merged now-ish ? then lets address those, if any.

and which one is "the one" ? https://github.com/TokTok/c-toxcore/pull/1431 https://github.com/TokTok/c-toxcore/pull/2651 (shrug)

Green-Sky commented 2 months ago

and which one is "the one" ? #1431 #2651 (shrug)

Oh I did not even see the other. the newer one has no conflicts, so...

Green-Sky commented 2 months ago

back to draft, until either toxav prs is merged.