[prev in list] [next in list] [prev in thread] [next in thread]
List: gwenview-devel
Subject: Re: Review Request 122873: Fix for wrong rotation of RAW images
From: Aurélien Gâteau <agateau () kde ! org>
Date: 2015-04-24 16:55:14
Message-ID: 20150424165514.5446.78589 () mimi ! kde ! org
[Download RAW message or body]
--===============1978586664831139383==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
> On mars 13, 2015, 5:23 après-midi, 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/
Cool! Good to know there won't be any need for some Qt version runtime check. Thanks \
for letting us know about it.
- Aurélien
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122873/#review77427
-----------------------------------------------------------
On avr. 3, 2015, 2:47 après-midi, Martin Kyral wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122873/
> -----------------------------------------------------------
>
> (Updated avr. 3, 2015, 2:47 après-midi)
>
>
> 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
>
>
--===============1978586664831139383==
MIME-Version: 1.0
Content-Type: text/html; charset="utf-8"
Content-Transfer-Encoding: 8bit
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 \
solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;"> \
<tr> <td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/122873/">https://git.reviewboard.kde.org/r/122873/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On mars 13th, 2015, 5:23 après-midi CET, <b>David \
Edmundson</b> wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px solid \
#d0d0d0; padding-left: 10px;"> <pre style="white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">Do you have commit access?</pre> </blockquote>
<p>On mars 13th, 2015, 5:28 après-midi CET, <b>Martin Kyral</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">Unfortunatelly not. I made 2 patches for gwenview so far, both committed by \
Aurelien on my behalf.</p></pre> </blockquote>
<p>On mars 18th, 2015, 2:36 après-midi CET, <b>Martin Kyral</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Could \
you, please, commit it?</p></pre> </blockquote>
<p>On mars 22nd, 2015, 5:01 après-midi CET, <b>Aurélien Gâteau</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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?</p></pre> </blockquote>
<p>On mars 23rd, 2015, 11:22 matin CET, <b>Martin Kyral</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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).</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p> <p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">Unfortunatelly I am not familiar enough with the JPEG format nor the EXIF \
metadata so I am unable now to see the cause.</p></pre> </blockquote>
<p>On mars 23rd, 2015, 3:07 après-midi CET, <b>Aurélien Gâteau</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p></pre> </blockquote>
<p>On mars 23rd, 2015, 4:07 après-midi CET, <b>Martin Kyral</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">By \
'displayed improperly' I meant only rotation. Thanks for explanation, thay may be \
it...</p></pre> </blockquote>
<p>On mars 31st, 2015, 2:10 après-midi CEST, <b>Martin Kyral</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Could \
the patch be committed, please? Or are there any concerns, qestions or requests for \
further work on it? Thanks.</p></pre> </blockquote>
<p>On mars 31st, 2015, 4:55 après-midi CEST, <b>Aurélien Gâteau</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p> <p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">I believe a better fix would be to:</p> <ol \
style="padding: 0;text-rendering: inherit;margin: 0 0 0 2em;line-height: \
inherit;white-space: normal;"> <li style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: normal;">Check if we are running on the Qt \
version which is affected by the bug (don't know how :/)</li> <li style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">If \
this is the case: disable orientation handling for both RAW and JPEG formats.</li> \
</ol></pre> </blockquote>
<p>On mars 31st, 2015, 6:45 après-midi CEST, <b>Martin Kyral</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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+?</p></pre> </blockquote>
<p>On avril 1st, 2015, 10:04 matin CEST, <b>Aurélien Gâteau</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p></pre> </blockquote>
<p>On avril 20th, 2015, 10:36 matin CEST, <b>Martin Kyral</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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).</p> <p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">https://bugreports.qt.io/browse/QTBUG-37946 \
https://codereview.qt-project.org/#/c/110668/</p></pre> </blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Cool! \
Good to know there won't be any need for some Qt version runtime check. Thanks for \
letting us know about it.</p></pre> <br />
<p>- Aurélien</p>
<br />
<p>On avril 3rd, 2015, 2:47 après-midi CEST, Martin Kyral wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: \
1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; \
-webkit-border-radius: 6px;"> <tr>
<td>
<div>Review request for Gwenview and Lukáš Tinkl.</div>
<div>By Martin Kyral.</div>
<p style="color: grey;"><i>Updated avr. 3, 2015, 2:47 après-midi</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
gwenview
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border: 1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">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.</p></pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">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).</p> <p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Seems \
OK.</p> <p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">Nevertheless, the fix is not complete as jpeg files \
extracted from the RAWs using 'dcraw -e' are still mis-rotated.</p></pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>lib/document/loadingdocumentimpl.cpp <span style="color: \
grey">(cc8bea9)</span></li>
<li>lib/thumbnailprovider/thumbnailgenerator.h <span style="color: \
grey">(4571832)</span></li>
<li>lib/thumbnailprovider/thumbnailgenerator.cpp <span style="color: \
grey">(54875f5)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/122873/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
--===============1978586664831139383==--
[Attachment #3 (text/plain)]
_______________________________________________
Gwenview-devel mailing list
Gwenview-devel@kde.org
https://mail.kde.org/mailman/listinfo/gwenview-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic