Use the NativeEngine TextureData definition to avoid crashes unwrapping Napi::External<NativeEngine::TextureData> for jsi by chrisfromwork · Pull Request #805 · BabylonJS/BabylonNative

chrisfromwork · 2021-06-17T19:32:05Z

Defining a second TextureData definition for the Canvas seemed to resolve correctly in BabylonNative. But in BabylonReactNative the jsi wrapper unwrapping the external type threw exceptions. This change updates the Canvas polyfill to consume the TextureData struct defined by NativeEngine. It seems like this is what we should be doing to avoid getting into scenarios where the struct definition changes and we need to update multiple locations.

We also update loadTTF to only load a font once. Once a font is getting used if its updated on the js thread it can cause crashes for the graphics thread.

Note: this change requires having the Canvas polyfill reference internal headers for NativeEngine. It seems like this is already an expected practice. I'm not sure if polyfills are supposed to consume Core/Plugin definitions, but In this scenario we need to.

ryantrem · 2021-06-17T22:49:02Z


+    struct NativeCanvas::Impl
+    {
+        std::unique_ptr<Babylon::TextureData> m_textureData;


For structs with public members, I think this should be TextureData instead of m_textureData.

sounds good. i see this vary in different places. But am happy to start using capitalization throughout.

ryantrem · 2021-06-17T22:50:37Z

 {
    static constexpr auto JS_CONSTRUCTOR_NAME = "NativeCanvas";

+    struct NativeCanvas::Impl


Suggested change

struct NativeCanvas::Impl

struct NativeCanvas::Impl final

this should now be fixed.

ryantrem · 2021-06-17T22:55:03Z

-        data->Width = m_width;
-        data->Height = m_height;
-        return Napi::External<TextureData>::New(info.Env(), data);
+        m_impl->m_textureData = std::make_unique<TextureData>();


It looks like this gets deleted at the end of the function, but you are passing a pointer to the underlying data to the Napi::External. I think the default finalizer action for Napi::External is to delete the pointer, so it seems like it was correct the way it was, but maybe I'm missing something.

It looks like Napi::External only deletes if you provide your own finalizer. This was likely leaking before.

Hmm I wonder if we have leaks elsewhere…

id bet we definitely have leaks elsewhere

whatever was going on before, calling new just seems like a bad time for the TextureData when theres no subsequent delete call. To be fair though Cedric called out that this was leaking if i remember correctly.

chatted with ryan offline. seems like a finalizer is what we should do here.

bghgary · 2021-06-18T18:36:22Z

+        if (!info[0].IsExternal() ||
+            !info[1].IsExternal())
+        {
+            return;
+        }
+


This is a bit weird. We just silently do nothing when the types are wrong? Is that the intended behavior?

more was a check because the external wasn't unwrapping correctly for jsi when it was a unique struct definition. I'll undo this check.

this should now be fixed.

bghgary · 2021-06-18T18:38:50Z


+    struct NativeCanvas::Impl final
+    {
+        std::unique_ptr<Babylon::TextureData> TextureData;


Do you need the unique_ptr?

no we don't, good call out.

this should now be fixed.

see changes, this struct no longer exists.

bghgary · 2021-06-18T18:39:12Z

 {
    static constexpr auto JS_CONSTRUCTOR_NAME = "NativeCanvas";

+    struct NativeCanvas::Impl final


I don't understand this. Why do you need an Impl class?

I added an impl to have a data container so that i wouldn't have to declare the babylon/NativeEngine.h header in the the canvas header.

tking another look it seems like there are two canvas headers and we already declare bgfx types in the internal canvas header. I'll remove this container and will just declare the texturedata as a property of the canvas struct for now.

this should now be fixed. impl is removed and we just declare the property for the native canvas.

based on other conversations, this datas lifecycle is going to be maintained by the external through a finalizer.

that would make a lot more sense. create it for javascript and finalize with javascript.

this should now be fixed

… data as property. remove no opt checks

ryantrem · 2021-06-18T20:16:37Z

    {
-        std::string fontName = info[0].As<Napi::String>().Utf8Value();
+        auto& graphicsImpl{Babylon::GraphicsImpl::GetFromJavaScript(info.Env())};
+        const auto cancellationSource = std::shared_ptr<arcana::cancellation_source>();


Unused, remove?

this should now be fixed.

ryantrem · 2021-06-18T20:17:24Z

+        std::vector<uint8_t> fontBuffer{};
+        fontBuffer.resize(buffer.ByteLength());
+        memcpy(fontBuffer.data(), (uint8_t*)buffer.Data(), buffer.ByteLength());
+        arcana::make_task(graphicsImpl.BeforeRenderScheduler(), arcana::cancellation::none(), [fontName{ info[0].As<Napi::String>().Utf8Value() }, fontData{ std::move(fontBuffer) }]() {


If this is going to be async, then this function should return a promise right?

if i understand correctly this function doesn't need to resolve before declaring text. Given returning a promise would require updating babylon.js, i'm inclined to say that returning a promise here is beyond the scope of this review and should be addressed when we finalize the public api contract for loading fonts (fontface, etc.)

actually loadTTF isn't defined anywhere public. let me return a promise here.

this should now be fixed

Co-authored-by: Gary Hsu <bghgary@users.noreply.github.com>

manage memory of texture data and use correct struct

3807ea4

chrisfromwork requested review from CedricGuillemet, bghgary, rgerd, rickfromwork, ryantrem, syntheticmagus and tymlipari and removed request for bghgary June 17, 2021 19:32

chrisfromwork mentioned this pull request Jun 17, 2021

Consume canvas polyfill to enable text support BabylonJS/BabylonReactNative#251

Merged

avoid loading multiple fonts once rendering fonts

31ce2b3

ryantrem reviewed Jun 17, 2021

View reviewed changes

Chris Barth added 2 commits June 18, 2021 10:19

avoid owning handle in texture reference

2d794da

address review comments

56896ac

chrisfromwork requested a review from ryantrem June 18, 2021 18:31

bghgary reviewed Jun 18, 2021

View reviewed changes

address review comments, move away from impl and just declare texture…

187907c

… data as property. remove no opt checks

chrisfromwork requested a review from bghgary June 18, 2021 19:00

Chris Barth added 2 commits June 18, 2021 12:33

map texturedata lifecycle to external js object

c84e756

move font load to graphics thread

ee532e2

ryantrem reviewed Jun 18, 2021

View reviewed changes

move loadTTF to async function

ff00fe9

ryantrem approved these changes Jun 18, 2021

View reviewed changes

bghgary approved these changes Jun 18, 2021

View reviewed changes

Comment thread Polyfills/Canvas/Source/Canvas.cpp Outdated

improve fontBuffer construction

cfb2256

Co-authored-by: Gary Hsu <bghgary@users.noreply.github.com>

chrisfromwork enabled auto-merge (squash) June 18, 2021 22:41

chrisfromwork merged commit c00b503 into BabylonJS:master Jun 18, 2021

rgerd mentioned this pull request Jul 13, 2021

Add a NAPI-JSI build to pre-test Babylon React Native #828

Closed

Conversation

chrisfromwork commented Jun 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

chrisfromwork commented Jun 17, 2021 •

edited

Loading