Pulseaudio: Update pulseaudio comments#1046
Pulseaudio: Update pulseaudio comments#1046illuusio wants to merge 10 commits intoPortAudio:masterfrom
Conversation
philburk
left a comment
philburk
left a comment
There was a problem hiding this comment.
Thanks. These comments are more clear.
| { | ||
| PaPulseAudio_Lock(stream->mainloop); | ||
| /* Pause stream so it stops faster */ | ||
| /* The stream should be paused to facilitate a quicker cessation |
There was a problem hiding this comment.
The original comment was more concise and more clear. How were these comments generated?
There was a problem hiding this comment.
Mostly with GPT and then altered by hand
|
I've made more small adjustments to comments. Now they should be more shorter and precises. |
|
@illuusio I have pushed some edits (see my latest commit). Could you please review them? I have some additional remarks which I'll put into a review now. |
RossBencina
left a comment
RossBencina
left a comment
There was a problem hiding this comment.
I have added specific review comments on the lines where I think that there are obvious remaining issues. I found a few more minor grammatical issues during review and I will commit those now.
There are still a few places wher ethe code comments which have unclear meaning.
There are a some cases where the code comments indicate half duplex input-only code paths, but the logic of the code seems to activate for both input-only and input+output code paths. I don't know whether the code has a bug or the comments are wrong. Please review the code and the logic and let me know whether you see what I mean. Otherwise I can explain further.
I might have found a bug: missing usleep() call in PaPulseAudio_ReleaseOperation
I fear than many of the GPT translations were hallucinated and not consistent with the intent of the code. I am concerned that you did not already pick this up. Before we finalise the PR please make sure that every comment in the final diff is consistent with your understanding of the code. If there are remaining cases where no one understands the meaning of a comment then the comments can only serve to confuse and we should delete them.
Also please let's work methodically through my review comments, plase don't make other unrelated changes as that will only confuse things further.
| PaPulseAudio_UnLock( hostapi->mainloop ); | ||
|
|
||
| waitOperation --; | ||
| wait --; |
There was a problem hiding this comment.
?BUG: if the earlier comment is correct, and the retry loop runs for seconds/milliseconds, shouldn't there be a call to usleep() here at the end of the loop body?
There was a problem hiding this comment.
Yes there should. Good catch.
There was a problem hiding this comment.
Fixed this one also
There was a problem hiding this comment.
Each PR needs to address a specific issue. Changing code for an unrelated purpose is not appropriate in the current PR. Please factor all of the changes to PaPulseAudio_ReleaseOperation (including both the code and the comments) into a separate PR. The comments in this function may need to be clarified but we can discuss that in the new PR.
| while( waitOperation > 0 ) | ||
| /* Since the primary operations are conducted locally, a wait time | ||
| * of 1 to 3 seconds, followed by an additional 1000 milliseconds, | ||
| * is deemed sufficient to to detect successful completion or to detect an error. |
illuusio
force-pushed
the
update-pulseaudio-comments
branch
from
0b26a76 to
1bc092c
Compare
|
Is this ready to merge? |
illuusio
force-pushed
the
update-pulseaudio-comments
branch
2 times, most recently
from
2152cb2 to
f62186b
Compare
illuusio
force-pushed
the
update-pulseaudio-comments
branch
from
f62186b to
a4e2d5d
Compare
|
Thanks for the updates. I have deleted two comments that were low-quality and redundant. Remaining issues:
|
illuusio
force-pushed
the
update-pulseaudio-comments
branch
from
eddf436 to
35551a7
Compare
I've decided to remove comment 402 as it does belong there and it's not correct place to comment that. On line 360 I've updated commenting and I've formed new PR (actually 2 to also remove that missedBytes which is not used anywhere and should not be there anymore) |
…d for clarity and specificity. The comments in _PaPulseAudio_ProcessAudio required enhancement; therefore, the comments have been revised to provide greater specificity and improved clarity in the English language.
…ated. The remaining comments in the PulseAudio callback file should be updated to utilize passive voice and improved English for clarity.
Make more adjustments to pulseaudio cb comments.
As Ringbuffer it does not underrun but it does overrun data. Co-authored-by: Ross Bencina <rossb@audiomulch.com>
Simplify comment about buffer size calculation Co-authored-by: Ross Bencina <rossb@audiomulch.com>
Update unlocking comment wording to more understandable Co-authored-by: Ross Bencina <rossb@audiomulch.com>
illuusio
force-pushed
the
update-pulseaudio-comments
branch
from
35551a7 to
1fb706f
Compare
3 participants
This pull request addresses concerns regarding the comments in the file by updating several comments within the PulseAudio files.