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

C blitbuffer implementation #571

Merged
merged 6 commits into from
Nov 5, 2017
Merged

C blitbuffer implementation #571

merged 6 commits into from
Nov 5, 2017

Conversation

chrox
Copy link
Member

@chrox chrox commented Nov 5, 2017

The C implementation is only built for the Android platform and the Emulator. With this patch mcode allocation failure on Android don't degenerate the performance. And we can even disable jit on Android that also means we can port Koreader to iOS on which jit is disabled by the system.

This implementation does not change the blitbuffer API and is transparent to the frontend.

@@ -766,7 +766,7 @@ local function get_pthread()
else
-- Kobo devices strangely have no libpthread.so in LD_LIBRARY_PATH
-- so we hardcode the libpthread.so.0 here just for Kobo.
for _, libname in ipairs({"libpthread.so", "libpthread.so.0"}) do
for _, libname in ipairs({"pthread", "libpthread.so.0"}) do
Copy link
Member

Choose a reason for hiding this comment

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

I saw this before and wondered about changing it, but I figured there might've been a reason for it (though I can't think of any). :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, benefit is that it can load libpthread.dylib on Mac now. Btw, this PR is developed solely on my Mac. Koreader is stable enough on Mac OSX now for development purpose. :)

Copy link
Member

Choose a reason for hiding this comment

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

Cool. :)

Btw, I wanted to look into that Travis Mac build business, but at least on Travis that's not an option due to insufficient resources. See #564 (comment) (I imagine everything would fail spectacularly as written there, but that aside. I just wanted to play around with it a bit.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if the brew install stuff is sufficient. Probably there are hidden hacks on my Mac to make it work. Anyway I will verify that on a clean Mac when I have time.

self.setPixelBlend)
local color = Color8A(255, 255*(by or 0.5))
if use_cblitbuffer then
c = color:getColorRGB32()
Copy link
Member

Choose a reason for hiding this comment

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

I think this line should probably be removed?

@Frenzie Frenzie merged commit 62a73d8 into master Nov 5, 2017
Frenzie added a commit to Frenzie/koreader that referenced this pull request Nov 6, 2017
Includes C blitbuffer implementation koreader/koreader-base#571 by @chrox

Should help performance degeneration on Android due to mcode allocation failure, see koreader#1416
Frenzie added a commit to koreader/koreader that referenced this pull request Nov 6, 2017
Includes C blitbuffer implementation koreader/koreader-base#571 by @chrox

Should help performance degeneration on Android due to mcode allocation failure, see #1416
@poire-z
Copy link
Contributor

poire-z commented Nov 17, 2017

Just curious: why did you decide to not enable it for other platforms? Isn't pure C faster that JIT compiled lua?

@Frenzie
Copy link
Member

Frenzie commented Nov 17, 2017

It's FFI data structures so they're faster (or should be) than Lua C API I think. Easier to maintain in any case. But the problem here is memory allocation on Android, not blitting performance.

@chrox
Copy link
Member Author

chrox commented Nov 18, 2017

Actually I didn't make a real performance benchmark on the two blitbuffer implementations. The lua implementation is elegant and easy to extend. This one is intended to be used on platforms on which jit does not work.

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

3 participants