[TIMOB-25461] HEX backgroundColor with alpha channel act as a mask

[ypbnv]

JIRA: https://jira.appcelerator.org/browse/TIMOB-25461

Description:
Change check for HW acceleration disabling for border drawing and TiUIWebView.
On the devices I got at hand HW acceleration was never disabled because isHardwareAccelerated() always returned true.
According to this discussion
https://stackoverflow.com/questions/12119753/view-ishardwareaccelerated-is-always-false
It would report properly after the view has been attached to the window.

Most recent possibly affected tickets that should be checked:
https://jira.appcelerator.org/browse/TIMOB-25266
https://jira.appcelerator.org/browse/TIMOB-25238
https://jira.appcelerator.org/browse/TIMOB-24655 (That's reported specifically for API 21,22)
https://jira.appcelerator.org/browse/TIMOB-23885

I have tested them with the following devices:
Nexus 9 (API 25), Moto G (API 23), Samsung S3 (API 18), Samsung S8 (API 24), Android Emulator (API 16).
Got an unexpected result for the following combinations:
TIMOB-25238 - Emulator (API 16) - some tearing while scrolling. I expect it to be an emulator problem.
TIMOB-23885 - Samsung S3 (API 18) - videos from the test link did not work in browser applications as well.

[jquick-axway]

This was added to resolve a WebView crash on Android 4.1...
https://jira.appcelerator.org/browse/TIMOB-25266

The reason it crashed is because a BorderView was always being applied to a WebView (because it initializing itself with a zero border width unlike other views) and a hardware accelerator border causes a C/C++ segfault, likely because the WebKit driven WebView's hardware accelerated rendering conflicts with it.

We don't want to re-introduce this crash. So, if you feel we need to change the code here, then we may need to look into an alternative solution for Android 4.1 WebViews. We could simply always disable hardware accelerated WebView rendering on 4.1, but my understanding is that could break HTML5 video rendering. The other approach is to keep hardware accelerated rendering but don't support a border. Hmm...

[ypbnv]

I double checked the test cases in the ticket and the PR. There are no crashes on API 16. But I have tested only on emulator - no device with that version at hand.

TiBorderWrapperView currently draws a border if we have set a borderColor or a borderWidth greater than 1 or borderRadius greater than 1.
https://github.com/appcelerator/titanium_mobile/blob/master/android/titanium/src/java/org/appcelerator/titanium/view/TiUIView.java#L445
So I believe there is not reason to reintroduce this problem.

As for the conflict for HTML 5 video rendering when having a border on API 16 - I suppose we could drop the support for borderRadius only. It is the one which combined with opacity and background color with alpha channel requires disabling of HW acceleration - the unsupported methods for this API from:
https://developer.android.com/guide/topics/graphics/hardware-accel.html#unsupported
are called in case we have a radius:
https://github.com/appcelerator/titanium_mobile/blob/master/android/titanium/src/java/org/appcelerator/titanium/view/TiBorderWrapperView.java#L86

[jquick-axway]

Thanks for looking into this.

I suppose the thing to do is have QE re-run the test attached to PR #9432 when testing this PR, just in case. @lokeshchdhry has done the test before and has a physical Android 4.1. device to test with.

[jquick-axway]

Also, the reason we disable hardware acceleration below certain API Levels is because Google doesn't support certain canvas drawing operation until particular Android OS versions. Google lists it out here...
https://developer.android.com/guide/topics/graphics/hardware-accel.html#unsupported

I haven't personally verified if we're disabling hardware acceleration for the right API Levels for myself. So, if you could double check this, then that would be great. Our "TiBorderWrapperView" is the class that's doing canvas draws/clipping and that's what you have to look out for.

[ypbnv]

I went through the list and I found a few usage of methods in it that may require additional attention. And this whole thing is getting a bit tricky and complicated. Maybe we could have a discussion in order to decide what is the best way to approach all the cases?

[hansemannn]

@ypbnv Can you add unit-tests for this? Just to ensure the combination of colors + borders doesn't crash, although it's rather a UI-related change.

[garymathews]

