Bug Hunting Session
Bug 90319 - Image flip vertically or horizontally not working with png files in writer
Summary: Image flip vertically or horizontally not working with png files in writer
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Writer (show other bugs)
Version:
(earliest affected)
4.5.0.0.alpha0+ Master
Hardware: Other Linux (All)
: medium normal
Assignee: Not Assigned
URL:
Whiteboard: target:5.2.0 target:5.1.0.2 target:5...
Keywords: bibisected, bisected, regression
Depends on:
Blocks: Writer-Images
  Show dependency treegraph
 
Reported: 2015-03-29 11:42 UTC by Yousuf Philips (jay) (retired)
Modified: 2016-10-12 12:34 UTC (History)
6 users (show)

See Also:
Crash report or crash signature:


Attachments
Test case: two flipped images, jpg and png (22.62 KB, application/vnd.oasis.opendocument.text)
2015-12-10 09:26 UTC, Jacobo Aragunde Pérez
Details
png image (with transparency) flipped horizontally (22.00 KB, application/msword)
2016-02-13 17:14 UTC, Justin L
Details
I must have uploaded the wrong file. This is not a screenshot as expected. (9.61 KB, application/pdf)
2016-02-15 06:00 UTC, Justin L
Details
screenshot that belonged in comment 22 which documents the problem (86.46 KB, image/png)
2016-04-07 16:26 UTC, Justin L
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Yousuf Philips (jay) (retired) 2015-03-29 11:42:02 UTC
Steps:
1) Open writer
2) Insert image
3) Click the flip vertically or flip horizontally buttons in the image toolbar
4) Nothing happens

Regression as this works fine in 4.4 daily but not master.

Version: 4.5.0.0.alpha0+
Build ID: 0fb66458c08b9c0ce59bca85e77d26fad8b59e4b
TinderBox: Linux-rpm_deb-x86@45-TDF, Branch:master, Time: 2015-03-26_06:47:37

Version: 4.4.2.0.0+
Build ID: a6fa883bd74c963123a37025c4836a96babc6168
TinderBox: Linux-rpm_deb-x86@45-TDF, Branch:libreoffice-4-4, Time: 2015-03-09_02:43:46
Comment 1 Matthew Francis 2015-03-29 12:11:00 UTC
I can confirm this for .png images, not for jpeg images

-> NEW
Comment 2 Matthew Francis 2015-03-29 12:12:12 UTC
This seems to have started as of the below commit.
Adding Cc: to quikee@gmail.com; Could you possibly take a look at this? Thanks

commit 009c1752b1eff36827dae4fa9bd394c5089dcc55
Author: Tomaž Vajngerl <tomaz.vajngerl@collabora.co.uk>
Date:   Sat Jan 3 13:14:55 2015 +0900

    Extract slow path of DrawDeviceAlphaBitmap into its own method
    
    Additioanlly cleanup and use ScopedReadAccess
    
    Change-Id: Ia3365f4dc968368bdd90d4398188bffe2d56e89b
Comment 3 pasqual milvaques 2015-12-03 18:51:10 UTC
I can confirm that it's still not working in 

Version: 5.2.0.0.alpha0+
Build ID: 06e32106cc4c0886c228b4dbfe7301222a96a231
Comment 4 pasqual milvaques 2015-12-05 16:47:58 UTC
I have created a patch and submitted it to gerrit, is located here:
https://gerrit.libreoffice.org/#/c/20415/

it needs review, if it's approved it will go to master, later will be checked if it works in the daily builds and from them we would port it to the stable releases
Comment 5 Bryan Quigley 2015-12-08 20:11:33 UTC
I attempted to test that this doesn't work (to test that it does work in code posted to gerrit) and couldn't reproduce with the few PNG files I tested.

Does it fail with http://vignette1.wikia.nocookie.net/zelda/images/8/8e/Link_Wind_Waker_7.png/revision/latest?cb=20081018163305 for anyone?

Can anyone link a file that definitely fails?
Comment 6 pasqual milvaques 2015-12-08 22:48:05 UTC
I can reproduce the problem with the image you attach, pay attention of two things:
*the problem happens if you do horizontal flipping OR vertical flipping. If you do both flippings they are treated as a rotation and work correctly
*versions prior to 5.0 are unaffected, I'm testing with version of LibreOffice which comes with fedora 23 (5.0.3.2) and the problem reproduces

I have done more testing and in a Windows machine the problem doesn't reproduce, I'm not sure why this happens, there are multiple code paths in the code, probably in windows the codepath taken to draw the image is different and that is the reason the bug doesn't reproduce there

I update the bug to indicate that only Linux machines are affected, please re-test the bug or indicate if it doesn't reproduce in Linux too
Comment 7 Bryan Quigley 2015-12-09 14:35:27 UTC
Apologies I hadn't done the testcase correct.  I can reproduce now. I did find a bug with the new proposal though (mentioned in gerrit).
Comment 8 Jacobo Aragunde Pérez 2015-12-10 09:26:14 UTC
Created attachment 121188 [details]
Test case: two flipped images, jpg and png
Comment 9 Robinson Tryon (qubit) 2015-12-13 10:30:18 UTC Comment hidden (obsolete)
Comment 10 pasqual milvaques 2015-12-18 00:18:51 UTC
Hi, I have redone the patch in a different way as the one I sent was flawed, the new one seems to do the work correctly 

The new patch is here.
https://gerrit.libreoffice.org/20782

Please, can you test if with this one the problem is solved?

Thanks :)
Comment 11 Commit Notification 2015-12-18 18:41:44 UTC
pasqualm committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=3119440a80282692640378fde5e37974ab63f096

tdf#90319: make image flipping work in writer for png

It will be available in 5.2.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 12 Jacobo Aragunde Pérez 2015-12-18 18:48:43 UTC
Congratulations Pasqual, you landed your first patch! :)

Now, feel free to backport it to 5.1 and even 5.0 branches. It's very easy to do with the "cherry pick" button in the gerrit UI. The name of the branches are libreoffice-5-1 and libreoffice-5-0.

The patch will need two more reviews to be merged in those branches, others will jump in to do the additional reviews. Please be patient! :)
Comment 13 pasqual milvaques 2015-12-20 14:14:48 UTC
Thank you Jacobo!!

Yousuf Philips has cherrypicked the patch for 5.1 but I see that jenkins has failed to do the build (https://gerrit.libreoffice.org/#/c/20805/) in windows, the error happens testing a publisher qa file. I'm not sure about if this is normal and the review process will continue without intervention. 

thanks!!
Comment 14 Jacobo Aragunde Pérez 2015-12-21 10:06:06 UTC
Rebased the patch to force another Jenkins build.
Comment 15 Yousuf Philips (jay) (retired) 2015-12-21 11:01:18 UTC
(In reply to pasqual milvaques from comment #13)
> Yousuf Philips has cherrypicked the patch for 5.1 but I see that jenkins has
> failed to do the build (https://gerrit.libreoffice.org/#/c/20805/) in
> windows, the error happens testing a publisher qa file. I'm not sure about
> if this is normal and the review process will continue without intervention. 

Jenkins windows builds are broken and always fail tests.
Comment 16 Commit Notification 2015-12-23 16:26:51 UTC
pasqualm committed a patch related to this issue.
It has been pushed to "libreoffice-5-1":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=c9c979a5adc137df89e50809e0f810464f60894c&h=libreoffice-5-1

tdf#90319: make image flipping work in writer for png

It will be available in 5.1.0.2.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 17 Adolfo Jayme 2016-01-07 17:17:33 UTC
(In reply to Yousuf (Jay) Philips from comment #15)
> Jenkins windows builds are broken and always fail tests.

A dangerous misconception and worse generalization.
Comment 18 Justin L 2016-02-13 17:14:37 UTC
Created attachment 122624 [details]
png image (with transparency) flipped horizontally

Some png's fall through the cracks still.  I'm guessing they are ones with transparency.  They follow this path (and thus miss the patch):

if (aLinearContext.blendBitmap( Bitmap::ScopedWriteAccess(aBmp).get(), pBitmapReadAccess.get(), pAlphaReadAccess.get(),
                    nDstWidth, nDstHeight))
            {
                aNewBitmap = aBmp;
            }
Comment 19 Justin L 2016-02-13 18:38:24 UTC
potential fix:
if (!bHMirr && !bVMirr && aLinearContext.blendBitmap( Bitmap::ScopedWriteAccess(aBmp).get(), pBitmapReadAccess.get(), pAlphaReadAccess.get(), nDstWidth, nDstHeight))
Comment 20 pasqual milvaques 2016-02-14 12:00:21 UTC
I will take a look to it, thanks for the report :)
Comment 21 pasqual milvaques 2016-02-14 20:15:17 UTC
In which version and os do you reproduce the problem? In master it seems solved in my Linux build but it reproduces in 5.0.4. The patch was ported to 5.1 but not to 5.0, perhaps that was the problem. I have cherry-picked for 5.0 now 

Additional code changes have been made to the code so in master the patched BlendBitmap  function seems to not being used anymore, I'm rebuilding 5.0 in my testing machine to verify the is a change of behavior.
Comment 22 Justin L 2016-02-15 06:00:09 UTC
Created attachment 122648 [details]
I must have uploaded the wrong file.  This is not a screenshot as expected.

(In reply to pasqual milvaques from comment #21)
> In which version and os do you reproduce the problem? 
Cinnamon Mint Maya (Ubuntu 12.04) on master (LO 5.2 development)

Am export to PDF seems to work properly (it's different from the LO screenshot), so perhaps this is just a 12.04 visual thing.  There is a similar sounding bug relating to flipping and LXDE as well.  If my test document works for you, then it isn't a big deal.
Comment 23 Justin L 2016-02-17 06:56:13 UTC
I compiled on Ubuntu 16.04, and flipping works well - BUT it always takes the "else" path (whereas in 12.04 "if" returned true), so my suggestion in Comment 19 still seems valid to force this in all flipped occurrences.  Pasqual can probably confirm that the ELSE path is always chosen in 14.04 too.
Comment 24 Justin L 2016-02-17 09:25:28 UTC
(In reply to Justin L from comment #23)
> I compiled on Ubuntu 16.04, and flipping works well - BUT it always takes
> the "else" path 

Somehow I was mistaken - it does NOT always take the else path.  It always does display properly in 16.04 so marking as resolved/fixed again - I only see the problem in 12.04 which is nearing the end of its supported life.
Comment 25 pasqual milvaques 2016-02-18 00:12:41 UTC
Problem reproduces in cinnamon mint 17.3 (which is based on ubuntu 14.04). I'm not sure if in a vanilla ubuntu 14.04 the problem reproduces, I will try to take a look tomorrow. Anyway I now have a functional development environment in mint 17.03 so perhaps I could take a deeper look to what is happening there
Comment 26 pasqual milvaques 2016-02-28 23:22:01 UTC
(In reply to Justin L from comment #22)
> > In which version and os do you reproduce the problem? 
> Cinnamon Mint Maya (Ubuntu 12.04) on master (LO 5.2 development)

Justin, please take a look to patch in:
https://gerrit.libreoffice.org/#/c/22753/

to see if it solves the problem with mint 13(or 17 or …), for me it's working

Thanks :)
Comment 27 Commit Notification 2016-04-06 09:27:39 UTC
pasqualm committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=1e3d634e413abf2b4c2a6c70a87586d8c598054c

tdf#90319: make image flipping work when aLinearContext(...) code path chosen

It will be available in 5.2.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 28 Justin L 2016-04-07 16:26:00 UTC
Created attachment 124167 [details]
screenshot that belonged in comment 22 which documents the problem

confirmed in 12.04 that the bug is resolved now.
confirmed that 16.04 continues to work properly.
Marking as verified: fixed.
Comment 29 Commit Notification 2016-05-03 09:36:38 UTC
pasqualm committed a patch related to this issue.
It has been pushed to "libreoffice-5-1":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=9a425218aae218356421df9e62bf9dd5d1d809bb&h=libreoffice-5-1

tdf#90319: make image flipping work when aLinearContext(...) code path chosen

It will be available in 5.1.4.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.