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

fix(android): There were invalidate conflicts due to asynchronous executions #71

Merged

Conversation

CatchABus
Copy link
Contributor

@CatchABus CatchABus commented Nov 30, 2021

In android, there is a random bug that makes canvas blank.
There are references about it including mine in issue #67 .
This has discouraged me to use plugin so far because most of our clients have android phones.

Let's start with this. We have android doFrame callback to handle canvas invalidation.
That is an overridden method inside TNSCanvas: https://github.com/NativeScript/canvas/blob/master/packages/canvas/src-native/canvas-android/canvas/src/main/java/org/nativescript/canvas/TNSCanvas.kt#L125

        override fun doFrame(frameTimeNanos: Long) {
		if (!isHandleInvalidationManually) {
			if (pendingInvalidate) {
				flush() // THIS LINE IS VERY IMPORTANT REGARDING THIS BUG
			}
		}
		Choreographer.getInstance().postFrameCallback(this)
	}

Flush execution will indirectly call GLContext method flush that contains invalidation logic, wrapped inside queueEvent method here: https://github.com/NativeScript/canvas/blob/master/packages/canvas/src-native/canvas-android/canvas/src/main/java/org/nativescript/canvas/GLContext.kt#L148

The queue will eventually be handled from GLThread and execute invalidation logic here: https://github.com/NativeScript/canvas/blob/master/packages/canvas/src-native/canvas-android/canvas/src/main/java/org/nativescript/canvas/GLContext.kt#L599

        while (true) {
		try {
			if (!isPaused) {
				makeEGLContextCurrent()
				mQueue.take().run()
			}
		} catch (e: InterruptedException) {
			break
		}
	}

So, where is the problem?
On a first look, this whole logic seems good. However, there is something really deceiving about it.
There is the call of Choreographer.getInstance().postFrameCallback(this) at the end of doFrame callback that performs the next call of doFrame itself.

The problem here is there is no guarantee that postFrameCallback will be executed once flush is done. After all, flush logic is inserted inside a BlockingQueue and another thread (GLThread) is pulling the event and running it inside thread's loop cycle.

As a result, frame callback can be called anew before flush is done and will attempt an additional flush causing a conflict between them.

So

        override fun doFrame(frameTimeNanos: Long) {
		if (!isHandleInvalidationManually) {
			if (pendingInvalidate) {
				flush() // THIS MAY NOT EXECUTE INSTANTLY
			}
		}
		Choreographer.getInstance().postFrameCallback(this) // AS A RESULT, THIS WILL EXECUTE SOONER
	}

The defect of this flaw is that it makes canvas completely blank. This bug is extremely random because the code itself is good but there is a flaw caused by multi-threading. I have personally dealt with Java threads for few years now and I can see the evil in them.

I had to read native code and search a lot until I made the conclusion above, though I'm really excited about the time spent.
In order to fix this, I replaced pendingInvalidate with invalidateState flag which has 3 enum states (NONE, PENDING, INVALIDATING).
This flag keeps the old functionality and additionally lets canvas be aware if it's in the process of invalidating.

These changes seem fine to me regarding conventions but I'm not very familiar with kotlin, so please feel free to correct me if needed.

@cla-bot cla-bot bot added the cla: yes label Nov 30, 2021
@triniwiz
Copy link
Member

Love it 🤩

@triniwiz triniwiz merged commit 4835900 into NativeScript:master Nov 30, 2021
@CatchABus
Copy link
Contributor Author

@triniwiz A release that includes these changes would be good. I'm planning to use canvas in my app and help with any possible issues, especially on android.

@triniwiz
Copy link
Member

Sorry been a little bit busy with our {N} work

@CatchABus
Copy link
Contributor Author

Sorry been a little bit busy with our {N} work

Nothing to apologize for, I was curious about its progress and didn't think about your priorities.
I'll run it on a test app and provide any possible feedback while waiting for next release. Thank you very much for your efforts!!

@kuvelas
Copy link

kuvelas commented Jan 5, 2022

I would love a release with this fix. Would solve the only major issue I'm having with this package.

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 this pull request may close these issues.

None yet

3 participants