Home / SumatraPDF 11061 prerelease cbr page order bug

ianas

trying to open a 25 page cbr file with prerelease 11061 (I build it from the git source) sumatra opens to page 3 (filename 002.jpeg) instead of page 1 (filename 000.jpg) pages 1 and 2 (filename 000.jpeg and filename 001.jpg) are rendered last
on another cbr with 54 pages sumatra opens to page 7 (filename 006.jpg) and then renders pages 1-6 in order after the final page (which usually is a tag image zztag.jpg)
on yet another 34 page cbr Sumatra skips to page 9 then renders in order to the end then shows pages 1-8
this happens only with rar files if I convert them to zip (cbz) sumatra opens the pages in the correct order
this bug does not exist with 11040 available from https://www.sumatrapdfreader.org/prerelease and I believe is a recent regression
I thought it might have something to do with the jpg names but this happens with all cbr’s I currently have and the page number Sumatra opens to seams to be related with how many pages the document has it’s strange

edit
a copy of the sumatra binaries I build can be found here
https://mega.nz/#F!WwIiFDyZ!eA2g4lws9YNOuEI0a9Dkpw
I don’t recommend using SumatraPDF_3.2.11061_xxx.exe because of the aforementioned cbr issue

ianas

I know the current prerelease available on
https://www.sumatrapdfreader.org/prerelease
is 11040 where this bug is not present but can I at least get a confirmation that 11061 has issues with page order in cbr files
any ideas on how to fix this
I’m guessing the internal unrar support from unarr has issues with ImageDirEngineImpl::LoadImageDir maybe undo pull request 1060?

GitHubRulesOK

Hi Ianis there was a double sort removed recently in a commit supposedly to resolve another comic book sort issue so I’m guessing removal broke the other ? Should be easy to revert those few lines, and if you can resolve why they cause an either or issue perhaps kjk may be able to solve “all sorts”

ianas

I did open a bug report but had zero replies

I’ll try to undo the pull request 1060 if it’s not too complex but as this is only present in cbr files I’m guessing it’s an unarr issue
btw I thought it was the Sumatra devs that created unarr but it’s spread across the Linux world any ideas who’s developing it now

GitHubRulesOK

@ianas
If I remember the sequence ( and without dredging through old wounds) SumatraPDF was at one time using an unrar that was considered contentious by the free software foundation, so they had to unbundle and include an either or approach to clear the stigma at that time, licensing possibly moved on ? however there may be legacy code involved.
Actually although it may be a red herring you may have identified why there were two sorts and different users with and without external unrar may need different combinations’ (I hope I’m wrong otherwise you would need a switch for different users)
probably related to Error loading cbr comic files

ianas

I remember it’s just strange that this only happens with cbr files not cbz and the image order sequence is a bit strange it starts with Image_004.jpg renders it in order to the end then it renders image_000.jpg etc.
this makes reading cbr files difficult I can always convert them to cbz but it is bug

GitHubRulesOK

@ianas
I’m having difficulty getting the same behaviour I use your SumatraPDF_3.2.11061_portable_x64 on a 64bit win10
I check test cbr files start RAR not PK(zip)
I open cbr files they start at page 1 so I add the WinRAR dll that you link to (03/Oct version 5.61.100.2835).
I delete Sumatra settings file (so it does NOT remember I was on page 1)
I open any cbr (both the one from before and others ) They all open on page 1
I try open from menu and drag N drop

I see from rar release notes but do not know if related
– 3 October 2018
UnpVer field of RARHeaderData and RARHeaderDataEx structures erroneously
returned 200 instead of 50 for RAR 5.0 archives. Fixed now.

ianas

I just tested the portable build against a cbr files and well I’ll post 2 screenshots one from the cbr the other from the cbz

[Album] Imgur

as you can see from the screenshots in cbr page 1 is rendered as page 17 if I convert it to cbz then Sumatra opens it fine

GitHubRulesOK

Downloaded Hawkman 005 (2018) (38,346,548 bytes)
Agreed file opens as you illustrate page 1 is page 17/23

None of my previous .cbr files does that so have to suspect the source as different
opened in WinRAR 5.4 check how natural order is shown
seen as out of sequence
start = -007 -008 ….-021 zwater ……-005 -006 = end

`Curiouser and curiouser!’ cried Alice
That’s why start=page -007 and on conversion gets sorted

My files must all be in correct sort order

SumatraPeter

Finally had the time to look into this using SumatraPDF_3.2.11061_portable_x64.exe, and it seems this pre-release version is completely failing to sort the filenames inside archives before displaying.

I tested with the aforementioned Hawkman CBR, as well as my own CBRs and CBZs that were deliberately created with pages both in and out of sorted order. As expected, 11061 failed to sort pages of both CBRs and CBZs properly, but of course archives created with pages already in sorted order were displayed properly.

Commit 1060 states that “Files were already sorted in ImageDirEngineImpl::LoadImageDir.” Indeed, LoadImageDir() contains this line:

github.com

sumatrapdfreader/sumatrapdf/blob/master/src/ImagesEngine.cpp#L672


do {

    if (!(fdata.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)) {

        if (ImageEngine::IsSupportedFile(fdata.cFileName))

            pageFileNames.Append(path::Join(dirName, fdata.cFileName));

    }

} while (FindNextFile(hfind, &fdata));

FindClose(hfind);



if (pageFileNames.size() == 0)

    return false;

pageFileNames.SortNatural();



mediaboxes.AppendBlanks(pageFileNames.size());



ImagePage* page = GetPage(1);

if (page) {

    fileDPI = page->bmp->GetHorizontalResolution();

    DropPage(page);

}



return true;

Now the question is, if the re-sorting using cmpArchFileInfoByName() was deleted in 1060, what is pageFileNames.SortNatural(); doing? Why is it failing to sort out of order filenames in all archives? Is the call being skipped somehow, or is that function being called but failing to sort in natural order for some reason? @kjk: Please investigate because it certainly looks like #659 needs to be reopened:

GitHubRulesOK

@ianas
Were you able to revert the 7 + 1 lines that were removed (see below) and check it the behaviour reverts to as before (I think it should), there seems to be a conflict between “Natural order” ~ sequence as built (e.g. like date order) and SortNatural ~ which should be “Natural sort” but is not.

At present my understanding is
The previous override of “Natural order” to “Sort by name” (Named sort, which was working) has been removed
SortNatural seems not to make any difference ?
Looks like CBZ (& CBR possibly others ?) “Natural sort” was never implemented

So to revert code in src/ImagesEngine.cpp we would need to return the missing lines
[edit code removed to avoid inaccurate use]

@Ianis rename the current master version (1107# ?) and replace with this copy (apart from the 11061 change there are no others recorded) [LATER EDIT LINK REMOVED since issue is resolved]

SumatraPeter

Honestly, I’d like the code fixed so that SortNatural() works properly and #659 is properly closed.

Now SortNatural() is defined here:

github.com

sumatrapdfreader/sumatrapdf/blob/master/src/utils/Vec.h#L445


              Append(str::DupN(s, next - s));

          s = next + str::Len(separator);

      }

      if (!collapse || *s)

          Append(str::Dup(s));



      return len - start;

  }



  void Sort() { Vec::Sort(cmpAscii); }

  void SortNatural() { Vec::Sort(cmpNatural); }



private:

  static int cmpNatural(const void* a, const void* b) {

      return str::CmpNatural(*(const WCHAR**)a, *(const WCHAR**)b);

  }



  static int cmpAscii(const void* a, const void* b) { return wcscmp(*(const WCHAR**)a, *(const WCHAR**)b); }

};

#endif

That in turn calls CmpNatural() which I believe is defined here:

github.com

sumatrapdfreader/sumatrapdf/blob/master/src/utils/StrUtil_win.cpp#L457


 WCHAR *roman = AllocArray<WCHAR>(len + 1), *c = roman;

 for (int n = number, i = 0; i < dimof(romandata); i++) {

     for (; n >= romandata[i].value; n -= romandata[i].value) {

         c += str::BufSet(c, romandata[i].numeral[1] ? 3 : 2, romandata[i].numeral);

     }

 }



 return roman;

}



/* compares two strings "naturally" by sorting numbers within a string

numerically instead of by pure ASCII order; we imitate Windows Explorer

by sorting special characters before alphanumeric characters

(e.g. ".hg" < "2.pdf" < "100.pdf" < "zzz")

TODO: use StrCmpLogicalW instead once we no longer support Windows 2000 */

int CmpNatural(const WCHAR* a, const WCHAR* b) {

 CrashAlwaysIf(!a || !b);

 const WCHAR *aStart = a, *bStart = b;

 int diff = 0;



 for (; 0 == diff; a++, b++) {

Now someone needs to figure out why CmpNatural() is not doing its job. Alternately, it might be preferable to use proven third party code as I mentioned in my comment on #659.

ianas

I tested a few more files including cbt and cb7 and this issue only occurs with cbr and I tested half a dozen cbr files so I’m guessing it’s not the image sorting algorithm but something with the build-in unrar library imaybe the list of files to unpack is out of order?

SumatraPeter

No, it seems like you’re simply not testing properly. As I already mentioned above, this is a failure to sort the filenames irrespective of what the archive format is. I have tested 7z, RAR4, RAR5, TAR and ZIP with my own custom archives, that I created specifically with images in the right and wrong order.

Here are 10 archives (2 each for 7z/CB7, RAR4/CBR, RAR5/CBR, TAR/CBT and ZIP/CBZ) for testing. The “Good” ones have all the images inside in proper order (pages 1 to 10), whereas the “Bad” ones have been created specifically with the images in the wrong order (pages 6 to 10, then 1 to 5).

SumatraPDF_3.2.11061_portable_x64.exe with UnRAR64.dll (5.61.100.2835) will display the “Good” archives in the correct order (pages 1 to 10) because that is how I stored the files in those archives.
However it will display the “Bad” archives in the wrong order (pages 6 to 10, then 1 to 5) because again that is how I stored the files in those archives.
Naturally 3.1.2 which lacks this bug (due to the re-sorting using cmpArchFileInfoByName()) will display all the archives in the correct order, but will fail with other archives where natural sorting is required for the filenames. That’s why I said that cmpArchFileInfoByName() needs to be deleted as in commit 1060, but SortNatural()/CmpNatural() needs to be fixed. Once that’s done it should take care of all the cases.

GitHubRulesOK

@SumatraPeter and @ianas not wishing to stir this too much but my view is your both right as there are two inter related problems.
Ianas finds there is no longer a cbr sort compared to before, where the previous working sort of -010 -011 -009 has been lost
SumatraPeter has pointed out there has been a long standing issue that the sort of -10 -11 -9 has never been resolved
BOTH should be solved by fixing the later.

SumatraPeter

There’s nothing to stir up because we’re all ultimately trying to get to the bottom of this vexed issue.

With all due respect, please don’t reiterate this wrong finding because it only serves to continue the confusion of some folks here. The loss of filename sorting in 3.2.11061 affects every single supported archive type as I have mentioned multiple times now, and you can easily confirm using my test archives. This is not, I repeat not, a CBR-only bug, period. It affects CB7s, CBTs and CBZs just as it does CBRs, so I don’t see how it can possibly be a problem with the unarr/unrar code.

3.2.11061 is simply displaying the images in whatever order they are stored in any archive without sorting them at all.

The secondary issue of natural sort never having worked of course needs to be fixed as well. As you’ve rightly stated, once natural sorting is properly implemented there will be no need for cmpArchFileInfoByName() and both issues will be fixed together.

kjk

Apologies for braking this. Should be fixed (and even improved) in pre-release 11063 or later https://www.sumatrapdfreader.org/prerelease.html

GitHubRulesOK

@kjk my initial tests suggest you’ve got it right, thanks for the hard work also to @SumatraPeter and @ianas for their insights

Ianis can you recompile? I greatly appreciate your enhanced previewer

SumatraPeter

As per my tests this looks like it’s been fixed successfully at last!

5 archives (7z/CB7, RAR4/CBR, RAR5/CBR, TAR/CBT and ZIP/CBZ) for testing can be found here and my test results are as follows:

|=====================|==============|=============|
|    No Sorting or    | Alphabetical |   Natural   |
| Original File Order |  Sort Order  | Sort Order  |
|     (3.2.11061)     |   (3.1.2)    | (3.2.11063) |
|=====================|==============|=============|
| P90.png             | P1.png       | P1.png      |
| P9.png              | P10.png      | P2.png      |
| P8.png              | P100.png     | P3.png      |
| P7.png              | P101.png     | P4.png      |
| P6.png              | P102.png     | P5.png      |
| P50.png             | P11.png      | P6.png      |
| P5.png              | P12.png      | P7.png      |
| P4.png              | P13.png      | P8.png      |
| P3.png              | P14.png      | P9.png      |
| P20.png             | P15.png      | P10.png     |
| P2.png              | P16.png      | P11.png     |
| P19.png             | P17.png      | P12.png     |
| P18.png             | P18.png      | P13.png     |
| P17.png             | P19.png      | P14.png     |
| P16.png             | P2.png       | P15.png     |
| P15.png             | P20.png      | P16.png     |
| P14.png             | P3.png       | P17.png     |
| P13.png             | P4.png       | P18.png     |
| P12.png             | P5.png       | P19.png     |
| P11.png             | P50.png      | P20.png     |
| P102.png            | P6.png       | P50.png     |
| P101.png            | P7.png       | P90.png     |
| P100.png            | P8.png       | P100.png    |
| P10.png             | P9.png       | P101.png    |
| P1.png              | P90.png      | P102.png    |
|=====================|==============|=============|

ianas

I just noticed the update I’ll recompile tonight
tanks guys
just updated the build you can find it at my mega dir
https://mega.nz/#F!WwIiFDyZ!eA2g4lws9YNOuEI0a9Dkpw