Implement aufile_set_position #943
Conversation
rem/aufile/aufile.c
Outdated
| * | ||
| * @return position in bytes or (size_t)-1 in case of an error. | ||
| */ | ||
| size_t aufile_set_position(struct aufile *af, struct aufile_prm *prm, size_t pos_ms) |
There was a problem hiding this comment.
Normally in re all functions except getter return an error code if there are errors possible.
API functions should contain NULL pointer checks.
if (!af || !prm)
return EINVAL;
rem/aufile/aufile.c
Outdated
| */ | ||
| size_t aufile_set_position(struct aufile *af, struct aufile_prm *prm, size_t pos_ms) | ||
| { | ||
| size_t pos = prm->srate * aufmt_sample_size(prm->fmt) * prm->channels * pos_ms / 1000; |
There was a problem hiding this comment.
This is only an approximation, right? Is it a good approximation for all WAV files?
Shouldn't the WAV header size be added?
There was a problem hiding this comment.
Thanks & yes. The wav header needs to be taken into account.
Also observe max column length of 80.
rem/aufile/aufile.c
Outdated
| return EINVAL; | ||
| } | ||
|
|
||
| // Seek to the beginning of the file |
There was a problem hiding this comment.
Can be removed. It's obvious.
rem/aufile/aufile.c
Outdated
| return errno; | ||
| } | ||
| // this is only used for the side effect of moving the file ptr to the | ||
| // first data block. |
There was a problem hiding this comment.
- Use C comments!
- Please shorter: `/* skip wav header */
rem/aufile/aufile.c
Outdated
| * | ||
| * @return position in bytes or (size_t)-1 in case of an error. | ||
| */ | ||
| size_t aufile_set_position(struct aufile *af, struct aufile_prm *prm, |
There was a problem hiding this comment.
- return int as error code
- correct @return doxygen
rem/aufile/aufile.c
Outdated
| * | ||
| * @return 0 if success, otherwise errorcode | ||
| */ | ||
| int aufile_set_position(struct aufile *af, struct aufile_prm *prm, |
There was a problem hiding this comment.
is prm modified by the function ? can it be const ?
rem/aufile/aufile.c
Outdated
|
|
||
| if (!af || !prm) { | ||
| return EINVAL; | ||
| } |
There was a problem hiding this comment.
please remove curly braces that is not needed here ... same for the rest of the function
rem/aufile/aufile.c
Outdated
| struct wav_fmt fmt; | ||
| int err; | ||
| off_t pos; | ||
| size_t datasize; |
There was a problem hiding this comment.
please use C99 style declare+assign if possible ...
include/rem_aufile.h
Outdated
| @@ -26,3 +26,5 @@ int aufile_read(struct aufile *af, uint8_t *p, size_t *sz); | |||
| int aufile_write(struct aufile *af, const uint8_t *p, size_t sz); | |||
| size_t aufile_get_size(struct aufile *af); | |||
| size_t aufile_get_length(struct aufile *af, struct aufile_prm *prm); | |||
| int aufile_set_position(struct aufile *af, struct aufile_prm *prm, | |||
| off_t pos_ms); | |||
There was a problem hiding this comment.
it is probably better to use size_t instead of off_t here ...
There was a problem hiding this comment.
I did that first, but it's fseek(FILE *, long, int); and using size_t breaks the Windows build.
|
my idea was to use |
|
Done. The cast to |
|
LGTM |
This PR implements
aufile_set_position, which sets the position in milliseconds.