[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