docs(toxav): fix docs of toxav.h

[Green-Sky]

• fix units to be better readable (Kb/sec -> kbit/sec)
• use width before height consistently
• video -> audio typo

---

for another pr: fix video being actually Mbit/sec #2782

---

This change is 

[zoff99]

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

[zoff99]

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]

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]

@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]

@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]

yeah its wrong in toktok:

c-toxcore/toxav/video.c

Line 420 in 5344d7f

cfg.rc_target_bitrate = bit_rate;

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]

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

[zoff99]

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]

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

[Green-Sky]

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]

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" ?
#1431
#2651
(shrug)

[Green-Sky]

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]

back to draft, until either toxav prs is merged.

[Green-Sky]

Update: this no longer changes the described bitrate of video to mega bits.
#2782

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.00%. Comparing base (5f97890) to head (06aea13).
Report is 8 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2754      +/-   ##
==========================================
- Coverage   73.04%   73.00%   -0.04%     
==========================================
  Files         149      149              
  Lines       30478    30478              
==========================================
- Hits        22263    22251      -12     
- Misses       8215     8227      +12     

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

[nurupo]

That's good. The Mbit change is API / behavior breaking, so it's good to have it in a separate PR until we are ready to break the API.

[nurupo]

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 0 of 2 approvals obtained (waiting on @Green-Sky, @iphydf, and @zoff99)

[nurupo]

The Mbit change is API / behavior breaking, so it's good to have it in a separate PR until we are ready to break the API.

I would like to correct my earlier comment that that Mbit change is, in fact, not API breaking.

I thought the API used to say Mbit/s before, due to various force-push diffs in this PR doing the Mbit/s -> kbit/s function doc change, in which case changing it to kbit/s would be API-breaking, but I went and checked 5 years worth of the past revisions of toxav.h and apparently it has always been kbit/s, so no API is being broken.

[Green-Sky]

@nurupo yep, and it's fixed now on master #2782

[nurupo]

Yeah, that PR being merged so unceremoniously is what made me go back and double-check if it was actually API breaking or not, before I yell at iphydf for breaking the API. Apparently it wasn't, which is good.