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

createEvent broken by mutating strings #77

Closed
ThaUnknown opened this issue Oct 20, 2020 · 5 comments · Fixed by #122
Closed

createEvent broken by mutating strings #77

ThaUnknown opened this issue Oct 20, 2020 · 5 comments · Fixed by #122
Labels

Comments

@ThaUnknown
Copy link

ThaUnknown commented Oct 20, 2020

adding an event with createEvent

{
Duration: 4880,
Effect: "",
Layer: 0,
MarginL: 0,
MarginR: 0,
MarginV: 0,
Name: "",
ReadOrder: 0,
Start: 7870,
Style: 1,
Text: "Text."
}

for some reason creates an event that looks like:

{
Duration: 4880,
Effect: "Text.",
Layer: 0,
MarginL: 0,
MarginR: 0,
MarginV: 0,
Name: "Text.",
ReadOrder: 0,
Start: 7870,
Style: 1,
Text: "Text."
}

breaking the entire renderer, adding more subtitles with the createEvent function overwrites the text, effect and name values of the previous events

@ThaUnknown
Copy link
Author

are there any fixes for this? its becoming a serious issues for very well typesetted subs like [μtw-Kaleido] releases, which can be shown here: 60a5523d6380ed07ff63648b76324c6ca8192faf, i currently work around this issue by building the sub file and passing it via octopusInstance.setTrack() but for videos with a few thousand subtitles it kills the service worker thread, even on high-end hardware!

@TheOneric
Copy link
Member

TheOneric commented Aug 7, 2021

Can confirm this still happens with current master. And even worse, if you add another event all strings in the previously created events change to the newly added string. This appears to be an issue with improperly passing memory/pointers from JS to WASM.

On a side note though, you should not create events with a non-unique ReadOrder value. Your example sets ReadOrder: 0 which would be valid if the track was empty before, but otherwise this has the potential to mess things up.

@TheOneric TheOneric added the bug label Aug 7, 2021
@TheOneric TheOneric changed the title createEvent issue createEvent broken by mutating strings Aug 7, 2021
@TheOneric
Copy link
Member

TheOneric commented Aug 7, 2021

@TFSThiagoBR98: Seems like you added this in ec21ec3, do you know what the "best practce"-way of passing strings from JS to WASM is? This likely affects more than just createEvent.
EDIT: Probably affected are createEvent, setEvent, createStyle and getStyle. Furthermore setEvent, setStyle, removEvent and removeStyle are hard to use right as such direct modifications aren't always allowed by libass.

@TFSThiagoBR98
Copy link
Collaborator

WebIDL uses this 2 generated functions:

void EMSCRIPTEN_KEEPALIVE emscripten_bind_ASS_Event_set_Text_1(ASS_Event* self, char* arg0) {
  self->Text = arg0;
}

and its JS counterpart:

ASS_Event.prototype['set_Text'] = ASS_Event.prototype.set_Text = /** @suppress {undefinedVars, duplicate} */function(arg0) {
  var self = this.ptr;
  ensureCache.prepare();
  if (arg0 && typeof arg0 === 'object') arg0 = arg0.ptr;
  else arg0 = ensureString(arg0);
  _emscripten_bind_ASS_Event_set_Text_1(self, arg0);
};

Also the ensureString function:

function ensureString(value) {
  if (typeof value === 'string') {
    var intArray = intArrayFromString(value);
    var offset = ensureCache.alloc(intArray, HEAP8);
    ensureCache.copy(intArray, HEAP8, offset);
    return offset;
  }
  return value;
}

This uses the intArrayFromString function but Emscripten also have the stringToUTF8, see here

@ThaUnknown
Copy link
Author

WebIDL uses this 2 generated functions:

void EMSCRIPTEN_KEEPALIVE emscripten_bind_ASS_Event_set_Text_1(ASS_Event* self, char* arg0) {
  self->Text = arg0;
}

and its JS counterpart:

ASS_Event.prototype['set_Text'] = ASS_Event.prototype.set_Text = /** @suppress {undefinedVars, duplicate} */function(arg0) {
  var self = this.ptr;
  ensureCache.prepare();
  if (arg0 && typeof arg0 === 'object') arg0 = arg0.ptr;
  else arg0 = ensureString(arg0);
  _emscripten_bind_ASS_Event_set_Text_1(self, arg0);
};

