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

MuPDF 1.12 #577

Merged
merged 29 commits into from
Dec 20, 2017
Merged

MuPDF 1.12 #577

merged 29 commits into from
Dec 20, 2017

Conversation

TnS-hun
Copy link
Contributor

@TnS-hun TnS-hun commented Dec 15, 2017

No description provided.

TnS-hun and others added 25 commits December 15, 2017 18:07
Requires changes to PdfDocument:saveHighlight too (that is outside of
base).
@TnS-hun
Copy link
Contributor Author

TnS-hun commented Dec 15, 2017

Closes #562.


# TODO: ignore shared git submodules built outside of mupdf by ourselves
# https://git.ghostscript.com/mupdf.git is slow, so we use the official mirror on GitHub
# v1.12 isn't tagged, so we refer to it with a hash
Copy link
Member

Choose a reason for hiding this comment

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

The tag 1.12.0 seems to have been added two days ago, which I think is a lot more legible than that hash below. ;-)

@Frenzie
Copy link
Member

Frenzie commented Dec 15, 2017

The shellchecks speak for themselves:

ffi/mupdf.lua:398:11: unused variable 'char_bbox'

ffi/mupdf.lua:416:1: line contains only whitespace

Then there's Android.

Setting up a basic Android build system is fairly easy (./kodev build android should take care of it all; see the README for the rest) but testing Android isn't necessarily self-evident. Hopefully I can find some time for that tomorrow but I'm not guaranteeing it.

Thanks for all the work!

@houqp
Copy link
Member

houqp commented Dec 16, 2017

Great work! Thanks for keeping this giant dependency uptodate :)

@@ -69,7 +69,7 @@ ep_get_source_dir(SOURCE_DIR)
ko_write_gitclone_script(
GIT_CLONE_SCRIPT_FILENAME
https://github.com/ArtifexSoftware/mupdf.git
b7749e563f93160de82c97fe34fb3fb0d3396304
tags/1.12.0
Copy link
Member

Choose a reason for hiding this comment

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

You forgot to remove the comment up above. ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:)

@Frenzie
Copy link
Member

Frenzie commented Dec 16, 2017

Alright, I have a few minutes now. I ascertained that it seems to be working on PC and building for Android. Should I also test it on Android or were you already able to?

@TnS-hun
Copy link
Contributor Author

TnS-hun commented Dec 16, 2017

I just built it on Android.

@TnS-hun
Copy link
Contributor Author

TnS-hun commented Dec 16, 2017

I tried to build the apk with ./kodev release android but it failed:

[armeabi-v7a] Install        : libluajit.so => libs/armeabi-v7a/libluajit.so
[armeabi-v7a] Install        : liblzma.so => libs/armeabi-v7a/liblzma.so
make[3]: Leaving directory `/home/tns/source/koreader/platform/android/luajit-launcher'
ant debug
make[2]: execvp: ant: Permission denied
make[2]: *** [apk] Error 127
make[2]: Leaving directory `/home/tns/source/koreader/platform/android/luajit-launcher'
make[1]: *** [androidupdate] Error 2
make[1]: Leaving directory `/home/tns/source/koreader'
Makefile:340: recipe for target 'update' failed
make: *** [update] Error 2

@Frenzie
Copy link
Member

Frenzie commented Dec 16, 2017

It seems to be working. :-)

I'll hold off on merging another day or two. I haven't been able to read it properly yet myself.

@Frenzie
Copy link
Member

Frenzie commented Dec 16, 2017

make[2]: execvp: ant: Permission denied

Huh, weird. I have version 1.9.9.

PS "I haven't been able to read it properly yet myself" and I'm sure others might want to take a look first too, pinging @houqp (who already took a peek) @chrox @NiLuJe .

@houqp
Copy link
Member

houqp commented Dec 18, 2017

@TnS-hun are you able to run ant manually?

@TnS-hun
Copy link
Contributor Author

TnS-hun commented Dec 18, 2017

@houqp: that was the problem, ant wasn't installed. I should've read the readme more carefully.

I tried to run the resulting apk with Genymotion Android Emulator but it didn't start. I tried the last two KOReader releases too but got the same result.

@Frenzie
Copy link
Member

Frenzie commented Dec 18, 2017

@TnS-hun On PC I'd use x86 builds. Anyway like I said wfm after your patch. :-)

Copy link
Member

@Frenzie Frenzie left a comment

Choose a reason for hiding this comment

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

Did I miss simple-out.pdf being used in the unit tests somewhere or is that an accidental include?

cdecl_type(fz_text_sheet)
cdecl_struct(fz_text_sheet_s)
cdecl_func(mupdf_new_text_sheet)
cdecl_func(fz_drop_text_sheet) // NOTE: libk2pdfopt uses old fz_free_text_sheet symbol
Copy link
Member

Choose a reason for hiding this comment

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

What about these comments? Do they no longer apply?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The fz_drop_text_sheet function no longer exists.

I don't think those comments are relevant anymore because libk2pdfopt is built without using MuPDF (HAVE_MUPDF_LIB).

cdecl_type(fz_page_block)
cdecl_struct(fz_text_page_s)
cdecl_func(mupdf_new_text_page)
cdecl_func(fz_drop_text_page) // NOTE: libk2pdfopt uses old fz_free_text_page symbol
Copy link
Member

Choose a reason for hiding this comment

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

And this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That cdecl and the comment are still there but fz_drop_text_page has been renamed to fz_drop_stext_page.

return c == 0x2022 or -- Bullet
c == 0x2023 or -- Triangular bullet
c == 0x25a0 or -- Black square
Copy link
Member

Choose a reason for hiding this comment

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

Is there supposed to be an order to these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know if there is any order here — maybe number of occurances? —, so the three I added has been simply ordered by value.

elseif ncomp == 4 then bbtype = BlitBuffer.TYPE_BBRGB32
else error("unsupported number of color components")
end
local p = M.fz_pixmap_samples(context(), pixmap)
local bb = BlitBuffer.new(p_width, p_height, bbtype, p):copy()
M.fz_drop_pixmap(context(), pixmap)
M.fz_drop_image(context(), image)
Copy link
Member

Choose a reason for hiding this comment

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

This was already done earlier or is it no longer necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's done earlier, as suggested by poire-z, so the error handling is cleaner.

typedef void fz_store_drop_fn(fz_context *, fz_storable *);
struct fz_storable_s {
int refs;
fz_store_drop_fn *drop;
};
struct fz_key_storable_s {
fz_storable storable;
Copy link
Member

Choose a reason for hiding this comment

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

Four-space indentation, right? :-)

(I know, old file.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's generated code by ffi-cdecl. :)

@TnS-hun
Copy link
Contributor Author

TnS-hun commented Dec 19, 2017

Did I miss simple-out.pdf being used in the unit tests somewhere or is that an accidental include?

Simple-out.pdf is used by the unit test to compare the tested and the expected output. I had to update it because the output by MuPDF v1.12 is a bit different.

@Frenzie Frenzie merged commit bd97e1b into koreader:master Dec 20, 2017
@Frenzie
Copy link
Member

Frenzie commented Dec 20, 2017

Alright, thanks! Please include the base update in koreader/koreader#3547

@Frenzie Frenzie mentioned this pull request Dec 20, 2017
Frenzie pushed a commit to koreader/koreader that referenced this pull request Dec 20, 2017
* addMarkupAnnotation expects a float array

* Use the latest from koreader-base with koreader/koreader-base#577

Closes #3519.
@TnS-hun TnS-hun deleted the patch-mupdf-update branch December 20, 2017 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants