D8415: Soften correctness of image file open check

[Alexander Trufanov]

This revision was automatically updated to reflect the committed changes.
Closed by commit R223:4b2ba6a7af25: Try to display a malformed image if 
feasible (authored by trufanov).

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8415?vs=22327=22331

REVISION DETAIL
  https://phabricator.kde.org/D8415

AFFECTED FILES
  generators/kimgio/generator_kimgio.cpp

To: trufanov, ngraham, #okular, aacid
Cc: aacid, progwolff, ngraham

D8415: Soften correctness of image file open check

[Alexander Trufanov]

trufanov updated this revision to Diff 22327.
trufanov added a comment.


  Wording of a warning message is changed.
  With dot at sentence end.
  A screenshot of warning appearance is 
attached{https://phabricator.kde.org/F5491726}

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8415?vs=22254=22327

REVISION DETAIL
  https://phabricator.kde.org/D8415

AFFECTED FILES
  generators/kimgio/generator_kimgio.cpp

To: trufanov, ngraham, #okular, aacid
Cc: aacid, progwolff, ngraham

D8415: Soften correctness of image file open check

[Nathaniel Graham]

ngraham added a comment.


  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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D8415

To: trufanov, ngraham, #okular, aacid
Cc: aacid, progwolff, ngraham

D8415: Soften correctness of image file open check

[Nathaniel Graham]

ngraham added a comment.


  OK, will do!

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D8415

To: trufanov, ngraham, #okular, aacid
Cc: aacid, progwolff, ngraham

D8415: Soften correctness of image file open check

[Alexander Trufanov]

trufanov added a comment.


  @ngraham Feel free to do it yourself.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D8415

To: trufanov, ngraham, #okular, aacid
Cc: aacid, progwolff, ngraham

D8415: Soften correctness of image file open check

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.


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

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D8415

To: trufanov, ngraham, #okular, aacid
Cc: aacid, progwolff, ngraham

D8415: Soften correctness of image file open check

[Albert Astals Cid]

aacid accepted this revision.
aacid added a comment.


  Personally i'd change the : for a .
  
  But i don't really care, go ahead i'd say.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D8415

To: trufanov, ngraham, #okular, aacid
Cc: aacid, progwolff, ngraham

D8415: Soften correctness of image file open check

[Nathaniel Graham]

ngraham edited the summary of this revision.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D8415

To: trufanov, ngraham, #okular, aacid
Cc: aacid, progwolff, ngraham

D8415: Soften correctness of image file open check

[Nathaniel Graham]

ngraham added a comment.


  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:

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D8415

To: trufanov, ngraham, #okular, aacid
Cc: aacid, progwolff, ngraham

D8415: Soften correctness of image file open check

[Albert Astals Cid]

aacid added a comment.


  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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D8415

To: trufanov, ngraham, #okular, aacid
Cc: aacid, progwolff, ngraham

D8415: Soften correctness of image file open check

[Nathaniel Graham]

ngraham added a comment.


  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. :-)

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D8415

To: trufanov, ngraham, #okular, aacid
Cc: aacid, progwolff, ngraham

D8415: Soften correctness of image file open check

[Albert Astals Cid]

aacid added a comment.


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

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D8415

To: trufanov, ngraham, #okular, aacid
Cc: aacid, progwolff, ngraham

D8415: Soften correctness of image file open check

[Alexander Trufanov]

trufanov added a comment.


  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.F5489936: Screenshot_20171113_113818.png 

  
  F5489935: Screenshot_20171113_113834.png 


REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D8415

To: trufanov, ngraham, #okular, aacid
Cc: aacid, progwolff, ngraham

D8415: Soften correctness of image file open check

[Nathaniel Graham]

ngraham added a comment.


  That's a good idea. @trufanov, can you add that?

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D8415

To: trufanov, ngraham, #okular, aacid
Cc: aacid, progwolff, ngraham

D8415: Soften correctness of image file open check

[Albert Astals Cid]

aacid accepted this revision.
aacid added a comment.
This revision is now accepted and ready to land.


  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"?

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D8415

To: trufanov, ngraham, #okular, aacid
Cc: aacid, progwolff, ngraham

D8415: Soften correctness of image file open check

[Nathaniel Graham]

ngraham added a comment.


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

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D8415

To: trufanov, ngraham, #okular
Cc: aacid, progwolff

D8415: Soften correctness of image file open check

[Alexander Trufanov]

trufanov added a comment.


  In https://phabricator.kde.org/D8415#162014, @aacid wrote:
  
  > In https://phabricator.kde.org/D8415#160837, @trufanov wrote:
  >
  > > In https://phabricator.kde.org/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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D8415

To: trufanov, ngraham, #okular
Cc: aacid, progwolff

D8415: Soften correctness of image file open check

[Alexander Trufanov]

trufanov added a comment.


  In https://phabricator.kde.org/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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D8415

To: trufanov, ngraham, #okular
Cc: aacid, progwolff

D8415: Soften correctness of image file open check

[Albert Astals Cid]

aacid added a comment.


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

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D8415

To: trufanov, ngraham, #okular
Cc: aacid, progwolff

D8415: Soften correctness of image file open check

[Julian Wolff]

progwolff added a comment.


  In https://phabricator.kde.org/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".

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D8415

To: trufanov, ngraham, #okular
Cc: aacid, progwolff

D8415: Soften correctness of image file open check

[Albert Astals Cid]

aacid added a comment.


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

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D8415

To: trufanov, ngraham, #okular
Cc: aacid, progwolff

D8415: Soften correctness of image file open check

[Julian Wolff]

progwolff added a comment.


  In https://phabricator.kde.org/D8415#158800, @trufanov wrote:
  
  > 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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D8415

To: trufanov, ngraham, #okular
Cc: progwolff, aacid

D8415: Soften correctness of image file open check

[Alexander Trufanov]

trufanov added a comment.


  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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D8415

To: trufanov, ngraham, #okular
Cc: aacid

D8415: Soften correctness of image file open check

[Alexander Trufanov]

trufanov edited the summary of this revision.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D8415

To: trufanov, ngraham, #okular
Cc: aacid

D8415: Soften correctness of image file open check

[Nathaniel Graham]

ngraham added a comment.


  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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D8415

To: trufanov, ngraham, #okular
Cc: aacid

D8415: Soften correctness of image file open check

[Alexander Trufanov]

trufanov created this revision.
trufanov added reviewers: ngraham, Okular.
trufanov added a project: Okular.

REVISION SUMMARY
  resolves bug https://bugs.kde.org/show_bug.cgi?id=385384
  
  Bug 385384 - Some PNG files produced by acdsee can't be displayed
  
  > Hi,
  >  I've got a png file that could be opened in gwenview, GIMP and 
ImageMagick. But can't be viewed in Okular. My app had the same problem so I 
dig into gwenview sourcecode to find the solution and now would like to share 
my findings.
  > 
  > As gwenview my app uses Qt's QImage/QImageReader to load this png file. And 
Qt uses libpng1.6.28-1 that prints in std::cout "libpng error: Read Error" in 
both cases. But gwenview ignores this error and still able to show image. Bcs 
instead of `void QImage::load(...)` or `QImage QImageReader::read()` it invokes 
`bool QImageReader::read(QImage *)`. And it ignores its result value if 
returned QImage is valid.
  > 
  > This allowed me to workaround problem in my app.
  > 
  > Gwenviews code: 
  >  They log an error here 

 but as loadImageData() is a void function and mImage still contains valid 
image the app displays it as usual.
  
  My assumption is that png has incomplete or incorrect set of flags in header 
so libpng reports error while still loads image data to RAM.
  
  A test png is attached.
  I was told that this was a tiff file (result of scanning) that was converted 
to png with acdsee v3.1 under Win.
  
  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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D8415

AFFECTED FILES
  generators/kimgio/generator_kimgio.cpp

To: trufanov, ngraham, #okular
Cc: aacid