You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "anchao (via GitHub)" <gi...@apache.org> on 2025/03/10 02:15:04 UTC
[PR] system/nxplayer/nxrecorder: move apb buffer instance to stack [nuttx-apps]
anchao opened a new pull request, #3023:
URL: https://github.com/apache/nuttx-apps/pull/3023
## Summary
system/nxplayer/nxrecorder: move apb buffer instance to stack
move apb buffer instance to stack to avoid alloc buffer from heap
Signed-off-by: chao an <an...@bytedance.com>
## Impact
N/A
## Testing
ci-check
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] system/nxplayer/nxrecorder: move apb buffer instance to stack [nuttx-apps]
Posted by "anchao (via GitHub)" <gi...@apache.org>.
anchao commented on code in PR #3023:
URL: https://github.com/apache/nuttx-apps/pull/3023#discussion_r2012122191
##########
system/nxplayer/nxplayer.c:
##########
@@ -819,23 +818,11 @@ static FAR void *nxplayer_playthread(pthread_addr_t pvarg)
/* Create array of pointers to buffers */
- buffers = (FAR struct ap_buffer_s **)
- malloc(buf_info.nbuffers * sizeof(FAR void *));
- if (buffers == NULL)
- {
- /* Error allocating memory for buffer storage! */
-
- ret = -ENOMEM;
- running = false;
- goto err_out;
- }
+ FAR struct ap_buffer_s *buffers[buf_info.nbuffers];
/* Create our audio pipeline buffers to use for queueing up data */
- for (x = 0; x < buf_info.nbuffers; x++)
- {
- buffers[x] = NULL;
- }
+ memset(buffers, 0, sizeof(FAR void *) * buf_info.nbuffers);
Review Comment:
Done
##########
system/nxrecorder/nxrecorder.c:
##########
@@ -551,23 +550,11 @@ static FAR void *nxrecorder_recordthread(pthread_addr_t pvarg)
/* Create array of pointers to buffers */
- pbuffers = (FAR struct ap_buffer_s **) malloc(buf_info.nbuffers *
- sizeof(FAR void *));
- if (pbuffers == NULL)
- {
- /* Error allocating memory for buffer storage! */
-
- ret = -ENOMEM;
- running = false;
- goto err_out;
- }
+ FAR struct ap_buffer_s *pbuffers[buf_info.nbuffers];
/* Create our audio pipeline buffers to use for queueing up data */
- for (x = 0; x < buf_info.nbuffers; x++)
- {
- pbuffers[x] = NULL;
- }
+ memset(pbuffers, 0, sizeof(FAR void *) * buf_info.nbuffers);
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] system/nxplayer/nxrecorder: move apb buffer instance to stack [nuttx-apps]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #3023:
URL: https://github.com/apache/nuttx-apps/pull/3023#discussion_r1986710953
##########
system/nxrecorder/nxrecorder.c:
##########
@@ -551,23 +550,11 @@ static FAR void *nxrecorder_recordthread(pthread_addr_t pvarg)
/* Create array of pointers to buffers */
- pbuffers = (FAR struct ap_buffer_s **) malloc(buf_info.nbuffers *
- sizeof(FAR void *));
- if (pbuffers == NULL)
- {
- /* Error allocating memory for buffer storage! */
-
- ret = -ENOMEM;
- running = false;
- goto err_out;
- }
+ FAR struct ap_buffer_s *pbuffers[buf_info.nbuffers];
/* Create our audio pipeline buffers to use for queueing up data */
- for (x = 0; x < buf_info.nbuffers; x++)
- {
- pbuffers[x] = NULL;
- }
+ memset(pbuffers, 0, sizeof(FAR void *) * buf_info.nbuffers);
Review Comment:
ditto
##########
system/nxplayer/nxplayer.c:
##########
@@ -819,23 +818,11 @@ static FAR void *nxplayer_playthread(pthread_addr_t pvarg)
/* Create array of pointers to buffers */
- buffers = (FAR struct ap_buffer_s **)
- malloc(buf_info.nbuffers * sizeof(FAR void *));
- if (buffers == NULL)
- {
- /* Error allocating memory for buffer storage! */
-
- ret = -ENOMEM;
- running = false;
- goto err_out;
- }
+ FAR struct ap_buffer_s *buffers[buf_info.nbuffers];
/* Create our audio pipeline buffers to use for queueing up data */
- for (x = 0; x < buf_info.nbuffers; x++)
- {
- buffers[x] = NULL;
- }
+ memset(buffers, 0, sizeof(FAR void *) * buf_info.nbuffers);
Review Comment:
```suggestion
memset(buffers, 0, sizeof(buffer));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] system/nxplayer/nxrecorder: move apb buffer instance to stack [nuttx-apps]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 merged PR #3023:
URL: https://github.com/apache/nuttx-apps/pull/3023
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] system/nxplayer/nxrecorder: move apb buffer instance to stack [nuttx-apps]
Posted by "nuttxpr (via GitHub)" <gi...@apache.org>.
nuttxpr commented on PR #3023:
URL: https://github.com/apache/nuttx-apps/pull/3023#issuecomment-2709267869
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues)
No, this PR does not fully meet the NuttX requirements. While it provides a summary of the change, it lacks crucial details.
Here's a breakdown of the missing information:
* **Insufficient Summary:** While the summary explains *what* was changed, it doesn't fully explain *why*. Why is allocating from the heap problematic in this specific case? What benefits does moving to the stack provide (e.g., performance improvement, deterministic memory usage, avoiding heap fragmentation)? What is "apb buffer"? More context is needed. It also lacks any mention of related issues.
* **Missing Impact Assessment:** Simply stating "N/A" is insufficient. Even seemingly small changes can have unforeseen impacts. Consider these points:
* **Impact on hardware:** Does this change affect any specific architectures or boards? It's unlikely, but needs explicit confirmation.
* **Impact on compatibility:** Does this change affect any existing applications using the nxrecorder? Again, unlikely but needs to be addressed.
* **Impact on security:** While unlikely in this case, always consider potential security implications. For example, does moving the buffer to the stack introduce any risks related to stack overflow? Explicitly stating "NO" with a brief justification is better than "N/A".
* **Inadequate Testing:** "ci-check" is not sufficient. While CI is important, the PR needs to demonstrate *specific* tests performed locally that validate the change. What functionality was tested? What were the expected results? Providing actual log snippets (even if they're short and show expected behavior) is crucial for reviewers to understand the testing process. Furthermore, the details of the build host and target(s) are missing.
**In short:** The PR needs more detail and explicit confirmation (even if negative) for several impact categories. The testing section requires significant improvement to show how the changes were validated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org