Soften correctness of image file open check
ClosedPublic

Authored by trufanov on Oct 22 2017, 9:48 AM.

Details

Summary

BUG: 385384

Some applications (like acdsee v3.1) produce PNG files that can't be opened in Okular while gwenview, GIMP and ImageMagick is able to display them. For such files QImageReader returns false as a result of open operation while resulting QImage still gets a valid data. This patch soften image file open checks. Now file open error is not reported if resulting QImage is valid even if read function returned false.

A test png is attached.
Attachment

Test Plan

Try to open attached png file. It can't be opened.
Apply patch (make sure Okular uses fixed okularGenerator_kimgio.so) - now it could be opened and displayed correctly.

Diff Detail

Repository
R223 Okular
Lint
Lint Skipped
Unit
Unit Tests Skipped
trufanov created this revision.Oct 22 2017, 9:48 AM

Thanks for the patch: The Summary needs some reformatting:

  • Add the special keyword "BUG 385384" on its own line
  • Remove the quote from the Bugzilla ticket; instead briefly explain the change in your own words
  • No need to explain the code change; it should be self-explanatory, or else have comments in the code itself

Can/did you test this patch with other PNG files to make sure it didn't break for other files?

Otherwise, this looks good to me.

trufanov edited the summary of this revision. (Show Details)Oct 23 2017, 11:50 AM

Ok, I've edited summary. Didn't know that KDE Bugs could be referred in a such way in phabricator summary instead of commit message.
Also I heard I can add
Differential Revision: https://phabricator.kde.org/D8415
in commit message to automatically close phabricator review. Is it so?

As for testing. So far so good.

Also I heard I can add
Differential Revision: https://phabricator.kde.org/D8415
in commit message to automatically close phabricator review. Is it so?

This is true, but it must be the last line. See https://community.kde.org/Policies/Commit_Policy#Special_keywords_in_GIT_and_SVN_log_messages .

You might also want to check out "arc", the command line tool for phabricator.
For example "arc land" will automatically commit, push and close the differential revision.

aacid added a subscriber: aacid.Oct 23 2017, 12:30 PM

Wouldn't it make more sense to fix this in QImageReader and not in every user of QImageReader ?

progwolff added a comment.EditedOct 23 2017, 12:37 PM
In D8415#158804, @aacid wrote:

Wouldn't it make more sense to fix this in QImageReader and not in every user of QImageReader ?

From my perspective the behaviour of QImageReader is correct.

QImage::read docs:

Reads an image from the device into image, which must point to a QImage. Returns true on success; otherwise, returns false.

QImage::isNull docs:

Returns true if it is a null image, otherwise returns false.
A null image has all parameters set to zero and no allocated data.

It seems totally possible that an empty image file is read correctly. In this case read returns true, but the resulting image is a null image.

For this patch I would however prefer a new error message, something like "the loaded document is empty".

Edit: Sorry, I misread this in whole. @aacid is right.

In D8415#158804, @aacid wrote:

Wouldn't it make more sense to fix this in QImageReader and not in every user of QImageReader ?

It also might be libpng problem which Qt uses under the hood. As stated in original bug report: The "libpng error: Read Error" is printed to std:out when QImageReader reads such file. Still libpng could behave as by design. And for sure it's acdsee v3.1 problem.

aacid added a comment.Oct 23 2017, 9:19 PM

Please consider fixing this in Qt before asking us to create a workaround.

In D8415#159049, @aacid wrote:

Please consider fixing this in Qt before asking us to create a workaround.

I'm fine with workarounded gwenview as well as working GIMP and ImageMagick.

aacid added a comment.Oct 30 2017, 8:49 PM
In D8415#159049, @aacid wrote:

Please consider fixing this in Qt before asking us to create a workaround.

I'm fine with workarounded gwenview as well as working GIMP and ImageMagick.

Is this your way of saying "i'm not interested in pursuing a fix in Qt"?

In D8415#162014, @aacid wrote:
In D8415#159049, @aacid wrote:

Please consider fixing this in Qt before asking us to create a workaround.

I'm fine with workarounded gwenview as well as working GIMP and ImageMagick.

Is this your way of saying "i'm not interested in pursuing a fix in Qt"?

I would be honered to commit a fix into Qt framework (never did yet). I'm also happy to commit something into KDE.
But in my opinion it's most probably a libpng problem. And if I suggest a workaround to Qt team they'll answer me "Please consider fixing this in libpng before asking us to create a workaround." So I have to investigate libpng beforehand. I don't know png format specifications at all. And don't want to. That'll take time to investigate and a weeks of discussion with responsible teams.
And then I'll face with one of 3 options:

  1. I need to add a warning level in binary "error/everything is ok" communication between Qt and libpng. And adjust Qt's images format plugin system. That would be best one.
  2. It's indeed a libpng-only problem. And they may return no error in such cases. (And they can argue that this is by design, by png standard or breaks binary compatibility).
  3. Most probable option - it's acdsee app problem. Perhaps, only N-years old acdsee version problem. Which is proprietary software and both Qt and libpng will ignore it.

So, I just have no time for digging into this further.
I have an app. I've got user feedback that some png images do not open. Got an example. I was surprised to find that some opensource Qt apps could open it and some could not and leveraged with this to fix my app. I'm fine. And I just wanted to share my findings in case they could be useful. So it just a bit less than feasible for me to keen digging into this,
(Btw, the proposed patch is that I find out from just a gwenview, I've not checked why particular GIMP and ImageMagick has no such problem. The reason may be different.)

I would summarise this as following. This could be marked as WONT_FIX. My users are quite a specific guys which scan books and conservative enough to use weird software and its versions for years (or perhaps a decade) following manuals and trying to keep quality. If I'll face with file that is created by modern software I would be more keen to fix it.

Could we add the local workaround here, and include a FIXME comment urging an upstream fix later? Gwenview already has this workaround...

aacid accepted this revision.Nov 11 2017, 11:07 PM

Sure, commit it, but when you do can you please emit a warning() message saying something like "the file has some errors we're showing the best we can"?

This revision is now accepted and ready to land.Nov 11 2017, 11:07 PM
ngraham added a comment.EditedNov 11 2017, 11:07 PM

That's a good idea. @trufanov, can you add that, and also add a comment indicating that we're working around an upstream issue?

trufanov updated this revision to Diff 22254.Nov 13 2017, 8:33 AM

warning message was added

Ok, I've added a warning for this. Please check wording - english isn't my native language. Feel free to change it.
Attached are screenshots of Okular showing current warning for eng and rus locales.

aacid added a comment.Nov 13 2017, 9:30 PM

@ngraham you seem like a native speaker, please commit this with proper english-ness

I am, yes. How does this sound?

This document appears malformed. Error: [error message]
Here is a best approximation of the document's intended appearance:

This wording (or something similar) has the benefit that it casts Okular as a hero--bravely doing its best to render the lousy input you're trying to feed it. :-)

Sounds good, do we actually get a meaningful error message in this case?

If not i'd say just remove the "Error: [error message]" part.

Yeah, in the above screenshot, reader.errorString() seems to have returned "Unable to read image data" which is not very helpful. How about a single line and the following text:

This document appears malformed. Here is a best approximation of the document's intended appearance:

ngraham edited the summary of this revision. (Show Details)Nov 13 2017, 10:46 PM
aacid accepted this revision.Nov 13 2017, 10:47 PM

Personally i'd change the : for a .

But i don't really care, go ahead i'd say.

ngraham accepted this revision.Nov 13 2017, 10:48 PM

@trufanov, would you like to make that change? If not I can do it, and either way, let's land this tonight.

@ngraham Feel free to do it yourself.

OK, will do!

Actually @trufanov, would you mind doing it? I'd rather not commandeer your revision or commit it by hand, both of which will break the history here on phabricator.

trufanov updated this revision to Diff 22327.Nov 14 2017, 11:21 AM

Wording of a warning message is changed.
With dot at sentence end.
A screenshot of warning appearance is attached{F5491726}

This revision was automatically updated to reflect the committed changes.