Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Omega] Fix loading ISO files on Android arm-v7a #25156

Merged
merged 1 commit into from May 13, 2024

Conversation

joseluismarti
Copy link
Contributor

Description

There is an issue on 32-bit Android devices when trying to open an ISO file from the internal storage, kodi force closes and displays the following debug message:

D Kodi : **ERROR: fseeko (): Invalid argument

The libcdio library is calling the fseeko function with a 64-bit off_t argument and this function is not available in Android API 21, was added in 24.

The fix would be to target 24 as done in v22.

I propose a workaround to continue at level 21 in v21: disable iso9660pp so that libudfread is used to open ISO files, similar to how network ISO files are opened.

How has this been tested?

Tested to open and play a DVD ISO file on a 32-bit Android device.

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • Student submission (PR was done for educational purposes and will be treated as such)
  • None of the above (please explain below)

Checklist:

  • My code follows the Code Guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the Contributing document
  • I have added tests to cover my change
  • All new and existing tests passed

@thexai thexai added Type: Fix non-breaking change which fixes an issue Platform: Android Component: FileSystem Filesystem v21 Omega labels May 7, 2024
@thexai thexai added this to the Omega 21.1 milestone May 7, 2024
@thexai thexai linked an issue May 7, 2024 that may be closed by this pull request
6 tasks
@joseluismarti
Copy link
Contributor Author

The builder has ignored the argument:

-- ISO9660PP enabled: Yes

Locally it has worked, any advice?

@thexai
Copy link
Member

thexai commented May 7, 2024

No idea, ping @fuzzard

@thexai
Copy link
Member

thexai commented May 7, 2024

Maybe False instead OFF ?

@joseluismarti
Copy link
Contributor Author

Maybe False instead OFF ?

Format is correct (also seen here) as it works locally. Maybe when configuring it's redefined, better wait for ping

@joseluismarti
Copy link
Contributor Author

Works! thank you both

@fuzzard
Copy link
Contributor

fuzzard commented May 8, 2024

My concern is what is disabling the iso9660p library actually going to do?

I dont understand how this is a new problem in Omega over Nexus. Nothing has changed regarding libcdio in Omega as far as i could see. is there something specifically changed in Omega that somehow caused this? Does this even work in Nexus?

@joseluismarti
Copy link
Contributor Author

I've checked that the last release that opens an iso file is 18.9, at that time libudfread was used.

From 19.0 onwards crashes with the invalid argument message in fseeko.

libcdio was added in 19.0 (#16262)

Copy link
Member

@thexai thexai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on Sony TV (arm-v7a): tested DVD ISO from USB storage and SMB.

Also tested Blu-Ray BDMV folder and BD ISO from SMB for possible regressions. All is working fine.

@howie-f
Copy link
Contributor

howie-f commented May 8, 2024

Tested on Sony TV (arm-v7a): tested DVD ISO from USB storage and SMB.

apart from fuzzard's concerns.. wasn't the error only happening when trying to open an ISO from internal storage? not SMB/USB

@thexai
Copy link
Member

thexai commented May 8, 2024

USB also was crashing for me in this Sony TV with Play Store version

@lrusak
Copy link
Contributor

lrusak commented May 9, 2024

What about making the UDF implementation higher priority?

return new CUDFDirectory();

@joseluismarti
Copy link
Contributor Author

What about making the UDF implementation higher priority?

Could also be fixed in this way, but it would affect all platforms. To reduce the risks better the current solution that only affects Android arm-v7a.

@lrusak
Copy link
Contributor

lrusak commented May 9, 2024

What about making the UDF implementation higher priority?

Could also be fixed in this way, but it would affect all platforms. To reduce the risks better the current solution that only affects Android arm-v7a.

I would check if that solves the problem.

Also has this change been made to master? I see it's PR'd to the Omega branch.

@joseluismarti
Copy link
Contributor Author

Also has this change been made to master? I see it's PR'd to the Omega branch.

Master targets Android API 24, at this level fseeko is "fully" available and therefore libcdio works properly, no change is necessary.

@lrusak
Copy link
Contributor

lrusak commented May 9, 2024

Also has this change been made to master? I see it's PR'd to the Omega branch.

Master targets Android API 24, at this level fseeko is "fully" available and therefore libcdio works properly, no change is necessary.

Could libcdio be patched to fix the issue?

@joseluismarti
Copy link
Contributor Author

Could libcdio be patched to fix the issue?

A 64-bit fseeko function or something similar should be implemented, but at the moment since it doesn't affect Master and there is a workaround that works for v21 I don't think it's worth.

@fritsch
Copy link
Member

fritsch commented May 13, 2024

@lrusak reasoning @joseluismarti fine with you? Given that it is about v21 stable?

@lrusak
Copy link
Contributor

lrusak commented May 13, 2024

@lrusak reasoning @joseluismarti fine with you? Given that it is about v21 stable?

I don't really care, I was just suggesting alternatives. There was some discussion in #android-external on slack about alternatives. I'm not sure where that got to though and ultimately it may be up to @fuzzard

@fritsch
Copy link
Member

fritsch commented May 13, 2024

Yeah - also not my usecase. So whatever was the winner of the discussion should make it.

@howie-f
Copy link
Contributor

howie-f commented May 13, 2024

So whatever was the winner of the discussion should make it.

there was no real winner. i just raised the concern that disabling iso9660pp could probably break playback of physical dvd‘s… but android is also not my use case nor can i test dvd playback anymore.

not an easy decision. maybe merge as is and revert if shit happens 🤷

@joseluismarti
Copy link
Contributor Author

I also don't use this feature just to search for the fix, in the tests I've done I can see the content of the iso file and the playback also works.

The reporter and someone else confirm that it works, apparently well.

@fritsch
Copy link
Member

fritsch commented May 13, 2024

Good - if we all don't know. Let's do what @howie-f suggested.

@fritsch fritsch merged commit be87ad2 into xbmc:Omega May 13, 2024
1 check passed
@fritsch
Copy link
Member

fritsch commented May 13, 2024

Thanks everyone involved.

@thexai thexai mentioned this pull request May 14, 2024
6 tasks
@joseluismarti joseluismarti deleted the iso9660pp branch May 14, 2024 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: FileSystem Filesystem Platform: Android Type: Fix non-breaking change which fixes an issue v21 Omega
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Loading *.iso forces close Kodi 21
6 participants