Android: Use storage access framework for custom SD card paths

[JosJuice]

This is part of my efforts to add support for scoped storage.

[Ebola16]

I'm able to set any file to the SD Card Path now. If we can't limit what is selectable in SAF, perhaps we should add checks after retrieving the file to see if the extension is expected?

[Ebola16]

Also, the path returned by SAF is different from what Dolphin uses for default paths (although they target the same file). Is it possible to convert the path returned by SAF to match the default paths? If not, the default paths could be changed to match what SAF returns, although this may require making the default paths Android-specific.

[JosJuice]

I'm able to set any file to the SD Card Path now. If we can't limit what is selectable in SAF, perhaps we should add checks after retrieving the file to see if the extension is expected?

I could add a check afterwards, but it seems like there are some content providers where the URIs don't actually contain filename extensions, like Google Drive. So I would be shutting those out by adding a check as far as I understand it. Perhaps there are also cases where you would want to name the extension something other than .raw. What do you think about showing a warning unless the extension is .raw or nothing, and having a "continue anyway" option in the warning?

(For what it's worth, I don't expect Google Drive to have good performance...)

Is it possible to convert the path returned by SAF to match the default paths?

I don't believe there is a reliable way to do this.

If not, the default paths could be changed to match what SAF returns, although this may require making the default paths Android-specific.

I don't believe there is a reliable way to do this either. I don't think that the paths not matching will cause any problems in practice, though.

[Ebola16]

I could add a check afterwards, but it seems like there are some content providers where the paths don't actually contain extensions, like Google Drive. So I would be shutting those out by adding a check as far as I understand it. Perhaps there are also cases where you would want to name the extension something other than .raw. What do you think about showing a warning unless the extension is .raw or nothing, and having a "continue anyway" option in the warning?

I can't look into this right now but is there a native way to tell if the file is actually a virtual SD card? I'd prefer that check over the warning but your suggestion sounds good if that isn't feasible.

My other previous concerns were minor and can be ignored / deferred.

[JosJuice]

I can't look into this right now but is there a native way to tell if the file is actually a virtual SD card?

Well, you could look at the part of the URI that specifies which storage provider is being used, but that's hacky and might break in the future and will likely make us shut out storage providers that otherwise would work fine. So I think I'll go with my warning.

[Ebola16]

Well, you could look at the part of the URI that specifies which storage provider is being used, but that's hacky and might break in the future and will likely make us shut out storage providers that otherwise would work fine. So I think I'll go with my warning.

I was thinking about something within the file that identifies it as a virtual SD card. But if that check doesn't already exist in C++ then the warning will be the least worst option.

[JosJuice]

Oh, sorry, I thought you meant an actual SD card that you put in your phone rather than an SD card file... No, the C++ code doesn't make any assumptions about what data is in a virtual SD card. It's up to the emulated console to do that.

[JosJuice]

Warning added.

[Ebola16]

SAF is such a pain...

The path of a valid virtual SD card saved by SAF causes a SIGABRT on current master when trying to load a game through Gecko OS. This creates yet another thing to account for when bisecting. Any ideas on preventing this?
Stack trace:

abort 0x000000708010332c
OpenAndroidContent(std::__ndk1::basic_string<char, std::__ndk1::char_traits<char>, std::__ndk1::allocator<char> > const&, std::__ndk1::basic_string<char, std::__ndk1::char_traits<char>, std::__ndk1::allocator<char> > const&) AndroidCommon.cpp:56
File::IOFile::Open(std::__ndk1::basic_string<char, std::__ndk1::char_traits<char>, std::__ndk1::allocator<char> > const&, char const*) File.cpp:80
IOS::HLE::Device::SDIOSlot0::OpenInternal() SDIOSlot0.cpp:65
IOS::HLE::Device::SDIOSlot0::Open(IOS::HLE::OpenRequest const&) SDIOSlot0.cpp:84
IOS::HLE::Kernel::OpenDevice(IOS::HLE::OpenRequest&) IOS.cpp:553
IOS::HLE::Kernel::HandleIPCCommand(IOS::HLE::Request const&) IOS.cpp:570
IOS::HLE::Kernel::ExecuteIPCCommand(unsigned int) IOS.cpp:621
CoreTiming::Advance() CoreTiming.cpp:334
<unknown> 0x0000006fce0010ec

[Ebola16]

A SIGABRT in this PR occured when attempting to load the virtual SD card with Gecko OS.

1. Set a valid virtual SD card via SAF. Also set a default ISO to load with Gecko OS.
2. Uninstall then Reinstall this PR.
3. Load Gecko OS.
4. Stack trace:

