Alt-R -> Alt-F -> Alt-C and MANY other things...

[tigran123]

Also, simplify the code a bit by using conditional inside the string concatenation.

[tigran123]

Oh, wait, don't merge yet --- I'll make sure the voice version of the message is audible. I fear that it would spell the word "STANDARD" letter by letter i.e. "Es, Tee, Ey, En, Dee..." :)

Update: no, all is fine it reads both "STANDARD Reader" and "KOPTReader" correctly...

[tigran123]

Argh.... wait. Another one of those "hardware vs emulator" differences. I forgot that on the real hardware Alt-R is the number 3, so we'll have to choose another key.... I'll put this fix in this PR, don't merge it just yet.

[tigran123]

Ok, done, switched to Alt-F, with "F" standing for "reFlow" :)
Also, shortened the help description as the old one didn't fit on the screen.

[tigran123]

Also, there is a slight bug in my implementation: for a per-file setting of the reader choice to be honoured between open/close of different files of the same type one has to restart KPV, but I am working on the fix right now.

[tigran123]

The latest commit has the fix for the bug mentioned above, i.e. now the use_koptreader setting is truly local to each open instance of a file.

[tigran123]

While swimming I realized that with a few simple changes we can enhance this implementation to re-open the file automatically instead of just dropping the user back to filechooser. I'll work on this. Also, while at it, I might as well fix/restore the intended functionality of Home key in the reader to exit KPV instead of closing the document, i.e. to behave according to its description in the help page.

But this PR can be merged without waiting for the above changes as it is complete in itself, imho.

[dpavlin]

Can you move it to some other key than F please? I was planning to use alt+f for some column mode (probably multi-column mode or right-to-left) or something similar to have all shortcuts concerning columns on F.

Moving it to zoom menu (M) might be one possibility, since I don't think that two key-presses would are too much for this. Or shift+O (as other) might be another possibility.

[tigran123]

Ok, I'll move it to another key, no problem.

Also, apart from Alt-F change and the need to do re-open, there is another little bug here (switching to koptreader is sometimes TOO persistent, i.e. you can't get back to standard under certain sequence of events --- I'll fix it either today or tomorrow, don't worry :)

[tigran123]

The bug with "stubborn sticking to koptreader mode" is now fixed.

Now, I will deal with the following:

1. Switch to a new key
2. Add the code to re-open the file instead of dropping to filechooser
3. (in a separate PR) Fix the Home key to work as exit from reader rather than closing the document.

[tigran123]

Ok, I think there is enough for this PR. The code to re-open on Alt-C should go into the next PR as it is logically independent.

[tigran123]

No, no, no, wait --- don't merge this PR yet....

[tigran123]

I found yet another bug, seems UNRELATED to my changes, but to make 100% sure that it is unrelated I need to test it a bit more. Leave it until tomorrow when I have tested and reported one way or the other.

[tigran123]

Phewww.... that's a relief --- the crash has nothing to do with my changes. Namely, it is just a crash coming from k2pdfopt internals on certain pages of some complex PDF files. I can supply the files, of course, but they are not easily reproducible --- it was just bad luck that I happened to open that file first when testing this PR on the real Kindle :)
All other files work fine, both pdf and djvu.

[tigran123]

Actually, out of curiousity, I am going to debug this crash myself, let's see what I find. So perhaps leave this PR as is and wait if I can put the fix for this crash into this PR as well....

[tigran123]

Here is the stacktrace:

(gdb) bt
#0  0x0000003411744939 in __memcpy_ssse3_back () from /lib64/libc.so.6
#1  0x0000000000415c03 in blitToBuffer (L=<optimized out>) at blitbuffer.c:239
#2  0x000000000051abf3 in lj_BC_FUNCC ()
#3  0x0000000000509b20 in lua_pcall ()
#4  0x00000000004076be in main (argc=<optimized out>, argv=0x7ffffe0e88c8) at kpdfview.c:76

I'll now enable blitbuffer boundary checking and see if it catches it...

[tigran123]

Hmm, blitbuffer debug checking did not catch it, but I used the "old-fashioned" way, put a printf() at line 240 of blitbuffer.c like this:

      for(y = 0; y < h; y++) {
            ASSERT_BLITBUFFER_BOUNDARIES(dst, dstptr);
            ASSERT_BLITBUFFER_BOUNDARIES(src, srcptr);
            printf("dstptr=%p, srcptr=%p, w=%d\n", dstptr, srcptr, w);
            memcpy(dstptr, srcptr, w / 2);
            if(w & 1) {
                dstptr[w/2] &= 0x0F;
                dstptr[w/2] |= (srcptr[w/2] & 0xF0);
            }
            dstptr += dst->pitch;
            srcptr += src->pitch;
        }

and, lo and behold:

dstptr=0x2aeb600, srcptr=(nil), w=599
Segmentation fault (core dumped)

But srcptr is calculated a few lines above to be:

srcptr = (uint8_t*)(src->data + yoffs * src->pitch + xoffs / 2 );

So, I put another printf right after the above assignment:

printf("src->data=%p, yoffs=%d, src->pitch=%d, xoffs=%d\n", src->data, yoffs, src->pitch, xoffs);

and got this result:

src->data=(nil), yoffs=0, src->pitch=300, xoffs=0

So, something has passed src->data = NULL to blitToBuffer() function. To reproduce this you need the following steps:

1. Download the PDF file: https://github.com/downloads/tigran123/urantia-study-edition/urantia-study-edition-Garamond.pdf
2. Goto page 5
3. Switch to KOPTReader (Alt-C)
4. Open the file again and observe the crash.

NB. Reproducible in the master branch, i.e. it is totally unrelated to this PR.

[tigran123]

Actually, although this is unrelated to the present PR, I still think I am the one to blame, because if I restart KPV and re-open the same PDF file on the same page using koptreader then it does not crash. So, it is only when we are staying within KPV and re-opening it it crashes. This means that something is missing from the way I close it in the command handler like this:

self.doc:close()
self.doc = nil

Plus, of course, the usual cleanup done in the the UniReader:inputLoop() function (e.g. the cache is cleared). But something to do with blitbuffer needs to be cleaned/closed as well. Any ideas what that might be?

[tigran123]

And, btw, with DjVu files it will also coredump in the same function a few lines earlier at the code *dstptr |= *srcptr & 0x0F; for the same reason srcptr == NULL because src->data == NULL

[tigran123]

The crash is actually coming from this line in UniReader:show():

fb.bb:blitFrom(bb, self.dest_x, self.dest_y, offset_x, offset_y, width, height)

for some reason the bb passed to it has the data field equal to NULL...

[chrox]

Need we have a reflow flag in pagehash? So that when switched to koptreader the blitbuffer is refreshed.

[tigran123]

That bb indeed comes from the tile cache, so your suggestion would make sense IF the tile cache was not cleared on closing the current document, via inputLoop() calling self:clearCache(). But, yes, something is happening in this area, i.e. for some reason the tile cache (pagehash) seems to be re-used, or more correcly, MIS-used...
We need to figure out exactly how and why this happens, yes.

[tigran123]

Oh, you are right --- something impossible happens, i.e. in KOPTReader:drowOrCache() the code if self.cache[pagehash] ~= nil then is executed even though that entry should be nil right after calling clearCache(). Very strange, but we are getting closer...

Update: clearCache is called for DJVUReader but somehow that pagehash entry is re-used by KOPTReader. How can that be?

[tigran123]

Yes, adding an 'X' at the tail of pagehash expression in KOPTReader:drawOrCache() function makes the problem go away, but is this really the correct fix? I.e. I still don't understand why that entry is re-used between different reader objects, i.e. DJVUReader.cache and KOPTReader.cache should be completely different objects, right?

[tigran123]

Also, the problem is asymmetric, i.e. it only happens in the switch from standard to koptreader, but NOT when switching from koptreader to standard. If the solution was to have a different pagehash expression between different readers then the problem would also happen when switching from koptreader to standard, right?

Actually, putting some debug print()s in UniReader:drawOrCache() I see that the pagehash entry is also re-used when switching from koptreader to standard but the crash doesn't happen probably by just pure luck. So, yes, looks like we do need different pagehash expressions between standard and koptreader. But I still don't understand why. I thought self:cache really depends on self i.e. the two objects DJVUReader:cache and KOPTReader:cache live in completely different areas of memory and cannot possibly be re-used. Let's see if @houqp has any ideas --- I think he wrote the whole tile cache subsystem.

[tigran123]

This is getting very interesting. Here is the state of both DjVu and KOPT caches at the time of closing the DjVu document, just before clearCache() call in inputLoop():

breaking out of inputLoop()
DJVUReader.cache BEGIN
self.cache[2_0.17148981779207_0_1].bb=  userdata: 0x4155cae0
self.cache[1_0.17148981779207_0_1].bb=  userdata: 0x4155c6f8
self.cache= table: 0x418a2f98   self.cache_current_memsize= 423648.44587353
DJVUReader.cache END
KOPTReader.cache BEGIN
self.cache[2_0.17148981779207_0_1].bb=  userdata: 0x4155cae0
self.cache[1_0.17148981779207_0_1].bb=  userdata: 0x4155c6f8
self.cache= table: 0x418a2f98   self.cache_current_memsize= 0
KOPTReader.cache END
clearing cache...

As you see KOPT cache is duplicating DJVU cache. I am dumping them using the proper object, of course, i.e. like this:

    print("DJVUReader.cache BEGIN")                                                   
    DJVUReader.dumpCache(DJVUReader)                                                  
    print("DJVUReader.cache END")

    print("KOPTReader.cache BEGIN")                                                   
    KOPTReader.dumpCache(KOPTReader)                                                  
    print("KOPTReader.cache END")

and the function dumpCache() is defined to be:

function UniReader:dumpCache()
    for k, _ in pairs(self.cache) do
        print("self.cache["..k.."].bb=", self.cache[k].bb)
    end
    print("self.cache=", self.cache, "self.cache_current_memsize=", self.cache_current_memsize)
end

I am trying to find out why KOPTReader.cache is a duplicate of DJVUReader.cache

[tigran123]

It turns out that ALL readers share the cache and this is seen on normal close of the document, i.e. NOT using the new Alt-C way of getting out, look:

DJVUReader.cache BEGIN
self.cache[2_0.17148981779207_0_1].bb=  userdata: 0x40dbb528
self.cache[1_0.17148981779207_0_1].bb=  userdata: 0x40dbb140
self.cache= table: 0x40618bc0   self.cache_current_memsize= 423648.44587353
DJVUReader.cache END
KOPTReader.cache BEGIN
self.cache[2_0.17148981779207_0_1].bb=  userdata: 0x40dbb528
self.cache[1_0.17148981779207_0_1].bb=  userdata: 0x40dbb140
self.cache= table: 0x40618bc0   self.cache_current_memsize= 0
KOPTReader.cache END
PDFReader.cache BEGIN
self.cache[2_0.17148981779207_0_1].bb=  userdata: 0x40dbb528
self.cache[1_0.17148981779207_0_1].bb=  userdata: 0x40dbb140
self.cache= table: 0x40618bc0   self.cache_current_memsize= 0
PDFReader.cache END
CREReader.cache BEGIN
self.cache[2_0.17148981779207_0_1].bb=  userdata: 0x40dbb528
self.cache[1_0.17148981779207_0_1].bb=  userdata: 0x40dbb140
self.cache= table: 0x40618bc0   self.cache_current_memsize= 0
CREReader.cache END

So, when closing DjVu document, just before clearCache() all other reader's caches contain the same pagehash entries except their cache_current_memsize is equal to 0. I am very interested to find out why. I will check how blitbuffer.c allocates/frees the caches and find out when the cache is first allocated (I thought it was done on cache miss in drawOrCache() function but obviously this cannot be the case because when reading a DjVu document one doesn't do "cache misses" in a CREReader or PDFReader etc).

[tigran123]

Looks like a bug in LuaJIT, i.e. if your object has two fields (a table and a number) and you create two derived objects, then the table field they share, but the number field each instance gets his own copy. I will try to reproduce it with a small test.lua program.

Well, we have the solution already anyway --- to add something specific to KOPTReader (like a letter K at the end) to its pagehash formula. Then it will never re-use DjVu or PDF object's cache.

[tigran123]

Ok, it is not a bug, it is the way we manage blitbuffer metatable in blitbuffer.c. So, I'll put a patch with the fix making koptreader's pagehash different from others to prevent it from re-using their cache.

[chrox]

@tigran123 I'm now implementing the file association version of switching between Standard reader and Koptreader. So please don't merge this PR and wait for several hours and see which way is more appropriate.

[tigran123]

Well, the first part of my implementation is already in the master. But it can be reverted, of course, if your implementation is better. Anyway I'll put the crash fix into this PR now. And then we shall compare the two approaches "side by side", so to speak :)

[chrox]

@tigran123 Thanks. I'm going to do my work now.

[tigran123]

@chrox Ok, and I promise to review it in an unbiased way.

[tigran123]

Btw, this PR cannot be automatically merged in ANY case, so I'll extract some bits into a separate PR which can be merged regardless of our choice of the way to switch between standard/koptreader. E.g. removal of unneeded code from reader.lua.

[tigran123]

Ok, I am going away now until the evening --- please (@dpavlin and @houqp ) don't make the final decision until then, as I wish to take part in the review process too.

[tigran123]

Before I go I decided to put my implementation into a separate PR with a single compact commit so we can truly compare the code of two mergable PRs "side by side" (except the bit that is already in the master, but your PR would be undoing it so still we can do the comparison). The new PR is #479 so I am closing this one.

[houqp]

@tigran123 , very interesting bug hunting process :)

Now you know how to fix the bug :) Following code can be an alternative way to fix the bug:

PDFReader = UniReader:new{
    cache = {}
}

This make sure a specific reader uses it's own cache instead of the cache shared by all readers.

BTW: the cache system is designed and implemented by @hwhw , he designed and implemented all the basic layouts of KPV, including the new_ui_branch.

[hwhw]

Yep, I'm probably the one to blame. I guess emptying the cache is the way
to go. But strictly speaking, that should not be necessary. It ought to
work as it is, just rotating out entries for no longer opened documents.
And it shouldn't depend on open documents at all for existing entries.