Also the ensureString function:

function ensureString(value) {
  if (typeof value === 'string') {
    var intArray = intArrayFromString(value);
    var offset = ensureCache.alloc(intArray, HEAP8);
    ensureCache.copy(intArray, HEAP8, offset);
    return offset;
  }
  return value;
}

This uses the intArrayFromString function but Emscripten also have the stringToUTF8, see here

so any chance this will get fixed?

TFSThiagoBR98 added a commit to TFSThiagoBR98/JavascriptSubtitlesOctopus that referenced this issue Jan 30, 2022
TFSThiagoBR98 added a commit to TFSThiagoBR98/JavascriptSubtitlesOctopus that referenced this issue Jan 30, 2022
TFSThiagoBR98 added a commit to TFSThiagoBR98/JavascriptSubtitlesOctopus that referenced this issue Feb 14, 2022
TFSThiagoBR98 added a commit to TFSThiagoBR98/JavascriptSubtitlesOctopus that referenced this issue Feb 14, 2022
TFSThiagoBR98 added a commit to TFSThiagoBR98/JavascriptSubtitlesOctopus that referenced this issue Feb 14, 2022
TFSThiagoBR98 added a commit to TFSThiagoBR98/JavascriptSubtitlesOctopus that referenced this issue Feb 14, 2022
TFSThiagoBR98 added a commit to TFSThiagoBR98/JavascriptSubtitlesOctopus that referenced this issue Feb 15, 2022
TFSThiagoBR98 added a commit to TFSThiagoBR98/JavascriptSubtitlesOctopus that referenced this issue Feb 18, 2022
TFSThiagoBR98 added a commit to TFSThiagoBR98/JavascriptSubtitlesOctopus that referenced this issue Feb 19, 2022
From @TheOneric:
  For most libass APIs the char* strings remain owned by the caller with libass doing
  internal copies if needed. This means the caller must if necessary free the memory
  at some point after the libass API call finished. The track and event modification/creation
  APIs are different because they work close to library internals (which is also why their usage
  comes with more restrictions/obligations than other APIs and why they don't sensibly work with
  ass.h alone).

Since we transfer ownership of the pointer to libass, this is unlikely to cause memory leaks.
TFSThiagoBR98 added a commit to TFSThiagoBR98/JavascriptSubtitlesOctopus that referenced this issue Mar 11, 2022
Currently the ensureCache create a temporary pointer that will
transfer the value to the WASM/C part and recycle it with to use in the
next value, but since the libass struct pointers are owned by the
library, the pointers can't be recycled or freed or can lead into
an undefined behavior.

This patch allocate new memory and copy the pointer before set into struct
attribute.

To avoid creating complex code, I decided to fix it in the webidl binder, but
i plan to make a way to mark if the attribute need to copy the pointer.
TFSThiagoBR98 added a commit to TFSThiagoBR98/JavascriptSubtitlesOctopus that referenced this issue Mar 11, 2022
Currently the ensureCache create a temporary pointer that will
transfer the value to the WASM/C part and recycle it with to use in the
next value, but since the libass struct pointers are owned by the
library, the pointers can't be recycled or freed or can lead into
an undefined behavior.

This configure the binder to tranfer the pointer ownership instead of
recycle it.

To avoid creating complex code, I decided to fix it in the webidl
binder.
TFSThiagoBR98 added a commit to TFSThiagoBR98/JavascriptSubtitlesOctopus that referenced this issue Mar 13, 2022
Currently the ensureCache create a temporary pointer that will
transfer the value to the WASM/C part and recycle it with to use in the
next value, but since the libass struct pointers are owned by the
library, the pointers can't be recycled or freed or can lead into
an undefined behavior.

This configure the binder to tranfer the pointer ownership instead of
recycle it.

To avoid creating complex code, I decided to fix it in the webidl
binder.
TFSThiagoBR98 added a commit that referenced this issue Mar 13, 2022
Fix Events and Styles String Mutation, closes #77
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants