Fixed, Reduce mass-storage consumption with Android custom apps with embedded ZIM #3516
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I have tried many ways to get the FileDescriptor from the asset folder, but no one works.
But this error is not with
@kelson42, @mgautierfr is it possible to read the zim file via |
|
@mgautierfr This is for you. |
mgautierfr
reviewed
Oct 30, 2023
| } | ||
| } | ||
| } | ||
| val inputStream = assetManager.openFd(BuildConfig.PLAY_ASSET_FILE) |
There was a problem hiding this comment.
openFd returns a AssetFileDescriptor which is AutoCloseable.
So I suspect (to verify) that when we exit the function, inputStream is closed and so the fileDescriptor become invalid.
(You can check that with the valid function before passing it to Archive.)
It seems you can use getParcelFileDescriptor which return ParcelFileDescriptor which is also AutoCloseable.
So with some luck the ParcelFileDescriptor keep the fd open where FileDescriptor doesn't.
Comment on lines
1390
to
1394
| protected fun openZimFile( | ||
| file: File?, | ||
| isCustomApp: Boolean = false, | ||
| fileDescriptor: FileDescriptor? = null | ||
| ) { |
There was a problem hiding this comment.
I am a bit disturb by this function signature.
I think it would be better to have two methods call openZimFile and openZimFd instead of having option arguments.
(But it is maybe because I'm not familiar with Kotlin language)
There was a problem hiding this comment.
@mgautierfr Here we only need to check if it is file or fileDescriptor that comes from the CustomFileValidator class and we are creating an Archive object accordingly. We will improve our code but first it should work with fileDescriptor.
@kelson42 I have made changes locally to not compress the asset noCompress (But somehow it is doing changes in asset folder or might be
@mgautierfr Thanks for your feedback, yes we are checking
@mgautierfr Thanks for the suggestion, I have tried it, and it is giving the same error @mgautierfr It seems not related to valid fd because I have tried directly creating an Archive object in
|
|
@MohitMaliDeveloper Can you please read the first 100 bytes in Java from the fd you try to pass to the libkiwix and dump it here in hexa? |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #3516 +/- ##
=============================================
- Coverage 49.15% 48.99% -0.16%
+ Complexity 1075 1073 -2
=============================================
Files 285 285
Lines 10365 10382 +17
Branches 1382 1393 +11
=============================================
- Hits 5095 5087 -8
- Misses 4451 4473 +22
- Partials 819 822 +3
☔ View full report in Codecov by Sentry. |
|
@kelson42 Here is the first 100 bytes read from fd which is returned by But I wonder because the |
|
@MohitMaliFtechiz In hexadecimal?! |
|
@kelson42 Here is the hexadecimal with and this is with |
|
@mgautierfr To me it looks the fd is OK, so this is probably some kind of stupid problem around the fd mgmt. |
|
@MohitMaliFtechiz You need to use the Archive constructor with an offset and size (https://github.com/kiwix/java-libkiwix/blob/main/lib/src/main/java/org/kiwix/libzim/Archive.java#L41). getStartOffset and getLength should be the proper way to get the offset and size. |
|
I think the best way to do it is : val offset = assetFileDescriptor.getStartOffset();
val size = assetFileDescriptor.getLength();
val parcelFileDescriptor = assetFileDescriptor.getParcelFileDescriptor().dup();
val jniKiwixReader = Archive(parcelFileDescriptor.fileDescriptor, offset, size);
parcelFileDescriptor.detachFd(); |
|
@kelson42 @mgautierfr I didn't read the comment of Mohit properly. The fd() method does not return the proper content! |
@mgautierfr Thanks for providing more details now i am able to read the zim file with locally with FileDescriptor. This method of the Archive works for me. Before it, i was using but this method was not working. Now with the updated code i am able to read the file from asset folder with fileDescriptor, i am testing this with Screenrecorder-2023-10-31-16-33-39-182.mp4 |
|
@kelson42, @mgautierfr This is working with
|
MohitMaliFtechiz
force-pushed
the
Issue#56
branch
from
a92630f
to
a4f0132
Compare
|
@kelson42, @mgautierfr We have refactored our app code to create Having an issue uploading video on Github please see here https://drive.google.com/file/d/17evixXjjQdwphOWlMUTY1BIia2baDr2E/view?usp=drive_link The archive object is created with valid Attempting to create a reader with fd: 252
createKiwixServer: Size = 354893501
MainEntryTitle = mainPage
Article count = 244057Apart from this, I have tried to get @mgautierfr can you please have a look at this? Edited: ZimFileNotHostedOnKiwixServerWithFileDescriptor.mp4 |
This was referenced Nov 4, 2023
|
@MohitMaliFtechiz Pretty sure this is a bug (already known) in the libkiwix. If this is the only problem left, please open:
@veloman-yunkan @mgautierfr Might that be that libkiwix HTTP service can not work properly if ZIM files are loaded via an fd (no filename, so no humanid)? |
|
@MohitMaliFtechiz This line |
This is probably that. We can't store in library.xml information about fd (offset, ...). We can only store a path. And we need a path to open the archive later. We have no way to start a server on a existing zim archive. Server on libkiwix now always work on library, and on library pointing to plain zim file. (And it is somehow a constraints as it is the server/library which handle opening/closing/caching zim archive). |
@kelson42 I have deactivated the feature for custom apps, and it does not impact the
I have opened a ticket for this kiwix/libkiwix#1015.
No, this is required for getting the kiwix-android/custom/build.gradle.kts Line 37 in 8f099e6 So we need this to get the
|
… of creating a file to avoid using storage twice for same zim file.
* Telling android to not compress the `.zim` files in asset folder while building the apk/bundle.
* Now we are using fileDescriptor for custom apps to read zim file from asset folder, so we have improved our notes saving functionality according to this change.
* The problem was that we were saving the note name combined with `zimFileTitle` and `articleTitle`. When opening the saved note from `NotesFragment`, it retrieved the `noteTitle` from the database, which already contained the combined name. Subsequently, when saving the note again, it redundantly combined the name. To resolve this issue, we now extract the `articleName` when updating notes in `NotesFragment`, ensuring that it updates existing notes instead of creating new ones.
…Fragment` for custom apps. * In `ZimHostFragment`, we show Zim files that are saved in the database. These files are typically saved when downloading Zim files. In a custom app, where Zim files are already included within the app and not downloaded separately that's why they are not showing on the `ZimHostFragment`, we have addressed this issue by saving the Zim files in the database to ensure they appear in the `ZimHostFragment`. * Regarding the `FileDescriptor`, there are no file objects available because we read Zim files using `FileDescriptor`. To address this, we have created a `demo.zim` file to save it in the database so that it will be displayed in the `ZimHostFragment`. We handle this file within the `KiwixServer`. When the current Zim file is `demo.zim`, we create an `Archive` object with the `FileDescriptor` to host the Zim file on the `KiwixServer`.
* Since we are now using fd (FileDescriptor) to read the zim file from the asset folder. Currently, 'KiwixServer' is unable to host zim files via fd. This feature is temporarily hidden for custom apps. We will re-enable it for custom apps once the issue is resolved.
|
Unfortunately no review, but I have to merge. |
|
@kelson42 @MohitMaliDeveloper I'll review/test this. I'll open a ticket if I'll find something. |
This was referenced Oct 10, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #56
Fixes #3519
Fixes #3522
fileDescriptorinstead of creating a file to avoid using storage twice for the same zim file.fileDescriptorfor custom apps, and our notes functionality breaks with this change so we have refactored our notes functionality to properly work withfileDescriptor.Notesaves multiple times while we are updating existing notes fromNotesFragment. #3519. The problem was that we were saving the note name combined withzimFileTitleandarticleTitle. When opening the saved note fromNotesFragment, it retrieved thenoteTitlefrom the database, which already contained the combined name. Subsequently, when saving the note again, it redundantly combined the name. To resolve this issue, we now extract thearticleNamewhen updating notes inNotesFragment, ensuring that it updates existing notes instead of creating new ones.notesissuefixed.mp4
ZimHostFragmentfor custom apps.ZimHostFragment, we show Zim files that are saved in the database. These files are typically saved when downloading Zim files. In a custom app, where Zim files are already included within the app and not downloaded separately that's why they are not showing on theZimHostFragment, we have addressed this issue by saving the Zim files in the database to ensure they appear in theZimHostFragment.FileDescriptor, there are no file objects available because we read Zim files usingFileDescriptor. To address this, we have created ademo.zimfile to save it in the database so that it will be displayed in theZimHostFragment. We handle this file within theKiwixServer. When the current Zim file isdemo.zim, we create anArchiveobject with theFileDescriptorto host the Zim file on theKiwixServer.kiwixServerfeature fromCustom Apps. Since we are now using fd (FileDescriptor) to read the zim file from the asset folder. Currently, 'KiwixServer' is unable to host zim files via fd. This feature is temporarily hidden for custom apps. We will re-enable it for custom apps once the issue is resolved. Unable to host books with FileDescriptor onServerlibkiwix#1015DisebleServerFeatureFromCustomApps.mp4