Android: When the hardware/default decoder fails to initialize, fall back to software. #1933

20kdc · 2023-12-17T23:11:35Z

To be clear, this commit was only tested (and I only have the hardware to test it) with PhoneVR.

That version is https://github.com/20kdc/ALVR/tree/changeling-buffet-pt-2-phonevr-edition .

This code first attempts to use the hardware decoder as usual, and failing that, attempts the software decoder. These software decoder names are "well known" names. As far as I'm aware they're not strictly API-defined, but the alternative is API functions which are way too new to be usable to determine software-ness.

Since this functionality only triggers when the previous code would panic, it shouldn't make anything worse.

To be actually effective, this PR might need constrained baseline encoding. See #1932

Possible things of future research:

"Whatever Moonlight is doing is probably the right thing to do"?
Embed openh264 to avoid weirdness with system provided software decoders? (AHardwareBuffer needs to be abstracted into invisibility as much as possible)
Explicitly select a codec by name?

…back to software.

20kdc · 2023-12-18T00:31:44Z

Update: Have managed to get a build pair working to test on master and worn it to get an idea of the characteristics (and also, you know, test the PR).

On my device, ALVR (with the baseline profile PR)+PhoneVR (master with ALVR swapped for this commit) appears to eagerly predict the head rotation, possibly too far. Alone this results in a "snapback effect" on the desktop VR view, but then some sort of glitchy stutter turns this into a very weird sensation, sort of like as if the last few frames were being repeated. I don't know what causes this effect. My current suspicion is that the client can't keep up and reacts poorly, but I'm not sure; too much involved. Further research required.

I'm not strictly sure if the baseline profile PR is or is not necessary. It seems uncertain. In some configurations things definitely weren't working at all (often complete grey blanking of screen) and with other settings they were fine. It might help to reduce the load on the decoder, if nothing else.

zmerp · 2023-12-18T14:59:42Z

alvr/client_core/src/platform/android/decoder.rs

-            decoder.start().unwrap();
+
+            let preparing_decoder = if config.force_software_decoder {
+                decoder_try_software(sw_codec_name, &format, &image_reader).unwrap()


Instead of unwrapping you can pass the error upstream.

The code is within a thread. I'm not sure where it'd go. I am cleaning up to reduce the amount of unwrapping, but there still will be "an unwrap". In addition, trying to de-unwrapify the code results in loss of which specific call failed, which could be bad for debugging.

You could use the function alvr_common::show_err() to print the error. The error per se might not be interesting, but otherwise the unwrap() will crash the thread silently, which is worse.

Done as of 97eff12

zmerp · 2023-12-18T14:59:50Z

alvr/client_core/src/platform/android/decoder.rs

+                            "Attempting software fallback due to error in default decoder: {}",
+                            e
+                        );
+                        decoder_try_software(sw_codec_name, &format, &image_reader).unwrap()


alvr/client_core/src/platform/android/decoder.rs

zmerp · 2023-12-18T15:15:03Z

alvr/client_core/src/decoder.rs

+        // TODO: Actually get the real values for these and update accordingly.
+        // These are inherited from the previous code to avoid breaking anything.
+        // Would also be nice if force_software_decoder was configurable.
+        width: 512,
+        height: 1024,
+        force_software_decoder: false,


Have you checked it really matters? Video resolution is extracted from SPS/PPS (CSD-0).

I don't know that it, for certain, really matters. In my testing it does not aid my particular case. But given that this whole thing started because of codec breakage, I feel there is merit to setting these properties correctly. If they didn't somehow matter, why even set them in the first place?

The current code was translated as faithfully as possible from ancient java code. We are not in touch with the original author. I think this was used to mitigate some decoder implementation edge cases.
Since we are not changing the values at all, I think we should not add the width an height fields in DecoderInitConfig

Done as of 97eff12

alvr/client_core/src/decoder.rs

… can be figured out later

zmerp · 2023-12-19T20:17:52Z

alvr/session/src/settings.rs

+    #[schema(strings(
+        help = "Attempts to use a software decoder on the device. Slow, but may work around broken codecs."
+    ))]
+    pub force_software_decoder: bool,


Move this just above mediacodec_extra_options

Done as of d9a8731.

zmerp · 2023-12-19T20:22:09Z

alvr/client_core/src/decoder.rs

+        // TODO: Actually get the real values for these and update accordingly.
+        // These are inherited from the previous code to avoid breaking anything.
+        // Would also be nice if force_software_decoder was configurable.
+        width: 512,
+        height: 1024,
+        force_software_decoder: false,


The current code was translated as faithfully as possible from ancient java code. We are not in touch with the original author. I think this was used to mitigate some decoder implementation edge cases.
Since we are not changing the values at all, I think we should not add the width an height fields in DecoderInitConfig

…hould be more explicit

… Logging: cargo fmt --all

zmerp · 2023-12-20T20:18:54Z

alvr/common/src/logging.rs

 pub fn show_err<T, E: Display>(res: Result<T, E>) -> Option<T> {
-    res.map_err(|e| show_e_block(e, false)).ok()
+    res.map_err(|e| show_e_block(format!("{:#}", e), false))
+        .ok()
 }

 pub fn show_err_blocking<T, E: Display>(res: Result<T, E>) -> Option<T> {
-    res.map_err(|e| show_e_block(e, true)).ok()
+    res.map_err(|e| show_e_block(format!("{:#}", e), true)).ok()
 }


Why are you changing this code? I'm surprised it even compiles. {:#} is valid for E: Debug but the type bound is not specified. In practice, try not to depend on Debug, rather depend on Display.

Ah, an explanation of what happened here:
While testing the "no unwrap" changes, I found that I wasn't getting the "inner error" (i.e. the ErrorUnknown from the media error), just the context. Now, to me, this is very bad (because it makes it harder to debug codec error reports from users/etc).
However, I couldn't get the error details out of it without either duplicating the inner error details in the context or going through this. I'll change things to duplicate the inner error details in the context.

Remember you can get help from anyhow to format the error

Turns out that wasn't necessary. Not sure what I missed the first time; doesn't matter in any case.
d3645fe undoes this.

zmerp · 2023-12-20T20:21:16Z

alvr/client_core/src/platform/android/decoder.rs

