Rewrite font scale patches for 4.28.18220+

[pgaskin]

See #92 and #94.

[shermp]

Ok, taken a stab at this. See shermp/font-scale for my first attempt.

I've adjusted the epub scale factor as high as I can make it to decrease the epub font size. On my Glo, the sizes appear to be very close between epub/kepub.

Any further tweaking to make them closer would need to be made on the kepub scale factor.

Thankfully the offsets are the same between 18220 and 18730.

Can't seem to use ReplaceFloat directive, because vmov.f32 is... special. At least the literal variant, which is what the scale factors are using.

[pgaskin]

The question is whether this works consistently between different device classes, especially the elipsa.

I don't have libnickel open right now, but this is the screenshot from 18220:

Notice how it's dependent on the device DPI (in 18220, it didn't seem to be consistently set).

[shermp]

Hmm, so KepubBookReader::pageStyleCss() does not appear to do that DPI check, I assume because it's already handled by Qt. If that's the case, then in my mind, the font size differences between epub/kepub should be the same despite the device. (Of course, this is Kobo we're talking about so...

[shermp]

Fiddlesticks :(

You're right (of course). DPI has a role to play. What worked nicely on my Glo, fails miserably on my Forma

[shermp]

My patch does help somewhat on the Forma, the epub font size is significantly smaller than without it. But it still doesn't come close to the kepub font size, which is supposed to be the ultimate goal.

[shermp]

I decided to see what happens if the font size is not multiplied by the DPI.

Conclusion: The font size needs to be multiplied by the DPI (or some figure), else the font size is so small as to be effectively invisible.

[shermp]

We may have to split this into separate "decrease epub font sizes" and "increase kepub font sizes" that users can mix and match per device, which I guess is basically what we had before?

[NiLuJe]

What about tackling this the other way around and allowing to modify the DPI itself?

[pgaskin]

To do that, we'd have to figure out where that actually comes from (I suspect it's the Kobo QPA) (it's doable, but it might require patching more than just libnickel, unless I add some functionality to kobopatch to allow redirecting external symbols to newly-added empty space). Also, it still wouldn't fix the old issue with the size mismatch in general (although, if we did re-implement the DPI stuff to be completely custom, it might be fixable from there).

---

/*!
  \property QScreen::physicalDotsPerInchX
  \brief the number of physical dots or pixels per inch in the horizontal direction
  This value represents the actual horizontal pixel density on the screen's display.
  Depending on what information the underlying system provides the value might not be
  entirely accurate.
  \sa physicalDotsPerInchY()
*/
qreal QScreen::physicalDotsPerInchX() const
{
    return size().width() / physicalSize().width() * qreal(25.4);
}

(note: 25.4 is the conversion factor for mm/in)

---

/*!
  \property QScreen::physicalSize
  \brief the screen's physical size (in millimeters)
  The physical size represents the actual physical dimensions of the
  screen's display.
  Depending on what information the underlying system provides the value
  might not be entirely accurate.
*/
QSizeF QScreen::physicalSize() const
{
    Q_D(const QScreen);
    return d->platformScreen->physicalSize();
}

---

On a second thought, this might be trivial to fix with NickelHook.

---

Have a look at libkobo / KoboScreen*::{setupGeometry,logicalDpi,nativeOrientation}.

(note: you'll need to resolve the branches against the vtables since it's under multiple layers of inheritance)

(note: if hooking this with NickelHook, you'll either need to hook the QScreen function directly, or you'll need to hook libkobo and also patch the vtables)

Also, random thought: for my emulation project, it would probably be easier to intercept libkobo than to intercept libc functions, syscalls, kernel interface (this one's what I was working on already), or emulate the entire EPDC.

[shermp]

I feel adjusting the DPI is probably too big of a hammer to swing. Lots of potential for unintended consequences there. All the widget styling will be based around whatever the DPI each device uses.

[shermp]

Alright, I've tried a second approach, that seems to work on both my Glo and my Forma.

The idea is to negate the epub scale factor of 25.0 by setting it to 1.0. Then, replace the call to QScreen::logicalDotsPerInchX() with vmov.f64 d0,#6.5 to directly set the register value. This effectively is the multiplier.

This seems to be about as close as I can get using this method. Note that granularity seems to be fixed at 0.25 intervals for these values, up to a max of 31.0. I've tried both 6.5 and 6.75 for the multiplier, I think 6.5 is slightly closer.

@pgaskin if there are ways to improve the patch implementation, please feel free to let me know. I'm still very much a novice at this stuff.

Here's the current patch for the curious:

Unify font sizes:
  - Enabled: no
  - Description: |
      Attempt to unify the font sizes between epub and kepub. Without this patch 
      epub font sizes can be much larger than kepub at the same size setting.
  
  - BaseAddress: "AdobeStyling::update(QString const&)"
  # Negate the scale factor by setting it to 1.0
  - ReplaceBytes: 
      Offset: 0x1694
      FindH: F3 EE 09 0A # 25.0
      ReplaceH: F7 EE 00 0A # 1.0 vmov.f32 s1,#1.0
  # Replace QScreen::logicalDotsPerInchX() with our own Multiplier
  - ReplaceBytes:
      Offset: 0x16C2
      FindH: 8f f7 7e ed # QScreen::logicalDotsPerInchX()
      ReplaceH: B1 EE 0A 0B # vmov.f64 d0,#6.5

[shermp]

@NiLuJe I would be interested to know how this behaves on an Aura H2O/Elipsa/Sage

[pgaskin]

I feel adjusting the DPI is probably too big of a hammer to swing.

That's why I'm suggesting replacing the function with a new one entirely, so we can check who's calling us in the stack.

The idea is to negate the epub scale factor of 25.0 by setting it to 1.0. Then, replace the call to QScreen::logicalDotsPerInchX() with vmov.f64 d0,#6.5 to directly set the register value. This effectively is the multiplier.

That's interesting. I've played around with that sort of thing before, but only the second part. I don't remember the results though.

This will need to be tested on each of the three new devices (cadmus/sage/io), and on a device from each each top-level codename (dragon/trilogy/daylight/phoenix).

This seems to be about as close as I can get using this method.

Since you're replacing the DPI call, there should be at least another two 4-byte instructions you can replace. This means you can load a value, then load another multiplier and multiply by that too. Alternatively, if you figure out the calculations, a script could be written to generate different combinations for a target scale.

if there are ways to improve the patch implementation, please feel free to let me know

Since AdobeStyling is such a large function (and it's changed quite often in the past), I recommend using a different base offset. I suggest finding the offset of a known function (probably the DPI one), then using that as a Rel argument to the symbol one at the top to ease updating. Also, note that we generally use base-10 integers for smaller relative offsets.

For the instructions themselves, there's nothing we can do about them right now other than write a generator for them and add some good values for each device in the comments to make things easier for people. If we keep this approach, I may add support for generating VMOV instructions in the next version of KP.

[NiLuJe]

I'll have some time to test on Thursday ;). Anything in particular to look out for? And/or test files? (I haven't actually read in Nickel in years, and I've never actually read ePub there, so I'm only mildly familiar with the issue).

…

On Mon, Oct 25, 2021, 03:50 Sherman Perry ***@***.***> wrote: @NiLuJe <https://github.com/NiLuJe> I would be interested to know how this behaves on an Aura H2O/Elipsa/Sage — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#96 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAA3KZSQI6RIATE55ZRTRVLUISZVBANCNFSM5GPTQQCQ> .

[pgaskin]

Anything in particular to look out for? And/or test files?

• Don't use books with font sizes defined in CSS; there are too many quirks between renderers to compare them properly -- just make a simple EPUB file from scratch with a few paragraphs of text (and remember to put a separate cover html page in the spine since the first one is rendered as FXL in kepub).
• Check the minimum font size, the maximum font size, and a few in between.
• Ensure the font size increase between steps is at least somewhat consistent.
• The smallest EPUB font size (without other font patches) shouldn't seem abnormally large; it should be a bit on the small side for reading.
• I suggest adding a chapter with a bunch of numbered lines (~100 lines) so you can easily compare sizes by seeing which one appears last on the page.

[shermp]

That's interesting. I've played around with that sort of thing before, but only the second part. I don't remember the results though.

This will need to be tested on each of the three new devices (cadmus/sage/io), and on a device from each each top-level codename (dragon/trilogy/daylight/phoenix).

Yeah, you may or may not need the first change, but it does give you more wiggle room to work with. vmov is not very flexible unfortunately, but it seems to be the only means of easily getting a float literal to a register.

Since you're replacing the DPI call, there should be at least another two 4-byte instructions you can replace. This means you can load a value, then load another multiplier and multiply by that too. Alternatively, if you figure out the calculations, a script could be written to generate different combinations for a target scale.

This is probably possible, I'm a bit worried about clobbering a register that's used elsewhere though. Looks like you have to use a generic mov into a standard register, then mov to a float register. Any tips?

Since AdobeStyling is such a large function (and it's changed quite often in the past), I recommend using a different base offset. I suggest finding the offset of a known function (probably the DPI one), then using that as a Rel argument to the symbol one at the top to ease updating. Also, note that we generally use base-10 integers for smaller relative offsets.

I'm afraid that your documentation on the patch format is... lacking. I wanted to do something like this, but couldn't figure out how to do it.

[shermp]

As far as tweaking the patch goes, the code is essentially doing the following:

( font_size / scale_factor ) * DPI

We can tweak both scale_factor and DPI to dial in the final values.

[shermp]

Alright, I managed to test this patch on my mum's Libra. Again it seems to work well.

I've now tested it on Phoenix, Dragon & Daylight class devices, and they all seem to behave the same. For the most part, unless there's any device specific quirks, this looks like this may be a "one-size-fits-all" solution. Only device class I can't test on is Trilogy.

[pgaskin]

I'm afraid that your documentation on the patch format is... lacking. I wanted to do something like this, but couldn't figure out how to do it.

This (and the source itself) is probably the best documentation there is right now other than the other patches: https://pkg.go.dev/github.com/pgaskin/kobopatch@v0.15.1/patchfile/kobopatch

Any tips?

Most disassemblers can tell you what's used and trace which instructions will be affected. Other than that, just read up on callee-save registers for the ARM ABI.

As for the replacement instructions themselves, there's not too many options when you need to use immediate values.

Only device class I can't test on is Trilogy

I have a trilogy; I can do some testing when I have time (probably not this week).

[shermp]

Most disassemblers can tell you what's used and trace which instructions will be affected. Other than that, just read up on callee-save registers for the ARM ABI.

As for the replacement instructions themselves, there's not too many options when you need to use immediate values.

In this case, since we can set both the scale_factor and multiplier, We can probably get close enough with immediate values. For example, we could set scale_factor to 2.0 and the multiplier to 12.5 for a similar but different result.

From what I've gleaned, unless I'm missing something, Kobo no longer actually needs to do any DPI calculations in this function, as it must be being done elsewhere. They just need to set a suitable constant multiplier.

[shermp]

Ok, patch has been updated to find QScreen::logicalDotsPerInchX() using FindInstBLX, and added to 4.30.18838

[pgaskin]

Minor nitpick: we generally use MR usernames rather than GH ones for and within the patch files themselves to avoid confusion. You'll need to rename shermp.yaml to sherman.yaml and replace shermp with sherman in the header comment. Also, shouldn't "Multiplier" be lowercase?

Can you open a PR for your branch? I'll review it and merge it for the release if I have time tonight and it works on my devices.

Can you also add a comment (not text in the description) under the description explaining why the same value can work on all devices along with a link to this issue?

[shermp]

Yeah, I can make those changes.

[shermp]

Yeah, I can make those changes.

And done.

[pgaskin]

I've merged the unify font scales patch so people can try it in v73. Thanks @shermp!

P.S. I made a typo in the commit message quotes... oh, well...

[pgaskin]

@shermp: I've done some cleanup in a2ffddf.

[shermp]

@shermp: I've done some cleanup in a2ffddf.

Oh, thanks. I couldn't quite figure out how the relative offsets was supposed to work, so thanks for doing that.

Although I tried, I never could figure out the magic demangled symbol that would make kobopatch happy, so yet another thanks! I had forgotten about the const qualifier.

[shermp]

Just wondering, to avoid potential confusion, should the patch be renamed to something like Unify kepub and epub font sizes?

[pgaskin]

@shermp's new Unify font sizes patch has been released in v73. I'm going to leave this issue open until we get some feedback and can confirm this can replace the old font scale patches.

Just wondering, to avoid potential confusion, should the patch be renamed to something like Unify kepub and epub font sizes?

Too late for that now.

[pgaskin]

Since the updated Custom font sizes and new Unify font sizes patches seem to cover everything and have received positive feedback on MR over the last month, I'm going to close this issue.

[JSWolf]

The original patch ePub uniform font scale: worked by making the KePub font size match that of the ePub font size. For those that need/want larger font sizes, this worked best. This patch works by reducing the ePub font size. Can this the swapped around so it works like the original patch to make the KePub font size larger to match the ePub font size?