Implementation new libpdfium so libs #16
Conversation
|
I hope to test this and make some other change to the project today (but my day is flying by very quickly already). |
|
Crashes on the very first test, never runs any of the others. Your branch loads way fewer native libraries. Is it statically linking everything? If so, we should at least discuss the ins and outs of that. Without your changes, it runs 60+ tests cleanly. The log is from x86 in GenyMotion emulator. Does the same thing on Samsung S10 (with an arm process). I gather you can run the tests, there must be something your environment? If I can't checkout your branch, and successfully run the tests, it won't get merged. If you have any ideas why it works for you and not for me, I'd be happy to hear them. Are you sure you don't already have some of libraries on the device your testing on? Can you test on a clean device (like a newly created emulator)? 02-05 19:16:02.753 25698 25698 F DEBUG : Cmdline: io.legere.pdfiumandroid.test |
|
Hello, regarding libraries, its a shared linking. Thanks for the logs, i will investigate it and will come back to you with results |
|
From my side, i have tested it with real devices and on Android emulators different versions and all is works |
|
Hello, @johngray1965 |
|
Hello, @johngray1965 . Im already fixed tests, checked it one by one, looks like good, pushed changes, please check |
|
Hello @johngray1965 |
| @@ -57,11 +57,6 @@ find_library( # Sets the name of the path variable. | |||
| target_link_libraries( # Specifies the target library. | |||
| pdfiumandroid | |||
|
|
|||
| libabsl | |||
There was a problem hiding this comment.
Why are you changing these dependencies? Its not necessarily a dead-breaker, but it needs to be clearly justified.
There was a problem hiding this comment.
Because, libpdfium.so, already contains all required dependencies
| @@ -126,6 +126,38 @@ FPDF_EXPORT FPDF_PAGE FPDF_CALLCONV FPDFPage_New(FPDF_DOCUMENT document, | |||
| FPDF_EXPORT void FPDF_CALLCONV FPDFPage_Delete(FPDF_DOCUMENT document, | |||
| int page_index); | |||
|
|
|||
| // Experimental API. | |||
There was a problem hiding this comment.
Are these just due to the new version of pdfium?
| @@ -284,8 +284,8 @@ Java_io_legere_pdfiumandroid_PdfiumCore_nativeOpenMemDocument(JNIEnv *env, jobje | |||
| } | |||
|
|
|||
| int size = (int) env->GetArrayLength(data); | |||
| auto *cDataCopy = new jbyte[size]; | |||
| env->GetByteArrayRegion(data, 0, size, cDataCopy); | |||
| auto *cDataCopy = malloc(size); | |||
There was a problem hiding this comment.
why is this being changed to malloc?
|
I have a number of concerns. There are lot of changes to the pdfium headers and whatnot. Are they just do a new pdfium? I can update the pdfium version (In fact I should). The tests still failed (i haven't looked into why). The PR is rather large. I'm concerned about accepting binaries changes (and I don't much want to go back to building it myself). It's getting harder and harder to justify targeting less than API 23. |
|
I'm updating the libraries to the latest from: https://github.com/bblanchon/pdfium-binaries |
@johngray1965 It will not work with Android 23-24-25,. Im also working with pdfium-binaries, but i added changes to the configurations and rebuild it for fixing Android 23-24-25 |
|
|
As i see in your description, minimal Android API is 21 |
|
So, what we will do? Because your PDF library not working with Android 21-22-23-24 and you declared that it should work from 21. Im trying to help you with that and i dont understand your todays comment, tests are fixed, libraries worked from > Android 23. I can leave your so files, which i removed in current MR, but it no sense, because its already included in new pdfium so |
|
It was working with 21, it's probably do to pdfium updates. But sorry, I'm not willing to accept the binaries or build them myself. So the only real options for you to release your own version or get Pdfium to accept the changes so they go into the offical (or semi-official builds) But when I update the libraries, I'll work out some of the issues, then you have a smaller delta. |
|
So, go step by step:
|
|
I pushed an update to use the latest version of pdfium. That moved to just one library like yours. So it now has a lot of your changes. I still don't know I can run the tests for yours. After I updated the libraries and headers, I just needed to adjust the CMakeLists.txt and it worked. No changes to the code. |
|
Update libpdfium so libs for fixing pdf view on Android 23 API