abort 0x000000708010332c
art::Runtime::Abort(char const*) 0x0000006ffa329fa4
android::base::LogMessage::~LogMessage() 0x000000707f64ece4
art::JavaVMExt::JniAbort(char const*, char const*) 0x0000006ffa1aa06c
art::JavaVMExt::JniAbortV(char const*, char const*, std::__va_list) 0x0000006ffa1aa1dc
art::(anonymous namespace)::ScopedCheck::AbortF(char const*, ...) 0x0000006ff9fc1600
art::(anonymous namespace)::ScopedCheck::CheckPossibleHeapValue(art::ScopedObjectAccess&, char, art::(anonymous namespace)::JniValueType) 0x0000006ff9fbffdc
art::(anonymous namespace)::ScopedCheck::Check(art::ScopedObjectAccess&, bool, char const*, art::(anonymous namespace)::JniValueType*) 0x0000006ff9fbf348
art::(anonymous namespace)::CheckJNI::NewString(_JNIEnv*, unsigned short const*, int) 0x0000006ff9fb4264
_JNIEnv::NewString(unsigned short const*, int) jni.h:829
ToJString(_JNIEnv*, std::__ndk1::basic_string<char, std::__ndk1::char_traits<char>, std::__ndk1::allocator<char> > const&) AndroidCommon.cpp:30
OpenAndroidContent(std::__ndk1::basic_string<char, std::__ndk1::char_traits<char>, std::__ndk1::allocator<char> > const&, std::__ndk1::basic_string<char, std::__ndk1::char_traits<char>, std::__ndk1::allocator<char> > const&) AndroidCommon.cpp:73
File::IOFile::Open(std::__ndk1::basic_string<char, std::__ndk1::char_traits<char>, std::__ndk1::allocator<char> > const&, char const*) File.cpp:74
IOS::HLE::Device::SDIOSlot0::OpenInternal() SDIOSlot0.cpp:65
IOS::HLE::Device::SDIOSlot0::Open(IOS::HLE::OpenRequest const&) SDIOSlot0.cpp:84
IOS::HLE::Kernel::OpenDevice(IOS::HLE::OpenRequest&) IOS.cpp:553
IOS::HLE::Kernel::HandleIPCCommand(IOS::HLE::Request const&) IOS.cpp:570
IOS::HLE::Kernel::ExecuteIPCCommand(unsigned int) IOS.cpp:621
CoreTiming::Advance() CoreTiming.cpp:334
<unknown> 0x0000006fce0010ec

[JosJuice]

Regarding your first crash, I can't retroactively fix the current version of master. The fix is in the "Android: Make the handling of SAF open modes more robust" commit. (For what it's worth, if you use a version of Dolphin that's a few months old, back when OpenAndroidContent didn't exist, then I believe it'll just fail to open the SD card instead of crashing.)

Regarding your second crash, that looks pretty weird. I'll try to see if I can reproduce it.

[JosJuice]

I've added a commit that should fix the second crash. Please test it.

[Ebola16]

Regarding your first crash, I can't retroactively fix the current version of master. The fix is in the "Android: Make the handling of SAF open modes more robust" commit. (For what it's worth, if you use a version of Dolphin that's a few months old, back when OpenAndroidContent didn't exist, then I believe it'll just fail to open the SD card instead of crashing.)

I had a feeling that was the case. I guess we can't do anything more about it.

I've added a commit that should fix the second crash. Please test it.

Instead of not reading from files that raise a SecurityException, should a request for the missing permission be added to Dolphin's other permission requests at startup? The current workaround seems like something that will cause further problems as SAF is used to set more paths. I'm not sure exactly which permission would be missing though.

[JosJuice]

Instead of not reading from files that raise a SecurityException, should a request for the missing permission be added to Dolphin's other permission requests at startup?

I'm not aware of any way to do that other than opening the file picker and hoping the user selects the right file, which would be a rather annoying prompt to get at startup.

I'm not sure exactly which permission would be missing though.

It's the permission for the specific file. The point of scoped storage is that the user grants permissions on a file-by-file basis.

[Ebola16]

I should make looking into the specific details of scoped storage a higher priority...

Re-requesting the user to select a SD file path sounds like a better option than needing to look at logs to figure out that permission hasn't been granted.

How about after running Dolphin's permission checks, run another check to see if we have permission to access the SD card file. If not, alert the user something along the lines of "Dolphin needs permission to access the SD card file. Please select an SD card file.", then open the SAF.

Alternatively, this could be done at emulation startup but switching activities during startup may cause more surface destruction issues.

[JosJuice]

What do you think about just turning the "couldn't open SD card" log message into a panic alert? That way it's clear to the user that something is wrong, but you wouldn't be bothered as soon as the app starts.

[Ebola16]

I think we should still give the user the ability to set a path without searching for the path in settings if they don't have permission to access their SD card file. What about my previous suggestion except instead of running the check at startup, we run it right before calling the emulation activity (after the user has selected a game). That should address both of our concerns.

[JosJuice]

I think sending the user straight to the file picker is going to be annoying once we start using SAF for more settings than just the SD card, as the user then would get prompted several times in a row and might not remember which prompt is for which path once the file picker is open (as we can't show any custom UI inside the file picker to remind the user which path they're currently selecting). What do you think about this: If any path in the settings is set to something which is inaccessible (either because it doesn't exist or because of permission reasons), Dolphin asks the user if they want to go to the paths settings when trying to start emulation. In the paths settings, the paths which are inaccessible are highlighted in red so you have a quick overview of what needs to be fixed. This would also make it possible for the user to reset the paths if they would prefer that.

[Ebola16]

I think sending the user straight to the file picker is going to be annoying once we start using SAF for more settings than just the SD card, as the user then would get prompted several times in a row and might not remember which prompt is for which path once the file picker is open (as we can't show any custom UI inside the file picker to remind the user which path they're currently selecting). What do you think about this: If any path in the settings is set to something which is inaccessible (either because it doesn't exist or because of permission reasons), Dolphin asks the user if they want to go to the paths settings when trying to start emulation. In the paths settings, the paths which are inaccessible are highlighted in red so you have a quick overview of what needs to be fixed. This would also make it possible for the user to reset the paths if they would prefer that.

SGTM

[JosJuice]

Actually, I shouldn't check for both if it doesn't exist and if we don't have permission, only if we don't have permission. The core will automatically create a 128 MiB SD card image for you if there is none already. Guess I'll have to look up specifically how to check whether we have permission to access a certain file which may or may not exist...

[Ebola16]

The documentation isn't fantastic on this subject. If all else fails I think we could stick with catching SecurityException to find out if we don't have permission.

[JosJuice]

Actually, it seems like SAF won't let us recreate something we have been granted permission to directly (as opposed to something we have been granted permission to through a parent directory). So we should flag SAF URIs which point to something which doesn't exist.

[Ebola16]

Sorry if I'm missing something obvious.

I haven't looked into this in too much detail but I see we currently request https://developer.android.com/reference/android/Manifest.permission#WRITE_EXTERNAL_STORAGE

This permission seems more broad: https://developer.android.com/reference/android/Manifest.permission#MANAGE_EXTERNAL_STORAGE

It might be worth looking into seeing if MANAGE_EXTERNAL_STORAGE improves handling scoped storage permissions. Although I've never tried conditionally requesting permissions based on API version.

Actually, it seems like SAF won't let us recreate something we have been granted permission to directly (as opposed to something we have been granted permission to through a parent directory). So we should flag SAF URIs which point to something which doesn't exist.

Sounds worth trying.

[JosJuice]

MANAGE_EXTERNAL_STORAGE is more or less an opt-out of scoped storage. Google Play will not allow us to use it, as it's only for apps that really require it, like file managers or backup apps.

[mbc07]

(as we can't show any custom UI inside the file picker to remind the user which path they're currently selecting)

A "trick" that I see on various SAF-enabled apps is generating a toast message with a reasonably long timeout duration right before calling the File Picker, as the toast will remain on top even after switching activities. It's not an ideal approach as the toast will inevitably be gone after some time, but given the constraints it might still be better than nothing...

[Ebola16]

I don't know if this is valid but I think we could justify that SAF isn't sufficient because https://developer.android.com/guide/topics/providers/document-provider says:

"Documents can be either an openable file (with a specific MIME type), or a directory containing additional documents (with the MIME_TYPE_DIR MIME type)."

The files we deal with aren't recognized as a specific MIME type but we want to manipulate them in all of the ways mentioned in the "Document management apps" case for MANAGE_EXTERNAL_STORAGE justification (https://support.google.com/googleplay/android-developer/answer/9956427?hl=en).

It might be worth requesting this permission if it hasn't already been tried.

[JosJuice]

The files we want to open do get assigned a MIME type, it's just that the MIME type is uselessly vague. So this only stops us from being able to filter files in the file picker in a useful way, not from opening and otherwise dealing with files.

Personally, I'm not willing to go through trying to request this permission from Google, partially because I don't think they'll allow it and partially because I think it's possible to use SAF to make something which is good enough. The only use case that's noticeably impacted so far is reinstalling Dolphin, which is not something the typical user does often.

[Ebola16]

Ok, a useless MIME type is still a problem that our old file picker didn't need to worry about.

Losing the ability to filter files means we need to rely on users to select a valid file, which will definitely lead to problems if they ignore our warning alertmessage. We have the ability to set custom paths for files too, which further complicates our file access permission needs. This will likely create problems down the road when trying to load a path (such as a SD file) from custom game settings that a user will need to resolve.

From what I've read it sounds like Dolphin could be considered for MANAGE_EXTERNAL_STORAGE permission. I still think this is something worth pursuing. Filling out their permission request form sounds like a better alternative than pursuing these options that are worse than our current file picker. I can't do it myself however so if you still feel strongly about not doing so I'll drop this subject.

[JosJuice]

Keep in mind that the core functionality of Dolphin is to run games, and it seems like the core functionality is what Google looks at for determining if we can get an exception. Selecting a game of the wrong file type will just lead to nothing showing up in the game list (if picking a folder) or the game failing to boot (if picking a single game). (This is why I created PR #9229, by the way.) Setting custom paths for the SD card and such is not really something I think they would consider to be core functionality. I do agree that there are annoyances with SAF, but I don't think they are major enough that Google would grant us an exception.

Filling out their permission request form sounds like a better alternative than pursuing these options that are worse than our current file picker. I can't do it myself however so if you still feel strongly about not doing so I'll drop this subject.

I have no more access to the Google Play Console than you do.

[Ebola16]

Under the exceptions section of https://support.google.com/googleplay/android-developer/answer/9956427?hl=en:

"1. Use of the permission enables the app’s core functionality..."

The user will eventually need to pick a directory or file, which will enable Dolphin to launch a game (our core functionality). This is why I think Dolphin qualifies for this.

Since their SAF is making our core functionality unnecessarily more complicated, I see no reason not to request MANAGE_EXTERNAL_STORAGE when Google's alternatives don't fully suit our needs.

Unless I'm missing something about the permissions update process I think this could save both of us time in the future and filling out a form is a small potential waste of time. I don't want to ping the person with Google Play Console access until I've made a convincing argument for MANAGE_EXTERNAL_STORAGE. I also don't know who that person is.

But again, if you still think this is a bad idea I'll drop it.

[JosJuice]

According to that section, SAF would need to have a "substantially detrimental impact" on the core functionality in order for them to grant us an exception. I don't believe something as minor as not being able to filter by file name extension is something that Google would count. Keep in mind that they're treating this as a "high risk or sensitive permission" – it's not something they're happy to hand out to anyone who wants it.

Also, using SAF does actually have benefits such as being able to run games from an SD card, so using SAF instead of MANAGE_EXTERNAL_STORAGE might actually be a desirable feature for users depending on what you value more. In other words, there is a reason to switch to SAF for game picking even if it wasn't for scoped storage being forced on us. (This is less of a good reason when it comes to the path settings, but like I mentioned before, Google won't count that as core functionality anyway.)

For the reference, I believe delroth has access to the Google Play Console for Dolphin.

[JosJuice]

Regarding saving us time, I've actually already implemented full SAF support for the game list in a separate branch which I will create a PR for once this is merged.

[Ebola16]

According to that section, SAF would need to have a "substantially detrimental impact" on the core functionality in order for them to grant us an exception. I don't believe something as minor as not being able to filter by file name extension is something that Google would count. Keep in mind that they're treating this as a "high risk or sensitive permission" – it's not something they're happy to hand out to anyone who wants it.

Failing to boot a game from picking an incorrect file could be argued as a "substantially detrimental impact".

Also, using SAF does actually have benefits such as being able to run games from an SD card, so using SAF instead of MANAGE_EXTERNAL_STORAGE might actually be a desirable feature for users depending on what you value more. In other words, there is a reason to switch to SAF for game picking even if it wasn't for scoped storage being forced on us. (This is less of a good reason when it comes to the path settings, but like I mentioned before, Google won't count that as core functionality anyway.)

I can run games from a physical SD card on master. I don't understand this point.

[JosJuice]

Failing to boot a game from picking an incorrect file could be argued as a "substantially detrimental impact".

I really don't think so. We can display an error message saying that the file isn't supported, and the user can easily pick a new file. It's a very small inconvenience for the user, not a substantial one.

I can run games from a physical SD card on master. I don't understand this point.

Oh, huh, I thought it didn't work at all in master... Maybe it's something like it only works on certain phones then? I do remember reading something about SAF letting you access SD cards in a way that normal file access doesn't, but I must admit I've never tested it myself in master.

EDIT: Perhaps it was that you need SAF if you want to write to the SD card. If so, you can ignore what I said about SD cards.

[Ebola16]

Oh, huh, I thought it didn't work at all in master... Maybe it's something like it only works on certain phones then? I do remember reading something about SAF letting you access SD cards in a way that normal file access doesn't, but I must admit I've never tested it myself in master.
EDIT: Perhaps it was that you need SAF if you want to write to the SD card. If so, you can ignore what I said about SD cards.

I guess I can justify gaining the ability to write to the SD card worth the sacrifice of bisecting inconveniences. Oh well, at least we learned something from this.

[JosJuice]

I have now added the warning about unavailable paths that we discussed earlier.

[Ebola16]

Otherwise, LGTM.