Review Request 122873: Fix for wrong rotation of RAW images

Thu May 7 08:51:05 UTC 2015

> On Bře. 13, 2015, 4:23 odp., David Edmundson wrote:
> > Do you have commit access?
> 
> Martin Kyral wrote:
>     Unfortunatelly not. I made 2 patches for gwenview so far, both committed by Aurelien on my behalf.
> 
> Martin Kyral wrote:
>     Could you, please, commit it?
> 
> Aurélien Gâteau wrote:
>     The changes done by the Qt patch do not affect only RAW images: JPEGs are not displayed correctly anymore since they rotated once by Qt and then again by Gwenview. I guess some of the special-handling code for JPEG files can simply be removed. Could you look into this?
> 
> Martin Kyral wrote:
>     It's strange, but RAW images and the previews dumped from RAW using dcraw -e are the only images affected by the double rotation. I tried removing the jpeg special-handling code unconditionally but it actually breaks the rest (I tried with the images from the gwenview tests and with images from https://github.com/recurser/exif-orientation-examples.git) so I decided to implement the fix as is in this patch. However, as I am stating in the description - the previews saved by dcraw -e (technically exactly the same data as the RAW handling code extracts) are not fixed here and this case needs to be further elaborated (probably using better detection from exif).
>     
>     It seems that there are two disctinct ways of lossless rotation of jpeg. I fiddled with some images in geeqie and gwenview with interesting results. While rotating jpeg in geeqie does not alter file size at all, rotating the same image in gwenview produces different file sizes for portrait and landscape orientation of the same image (but the file does not shrink upon multiple rotations so the data are not recompressed, thus the rotation is losless). When I rotated the image in geeqie and in gwenview I ended up with image which was displayed improperly by one of the viewers, maybe both.
>     
>     Unfortunatelly I am not familiar enough with the JPEG format nor the EXIF metadata so I am unable now to see the cause.
> 
> Aurélien Gâteau wrote:
>     My guess regarding the difference you see between Geeqie and Gwenview is that Gwenview losslessly rotates the pixels and resets the orientation flag to 1, whereas I assume Geeqie only changes the orientation flag. The way Gwenview behaves can produce weird results if the dimensions of the image are not multiples of 8, which may be the reason why your image was displayed improperly.
> 
> Martin Kyral wrote:
>     By 'displayed improperly' I meant only rotation. Thanks for explanation, thay may be it...
> 
> Martin Kyral wrote:
>     Could the patch be committed, please? Or are there any concerns, qestions or requests for further work on it? Thanks.
> 
> Aurélien Gâteau wrote:
>     I just committed it, but I did not want to and had to revert it, sorry for the mess. The reason I did not want to commit it is that from what I see the Qt change breaks JPEG loading as much as it breaks RAW loading, at least JPEG pictures from my camera were all badly oriented. I am happy to provide some examples if you want to dive into that.
>     
>     I believe a better fix would be to:
>     
>     1. Check if we are running on the Qt version which is affected by the bug (don't know how :/)
>     2. If this is the case: disable orientation handling for both RAW and JPEG formats.
> 
> Martin Kyral wrote:
>     Ah, I see. I know the fix is just partial - I can have plenty of still-affected jpegs using dcraw -e. I think pics from your camera is the very same case. Unfortunatelly, not all jpegs are affected - you can try these: https://github.com/recurser/exif-orientation-examples.git or even pictures from the gwenview internal testsuite. So we need to invent some heuristic/detection to check if the particular image is affected or not and decide based on the outcome. Unfortunatelly, I don't know jpeg format well enough :(
>     Per the QT version - would it be an option just to bump the required version of QT? Create a tag on the commit before this one so if anyone needs gwenview with QT <= 5.3, there is the marked source, anyone wanting master needs QT 5.4+?
> 
> Aurélien Gâteau wrote:
>     That is really strange, how does Qt discover the orientation? Either they read the exif tag or they don't. Going to read the code.
>     
>     Regarding bumping the required version of Qt: I am not following KDE development as closely as I used to and passed Gwenview maintainership over to Lukas Tinkl so the decision should be his. What is the minimum required Qt version for Frameworks and Plasma? I am not familiar with the latests policies so I don't know if Gwenview minimum Qt version can be bumped or not.
> 
> Martin Kyral wrote:
>     So, in the end it seems the rotation in gwenview won't need fix at all. There is an discussion in the qt project regarding the original change and its sanity. It seems likely that the change will be reverted because it breaks backwards compatibility and the auto-rotation will be reiplemented as optional feature (most likely opt-in, not opt-out).
>     
>     https://bugreports.qt.io/browse/QTBUG-37946
>     https://codereview.qt-project.org/#/c/110668/
> 
> Aurélien Gâteau wrote:
>     Cool! Good to know there won't be any need for some Qt version runtime check. Thanks for letting us know about it.

The change in QImage is being reverted from Qt 5.4.2 on. Thus, I'm scratching this review. No fix needed anymore.

https://codereview.qt-project.org/#/c/110668/

- Martin

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122873/#review77427
-----------------------------------------------------------

On Dub. 3, 2015, 12:47 odp., Martin Kyral wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122873/
> -----------------------------------------------------------
> 
> (Updated Dub. 3, 2015, 12:47 odp.)
> 
> 
> Review request for Gwenview and Lukáš Tinkl.
> 
> 
> Repository: gwenview
> 
> 
> Description
> -------
> 
> Displaying of RAW images in gwenview was broken by https://bugreports.qt.io/browse/QTBUG-37946. I am fixing (or rather workarounding it) by blocking EXIF rotation iff the loaded image is RAW.
> 
> 
> Diffs
> -----
> 
>   lib/document/loadingdocumentimpl.cpp cc8bea9 
>   lib/thumbnailprovider/thumbnailgenerator.h 4571832 
>   lib/thumbnailprovider/thumbnailgenerator.cpp 54875f5 
> 
> Diff: https://git.reviewboard.kde.org/r/122873/diff/
> 
> 
> Testing
> -------
> 
> Tested with various RAW files, used previously to test the RAW loading support itself & with some jpeg files (including https://github.com/recurser/exif-orientation-examples.git).
> 
> Seems OK.
> 
> Nevertheless, the fix is not complete as jpeg files extracted from the RAWs using 'dcraw -e' are still mis-rotated.
> 
> 
> Thanks,
> 
> Martin Kyral
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/gwenview-devel/attachments/20150507/7204afc6/attachment.html>