<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 březen 13th, 2015, 4:23 odp. UTC, <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 březen 13th, 2015, 4:28 odp. UTC, <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 březen 18th, 2015, 1:36 odp. UTC, <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 březen 22nd, 2015, 4:01 odp. UTC, <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 březen 23rd, 2015, 10:22 dop. UTC, <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 březen 23rd, 2015, 2:07 odp. UTC, <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 březen 23rd, 2015, 3:07 odp. UTC, <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 březen 31st, 2015, 12:10 odp. UTC, <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 březen 31st, 2015, 2:55 odp. UTC, <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 březen 31st, 2015, 4:45 odp. UTC, <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 duben 1st, 2015, 8:04 dop. UTC, <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>
</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;">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>
<br />
<p>- Martin</p>
<br />
<p>On duben 3rd, 2015, 12:47 odp. UTC, 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 Dub. 3, 2015, 12:47 odp.</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>