This change needs to be reverted, the circle in this test case will become a square on Jelly Bean 4.2 and below.

var window = Ti.UI.createWindow(),
    webView = Ti.UI.createWebView({
	    url: 'https://www.quirksmode.org/html5/tests/video.html',
	    borderWidth: '20',
	    borderColor: 'red',
	    width: '66%', height: '66%',
	    borderRadius: '20'
    }),
    circle = Ti.UI.createView({
        top: 10,
        backgroundColor: 'blue',
        width: 50, height: 50,
        borderRadius: 25
    });
window.add([webView, circle]);
window.open();

[garymathews]

See above

[ypbnv]

@garymathews Updated.

I was not able to keep the support for any sort of border for WebView on API 16 and 17 so I have ignored all three properties there (borderWidth, borderRadius and borderColor) in favor of playing properly HTML 5 videos. No issues found on later APIs with WebView with borders. Any ideas about another solution for this conflict are more than welcome.

cc: @jquick-axway

[garymathews]

see above

[garymathews]

minor, but a warning could be thrown suggesting the method does not return a value (even though it does). You should change it to this:

protected boolean hasBorder(KrollDict d) {
	if (LOWER_THAN_JELLYBEAN) {
		return false;
	}
	return super.hasBorder(d);
}

[ypbnv]

Updated.

[build]

	Messages
📖	💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

[ypbnv]

@garymathews Is that ready to go for QE testing after the latest changes ?

[jquick-axway]

@ypbnv, @garymathews,

At this point, I think we should avoid making changes to anything borderRadius related on Android, because when we make changes to it we sometimes inadvertently break some other edge-case that was working for someone else. The source of the buggy-ness is with Google's clipPath() and I'd like to find an alternative solution to it in the future.

For this JIRA case, the Titanium developer simply wants to draw a circle. The radial gradient feature TIMOB-9366 I'm adding to 7.1.0 provides this functionality, with hardware acceleration, and is the superior solution.

[ypbnv]

@jquick-axway @garymathews I am OK with that in case we provide an alternative in the same version.

The question that remains is do we want to keep disabling of HW acceleration? It is only used for views with border - regardless whether they have borderRadius set or not. Technically this PR does not use borderRadius property to change anything. The combination of it with transparent backgrounds showed that the HW acceleration was not disabled when expected.

[jquick-axway]

@ypbnv, disabling hardware acceleration can sometimes have negative consequences too. We ran into this problem in 7.0.2.RC with TIMOB-25733.

At this point, we have to conclude that our technique for implementing borderRadius via Google's clipPath() isn't reliable since we're making fixes/work-arounds for its issues in nearly every release. It's time to look into alternative solutions.

A couple of weeks ago, I experimented with re-working our borderRadius implementation to replace its clipPath() usage with a masking technique via Paint.setXfermode() and leaving hardware acceleration enabled. I was mostly successful (with a few issues) and it worked on Android 4.2 and above.
https://developer.android.com/reference/android/graphics/PorterDuff.Mode.html

But masking didn't work on Android 4.1 (only 4.2 and above). It also didn't work with SurfaceView derived classes, such as a VideoView. But I think this may be a viable solution. I just need more time to experiment with it.

[ypbnv]

@jquick-axway I agree with dropping clipPath() for borderRadius. I asked because the actual code changes here ensure (I think with one change needed that I noticed today) that the disableHWacceleration() happens when expected. As far as I can tell currently we are only disabling it for performance boost in list views.

[jquick-axway]

Let's not make this change to 7.1.0.

Sorry. I appreciate your effort into this, but like I said earlier, disable hardware acceleration too aggressively has caused issues as well.

[hansemannn]

@ypbnv Can you check the unit test linting-results? I'd like to schedule this for 7.4.0 if possible.

[ypbnv]

@hansemannn Linted the tests, but I am not sure if this is getting in the way it is now.

[build]

	Fails
🚫	Test suite crashed on iOS simulator. Please see the crash log for more details.

Generated by 🚫 dangerJS

[hansemannn]

Closing for now since it's in a stale state. Happy to reopen once it gets some traction again.