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

Handle CRe's weird-ass pixel-format once and for all #1265

Merged
merged 8 commits into from
Dec 23, 2020

Conversation

NiLuJe
Copy link
Member

@NiLuJe NiLuJe commented Dec 22, 2020

We expect RGBA, CRe uses BGRX (well, alpha is actually sane, but inverted. Which, AFAIK, is usually a weird legacy PNG/Gif quirk or something, but, oh, well).

As discussed in koreader/koreader#7021

Depends on koreader/crengine#402


This change is Reviewable

Too many codepaths inside CRe
invert here

But we don't gain much, if anything. The performance hit isn't so much
in *what* we do in the extra buffer walk, it's in the act of walking
the buffer in itself :/
@NiLuJe
Copy link
Member Author

NiLuJe commented Dec 22, 2020

Now, while this is only actually an issue visible in native buffer grabs on RGB32, there are a couple of quirks to take into account:

  • Handling the RGB <-> BGR inversion inside CRe was faster, and turned out to be not entirely insane, and didn't have many side-effects (but it did have some, c.f., Get rid of RevRGB & co crengine#402)
  • Handling the alpha inversion inside CRe is completely unfeasible without basically rewriting every bit of color handling code to not be a special snowflake.
  • Handling this in the native screenshot code is viable (c.f., the BGR swap on Kobo), but then, Android rears its ugly head: that'd be a pixel-loop in ffi/bb, in plain/FFI Lua, and as such, a big no-no because Android and mcode allocs and the JIT and everything being the worst.

So, this leaves handling it right after CRe's done with a buffer for a specific page, in C(++).
Which means an extra buffer walk, which tanks the performance a bit. Oh, well.

(FWIW, this is what the CRe Android GUI does, too).

@NiLuJe
Copy link
Member Author

NiLuJe commented Dec 22, 2020

Oh, and as the actual performance hit is the buffer walk in itself, not so much what we do inside, I moved the full conversion in there, to get rid of the various potential quirks of the in-CRe BGR swap.

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.

Fine by me.

@poire-z
Copy link
Contributor

poire-z commented Dec 22, 2020

Looks indeed like a long walk... But I trust you it's for the best.
That's only for emulators and android, our B&W eInk devices won't suffer, right ?

I guess at one point, you'll try optimizing it by using parallel same operation on multiple byte, the neon stuff, if I got that right :)

@NiLuJe
Copy link
Member Author

NiLuJe commented Dec 22, 2020

Yeah, that's only the color codepath (so, yeah, essentially Android & SDL).

And that's indeed something that could potentially be vectorized, but again, the issue isn't so much the cost of the operation being done, but the simple fact that we're churning over the buffer again. But still, probably some potential for improvements if someone were to take an interest in it ;).

@NiLuJe NiLuJe merged commit 4a46ff1 into koreader:master Dec 23, 2020
NiLuJe added a commit to koreader/koreader that referenced this pull request Dec 24, 2020
roygbyte pushed a commit to roygbyte/koreader-base that referenced this pull request Mar 3, 2022
* Convert CRe's insane pixel format post-blit

Too many codepaths inside CRe

* Bump CRe

koreader/crengine#402
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants