fix-huawei-imagereader-166481#181931
Conversation
|
The issue this PR tries to solve appeared in flutter 3.29.2 released on 13/03/2025. From my investigations I believe it was introduced by this PR: #164201 The PR disabled Vulkan on Huawei api 29 so it falls back to OpenGLES but OpenGLES still uses ImageReader. So I tried to add back the logic from && !flutterJNI.ShouldDisableAHB() |
There was a problem hiding this comment.
Code Review
This pull request introduces a fix for video playback issues on certain Huawei devices running Android API level 29 or below. The solution involves adding a native check to detect this specific device configuration and then falling back to SurfaceTextureSurfaceProducer instead of ImageReaderSurfaceProducer. The implementation is well-contained, spanning Java and C++ with the necessary JNI bindings, and is accompanied by a unit test to validate the new behavior. The changes appear correct and effectively address the reported issue.
engine/src/flutter/shell/platform/android/platform_view_android_jni_impl.cc
Outdated
Show resolved
Hide resolved
cad4f69 to
401c39b
Compare
|
Adding @gaaclarke TL of the engine team since the author of that pr is no longer a full time contributor to flutter. @gaaclarke if you are not the right person to review this can you find someone on your team who is? Second question what test should we author to prevent a regression on this issue in the engine? Do you need the android team to add huawei emulators or is there something lower level that should be tested. |
|
@serbandin thank you so much for your contribution. |
| private native boolean nativeShouldDisableImageReader(); | ||
|
|
||
| /** | ||
| * Checks if ImageReader should be disabled for this device. |
There was a problem hiding this comment.
nit: can you link to the source for which ImageReader that is keyword that can lead to a lot of different types of code.
| /** | ||
| * Checks if ImageReader should be disabled for this device. | ||
| * | ||
| * <p>Returns true for Huawei devices on API level 29 or below due to known issues with |
There was a problem hiding this comment.
Other than than the bug is there any other source we can point to that says that imagereader is known to not work on huawei devices?
There was a problem hiding this comment.
There are a lot of old issue with Huawei not playing videos on api 29.
#156459
#154068
Also here's an old PR that fixed it initially: flutter/engine#54879
Seems to be related to the OMX.hisi.* Decoder. It happens on Kirin 710/970/980 from what I saw in the comments / tested myself.
There was a problem hiding this comment.
@reidbaker I don't know about wether we want to disable the image reader or not on certain devices. I'll let the Android team decide on that. Ideally the linked issue should provide enough documentation.
I think we should avoid doing JNI here since the check can happen with the java apis.
| // SurfaceTextureSurfaceProducer handles crop and rotation, ImageReaderSurfaceProducer does not. | ||
| assertTrue(producer.handlesCropAndRotation()); |
There was a problem hiding this comment.
I'd prefer a more apropos assertion instead of relying on secondary information to test what you really want.
| // Huawei devices on API level 29 and below have known issues with | ||
| // ImageReader. This check is used to disable ImageReader on affected devices. | ||
| static jboolean ShouldDisableImageReader(JNIEnv* env, jobject jcaller) { | ||
| constexpr int kHuaweiMaxApiLevel = 29; | ||
| constexpr char kAndroidHuawei[] = "android-huawei"; | ||
| constexpr char kClientIdBaseProperty[] = "ro.com.google.clientidbase"; | ||
|
|
||
| if (android_get_device_api_level() > kHuaweiMaxApiLevel) { | ||
| return false; | ||
| } | ||
|
|
||
| char property[PROP_VALUE_MAX]; | ||
| __system_property_get(kClientIdBaseProperty, property); | ||
| return strcmp(property, kAndroidHuawei) == 0; | ||
| } |
There was a problem hiding this comment.
I don't believe there is any reason we have to use JNI for this. I think this same test can be performed from java.
There was a problem hiding this comment.
a check like this in java would be better?
return Build.VERSION.SDK_INT <= API_LEVELS.API_29
&& "HUAWEI".equalsIgnoreCase(Build.MANUFACTURER);
401c39b to
3163712
Compare
camsim99
left a comment
There was a problem hiding this comment.
Looks good to me overall--just one nit and a question!
| * | ||
| * @see <a href="https://github.com/flutter/flutter/issues/166481">#166481</a> | ||
| */ | ||
| private static boolean hasImageReaderIssue() { |
There was a problem hiding this comment.
Small thing: I would prefer a name that is more specific to the issue at hand like hasAndroidHardwareBufferDefect or requiresSurfaceTextureSurfaceProducer.
engine/src/flutter/shell/platform/android/platform_view_android_jni_impl.cc
Show resolved
Hide resolved
dca1aa6 to
8e0eb93
Compare
camsim99
left a comment
There was a problem hiding this comment.
This approach LGTM thanks for the fixes!
| // Verify: Should return SurfaceTextureSurfaceProducer instead of ImageReaderSurfaceProducer. | ||
| assertTrue( | ||
| "Expected SurfaceTextureSurfaceProducer on Huawei API <= 29 due to HardwareBuffer defect causing video playback failures", | ||
| producer instanceof FlutterRenderer.SurfaceTextureSurfaceProducer); |
There was a problem hiding this comment.
Looks like you're missing an import
/b/s/w/ir/cache/builder/engine/src/flutter/shell/platform/android/test/io/flutter/embedding/engine/renderer/FlutterRendererTest.java:842: error: cannot find symbol
producer instanceof FlutterRenderer.SurfaceTextureSurfaceProducer);
^
symbol: class SurfaceTextureSurfaceProducer
location: class FlutterRenderer
1 error|
After the test pass we would like @gaaclarke or another engine team member approval. |
gaaclarke
left a comment
There was a problem hiding this comment.
Changes looks good, the code is adequately tested. I don't know if we can definitively know all the values of Build.MANUFACTURER to make sure we catch everything, but this is progress regardless. I'll defer to the Android team as to wether they want the change or not (sounds like they do!)
Co-authored-by: Reid Baker <1063596+reidbaker@users.noreply.github.com>
Co-authored-by: Reid Baker <1063596+reidbaker@users.noreply.github.com>
|
auto label is removed for flutter/flutter/181931, Failed to enqueue flutter/flutter/181931 with HTTP 400: GraphQL mutate failed. |
@reidbaker, first of all thanks for the review + all the suggestions! Wanted to ask you if you have idea why the bot removed the auto-submit label? Should I do an interactive rebase on master and remove the merge commits? |
|
I don't have a link handy but we are having an outage on our side. Don't worry about it and I will try to get this landed Monday after our outage is resolved. No action needed from you |
|
autosubmit label was removed for flutter/flutter/181931, because - The status or check suite Linux linux_unopt has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
@serbandin the failure is legitimate; looks like you need to run the formatter! https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8688955671889939393/+/u/test:_test:_Check_formatting/stdout#:~:text=To%20fix%2C%20run%20%60et%20format%60%20or has the fix |
hmm seems that it happened after last comments pushed via suggestion. Thanks! I will run it |
6c547f0 to
fa0473d
Compare
|
@camsim99 I applied the format suggestion, now the tests are passing |
Fixes video playback issues on Huawei devices running Android API level 29 or below by falling back to SurfaceTextureSurfaceProducer instead of ImageReaderSurfaceProducer.
Problem
On certain Huawei devices with Android 10 (API 29), video playback fails when using ImageReaderSurfaceProducer. This is due to issues with AHB (Android Hardware Buffer) imports on these devices, similar to the Vulkan fallback already implemented in android_context_dynamic_impeller.cc. Fixes #166481
Solution
Added a new JNI method shouldDisableImageReader() that detects Huawei devices by checking the ro.com.google.clientidbase system property for the value "android-huawei" when the API level is 29 or below. When this method returns true, FlutterRenderer.createSurfaceProducer() uses SurfaceTextureSurfaceProducer instead of ImageReaderSurfaceProducer, which resolves the video playback issue.
Changes
FlutterJNI.java: Added shouldDisableImageReader() method and native bridge
platform_view_android_jni_impl.cc: Native implementation using __system_property_get() and android_get_device_api_level()
FlutterRenderer.java: Integration in createSurfaceProducer() condition (line 230)
FlutterRendererTest.java: Unit test for the new behavior
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.