+                decoder.stop().unwrap();
+                drop(decoder);
+            } else {
+                show_err(preparing_decoder);


Don't try to print the whole object. Rather, if you handle both Ok and Err cases, prefer using match instead of if-let, then print only the error in the Err branch.

zmerp · 2023-12-20T22:17:06Z

Thanks. There are more styling nits i would point out but I guess it's fine for now.

20kdc · 2023-12-21T14:50:24Z

Ok, so, things to do? should I squish these all into one commit? Etc.

zmerp · 2023-12-22T09:05:03Z

I'd like to get some more testers before merging

20kdc · 2023-12-22T12:49:34Z

I will say, in my testing - and I get this with a hardware decoder on another device also - I seem to have this problem where ALVR-in-PhoneVR flashes between the last two frames a lot.
I'm bringing this up not because it's related to the PR exactly, but because it's a situation that this PR could exacerbate when in-use (due to the relatively high latency of the software decoder), and I'm wondering if there's anything that's already known about it so I can make another PR to fix that.
If I could fix that, then all I really have left to do is figure out if there are any workarounds I can apply to make the hardware decoder work.

ShootingKing-AM · 2023-12-24T07:30:02Z

Tested this. I just release built with this pr and ran ALVR, works fine. Any specific test methodology/procedure to test this pr ?

20kdc · 2023-12-24T17:36:12Z

Tested this. I just release built with this pr and ran ALVR, works fine. Any specific test methodology/procedure to test this pr ?

Try force software, make sure it works. If you have a device which you know decoder init fails on reliably, it is good to test with that too; it should work (with high latency). Keep in mind that you can recursive-clone PhoneVR, change directly to this commit (not merge), and get deps setup, or even symlink PhoneVR's ALVR directory to your ALVR repo.

EDIT: Visual side-effects due to high latency are out of scope for this PR, but having a way to consistently trigger them should be productive to allow other developers to provide insight.

ShootingKing-AM · 2023-12-25T08:51:14Z

Tested this. I just release built with this pr and ran ALVR, works fine. Any specific test methodology/procedure to test this pr ?

Try force software, make sure it works. If you have a device which you know decoder init fails on reliably, it is good to test with that too; it should work (with high latency). Keep in mind that you can recursive-clone PhoneVR, change directly to this commit (not merge), and get deps setup, or even symlink PhoneVR's ALVR directory to your ALVR repo.

EDIT: Visual side-effects due to high latency are out of scope for this PR, but having a way to consistently trigger them should be productive to allow other developers to provide insight.

Dont have a device that fails, but everything else is working normally.

zmerp · 2024-01-02T18:06:59Z

I'll merge within a day if noone has more objections :)

20kdc added 2 commits December 17, 2023 22:17

Android: When the hardware/default decoder fails to initialize, fall …

70a61c2

…back to software.

Forgot to "cargo fmt --all", thank you CI

ab65b94

zmerp requested changes Dec 18, 2023

View reviewed changes

alvr/client_core/src/decoder.rs Show resolved Hide resolved

20kdc added 3 commits December 18, 2023 20:47

Android: Reduce unwraps in decoder setup to one unified place so that…

f8fdf7f

… can be figured out later

Android: decoder.rs: cargo fmt --all

fc739bd

Add option to force software decoding

ebdc6ad

zmerp requested changes Dec 19, 2023

View reviewed changes

20kdc added 2 commits December 20, 2023 16:33

Android: Reduce use of unwrapping in decoder setup. Logging: errors s…

97eff12

…hould be more explicit

Settings: Move force_software_decoder above mediacodec_extra_options,…

d9a8731

… Logging: cargo fmt --all

zmerp requested changes Dec 20, 2023

View reviewed changes

20kdc added 2 commits December 20, 2023 22:03

Revert logging changes made for Android decoder error messages

d3645fe

Android: Use match instead of if-let for decoder error handling

41bb3a7

zmerp approved these changes Dec 20, 2023

View reviewed changes

zmerp merged commit 377c556 into alvr-org:master Jan 3, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Android: When the hardware/default decoder fails to initialize, fall back to software. #1933

Android: When the hardware/default decoder fails to initialize, fall back to software. #1933

20kdc commented Dec 17, 2023 •

edited

Loading

20kdc commented Dec 18, 2023

zmerp Dec 18, 2023

20kdc Dec 18, 2023

zmerp Dec 19, 2023

20kdc Dec 20, 2023

zmerp Dec 18, 2023

zmerp Dec 18, 2023

20kdc Dec 18, 2023

zmerp Dec 19, 2023

20kdc Dec 20, 2023

zmerp Dec 19, 2023

20kdc Dec 20, 2023

zmerp Dec 19, 2023

zmerp Dec 20, 2023

20kdc Dec 20, 2023

zmerp Dec 20, 2023

20kdc Dec 20, 2023

zmerp Dec 20, 2023

zmerp commented Dec 20, 2023

20kdc commented Dec 21, 2023

zmerp commented Dec 22, 2023

20kdc commented Dec 22, 2023

ShootingKing-AM commented Dec 24, 2023

20kdc commented Dec 24, 2023 •

edited

Loading

ShootingKing-AM commented Dec 25, 2023

zmerp commented Jan 2, 2024

Android: When the hardware/default decoder fails to initialize, fall back to software. #1933

Android: When the hardware/default decoder fails to initialize, fall back to software. #1933

Conversation

20kdc commented Dec 17, 2023 • edited Loading

20kdc commented Dec 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zmerp commented Dec 20, 2023

20kdc commented Dec 21, 2023

zmerp commented Dec 22, 2023

20kdc commented Dec 22, 2023

ShootingKing-AM commented Dec 24, 2023

20kdc commented Dec 24, 2023 • edited Loading

ShootingKing-AM commented Dec 25, 2023

zmerp commented Jan 2, 2024

20kdc commented Dec 17, 2023 •

edited

Loading

20kdc commented Dec 24, 2023 •

edited

Loading