Skip to content
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

Extract named constants from magic numbers in toxav/audio.c. #698

Merged
merged 1 commit into from
Jan 19, 2018

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Jan 19, 2018

By @zoff99.


This change is Reviewable

@iphydf iphydf added this to the v0.2.0 milestone Jan 19, 2018
@iphydf iphydf force-pushed the named-constants branch 2 times, most recently from c6cc0a4 to 3577af1 Compare January 19, 2018 18:46
@sudden6
Copy link

sudden6 commented Jan 19, 2018

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


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

    /* Enough space for the maximum frame size (120 ms 48 KHz stereo audio) */
    int16_t temp_audio_buffer[5760 * 2];

use the constant to define this size


toxav/audio.h, line 32 at r1 (raw file):

#define AUDIO_JITTERBUFFER_COUNT 3
#define AUDIO_MAX_SAMPLING_RATE 48000

usually it's SAMPLE_RATE


toxav/audio.h, line 35 at r1 (raw file):

#define AUDIO_MAX_CHANNEL_COUNT 2

#define AUDIO_START_SAMPLING_RATE 48000

same


toxav/audio.h, line 36 at r1 (raw file):

#define AUDIO_START_SAMPLING_RATE 48000
#define AUDIO_START_BITRATE_RATE 48000

BITRATE_RATE seems strange, I recommend BITRATE


toxav/audio.h, line 47 at r1 (raw file):

// ((sampling_rate_in_hz * frame_duration_in_ms) / 1000) * 2 // because PCM16 needs 2 bytes for 1 sample
#define AUDIO_MAX_BUFFER_SIZE_PCM16_FOR_FRAME_PER_CHANNEL ((AUDIO_MAX_SAMPLING_RATE * AUDIO_MAX_FRAME_DURATION_MS) / 1000)

rename to AUDIO_MAX_BUFFER_SIZE_PCM16 and add a comment that this is per frame and per channel?


toxav/audio.h, line 48 at r1 (raw file):

// ((sampling_rate_in_hz * frame_duration_in_ms) / 1000) * 2 // because PCM16 needs 2 bytes for 1 sample
#define AUDIO_MAX_BUFFER_SIZE_PCM16_FOR_FRAME_PER_CHANNEL ((AUDIO_MAX_SAMPLING_RATE * AUDIO_MAX_FRAME_DURATION_MS) / 1000)
#define AUDIO_MAX_BUFFER_SIZE_BYTES_FOR_FRAME_PER_CHANNEL (AUDIO_MAX_BUFFER_SIZE_PCM16_FOR_FRAME_PER_CHANNEL * 2)

rename to AUDIO_MAX_BUFFER_SIZE_BYTES and add a comment that this is per frame and per channel?


Comments from Reviewable

@iphydf
Copy link
Member Author

iphydf commented Jan 19, 2018

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


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

Previously, sudden6 wrote…

use the constant to define this size

Done.


toxav/audio.h, line 32 at r1 (raw file):

Previously, sudden6 wrote…

usually it's SAMPLE_RATE

Done.


toxav/audio.h, line 35 at r1 (raw file):

Previously, sudden6 wrote…

same

Done.


toxav/audio.h, line 36 at r1 (raw file):

Previously, sudden6 wrote…

BITRATE_RATE seems strange, I recommend BITRATE

Done.


toxav/audio.h, line 47 at r1 (raw file):

Previously, sudden6 wrote…

rename to AUDIO_MAX_BUFFER_SIZE_PCM16 and add a comment that this is per frame and per channel?

Done.


toxav/audio.h, line 48 at r1 (raw file):

Previously, sudden6 wrote…

rename to AUDIO_MAX_BUFFER_SIZE_BYTES and add a comment that this is per frame and per channel?

Done.


Comments from Reviewable

@sudden6
Copy link

sudden6 commented Jan 19, 2018

:lgtm_strong:


Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@iphydf iphydf merged commit f1d726a into TokTok:master Jan 19, 2018
@iphydf iphydf deleted the named-constants branch January 19, 2018 20:45
zoff99 added a commit to zoff99/c-toxcore that referenced this pull request Jan 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants