Android hwdec utilizing AImageReader

[sfan5]

approach based on https://github.com/mpv-player/mpv/pull/5602#issuecomment-796826604 In short you can tell MediaCodec to render into an AImageReader and then retrieve the frames from it, no JNI required.

unsolved:

• [x] ~~hardcoded maximum dimensions in AImageReader_newWithUsage call~~
• [x] ~~raises the API requirement to 26, could possibly dlopen libmediandk.so instead~~
• [x] rarely(???) crashes on exit
• [x] ~~surface dimension mismatch with 1080p~~
• [x] unclear situation around buffer refcounting (ffmpeg patch pending)

what I'm wondering:

• the texture is always RGB0. ~~why? can we influence this?~~ can't fix
• AImageReader has an internal queue but only one frame is ever in-flight. Wouldn't it be more efficient to queue several frames? Does this even matter?

cc @jeeb @tmm1 @wang-bin @wnoun

[sfan5]

patch for testing inside mpv-android

diff --git a/app/src/main/java/is/xyz/mpv/MPVView.kt b/app/src/main/java/is/xyz/mpv/MPVView.kt
index a0a5539..32b5269 100644
--- a/app/src/main/java/is/xyz/mpv/MPVView.kt
+++ b/app/src/main/java/is/xyz/mpv/MPVView.kt
@@ -32,7 +32,7 @@ internal class MPVView(context: Context, attrs: AttributeSet) : SurfaceView(cont
 
         // hwdec
         val hwdec = if (sharedPreferences.getBoolean("hardware_decoding", true))
-            "mediacodec-copy"
+            "mediacodec"
         else
             "no"
 
@@ -359,7 +359,7 @@ internal class MPVView(context: Context, attrs: AttributeSet) : SurfaceView(cont
     fun cyclePause() = MPVLib.command(arrayOf("cycle", "pause"))
     fun cycleAudio() = MPVLib.command(arrayOf("cycle", "audio"))
     fun cycleSub() = MPVLib.command(arrayOf("cycle", "sub"))
-    fun cycleHwdec() = MPVLib.command(arrayOf("cycle-values", "hwdec", "mediacodec-copy", "no"))
+    fun cycleHwdec() = MPVLib.command(arrayOf("cycle-values", "hwdec", "mediacodec", "no"))
 
     fun cycleSpeed() {
         val speeds = arrayOf(0.5, 0.75, 1.0, 1.25, 1.5, 1.75, 2.0)

or an APK: https://0x0.st/-5YI.apk

includes ffmpeg patch (https://github.com/mpv-player/mpv/pull/9324#issuecomment-962454581)

[evelikov]

Overall I think it's a good thing to go for. Just keep in mind that using AImageReader can be fairly glitchy - you can see a bunch of issues when Bromite (Chromium fork) started using it. I'm wondering is it possible to factor out some of the common EGL code into helpers. IIRC we have a nearly verbatim copy in other EGL platforms.

[philipl]

I tried it out on a pixel 6 which can actually do 10bit hwdec for hevc and vp9 with HDR (maybe it can do av1 too but ffmpeg doesn't have support for it and the documentation seems to be missing) and I got visually acceptable results. It didn't look like it was completely un-tonemapped but I'm not sure how it's deciding to do tonemapping as the relevant stream metadata is presumably not available to mediacodec. In contrast, the old code path with mediacodec-copy has completely distorted output that seems consistent with not understanding 10bit. So this change is a strict improvement in that context, even with the forced rgb conversion. I support merging it even with that caveat.

[sfan5]

Backtrace of a crash (random, not easy reproducible). Don't think this one's my fault:

V mpv     : [cplayer:v] Set property: hwdec=no -> 1
V mpv     : [mkv:v] queuing seek to 30.448000
V mpv     : [mkv:v] cached range 0: 0.000000  90.050000 (bof=1, eof=1)
V mpv     : [mkv:v] ...using this range for in-cache seek.
V mpv     : [mkv:v] seeking stream 0 (video) to packet 29.739000/-9223372036854775808.000000
V mpv     : [mkv:v] seeking stream 1 (audio) to packet 30.442000/-9223372036854775808.000000
V mpv     : [mkv:v] seeking stream 2 (sub) to packet 29.593000/-9223372036854775808.000000
V mpv     : event: seek
V mpv     : [cplayer:v] hr-seek, skipping to 30.448000
V mpv     : [mkv:v] EOF reached.
V mpv     : [ffmpeg:warn] av_log callback called with bad parameters (NULL AVClass).
V mpv     : [ffmpeg:warn] This is a bug in one of Libav/FFmpeg libraries used.
F is.xyz.mpv: java_vm_ext.cc:578] JNI DETECTED ERROR IN APPLICATION: use of invalid jobject 0x6e696854
F DEBUG   : signal 6 (SIGABRT), code -1 (SI_QUEUE), fault addr --------
F DEBUG   : Abort message: 'JNI DETECTED ERROR IN APPLICATION: use of invalid jobject 0x6e696854'
F DEBUG   : backtrace:
F DEBUG   :       #12 pc 00000000003704cc  /apex/com.android.art/lib64/libart.so (art::(anonymous namespace)::CheckJNI::CallVoidMethod(_JNIEnv*, _jobject*, _jmethodID*, ...)+152)
F DEBUG   :       #13 pc 000000000069ed44  /data/app/~~mnM286k7wrH2NCEkA3hG6A==/is.xyz.mpv-XLwxEu6OBmP_fMRujHNgoA==/lib/arm64/libavcodec.so
	ff_AMediaCodec_releaseOutputBuffer at mpv-android/buildscripts/deps/ffmpeg/_build-arm64/src/libavcodec/mediacodec_wrapper.c:1437
F DEBUG   :       #14 pc 00000000001f06d0  /data/app/~~mnM286k7wrH2NCEkA3hG6A==/is.xyz.mpv-XLwxEu6OBmP_fMRujHNgoA==/lib/arm64/libmpv.so
	mapper_map at mpv-android/buildscripts/deps/mpv/_build-arm64/../video/out/hwdec/hwdec_aimagereader.c:322
F DEBUG   :       #15 pc 00000000001dcce0  /data/app/~~mnM286k7wrH2NCEkA3hG6A==/is.xyz.mpv-XLwxEu6OBmP_fMRujHNgoA==/lib/arm64/libmpv.so
	ra_hwdec_mapper_map at mpv-android/buildscripts/deps/mpv/_build-arm64/../video/out/gpu/hwdec.c:200
F DEBUG   :       #16 pc 00000000001e5184  /data/app/~~mnM286k7wrH2NCEkA3hG6A==/is.xyz.mpv-XLwxEu6OBmP_fMRujHNgoA==/lib/arm64/libmpv.so
	pass_upload_image at mpv-android/buildscripts/deps/mpv/_build-arm64/../video/out/gpu/video.c:3557
F DEBUG   :       #17 pc 00000000001e3efc  /data/app/~~mnM286k7wrH2NCEkA3hG6A==/is.xyz.mpv-XLwxEu6OBmP_fMRujHNgoA==/lib/arm64/libmpv.so
	gl_video_render_frame at mpv-android/buildscripts/deps/mpv/_build-arm64/../video/out/gpu/video.c:3297
F DEBUG   :       #18 pc 00000000001f88a8  /data/app/~~mnM286k7wrH2NCEkA3hG6A==/is.xyz.mpv-XLwxEu6OBmP_fMRujHNgoA==/lib/arm64/libmpv.so
	draw_frame at mpv-android/buildscripts/deps/mpv/_build-arm64/../video/out/vo_gpu.c:86
F DEBUG   :       #19 pc 00000000001f7a04  /data/app/~~mnM286k7wrH2NCEkA3hG6A==/is.xyz.mpv-XLwxEu6OBmP_fMRujHNgoA==/lib/arm64/libmpv.so
	render_frame at mpv-android/buildscripts/deps/mpv/_build-arm64/../video/out/vo.c:961

[wnoun]

does it crash with vd-lavc-o=delay_flush=1(although it may break the seek function)? looking at mediacodec_wrap_hw_buffer, mediacodec context is reference-counted only if delay_flush is set. if a mediacodec frame is used after decoder is destroyed, we may access freed memory?

[wnoun]

you may try the following patch without setting delay_flush. These codes were originally used to debug pr #5602 crashes.

--- a/libavcodec/mediacodecdec_common.c
+++ b/libavcodec/mediacodecdec_common.c
@@ -225,26 +225,30 @@ static void ff_mediacodec_dec_ref(MediaCodecDecContext *s)
     atomic_fetch_add(&s->refcount, 1);
 }
 
+static void ff_mediacodec_dec_release_resource(MediaCodecDecContext *s) {
+    if (s->codec) {
+        ff_AMediaCodec_delete(s->codec);
+        s->codec = NULL;
+    }
+
+    if (s->format) {
+        ff_AMediaFormat_delete(s->format);
+        s->format = NULL;
+    }
+
+    if (s->surface) {
+        ff_mediacodec_surface_unref(s->surface, NULL);
+        s->surface = NULL;
+    }
+}
+
 static void ff_mediacodec_dec_unref(MediaCodecDecContext *s)
 {
     if (!s)
         return;
 
     if (atomic_fetch_sub(&s->refcount, 1) == 1) {
-        if (s->codec) {
-            ff_AMediaCodec_delete(s->codec);
-            s->codec = NULL;
-        }
-
-        if (s->format) {
-            ff_AMediaFormat_delete(s->format);
-            s->format = NULL;
-        }
-
-        if (s->surface) {
-            ff_mediacodec_surface_unref(s->surface, NULL);
-            s->surface = NULL;
-        }
+        ff_mediacodec_dec_release_resource(s);
 
         av_freep(&s->codec_name);
         av_freep(&s);
@@ -265,7 +269,7 @@ static void mediacodec_buffer_release(void *opaque, uint8_t *data)
         ff_AMediaCodec_releaseOutputBuffer(ctx->codec, buffer->index, 0);
     }
 
-    if (ctx->delay_flush)
+    // if (ctx->delay_flush)
         ff_mediacodec_dec_unref(ctx);
     av_freep(&buffer);
 }
@@ -321,7 +325,7 @@ static int mediacodec_wrap_hw_buffer(AVCodecContext *avctx,
 
     buffer->ctx = s;
     buffer->serial = atomic_load(&s->serial);
-    if (s->delay_flush)
+    // if (s->delay_flush)
         ff_mediacodec_dec_ref(s);
 
     buffer->index = index;
@@ -872,7 +876,7 @@ int ff_mediacodec_dec_receive(AVCodecContext *avctx, MediaCodecDecContext *s,
 */
 int ff_mediacodec_dec_flush(AVCodecContext *avctx, MediaCodecDecContext *s)
 {
-    if (!s->surface || atomic_load(&s->refcount) == 1) {
+    if (!s->surface || !s->delay_flush) {
         int ret;
 
         /* No frames (holding a reference to the codec) are retained by the
@@ -890,6 +894,14 @@ int ff_mediacodec_dec_flush(AVCodecContext *avctx, MediaCodecDecContext *s)
 
 int ff_mediacodec_dec_close(AVCodecContext *avctx, MediaCodecDecContext *s)
 {
+    if (!s)
+        return 0;
+
+    if (!s->delay_flush) {
+        atomic_fetch_add(&s->serial, 1);
+        ff_mediacodec_dec_release_resource(s);
+    }
+
     ff_mediacodec_dec_unref(s);
 
     return 0;

[sfan5]

It's hard to conclusively say if your patch fixes the crash but it does not cause any problems at least. Is there a reason ffmpeg does not use refcounting with delay_flush=0? It would be a strict improvement.

[wnoun]

tested on my device, without the patch I can reproduce the crash by calling loadfile /path/to/file replace every second. Another question is whether it works on Adreno 3xx devices, because pr5602 has extra code to these devices.(maybe it doesn't matter, since it’s 2021).

[sfan5]

I still have the device with the Adreno GPU but there is no new enough Android version with support for Media NDK APIs. So I guess it does not matter.

Another thing I noticed is that with 1080p video a hwdec'd picture has exactly 8 pixels too much on the bottom. This matches the height reported by mediacodec but the crop parameters suggest this shouldn't be happening.

[ffmpeg/video:v] h264_mediacodec: Output MediaFormat changed to {crop-right=1919, vendor.rtc-ext-dec-caps-vt-driver-version.number=0, vendor.rtc-ext-dec-low-latency.enable=0, color-format=261, slice-height=1088, mime=video/raw, vendor.sec-ext-dec-compressed-color-format=0, stride=1920, color-standard=1, color-transfer=3, crop-bottom=1079, crop-left=0, width=1920, color-range=2, crop-top=0, height=1088}
[ffmpeg/video:v] h264_mediacodec: Output crop parameters top=0 bottom=1079 left=0 right=1919, resulting dimensions width=1920 height=1080

This is fixable by reporting the extra height in struct ra_tex_params but I don't think I can find out at that point.

[wnoun]

does AImage_getCropRect return valid results? if so, you may add transform matrix like pr5602. it seems that SurfaceTexture compute the martix in GLConsumer::computeTransformMatrix

[sfan5]

It does but those methods only return 1920x1080, AHardwareBuffer_describe works for this. A transform matrix will not be necessary.

[wnoun]

will it use the full dimension of the image? the extra 8 pixels should be padding and should not be used.

[sfan5]

If the texture is larger than the frame the extra pixels will be cropped as intended.

[wnoun]

if so, it shoud be a better solution

[sfan5]

Okay so after talking to @mbouron today it was logical that delay_flush=1 would fix the refcounting issue. But I actually tried it just now and it entirely breaks upon doing a single seek:

12-13 23:39:44.199  4679  6791 V mpv     : [cplayer:v] Set property: time-pos=28 -> 1
12-13 23:39:44.200  4679  6791 V mpv     : [mkv:v] queuing seek to 28.000000
12-13 23:39:44.200  4679  6791 V mpv     : [mkv:v] cached range 0: 0.000000  90.050000 (bof=1, eof=1)
12-13 23:39:44.200  4679  6791 V mpv     : [mkv:v] ...using this range for in-cache seek.
12-13 23:39:44.200  4679  6791 V mpv     : [mkv:v] seeking stream 0 (video) to packet 27.320000/-9223372036854775808.000000
12-13 23:39:44.200  4679  6791 V mpv     : [mkv:v] seeking stream 1 (audio) to packet 27.989000/-9223372036854775808.000000
12-13 23:39:44.200  4679  6791 V mpv     : [mkv:v] seeking stream 2 (sub) to packet 23.623000/-9223372036854775808.000000
12-13 23:39:44.202  4679  6791 V mpv     : event: seek
12-13 23:39:44.202  4679  6791 V mpv     : [cplayer:v] hr-seek, skipping to 28.000000
12-13 23:39:44.202  4679  6791 V mpv     : [vd:warn] could not consume packet
12-13 23:39:44.202  4679  6791 V mpv     : [vd:warn] could not consume packet
12-13 23:39:44.202  4679  6791 V mpv     : [vd:warn] could not consume packet
12-13 23:39:44.202  4679  6791 V mpv     : [vd:warn] could not consume packet
12-13 23:39:44.202  4679  6791 V mpv     : [vd:warn] could not consume packet
12-13 23:39:44.202  4679  6791 V mpv     : [vd:warn] could not consume packet
12-13 23:39:44.202  4679  6791 V mpv     : [vd:warn] could not consume packet
12-13 23:39:44.202  4679  6791 V mpv     : [vd:warn] could not consume packet
[ thousands more of the same message, it never recovers ]

It seems some variant of the patch further above will be needed after all.

[wnoun]

feel free to modify the above patch to suit your need. in addition, the line if (!s->surface || !s->delay_flush) should be if (!s->surface || atomic_load(&s->refcount) == 1 || !s->delay_flush) to keep compatibility with delay_flush=1.

[sfan5]

Sure. Your patch works great, the issue is just that before having this merged into mpv I need it to work with unpatched FFmpeg.

[sfan5]

I took another long look at this and (finally) understood the point of the refcounting and various options properly. A patch with the required changes has been sent to ffmpeg-devel so this should be able to go in soon. Thanks again @wnoun for the hints.