fix(*): fix external buffers #7
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| // work through frame bufs checking whether allocation refcounts are shared | ||
| for ( int x = 1 ; x < AV_NUM_DATA_POINTERS ; x++ ) { | ||
| // printf("Buffer %i is %p\n", x, f->frame->data[x]); | ||
| if (f->frame->data[x] == nullptr) continue; | ||
| size_t bufSize = size; | ||
| if (f->frame->buf[x] == nullptr) | ||
| bufSize = f->frame->data[x] - f->frame->data[x-1]; | ||
| status = napi_create_external_buffer(env, bufSize, data, frameBufferFinalizer, ref, &element); | ||
| status = napi_create_buffer_copy(env, bufSize, data, &resultData, &element); |
There was a problem hiding this comment.
I noticed you've removed the finalizers. Is there something we should still do for those? It seems like those are called when the objects are garbage collected. Do we not need to clean up the same way anymore?
There was a problem hiding this comment.
I guess this is what you mean in the description for
I'm not sure about memory leaks.
| @@ -56,7 +56,7 @@ void readComplete(napi_env env, napi_status asyncStatus, void *data) { | |||
| } | |||
| REJECT_STATUS; | |||
|
|
|||
| c->status = napi_create_external_buffer(env, c->readLen, c->readBuf, readFinalizer, (void*)(uint64_t)c->readLen, &result); | |||
There was a problem hiding this comment.
would NODE_API_NO_EXTERNAL_BUFFERS_ALLOWED be helpful to define? I saw that here when reading docs
michaeljherrmann
approved these changes
Feb 1, 2024
|
I'm closing this PR, because we found a way how to run nwjs in production which does not require this change |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Electron does not allow to use https://nodejs.org/api/n-api.html#napi_create_external_buffer
You can follow nodejs documentation or look at electron/electron#35801 directly
Because of this limitation I had to run old version of electron.
This PR fixes that problem.
I tried to avoid fix this issue itself, because I was not sure how to build beamcoder, how that c++ code should look, etc.
But it is just 3 lines of code, I tested it and it seems like it works.
I'm not sure about memory leaks.
My plan is just to release it and try to run electron on production